feat: fail-fast preflight for extension update paths by WentingWu666666 · Pull Request #450 · documentdb/documentdb-kubernetes-operator

WentingWu666666 · 2026-08-27T13:40:50Z

Closes #448. Follows up on #439 (deferred e2e gaps) and relates to #426 / #443.

Problem

handleExtensionUpgrade runs a blanket ALTER EXTENSION documentdb UPDATE with no check that PostgreSQL can resolve an update path. If the extension image is missing a documentdb--<from>--<to>.sql script anywhere in the requested range, the ALTER fails at execution time with a raw PostgreSQL error that re-fires every reconcile. The user gets a cryptic failure, no status signal, and no indication of what to do.

What this does

Adds a read-only preflight against pg_extension_update_paths immediately before the ALTER:

SELECT COALESCE(
  (SELECT path FROM pg_extension_update_paths('documentdb')
    WHERE source = '<installed>' AND target = '<target>'),
  'NO_UPDATE_PATH') AS update_path

When the absence of a path is positively proven, the operator:

skips the ALTER entirely (no partial migration, status.schemaVersion does not move),
emits a Warning event, and
sets status.conditions[SchemaUpgradeBlocked] = True with reason NoUpdatePath and a message naming both versions plus the remediation,
and returns cleanly — retrying cannot help until the spec changes, so there is no retry storm.

It fails open on anything inconclusive. A SQL error, unparseable psql output, or an unexpected version format all proceed with the ALTER exactly as before. The preflight is an advisory improvement; an inconclusive check must never wedge an upgrade that would otherwise succeed (e.g. on a PostgreSQL build lacking the function).

Why the `COALESCE` sentinel

pg_extension_update_paths expresses "no path" two different ways: a NULL path when the pair is known-but-unreachable, and zero rows when either version isn't advertised at all. Those produce different psql output shapes (an empty field vs. a (0 rows) footer), which is fragile to parse. The COALESCE collapses both into a single always-present, never-empty row, so the parse is unambiguous and can't misread a gap as a valid path.

New status conditions

DocumentDBStatus gains a standard conditions list. SchemaUpgradeBlocked is reconciled on every migration-planning path — SchemaUpToDate, UpdatePathAvailable, NoMigrationPlanned (rollback / two-phase mode), NoUpdatePath — so a stale True cannot survive a user correcting their spec. meta.SetStatusCondition's return value suppresses redundant status writes, and the write is RetryOnConflict-wrapped because the informer read can lag a status update made earlier in the same reconcile.

$ kubectl get documentdb my-db -o jsonpath='{.status.conditions[?(@.type=="SchemaUpgradeBlocked")]}'

Test coverage

Unit (18 new specs, run in the PR gate): SQL shape, parser edge cases, both fail-open paths, the version-format guard, blocked → no ALTER fires, preflight targets the pinned version rather than the binary version, and both recovery transitions (revert to two-phase, retarget to a reachable version).

e2e — closes the three gaps deferred from #439:

Gap	Spec	Level
Jump of >1 minor	`upgrade_schema_multiversion_test.go`	Lowest
Multi-version chain	`upgrade_schema_multiversion_test.go`	Lowest
`ALTER EXTENSION UPDATE` fails	`upgrade_schema_preflight_test.go`	Low

The migration-failure spec was deferred in #439 because reproducing it needed a doctored extension image. It's now reachable with stock images: request a patch version that was never released (e.g. 0.109.999 while installed is 0.109.0 and the binary is 0.110.0). That is <= the binary version so the validating webhook admits it, and > the installed version so determineSchemaTarget plans a migration — but no update script exists. The spec skips itself if the configured old/new versions share a major.minor, where the trick wouldn't hold.

A new E2E_UPGRADE_DOCUMENTDB_VERSION_CHAIN env var (documented in test/e2e/README.md) drives the multi-version specs, defaulting to the published tags 0.109.0,0.110.0,0.113.0,0.114.0.

Open: the three contract questions in #448 are still unratified

#448 asks maintainers to ratify three points, and there's been no answer. This implementation deliberately doesn't foreclose any of them:

