Remove legacy Trino runtime in favor of shared pools

[benben]

Summary

Remove the retired standalone and fixed blue/green Trino runtime. The only supported configured topology is now the Duckgres-managed shared compute pool.

• Remove old coordinator environment variables, registry-only migration mode, implicit ownership, fixed backend catalog clients, static rollout/canary APIs, and direct-coordinator connection fallbacks.
• Remove unused fixed-coordinator credential probes and their Kubernetes exec client. Preserve shared-pool publication fences, credential/revision evidence, tenant admission, and transaction-aware Gateway lifecycle operations.
• Require the catalog writer when the pool operator is enabled. Read-only operators remain supported. Warn about enabled warehouses with unconfigured owners without changing any rows or blocking unrelated service startup.
• Replace the isolated CI topology with a real shared-pool operator, Gateway, publisher, and generated immutable blueprint. Preserve tenant/auth/Hoglake/performance scenarios. Pin build outputs by digest and resolve the Gateway image from the deployed test environment unless explicitly overridden.
• Keep historical migrations and persisted warehouse/storage data unchanged. Update UI guidance and operational documentation.
• Remove the unused coordinator statement-error classifier, its mount-lag Pending/retry branch, and obsolete tests. Generic catalog publication failures still record failure and recover on a later successful reconcile.
• Export bounded configured-pool, serving-instance Deployment identity, and minimum-serving metrics for the replacement alerts. Serving identity follows each instance's pinned namespace and does not claim Kubernetes health. Owner-scoped samples clear on term loss; configured expectations remain until process shutdown.

Deployment order

1. Deploy the legacy shutdown charts change, after explicitly disabling or moving affected warehouses.
2. Apply the legacy infrastructure deletion.
3. Merge and deploy this binary, then deploy the final charts configuration/RBAC cleanup.

Before step 3, verify no enabled warehouse retains an unassigned or removed owner. New warehouse provisioning that enables Trino needs a configured default pool. These are explicit operator gates; this PR does not reassign, disable, or delete customer warehouses.

The existing Gateway token/username/server-name settings remain because the shared-pool client still uses them. Deploy this binary before the final charts cleanup and pool-aware alerts in charts #16533.

Validation

• Red → green regressions for shared-pool-only configuration, removed topology rejection, ownership diagnostics, and enabled-operator/catalog-writer requirements.
• Red → green coverage for retired retry-code removal and new metrics; race-tested partial pool authority loss, stale-term cleanup, empty serving snapshots, pinned namespaces, and configuration lifetime.
• Complete control-plane, provisioner, and provisioning suites with the Kubernetes build tag, including native PostgreSQL pool/catalog tests.
• Configstore integration suite against isolated native PostgreSQL.
• Admin unit tests; UI typecheck and 121 Trino/org-detail tests.
• just test-mw-fixtures: all eight fixture/scenario packages, including blueprint validation and image-resolution contracts.
• just lint, Kubernetes-tagged changed-code lint, shell syntax checks, and git diff --check.
• Independent read-only adversarial reviews of runtime safeguards and CI bootstrap wiring; identified issues fixed.

Not run locally: Docker-bound admin PostgreSQL tests (Docker unavailable), or live Kubernetes end-to-end deployment. The updated CI fixture must validate the full deployed path before rollout.

[github-actions]

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area	Added	Changed	Deleted
Test files	6	37	25
E2E/journey files	0	0	0
Workflow files	0	3	0

Signals

• Test cases: +23 / -138
• Assertions: +95 / -639
• Skips or known failures added: 0
• Workflow continue-on-error added: 0
• Workflow path filters added: 0
• Test commands removed from justfile: 1
• E2E/journey retry lines added: 0

Coverage risk: likely reduced

Warnings

• Test files deleted (likely reduced)
  • controlplane/provisioner/trino_auth_gate_test.go
  • controlplane/provisioner/trino_auth_readiness_test.go
  • controlplane/provisioner/trino_cell_backends_test.go
  • controlplane/provisioner/trino_managed_catalogs_test.go
  • controlplane/provisioner/trino_nodes_test.go
  • controlplane/provisioner/trino_readiness_test.go
  • controlplane/provisioner/trino_secret_readiness_test.go
  • controlplane/provisioner/trino_shared_catalog_client_test.go
  • ... and 17 more
• Test cases removed (likely reduced)
  • 138 removed vs 23 added
• Assertions removed (needs review)
  • 639 removed vs 95 added
• Test command removed (likely reduced)
  • 1 test command line(s) removed from justfile

[bill-ph]

Reviewed the full diff (130 files, +1.3k/-11k) with the checkout at 2734e20. Focused on what survives rather than what is deleted: the registry/env contract (trino_inputs.go, trino_registry.go), the enablement + ownership path (trino_fleet.go, configstore/trino.go, EnableTrinoInTransaction, assignedCellOrgs), pool wiring and the new catalog-writer requirement (trino_pool_wiring.go, trino_pool_catalog.go), the rollout* → trinoPool* rename in the probe/validate path, the admin fleet API now that legacy is gone, and the rewritten CI fixture (poolfixture, run.sh, trino-controlplane-patch.tmpl.json, e2e/trino.sh).

Things I checked and found sound: the registry decoder keeps DisallowUnknownFields, so a stale backends/catalog_management registry file fails loudly rather than being half-understood; Namespace/CellID are now required at provisioner construction and the registry validates both; EnableTrino still stamps the validated deployment default so enablementCheck's "default exists" branch can't produce a row nobody reconciles; the pool operator refuses to start without the Gateway and the catalog writer, and the writer is still only installed under the won lease; the probe rename is mechanical and keeps the continuation-origin and redirect checks (well covered by the new trino_pool_probe_test.go); unavailableTrinoCatalogClient still refuses with ErrTrinoCatalogNotThisReplica so non-leaders leave state rows alone; no production code references any removed symbol (TrinoCustomerNamespace, DefaultTrinoCellID, ClaimTrinoCell, ErrTrinoNodeInventoryUnavailable); and the e2e fixture's instance/service/deployment names line up with trinopool.instantiate (<id>, <id>-worker) and the /trino/instances payload.

The deleted-test signal from the bot is expected — the removed suites cover removed code. Nothing here is a merge blocker.

P1

