Skip to content

fix(operations): fail upgrade ops fast on unresolvable componentDef or serviceVersion#10547

Open
weicao wants to merge 4 commits into
mainfrom
bugfix/upgrade-ops-fail-invalid-inputs
Open

fix(operations): fail upgrade ops fast on unresolvable componentDef or serviceVersion#10547
weicao wants to merge 4 commits into
mainfrom
bugfix/upgrade-ops-fail-invalid-inputs

Conversation

@weicao

@weicao weicao commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Problem

An Upgrade OpsRequest with deterministically invalid inputs — a nonexistent componentDefinitionName or a serviceVersion that no ComponentVersion release provides — never fails, and pollutes the cluster spec before the error even surfaces:

  1. upgradeOpsHandler.Action writes the invalid values into cluster.spec.componentSpecs[*].componentDef / serviceVersion and updates the Cluster.
  2. ReconcileAction -> getComponentDefMapWithUpdatedImages then returns a bare NotFound (from component.GetCompDefByName) or a bare "no matched image found for container ..." (from component.UpdateCompDefinitionImages4ServiceVersion).

Root cause

The ops framework only fails an OpsRequest on intctrlutil.NewFatalError; plain errors from Action/ReconcileAction are requeued indefinitely (pkg/operations/ops_manager.go:95-104, 184-192). The webhook-level validateUpgrade only checks semver syntax, so these deterministic failures are classified as retryable and the ops stays Running forever — with the cluster spec already polluted.

Changes (pkg/operations/upgrade.go)

  • upgradePreCheck (new, called at the top of Action): for each upgrade component, resolves the post-upgrade componentDef/serviceVersion and validates them before the cluster spec is patched, mirroring the switchoverPreCheck / rebuild-instance Action-phase Fatal-check idiom. Fatal on: component not found in cluster, componentDefinition not found, no image matching the serviceVersion.
  • getCompDefWithUpdatedImages (new, shared by both stages): classifies the deterministic resolution failures as intctrlutil.NewFatalErrorIsNotFound on the componentDefinition and "no matched image found" from image resolution. Name prefix / regex patterns are tolerated (compDefPatternMatched): if the name is a pattern matching existing componentDefinitions, the pre-check defers to the cluster controller's resolution, and the reconcile stage keeps the previous retry behavior (plain error) instead of failing.
  • getComponentDefMapWithUpdatedImages (ReconcileAction stage): now goes through the classifying helper, so a ComponentDefinition deleted after validation but before/during reconcile fails the ops instead of requeueing forever (belt for the validation-to-reconcile window).

Deliberately NOT changed

  • component.GetCompDefByName and resolveImagesWithCompVersions / UpdateCompDefinitionImages4ServiceVersion are untouched: they are shared by declarative reconcile paths where waiting on a not-yet-created object is correct. Error classification is done only at the ops consumption layer (consumer-side classification).
  • Webhook validateUpgrade untouched: existence checks belong in the controller pre-check, which also covers ops created while the webhook is bypassed.

Regression tests (RED-first, pkg/operations/upgrade_test.go)

Three new specs, verified to fail on origin/main code (fix stashed) and pass with the fix:

  1. non-existent componentDefinitionName -> ops Failed fast, and the cluster spec is asserted unchanged (Consistently still the original componentDef).
  2. unresolvable serviceVersion (99.99.99, valid semver, no matching release) -> ops Failed fast, cluster spec serviceVersion unchanged.
  3. ComponentDefinition deleted while the ops is Running -> Reconcile returns no error and the ops transitions to Failed (previously returned bare NotFound and requeued forever).

RED evidence (fix stashed, tests kept):

Summarizing 3 Failures:
  [FAIL] ... Test upgrade OpsRequest with a non-existent componentDefinitionName should fail fast without polluting the cluster spec
  [FAIL] ... Test upgrade OpsRequest with an unresolvable serviceVersion should fail fast without polluting the cluster spec
  [FAIL] ... Test upgrade OpsRequest should fail when the componentDefinition is deleted during reconciling
Ran 10 of 115 Specs in 35.612 seconds
FAIL! -- 7 Passed | 3 Failed

GREEN evidence (fix applied):