"Are multi-minor jumps supported?" — treated as yes, which is what the code already did. The preflight adds no new restriction; it only reports what PostgreSQL itself would have done. If the answer turns out to be "no", that's a separate spec validation, not a change here.
"Should the check live in the webhook too?" — controller-only, as feat: fail-fast preflight for extension update paths (+ e2e for >1-minor jumps and migration failure) #448 itself recommends as the pragmatic option. The webhook can't query a live database, so it can't do this check without a round trip at admission time.
"What's the recovery contract?" — implemented as "recoverable, not terminal": correcting the spec clears the condition and the migration proceeds. The e2e spec pins that.

Happy to adjust if maintainers land somewhere different.

Notes for reviewers

docs/.../api-reference.md carries some unrelated drift (ComponentResources, the postgres image pin, disableTLS). That's pre-existing staleness that make api-docs corrected — not part of this change.
make lint reports 8 SA1019 staticcheck findings. All 8 are present on main unchanged; this PR adds none.
The e2e specs are dry-run verified only; they need a kind cluster with the operator installed and E2E_UPGRADE=1 to actually execute.

Validation

make test — all packages pass (internal/controller coverage 56.3% → 56.5%)
make manifests generate fmt vet api-docs — no drift
helm unittest — 88/88 pass
go vet ./... + gofmt in test/e2e — clean
TEST_DEPTH=4 ginkgo --dry-run ./tests/upgrade/... — 11/12 specs construct (1 pre-existing pending), all new specs present with correct labels

The operator upgrades the documentdb extension with a blanket `ALTER EXTENSION documentdb UPDATE`, with no check that PostgreSQL can actually resolve an update path. When the extension image is missing a `documentdb--<from>--<to>.sql` script anywhere in the requested range, the ALTER fails at execution time with a raw PostgreSQL error that re-fires on every reconcile, leaving the user with no actionable signal. Add a read-only preflight against `pg_extension_update_paths` that runs immediately before the ALTER. When the absence of a path is positively proven, the operator skips the ALTER, emits a Warning event, and reports `SchemaUpgradeBlocked=True` / `NoUpdatePath` with a message naming both versions and the remediation. Any error, unparseable output, or unexpected version format fails open, preserving the pre-existing behavior of letting PostgreSQL decide. The query wraps the lookup in `COALESCE(..., 'NO_UPDATE_PATH')` so that both "no rows" (version not advertised) and "NULL path" (known but unreachable) collapse into a single, always-present row, which makes the psql output unambiguous to parse. Also adds `status.conditions` to the DocumentDB CRD, reconciled across every migration-planning path so a stale block cannot survive a user correcting their spec. Test coverage: - 18 unit specs covering the SQL shape, parser, fail-open paths, the blocked/unblocked transitions, and recovery. - Three e2e specs closing the gaps deferred from documentdb#439: jumps of more than one minor, a sequential chain through every published version, and a deterministic migration failure built from a never-released patch version (reachable with stock images now that the preflight turns it into a clean, observable stop). Closes documentdb#448 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

Copilot

Pull request overview

Adds a fail-fast (but fail-open on inconclusive) preflight for DocumentDB extension schema upgrades so the controller can prove when PostgreSQL has no ALTER EXTENSION ... UPDATE path, surface an actionable status signal, and avoid repeated reconcile-time ALTER failures. This also introduces standard status.conditions on the DocumentDB CRD and adds e2e coverage for previously deferred multi-version upgrade scenarios and the “no update path” failure mode.

Changes:

Add a pg_extension_update_paths preflight before running ALTER EXTENSION documentdb UPDATE, and report SchemaUpgradeBlocked=True (reason NoUpdatePath) when no path exists.
Introduce DocumentDB.status.conditions (with SchemaUpgradeBlocked condition + reasons) and reconcile it across schema-upgrade planning paths.
Add new e2e upgrade specs for multi-minor jumps, sequential version chains, and blocked schema migrations; document the new env var used to drive version chains.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`operator/src/internal/controller/documentdb_controller.go`	Adds update-path preflight + status condition reconciliation around schema upgrades.
`operator/src/internal/controller/documentdb_controller_test.go`	Extends unit coverage for the preflight, parsing, fail-open behavior, and condition transitions.
`operator/src/api/preview/documentdb_types.go`	Adds `status.conditions` and defines `SchemaUpgradeBlocked` + reason constants.
`operator/src/api/preview/zz_generated.deepcopy.go`	Updates deepcopy generation for the new `Conditions` field.
`operator/src/config/crd/bases/documentdb.io_dbs.yaml`	CRD schema update to include `status.conditions`.
`operator/documentdb-helm-chart/crds/documentdb.io_dbs.yaml`	Helm-packaged CRD update to include `status.conditions`.
`test/e2e/tests/upgrade/upgrade_schema_preflight_test.go`	Adds e2e coverage for blocked schema migration (no update path) and recovery after retargeting.
`test/e2e/tests/upgrade/upgrade_schema_multiversion_test.go`	Adds Lowest-tier e2e specs for multi-minor jump + sequential upgrade chain.
`test/e2e/tests/upgrade/helpers_test.go`	Adds helpers for version-chain config and reading `SchemaUpgradeBlocked` condition.
`test/e2e/README.md`	Documents `E2E_UPGRADE_DOCUMENTDB_VERSION_CHAIN`.
`docs/operator-public-documentation/preview/operations/upgrades.md`	Documents skipping versions + how `SchemaUpgradeBlocked` is surfaced.
`docs/operator-public-documentation/preview/api-reference.md`	Regenerated API reference output (includes unrelated drift fixes per PR description).
`CHANGELOG.md`	Records the preflight/condition behavior under Unreleased.

Files not reviewed (1)

operator/src/api/preview/zz_generated.deepcopy.go: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+	if err := r.setSchemaUpgradeBlockedCondition(ctx, documentdb, metav1.ConditionFalse,
+		dbpreview.ReasonUpdatePathAvailable,
+		fmt.Sprintf("Update path from %s to %s is resolvable.",
+			util.ExtensionVersionToSemver(installedVersion),
+			util.ExtensionVersionToSemver(schemaTarget)),
+	); err != nil {
+		logger.Error(err, "Failed to clear SchemaUpgradeBlocked condition")
+	}


+			// e.g. old "0.109.0" → unreachable "0.109.7": above the installed
+			// schema, below the new binary, and never released — so no
+			// documentdb--0.109-0--0.109-7.sql exists.


+// documentDBVersionChain returns the ascending list of published DocumentDB
+// versions used by the multi-version upgrade specs, read from
+// envDocumentDBVersionChain (comma-separated) or falling back to
+// defaultDocumentDBVersionChain. Specs that need more entries than are
+// configured should Skip rather than fabricate versions.
+func documentDBVersionChain() []string {
+	raw := envOr(envDocumentDBVersionChain, defaultDocumentDBVersionChain)
+	var out []string
+	for _, part := range strings.Split(raw, ",") {
+		if v := strings.TrimSpace(part); v != "" {
+			out = append(out, v)
+		}
+	}
+	return out
+}


WentingWu666666 · 2026-08-27T14:09:07Z

Relationship to #444

Flagging an overlap I found after opening this: #444 (open since 2026-08-13) already implements the core preflight against pg_extension_update_paths. This PR is a superset of it, built for #448:

	#444	this PR
`pg_extension_update_paths` preflight	✅	✅
Warning event on no-path	✅ (`SchemaUpgradePathMissing`)	✅ (`SchemaUpgradeBlocked`)
`status.conditions`	❌ — explicitly deferred as "needs a CRD/deepcopy regen"	✅ `SchemaUpgradeBlocked` reconciled on every planning path
e2e coverage	❌ — listed as a follow-up (">1 minor jump", broken chain)	✅ all three gaps from #439
Unit specs	4	21

Two deliberate behavioral differences worth a maintainer opinion:

Error handling. feat: fail-fast preflight for schema upgrade path (multi-minor jumps) #444 propagates a preflight SQL error (fail closed). This PR fails open — an error, unparseable output, or an unexpected version string proceeds with the ALTER exactly as before. The preflight is advisory, so an inconclusive check shouldn't wedge an upgrade that would otherwise succeed (e.g. on a PostgreSQL build without the function).
Query shape. feat: fail-fast preflight for schema upgrade path (multi-minor jumps) #444 checks for a non-NULL path directly; this PR wraps it in COALESCE(..., 'NO_UPDATE_PATH') so that "zero rows" (version not advertised) and "NULL path" (known but unreachable) collapse into one always-present row. Both mean "no path" but produce different psql output shapes, and the sentinel removes the parsing ambiguity.

The source == target short-circuit in #444 isn't needed here: determineSchemaTarget only returns a target strictly greater than the installed version, and the defaultVersion == installedVersion case returns earlier, so the preflight is never reached for a no-op.

Suggestion: land this one and close #444, rather than merging #444 first and rebasing this on top — they touch the same function and the merge would be mostly conflict resolution. Happy to do it the other way round if reviewers prefer; say the word.

`setup-test-environment` verified the loaded images with `docker images ... | grep -q "$IMAGE"`. Under the step's `set -o pipefail`, `grep -q` exits as soon as it matches, docker takes SIGPIPE, and the pipeline reports 141 — so a *successful* match fails the check. The race depends on how early the match appears in docker's output and how much output remains, which is why it fires intermittently and then fails every shard at once. Capture the image list into a variable and match against that, which removes the pipe entirely. Also switch to `grep -qxF` so the comparison is an exact literal line rather than a substring regex. The cert-manager `helm list | grep -q` check had the same shape and is fixed the same way. Repro of the old behavior: $ set -o pipefail $ seq 1 10000 | grep -q 1; echo $? 141 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

WentingWu666666 · 2026-08-27T14:21:03Z

Note on the second commit (`fix(ci):`)

The E2E matrix failed on all 18 shards at "Setup test environment", before any test ran. Root cause is a pre-existing race in .github/actions/setup-test-environment/action.yml, not anything in this change:

if ! docker images --format "table {{.Repository}}:{{.Tag}}" | grep -q "$OPERATOR_IMAGE"; then

The step runs under set -o pipefail. grep -q exits the moment it matches, docker takes SIGPIPE and returns 141, and pipefail propagates that — so finding the image fails the check. The logs show it plainly: all eight images load successfully, the arm64 operator image is listed, and the very next line is ❌ Required operator image not found for that same image.

Minimal repro:

$ set -o pipefail
$ seq 1 10000 | grep -q 1; echo $?
141

Whether it trips depends on how early the match lands in docker's output and how much output is left to write, which is why this has been intermittent (there's at least one earlier failed run on main and one on another PR) rather than constant.

Fix: capture the list into a variable and match against that, removing the pipe. Also switched to grep -qxF so it's an exact literal line match instead of a substring regex — . in the image name was being treated as a wildcard. The helm list | grep -q cert-manager check a few steps later had the same shape and is fixed the same way.

I'd normally keep a CI change out of a feature PR, but this one gates the E2E suite this PR is adding specs to. Happy to split it into its own PR if reviewers prefer — it's an independent commit.

`TEST_DEPTH` accepts Highest|High|Medium|Low|Lowest (see test/e2e/levels.go), but the workflow_dispatch `depth` choice only offered Low|Medium|High. Specs declared at `level:lowest` — including the multi-version schema-upgrade specs added in this branch — were therefore unreachable from CI: they compile and are label-selected, but the runtime depth gate skips them at every tier the dispatch could request. List all five tiers. The default stays Medium, so scheduled and PR runs are unchanged; this only widens what a manual dispatch can ask for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

WentingWu666666 · 2026-08-27T15:20:31Z

Note on the third commit (`ci:`)

While confirming the new e2e specs actually execute, I found they can't be reached from CI at all.

test/e2e/levels.go accepts five depth tiers (Highest|High|Medium|Low|Lowest), but test-e2e.yml's workflow_dispatch depth choice only listed three:

options:
  - Low
  - Medium
  - High