• Retired env vars are now silently ignored, so a mis-ordered deploy comes up with Trino completely off. trinoProvisionerEnabled() keys only on DUCKGRES_TRINO_DEFAULT_CELL/DUCKGRES_TRINO_CELLS_FILE (controlplane/trino_inputs.go:64). A pod that still carries only DUCKGRES_TRINO_COORDINATOR_URL/DUCKGRES_TRINO_CELL_ID/DUCKGRES_TRINO_NAMESPACE — i.e. step 3 shipped before the charts change, or the charts change is rolled back — starts cleanly with no fleet: /api/v1/trino/* routes disappear, no cell reconciles, and every enabled warehouse stops being projected, with no error and only the absence of a log line to notice. The rest of this PR is deliberately fail-loud in exactly this situation (unknown registry fields, operator-without-writer, missing namespace/cell id). Rejecting startup when any retired DUCKGRES_TRINO_* variable is still set would make the documented deployment gate self-enforcing instead of relying on ordering.

P2

• The ownership audit logs a bare count (controlplane/trino_fleet.go:30-34). The pre-merge gate is "verify no enabled warehouse retains an unassigned or removed owner", but the warning gives an operator no way to find those warehouses — a bounded sample of org ids plus the offending cell_ids would make it actionable. It also only runs at wiring time, so a row that drifts after startup is never re-reported.
• wait_trino_pool fails the whole deploy on the first transient psql blip (tests/mw-dev/run.sh:70): count="$(kubectl exec ... )" || return 1 inside the retry loop. The exec races with ordinary pod churn; continueing and only failing after the 180 attempts are exhausted matches the loop's intent.
• The catalog-writer gate swallows its ParseBool error (controlplane/trino_pool_wiring.go:57). DUCKGRES_TRINO_POOL_CATALOG_WRITER_ENABLED=ture (or yes) reports "OPERATOR_ENABLED requires CATALOG_WRITER_ENABLED" even though the operator did set it, which sends them looking in the wrong place.
• CI now hard-depends on reading the deployed Gateway (tests/mw-dev/run.sh:46). Neither e2e-mw-dev.yml nor scenario-dev.yml sets TRINO_GATEWAY_IMAGE, so every Trino lane takes the cluster-read path and requires (a) get deployment in the trino-gateway namespace for the CI role — not granted by anything in this repo's manifests — and (b) the deployed gateway image to be digest-pinned, or require_pool_images aborts. The previous shared-catalogs lane was opt-in and never enabled in CI, so this path is newly load-bearing; worth confirming on the first run.
• TestTrinoSharedPoolNeedsNoLegacyConfiguration asserts nothing about the legacy env (controlplane/trino_legacy_removal_test.go:29-30): it sets DUCKGRES_TRINO_REGISTRY_ONLY and DUCKGRES_TRINO_COORDINATOR_URL to "", which is indistinguishable from unset. Setting them to realistic non-empty values would turn this into the regression test the name promises (and would cover the P1 above if that guard lands).
• The mount-lag classifier is now unreachable. tenantSecretNotMountedYet (controlplane/provisioner/trino_provisioner.go:1843) matches *TrinoStatementError, which nothing constructs any more — the HTTP catalog clients that produced it are gone and the pool writer publishes through the store. The PR calls this out as intentionally retained, so just confirming the intent: the "tenant secret not mounted yet → Pending, not Failed" behavior it provided now has to come entirely from the pool's tenant-admission gate.

Inline comments

• controlplane/trino_inputs.go:65 — P1: with the coordinator vars gone from this predicate, a pod that still carries only DUCKGRES_TRINO_COORDINATOR_URL/DUCKGRES_TRINO_CELL_ID (charts change not yet applied, or rolled back) starts with no Trino fleet at all: admin /trino/* routes absent, no cell reconciling, every enabled warehouse silently unprojected — no error, just a missing log line. Everything else in this PR fails loudly in the same situation (unknown registry fields, operator-without-writer, missing namespace/cell id). Consider erroring at startup when any retired DUCKGRES_TRINO_* variable is still set, so the documented deployment ordering enforces itself.
• controlplane/trino_fleet.go:33 — P2: this is the signal for the pre-merge gate ("verify no enabled warehouse retains an unassigned or removed owner"), but a bare count gives the operator nothing to act on. A bounded sample of org ids plus the offending cell_ids would make it directly actionable. Also note this only runs at wiring time, so a row that drifts later is never re-reported.
• controlplane/trino_pool_wiring.go:57 — P2: the ParseBool error is dropped, so DUCKGRES_TRINO_POOL_CATALOG_WRITER_ENABLED=ture (or yes) is reported as "OPERATOR_ENABLED requires CATALOG_WRITER_ENABLED" even though the operator did set it. Surfacing the parse failure points at the actual problem.
• tests/mw-dev/run.sh:70 — P2: one transient kubectl exec/psql failure aborts the whole deploy instead of retrying — the exec races with ordinary pod churn, and this loop gates the entire Trino lane. continue on error and fail only once the 180 attempts are exhausted.
• tests/mw-dev/run.sh:48 — P2: neither e2e-mw-dev.yml nor scenario-dev.yml sets TRINO_GATEWAY_IMAGE, so every Trino lane now takes this path. It needs get deployment in the trino-gateway namespace for the CI role (nothing in this repo's manifests grants it) and a digest-pinned image on that deployment, or require_pool_images aborts the run. The old shared-catalogs lane was opt-in and never enabled in CI, so this is newly load-bearing — worth watching the first run.
• controlplane/trino_legacy_removal_test.go:29 — P2: setting these to "" is indistinguishable from unset, so the test doesn't actually assert that legacy configuration can't resurrect the old path. Realistic non-empty values would make the name true (and would cover the silent-disable case if a startup guard for retired vars lands).

— Robo Bill v2 (opus, high reasoning)

[bill-ph]

Re-reviewed at 975458e, focused on the delta since my review of 2734e20 (one commit, 8 files: the mount-lag classifier removal in trino_provisioner.go, the new pool alert metrics in trino_pool_observability.go + its wiring in multitenant.go/trino_pool_operator.go, the new tests, and the docs/trino-cells.md section). The rest of the PR is unchanged from what I already reviewed.

This commit closes out two of my earlier P2s, and does it properly:

• Retired retry code is gone, not just unreachable. TrinoStatementError, tenantSecretNotMountedYet and trinoSecretMountLagError are fully removed, no production or test reference survives (trinoDuckLakePasswordFileProperty is still used to build catalog properties, and errors/strings are still needed in the file, so no dead imports). The replacement TestReconcile_CatalogPublicationFailureRecoversOnNextTick is a real regression test: it covers both a plain error and context.DeadlineExceeded, asserts the error isn't swallowed, that failed_at is stamped, and that the next tick recovers to Ready. Worth noting that removing the only Pending branch in reconcileBackendCatalogs's create path doesn't widen the failure blast radius — the existingSet[name] shortcut still returns Existed before CreateCatalog is reached, so an already-published org can't be flipped to Failed by a publisher blip, and the all-orgs-Failed-on-ListCatalogs-error path is unchanged and pre-existing.
• The new metrics are carefully scoped. snapshot sets minServing/servingInstances only after the owners[pool] != o ownership check, so a stale term can't overwrite a newer one; clear deletes both by DeletePartialMatch on pool, and end() is ownership-guarded so an old term's end() can't wipe the new term's samples (covered by TestTrinoPoolServingInventoryMetricsFollowSnapshotAndTerm). The departed-identity pruning diffs against the previous o.serving set rather than resetting, so unchanged series stay present across scrapes. Label order in DeleteLabelValues matches WithLabelValues and the GaugeVec declaration. The test explicitly asserts no instance/namespace label collides with scrape identity — pool_instance/workload_namespace are the right choice. Cardinality is bounded by live pool membership. configurePools and snapshot share m.mu and touch disjoint gauges, so the startup call racing an early-winning operator loop is safe.

Nothing here is a merge blocker.

P2

• A failed tableflip reload permanently drops duckgres_trino_pool_configured while the process keeps serving metrics. apiServer.RegisterOnShutdown(...) (controlplane/multitenant.go:889) is the only consumer of the cleanup closure, and cp.apiServer.Shutdown has exactly one call site: handleUpgrade (controlplane/control.go:2727). On SIGTERM the hook never runs at all, which is harmless. But if upg.Upgrade() fails, recoverAfterFailedReload explicitly stands a fresh metrics server back up (controlplane/control.go:2875) while the API server is not recovered and configurePools is never re-run — so the process goes on reconciling pools and exporting serving_instance_info/min_serving/snapshot_timestamp, with the configured expectation series gone for good. That blinds exactly the alert the docs describe ("Alert separately when an expected configured pool has no fresh telemetry"): the expectation vanishes, so the alert stops evaluating instead of firing. Calling configurePools again from recoverAfterFailedReload, or tying the reset to process exit rather than to http.Server.Shutdown, would make the metric's lifetime match the sentence in the docs.
• duckgres_trino_pool_configured is gated on operatorEnabled, which the doc wording doesn't say. controlplane/trino_pool_observability.go:78 sets the gauge only for operators with operatorEnabled true — i.e. DUCKGRES_TRINO_POOL_OPERATOR_ENABLED, not "is this pool in the registry". docs/trino-cells.md:117-119 says "enabled pools from this process's startup registry, independently of operator ownership", which reads as if it's registry-derived; "ownership" (lease authority) and "enabled" (the env flag) are different things here. In the read-only-operator deployment the PR description says stays supported, the metric is absent entirely, so it can't distinguish "pool configured, operator deliberately off" from "pool not configured" — which matters because the alerts in charts #16533 are being written against this. One sentence in the doc pinning it to the operator flag would remove the ambiguity.
• The minNamespace re-label bookkeeping can never fire. controlplane/trino_pool_observability.go:167-170 deletes the previous minServing namespace label when it changes, but the namespace argument is always o.config.Namespace (trino_pool_operator.go:356), which is read from the registry at startup and never mutated for the operator's lifetime; a telemetry token also never outlives its term, and clear wipes the pool's samples by partial match at term boundaries anyway. It's harmless, but it's three lines of state implying a namespace change that the current wiring can't produce.
• TestTrinoRejectsRetiredCoordinatorCatalogRetryCode only greps one file. controlplane/trino_legacy_removal_test.go:14 reads provisioner/trino_provisioner.go by relative path, so the guard passes if the symbols are reintroduced in any sibling file in the same package (trino_catalog_client.go, etc.) — which is the likelier shape of a regression, since the type was a catalog-client concern. Walking the provisioner package's .go files would make the assertion match the test's name. (Grepping source text will also trip on a passing mention in a comment, but that's the conservative direction.)

Inline comments

• controlplane/multitenant.go:889 — P2: the cleanup closure only runs from http.Server.Shutdown, whose single call site is handleUpgrade (controlplane/control.go:2727). On SIGTERM it never runs (harmless), but on a failed tableflip upgrade recoverAfterFailedReload stands a fresh metrics server back up (control.go:2875) while configurePools is never re-run — so the process keeps reconciling and exporting serving_instance_info/min_serving/snapshot_timestamp with duckgres_trino_pool_configured gone permanently. That silently blinds the "expected configured pool has no fresh telemetry" alert the docs describe, rather than firing it. Re-running configurePools from recoverAfterFailedReload, or tying the reset to process exit instead of API-server shutdown, would match the documented lifetime.
• controlplane/trino_pool_observability.go:78 — P2: this gates on operatorEnabled (the DUCKGRES_TRINO_POOL_OPERATOR_ENABLED flag), but docs/trino-cells.md:117-119 describes the metric as coming "from this process's startup registry, independently of operator ownership" — ownership (lease authority) and enablement (the env flag) are different things. In the read-only-operator deployment the PR description keeps supporting, the series is absent entirely, so it can't distinguish "configured, operator deliberately off" from "not configured". Worth pinning down in the doc before charts #16533 builds alerts on it.
• controlplane/trino_pool_observability.go:167 — P2: this branch is unreachable. namespace is always o.config.Namespace (trino_pool_operator.go:356), read from the registry at startup and never mutated for the operator's lifetime; a telemetry token never outlives its term, and clear already wipes the pool's minServing samples by partial match at term boundaries. Harmless, but it implies a namespace change the current wiring can't produce.
• controlplane/trino_legacy_removal_test.go:14 — P2: this only reads provisioner/trino_provisioner.go, so the guard passes if TrinoStatementError/tenantSecretNotMountedYet come back in any sibling file of the same package — which is the likelier shape of a regression, given the type was a catalog-client concern. Walking the package's .go files would make the assertion match the test's name.

— Robo Bill v2 (opus, high reasoning)