Ran 10 of 115 Specs in 18.919 seconds
SUCCESS! -- 10 Passed | 0 Failed
ok  github.com/apecloud/kubeblocks/pkg/operations         59.996s
ok  github.com/apecloud/kubeblocks/pkg/operations/custom   3.477s
ok  github.com/apecloud/kubeblocks/pkg/operations/util    15.540s

go vet ./pkg/operations/ and gofmt -l pkg/operations/ clean.

Fixes #10546

🤖 Generated with Claude Code

…r serviceVersion

An Upgrade OpsRequest referencing a nonexistent ComponentDefinition or a
serviceVersion with no matched image was retried forever: Action wrote the
invalid values into the cluster spec first, and ReconcileAction then surfaced
bare NotFound / "no matched image found" errors that the ops framework treats
as retryable.

- add an upgrade pre-check in the Action phase that verifies the desired
  componentDefinition exists (name prefix/regex patterns are tolerated, as the
  cluster controller resolves them) and that the desired serviceVersion
  resolves to images, BEFORE the cluster spec is patched.
- classify the same deterministic failures as fatal in the ReconcileAction
  stage to cover the window where the ComponentDefinition is deleted between
  validation and reconcile.
- the shared resolvers (component.GetCompDefByName,
  resolveImagesWithCompVersions) are intentionally untouched; classification
  happens only at the ops consumption layer.

Fixes #10546

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@weicao weicao requested review from a team and wangyelei as code owners July 5, 2026 18:48
@apecloud-bot

Copy link
Copy Markdown
Collaborator

Auto Cherry-pick Instructions

Usage:
  - /nopick: Not auto cherry-pick when PR merged.
  - /pick: release-x.x [release-x.x]: Auto cherry-pick to the specified branch when PR merged.

Example:
  - /nopick
  - /pick release-1.1

CLA Recheck Instructions

Usage:
  - /recheck-cla: Trigger a re-check of CLA status for this pull request.
Example:
  - /recheck-cla

@github-actions github-actions Bot added the size/L Denotes a PR that changes 100-499 lines. label Jul 5, 2026
@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 24.05063% with 120 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.57%. Comparing base (ecd604d) to head (2c2197f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controller/component/component_version.go 0.00% 107 Missing ⚠️
pkg/operations/upgrade.go 74.46% 9 Missing and 3 partials ⚠️
.../apps/cluster/transformer_cluster_normalization.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10547      +/-   ##
==========================================
- Coverage   63.68%   63.57%   -0.11%     
==========================================
  Files         519      519              
  Lines       62852    62895      +43     
==========================================
- Hits        40027    39987      -40     
- Misses      19202    19299      +97     
+ Partials     3623     3609      -14     
Flag Coverage Δ
unittests 63.57% <24.05%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pkg/operations/upgrade.go Outdated
}
// compDefName may be a name prefix or a regex pattern that will be resolved
// to a specific componentDefinition by the cluster controller.
matched, matchedErr := u.compDefPatternMatched(reqCtx, cli, compDefName)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This still misses the pattern/regex ComponentDefinition path. If the current or requested ComponentDefinition is a prefix/regex and the requested serviceVersion has no compatible ComponentDefinition, compDefPatternMatched returns true and upgradePreCheck lets Action update the Cluster spec anyway. Later getComponentDefMapWithUpdatedImages keeps returning the non-fatal “waiting for the componentDefinition ... to be resolved” error, so the OpsRequest is back to retrying forever with the Cluster spec already polluted. Please resolve the pattern together with the requested serviceVersion, using the same compatibility semantics as cluster normalization, before treating this as retryable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d0ce3db. The pattern path is no longer treated as retryable-by-default:

  • compDefPatternMatched is gone. When the name does not Get as a concrete ComponentDefinition, the new resolveCompDefFromPattern resolves the pattern together with the requested serviceVersion, mirroring the compatibility semantics of cluster normalization's resolveCompDefinitionNServiceVersion (listCompDefinitionsWithPattern + serviceVersionToCompDefinitions): list pattern-matched cmpds via component.PrefixOrRegexMatched, build the provided-serviceVersion -> cmpd map from each cmpd's own spec.serviceVersion plus the releases of component.CompatibleCompVersions4Definition whose compatibility rules match, filter with component.CompareServiceVersion, look up the requested version (or the latest when empty) and pick the latest matching cmpd. The shared helpers are untouched; classification stays consumer-side in upgrade.go.
  • A deterministic no-match (no matched component definition found with componentDef ... and serviceVersion ...) is intctrlutil.NewFatalError raised by upgradePreCheck in the Action, before the Cluster spec is written. Transient conditions (list errors, ComponentVersion unavailable/not observed) remain plain retryable errors.
  • ReconcileAction: getComponentDefMapWithUpdatedImages goes through the same resolution, so the non-fatal waiting for the componentDefinition ... to be resolved retry-forever branch is removed; a pattern that stops matching mid-flight (e.g. cmpds deleted) is fatal there too, via the same helper.