So a spec declared at level:lowest compiles, is label-selected, and is then skipped by the runtime depth gate at every tier a dispatch can request. The multi-version specs in this PR sit at that tier, as do any future full-sweep specs. I listed all five options; the default stays Medium, so PR and push runs are untouched — this only widens what a manual dispatch can ask for.

E2E status

Green — 18/18 shards, 64/64 checks. Confirmed from the E2E upgrade (amd64) log that the new status condition is being written on a live cluster:

conditions:
  - type: SchemaUpgradeBlocked
    status: "False"
    reason: SchemaUpToDate
    message: Extension schema is at 0.113.0; no migration pending.

One caveat worth stating plainly: the four new specs are dry-run verified only (TEST_DEPTH=4 ginkgo --dry-run — all four construct with the right labels). CI runs TEST_DEPTH: Medium, so they were among the 6 skipped, and I don't have Docker/kind available locally to run them for real. With the third commit in place a maintainer can now exercise them via Actions → TEST - E2E → Run workflow → depth: Lowest, label: upgrade. I'd suggest doing that before merge rather than taking the dry-run on trust.

The one E2E failure on the previous attempt was an unrelated CSI flake (tar: ./pgdata/base/5: file changed as we read it while snapshotting a live PGDATA in cluster-replication backup) and passed on re-run.

documentdb-triage-tool · 2026-08-27T17:05:16Z

🤖 Auto-triaged by documentdb-triage-tool.

Applied: documentation, test, go, CI/CD, enhancement
Project fields suggested: Component docs · Priority P2 · Effort L · Status Needs Review
Confidence: 0.82 (mixed)

Reasoning

component from path globs (docs, test, api, controllers, ci, manifests); effort from diff stats (1663+65 LOC, 15 files); LLM: Multi-file change adding preflight logic, new status conditions, and reconcile-path handling to the extension upgrade controller — touches schema/status fields and multiple code paths.

If a label is wrong, remove it manually and ping @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

Copilot AI lite review requested due to automatic review settings August 27, 2026 13:40

WentingWu666666 requested review from alaye-ms, hossain-rayhan and xgerman as code owners August 27, 2026 13:40

WentingWu666666 mentioned this pull request Aug 27, 2026

feat: fail-fast preflight for extension update paths (+ e2e for >1-minor jumps and migration failure) #448

Open

6 tasks

Copilot started reviewing on behalf of WentingWu666666 August 27, 2026 13:41 View session

WentingWu666666 force-pushed the wentingwu666666-extension-update-path-preflight branch from f359f18 to 9ffca01 Compare August 27, 2026 13:45

Copilot AI reviewed Aug 27, 2026

View reviewed changes

WentingWu666666 mentioned this pull request Aug 27, 2026

feat: fail-fast preflight for schema upgrade path (multi-minor jumps) #444

Open

documentdb-triage-tool Bot added CI/CD documentation Improvements or additions to documentation enhancement New feature or request go Pull requests that update go code test labels Aug 27, 2026

Navigation Menu

Uh oh!

feat: fail-fast preflight for extension update paths - #450

feat: fail-fast preflight for extension update paths#450
WentingWu666666 wants to merge 3 commits into
documentdb:mainfrom
WentingWu666666:wentingwu666666-extension-update-path-preflight

WentingWu666666 commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

WentingWu666666 commented Aug 27, 2026

Uh oh!

WentingWu666666 commented Aug 27, 2026

Uh oh!

WentingWu666666 commented Aug 27, 2026

Uh oh!

documentdb-triage-tool Bot commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

WentingWu666666 commented Aug 27, 2026

Problem

What this does

Why the COALESCE sentinel

New status conditions

Test coverage

Open: the three contract questions in #448 are still unratified

Notes for reviewers

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

WentingWu666666 commented Aug 27, 2026

Relationship to #444

Uh oh!

WentingWu666666 commented Aug 27, 2026

Note on the second commit (fix(ci):)

Uh oh!

WentingWu666666 commented Aug 27, 2026

Note on the third commit (ci:)

E2E status

Uh oh!

documentdb-triage-tool Bot commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Why the `COALESCE` sentinel

Note on the second commit (`fix(ci):`)

Note on the third commit (`ci:`)