Skip to content

fix: wait before recreating restore account secrets#10575

Open
weicao wants to merge 3 commits into
mainfrom
bugfix/dp-restore-sharding-secret-race
Open

fix: wait before recreating restore account secrets#10575
weicao wants to merge 3 commits into
mainfrom
bugfix/dp-restore-sharding-secret-race

Conversation

@weicao

@weicao weicao commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #10574.

What changed

  • When restoring system account secrets, immutable secrets with mismatched credentials are no longer deleted and recreated in the same reconcile.
  • The restore path now removes the KubeBlocks cluster finalizer from the stale immutable account secret when present, deletes or waits for deletion, and returns a requeue error so the next reconcile creates the restored secret after the old object is gone.
  • Added focused coverage for component immutable replacement, sharding account replacement with cluster.kubeblocks.io/finalizer, and an already deletion-stamped sharding secret.

Why

For sharded restore, the apps controller can create an immutable generated sharding account secret before DataProtection restores the backed-up credential. If DataProtection deletes that immutable secret and immediately creates the same name, Kubernetes can keep the old object in Terminating due to the cluster finalizer, causing repeated object is being deleted ... already exists errors and blocking PVC restore.

Validation

  • make test-go-generate
  • go test ./controllers/dataprotection -run 'TestRestoreSystemAccountSecrets|Test.*SystemAccountSecret'

Boundary

  • Full go test ./controllers/dataprotection did not run locally because envtest cannot start: /usr/local/kubebuilder/bin/etcd is missing on this machine.
  • Runtime validation still needs the Valkey CT11 focused restore rerun with this patch image before removing draft status.

@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 7, 2026
@weicao weicao added the nopick Not auto cherry-pick when PR merged label Jul 7, 2026
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.66%. Comparing base (fa7ed3b) to head (ed4c9c0).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
...llers/dataprotection/volumepopulator_controller.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10575      +/-   ##
==========================================
+ Coverage   63.63%   63.66%   +0.03%     
==========================================
  Files         519      519              
  Lines       62849    62836      -13     
==========================================
+ Hits        39992    40005      +13     
+ Misses      19235    19218      -17     
+ Partials     3622     3613       -9     
Flag Coverage Δ
unittests 63.66% <66.66%> (+0.03%) ⬆️

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.

@weicao
weicao marked this pull request as ready for review July 16, 2026 04:31
@weicao
weicao requested review from a team, ldming and wangyelei as code owners July 16, 2026 04:31
@weicao

weicao commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Runtime validation for the production change was already completed by the original Valkey reporter/test owner.

Validated production commit: fdc90d59c8ecd314e3d40a8d1903f6754c60384d

The current PR head 27a32c7add6102a5001326f8b13e4971eeebcb4b only adds a test-tolerance follow-up on top of that production commit; it does not change the controller implementation.

Focused validation:

  • DP image: docker.io/library/kubeblocks-dataprotection:pr10575-fdc90d59-clara
  • live imageID: sha256:5405a2061912f986c7e234ffb0803fd09dac5d84e55d3a228218a19658882644
  • namespace: valkey-ct11-oracle-pr10575
  • runner: PASS:13 FAIL:0 SKIP:0, rc=0
  • the terminating/AlreadyExists secret-race grep count was 0
  • the restore path advanced past the former secret blocker and reached the expected unsupported sharding-restore refusal path

Evidence:

  • root manifest SHA256: f61aba3cec19bf60034bfc22317503f467044928288bf83c2c466c578d000de6
  • final-evidence manifest SHA256: 28dd6bddb978b56bd26a0e9c13e79a3745932e7e8e2784e13211a90070cb47de

Boundary: this validates removal of the account-Secret deletion/recreation blocker for this focused scenario. It is not a claim that sharded datafile restore is supported or that the full release suite is ready.

Comment thread controllers/dataprotection/volumepopulator_controller.go Outdated
constant.AccountNameForSecret: []byte(accountName),
constant.AccountPasswdForSecret: password,
},
if controllerutil.RemoveFinalizer(secret, constant.DBClusterFinalizerName) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] DataProtection must not remove an Apps-owned finalizer to force replacement of this Secret. This already fails for component-owned account Secrets, which carry component.kubeblocks.io/finalizer rather than this cluster finalizer and therefore remain Terminating forever. Teaching DP about that second finalizer would deepen the same ownership violation. Secret replacement needs an explicit API and ownership contract with the Apps controller; it must not depend on or bypass Apps internal finalizer implementations.

}
if err := r.Client.Create(reqCtx.Ctx, secret); err != nil {
uid := secret.UID
if err := r.Client.Delete(reqCtx.Ctx, secret, client.Preconditions{UID: &uid}); err != nil && !apierrors.IsNotFound(err) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] The UID precondition protects a newly created same-name Secret, but the resulting Conflict does not guarantee convergence. This ordinary error is treated as a successful reconcile by the outer handler once the PVC has its existing condition, and the controller does not watch Secret changes. A cached stale object, UID mismatch, or create race can therefore leave restoration stopped after preserving the replacement. Handle these identity races as an explicit requeue or converged replacement state, and test the complete outer reconcile path.

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/L Denotes a PR that changes 100-499 lines.

Projects

None yet

3 participants