Skip to content

feat(api): Improve VPA handling - #1015

Merged
iambriccardo merged 3 commits into
mainfrom
improve-endpoint
Sep 7, 2026
Merged

feat(api): Improve VPA handling#1015
iambriccardo merged 3 commits into
mainfrom
improve-endpoint

Conversation

@iambriccardo

@iambriccardo iambriccardo commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

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.

@iambriccardo
iambriccardo marked this pull request as ready for review September 7, 2026 09:44
@iambriccardo
iambriccardo requested a review from a team as a code owner September 7, 2026 09:44

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iambriccardo
iambriccardo merged commit 7cb9a7f into main Sep 7, 2026
23 checks passed
@iambriccardo
iambriccardo deleted the improve-endpoint branch September 7, 2026 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants