feat(api): Improve VPA handling - #1015
Conversation
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes restart/table-copy preflight logic and reworks how the VPA is created/patched against the Kubernetes API (merge-patch-first with 404/409 fallback), a human look would still be worthwhile.
What was reviewed: the shared publication_table_ids_query extraction and its use in both etl and etl-api (publication name is escaped via quote_literal before being interpolated, consistent with the pre-existing pattern); read_pipeline_tables_to_copy's join against etl.replication_state (uses is_current = true, scoped to pipeline_id, defaults untracked tables to TableState::Init); the new VPA create/patch/conflict-retry flow in k8s/http.rs and its scripted-HTTP tests; and the new integration tests covering explicit/schema/all-table publications, partition roots, and cross-pipeline state isolation.
Extended reasoning...
Overview
The PR reworks two related but separable concerns: (1) etl-api's restart preflight, which now expands live publication membership (explicit tables, schema publications, all-table publications, partition roots) via a shared publication_table_ids_query helper and joins it against etl.replication_state to decide whether a restart will copy any table, including newly published tables that have no state row yet; and (2) the VPA reconciliation flow in k8s/http.rs, switching from a single forced server-side-apply Patch (which required pre-reading the existing VPA to preserve a controller-promoted updateMode) to a merge-patch-first flow that omits updateMode on update, falls back to POST-create with the initial mode on 404, and retries the merge patch on a 409 create race.
Security risks
No new injection surface: the publication name is escaped through pg_escape::quote_literal before being embedded in the SQL text, matching the pre-existing pattern in raw.rs that this PR extracted rather than introduced. The dynamic query in pipelines.rs uses AssertSqlSafe only around the already-escaped, string-interpolated portion, with pipeline_id still passed as a bound parameter. Error handling for table-state inspection failures continues to map to a generic "Internal server error" response, consistent with the project's error-leakage conventions. The k8s client changes affect authorization-adjacent infrastructure (VPA lifecycle) but do not change any authn/authz checks.
Level of scrutiny
This is a moderate-to-high scrutiny change: it touches a preflight safety mechanism (deciding whether to reset a VPA that gates resource allocation for a live replicator pod) and a concurrency-sensitive Kubernetes reconciliation path (merge-patch vs. create vs. 409-retry). Logic bugs here would manifest as either unnecessary VPA resets (minor operational noise) or missed resets (subtle performance regressions under-provisioning a newly copying pipeline) rather than outright breakage, which makes it harder to catch in casual review. The PR includes substantial new test coverage (new integration tests for explicit-table, schema-publication, all-table-publication, partition-root, and cross-pipeline-state scenarios, plus scripted-HTTP unit tests for the 404/409 k8s paths), which is a good sign, but the number of interacting edge cases (partition roots, multiple pipelines sharing a table, is_current transitions, concurrent VPA creation) is large enough that an independent human read is warranted before merging.
Other factors
The change set is confined to etl-api and two small etl/etl-postgres helpers, is well-scoped, and does not touch CODEOWNERS-sensitive crypto/auth code. No prior review comments exist on this PR (first pass), and the automated bug hunt reported no findings. Given the operational blast radius (VPA and restart behavior in production Kubernetes deployments) and the non-trivial concurrency handling, this is a good candidate for a human sanity check even though no concrete bug was found.
Check current publication membership and durable table state before restarting a pipeline. Reset the VPA when any published table needs initial sync, including newly published tables and tables that skip copying existing rows. Removed tables and tables in SyncDone, Ready, or Errored do not trigger a reset. Inspection failures and database timeouts preserve the VPA and allow restart.
Use merge patches for existing VPAs to preserve the live update mode without overwriting concurrent controller promotions. Apply the configured initial mode only when creating a VPA, and retry the merge patch after concurrent creation.
The reset is best-effort: state changes after inspection, internal pipeline retries, container restarts, and Kubernetes Pod replacements can bypass it. It does not guarantee resource allocation throughout initial sync or clear the recommender’s retained usage history.