Tests (pkg/operations/upgrade_test.go):

  • negative: ops requests prefix pattern + serviceVersion 99.99.99 -> ops Failed at Action, Cluster spec Consistently unchanged. RED-verified against the previous upgrade.go (b3770b1): the spec fails with Expected <OpsPhase>: Creating to equal <OpsPhase>: Failed (timed out after 10s), i.e. the old code passed the precheck and kept retrying.
  • positive: same prefix + a supported serviceVersion passes the precheck, updates the Cluster spec, reconcile stays non-fatal while the pattern is not yet normalized, and the ops succeeds after the controller write-back is mimicked.

Full go test ./pkg/operations/... -count=1 green, go vet/gofmt clean.

… upgrade precheck

When the current or requested componentDefinition of an upgrade opsRequest is
a name prefix or regex pattern, the precheck only verified that the pattern
matched some existing componentDefinition. A requested serviceVersion with no
compatible componentDefinition then passed the precheck, polluted the cluster
spec, and left the opsRequest retrying the non-fatal 'waiting for the
componentDefinition ... to be resolved' error forever.

- resolve the pattern together with the requested serviceVersion in
  getCompDefWithUpdatedImages, mirroring the compatibility semantics of the
  cluster normalization (resolveCompDefinitionNServiceVersion) and reusing the
  shared helpers component.PrefixOrRegexMatched,
  component.CompatibleCompVersions4Definition and
  component.CompareServiceVersion; a deterministic no-match is a fatal error
  raised before the Action writes the cluster spec.
- getComponentDefMapWithUpdatedImages now resolves the pattern the same way in
  ReconcileAction, removing the retry-forever waiting branch: a pattern with no
  compatible componentDefinition mid-flight is fatal as well, while transient
  conditions (list errors, unavailable componentVersions) stay retryable.
- the shared resolution helpers are untouched; classification stays in the ops
  consumption layer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@weicao

weicao commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

/nopick

@apecloud-bot apecloud-bot added the nopick Not auto cherry-pick when PR merged label Jul 6, 2026
…ver in upgrade precheck

Extract resolveCompDefinitionNServiceVersion and its helpers from
controllers/apps/cluster into pkg/controller/component as the exported
ResolveCompDefinitionNServiceVersion, and replace the duplicated
pattern/serviceVersion resolution in the upgrade ops precheck with it.
The deterministic no-match failure is now signaled by the errors.Is-able
ErrCompDefinitionNServiceVersionUnmatched sentinel, which the ops layer
classifies as a fatal error.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Denotes a PR that changes 500-999 lines. and removed size/L Denotes a PR that changes 100-499 lines. labels Jul 6, 2026
@weicao

weicao commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 07c0158: extracted the cluster normalization's resolveCompDefinitionNServiceVersion (and its helpers) into pkg/controller/component as the exported ResolveCompDefinitionNServiceVersion, and replaced the precheck's duplicated resolveCompDefFromPattern/compatibleServiceVersions/compareServiceVersions (~140 lines) with calls to the shared resolver — the precheck and the cluster controller now share one implementation, so their compatibility semantics cannot drift. The deterministic no-match failure is signaled via the errors.Is-able ErrCompDefinitionNServiceVersionUnmatched sentinel and classified as fatal in the ops layer. Net -116 lines; pkg/controller/component, controllers/apps/cluster and pkg/operations suites pass locally.

Note: controllers/apps still has its own listCompDefinitionsWithPattern copy (used by shardingdefinition/componentversion controllers); consolidating that is left as a follow-up to keep this PR scoped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nopick Not auto cherry-pick when PR merged size/XL Denotes a PR that changes 500-999 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade OpsRequest with invalid componentDef/serviceVersion retries forever after polluting cluster spec

3 participants