Skip to content

fix(controller): patch CR metadata without owning spec (release-1.0)#10265

Closed
weicao wants to merge 1 commit into
release-1.0from
lily/metadata-only-cr-updates-release-1.0
Closed

fix(controller): patch CR metadata without owning spec (release-1.0)#10265
weicao wants to merge 1 commit into
release-1.0from
lily/metadata-only-cr-updates-release-1.0

Conversation

@weicao
Copy link
Copy Markdown
Contributor

@weicao weicao commented May 19, 2026

Summary

  • Backport the metadata-only patch behavior from fix(controller): patch CR metadata without owning spec #10264 to release-1.0.
  • Replace whole-object Update calls in finalizer handling with metadata-only merge patches.
  • Replace ComponentVersion supported ComponentDefinition metadata updates with a metadata-only merge patch.

Context

SQL Server addon in-place upgrade can fail with server-side apply field ownership conflicts after the controller reconciles chart-rendered definition objects. The conflicting fields are chart-owned spec fields, but whole-object controller updates can make apiserver managedFields record them under the kubeblocks manager.

This backport keeps controller writes scoped to metadata when the controller only changes finalizers, labels, or annotations. It preserves chart ownership of rendered spec fields during Helm upgrades.

Validation

  • go test ./pkg/controllerutil -run TestHandleCRDeletionPatchesFinalizerMetadata -count=1 -v
  • go test ./controllers/apps -run TestComponentVersionSupportedCompDefLabelsUsesMetadataPatch -count=1 -v
  • go test -c ./controllers/apps
  • go test -c ./pkg/controllerutil
  • git diff --check

Boundary

This PR is a release-1.0 backport of #10264. Live Helm in-place upgrade validation for the SQL Server addon will be run with a release-1.0 patch image after this PR head is available.

@weicao weicao requested a review from a team as a code owner May 19, 2026 12:41
@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

@github-actions github-actions Bot added the size/L Denotes a PR that changes 100-499 lines. label May 19, 2026
@weicao
Copy link
Copy Markdown
Contributor Author

weicao commented May 19, 2026

LGTM (peer review, 不能 formal approve 因为共享 GitHub identity).

Backport verification vs PR #10264 (main):

  • controllers/apps/componentversion_controller.go diff identical to main: client.Clientclient.Writer, MergeFrom(compVersion.DeepCopy()) patch, UpdatePatch
  • pkg/controllerutil/controller_common.go diff identical to main: two UpdatePatch sites with MergeFrom(cr.DeepCopyObject().(client.Object))
  • Cherry-pick clean (no manual conflict resolution) ✓
  • Diff stat controllers/apps/componentversion_controller.go +3/-2, pkg/controllerutil/controller_common.go +5/-2 matches main production scope
  • Tests directly ported from main; assertion semantics identical (patch-data contract: contains 'metadata', not 'spec')

Metadata clean:

  • title fix(controller): patch CR metadata without owning spec (release-1.0) ✓ no internal traces
  • body Summary / Context / Validation / Boundary 全 clean,无 Slock thread ID / @mention / case 编号 / msg= 引用
  • commit attribution scan clean (Wei Cao single author, no claude/anthropic)

Boundary: PR body 已写明 release-1.0 backport + live helm in-place upgrade validation 由 release-1.0 patch image 兜底(Tom mix-A-1 现场已 ready to rebuild from this head)。

CI 排队中,mergeStateStatus=BLOCKED 仅 formal review gate。

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.82%. Comparing base (2807dae) to head (ff76afc).

Files with missing lines Patch % Lines
pkg/controllerutil/controller_common.go 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release-1.0   #10265      +/-   ##
===============================================
+ Coverage        53.78%   53.82%   +0.04%     
===============================================
  Files              493      493              
  Lines            55343    55346       +3     
===============================================
+ Hits             29765    29789      +24     
+ Misses           22608    22578      -30     
- Partials          2970     2979       +9     
Flag Coverage Δ
unittests 53.82% <66.66%> (+0.04%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

@weicao
Copy link
Copy Markdown
Contributor Author

weicao commented May 19, 2026

Live verification — PASS (release-1.0 patch image + idc2 sqlserver vcluster, mix-A-1 v18 in-place helm upgrade scenario)

  • patch image 4-field identity: tag docker.io/apecloud/kubeblocks:pr10265-ff76afcb, imageID sha256:45d08d6c918502548944fbde0b26b3108934ba688370c100e5d6c0272a98c4dc, pod startTime 2026-05-19T13:08:42Z, source commit ff76afcbcdb96248ccfb1b273bb55033a0681b3b (this PR head)
  • controller boot: 21/21 controllers reached Starting workers, including componentversion + componentdefinition (binary × CRD set match, no no matches for kind warnings)
  • helm install (chart 1.0.4-with-fix1268) → REVISION=1 deployed, reconcile OK
  • helm upgrade in-place to chart 1.0.5 → STATUS=deployed, REVISION=2, rc=0, 5s, NO SSA conflict (previously failed with 4 Apply failed with 1 conflict errors on .spec.releases and .spec.actions[*].workload.podSpec.containers)
  • managedFields snapshots taken before + after helm upgrade on all 4 target objects (cmpv/mssql + opsdef/mssql-dynamic-modify-member + opsdef/mssql-dynamic-remove-ag + opsdef/mssql-dynamic-remove-member)

managedFields ownership delta — kubeblocks no longer claims any f:spec.*:

Object f:spec under helm f:spec under kubeblocks
cmpv/mssql compatibilityRules, releases (none)
opsdef/mssql-dynamic-modify-member actions, parametersSchema, podInfoExtractors (none)
opsdef/mssql-dynamic-remove-ag actions, parametersSchema, podInfoExtractors (none)
opsdef/mssql-dynamic-remove-member actions, parametersSchema, podInfoExtractors (none)

Evidence pack: 13 files including identity, helm install/upgrade logs with rc, 4 before-*.json + 4 after-*.json managedFields snapshots, and CLOSEOUT-v1-pr10265-VERIFIED.md. tar sha256 de934f4f0179eaed4afb19fcc717ee08bb08bea3ffdded0189bfa1d522ef98a4.

PR #10265 mechanism proven live; PR body's Boundary item ("Live Helm in-place upgrade validation should be run with a patch image") is satisfied.

@weicao
Copy link
Copy Markdown
Contributor Author

weicao commented May 19, 2026

Closing this manual release-1.0 backport to avoid a duplicate long-lived PR.

The main PR is #10264. Backports should be generated from the main PR via /pick release-1.1 release-1.0; #10265 was only used as a temporary release-1.0 patch-image validation branch because the main-head controller image did not match the release-1.0 vcluster CRD set.

The live validation evidence remains useful and is already cross-referenced from #10264. If auto-pick fails, this branch can be used as fallback evidence / reference, but it should not stay open in parallel with the normal backport flow.

@weicao weicao closed this May 19, 2026
@github-actions github-actions Bot added this to the Release 1.2.0 milestone May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants