Skip to content

chore: improve controllers dataprotection coverage - #10381

Merged
leon-ape merged 1 commit into
mainfrom
support/coverage-controllers-dataprotection
Jun 16, 2026
Merged

chore: improve controllers dataprotection coverage#10381
leon-ape merged 1 commit into
mainfrom
support/coverage-controllers-dataprotection

Conversation

@leon-ape

@leon-ape leon-ape commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add deterministic Ginkgo/Gomega coverage for controllers/dataprotection helper and controller branches.
  • Cover backup annotation helpers, backup policy driver env/encryption/selector helpers, backup policy template compatibility mapping, GC decisions, storage provider dependency tracking, and volume populator selector/system-account validation paths.
  • Fix GC deletability so expired Running/New non-terminal backups are not deleted before completion.
  • Fix encrypted system-account backup annotation collection so ordinary cluster Secrets with username/password are not captured, while preserving real component and sharding system-account Secret shapes.

Coverage

  • Before: 77.0% combined statement coverage for controllers/dataprotection.
  • After: 80.1% combined statement coverage for controllers/dataprotection.

Production Code Changes

  • GCReconciler.isBackupDeletable now returns false for non-terminal backup phases such as Running.
  • Trigger case: a backup can have status.expiration while still Running.
  • Previous behavior: GC treated any non-Completed backup as deletable once expired, which could delete an in-flight backup and its workload.
  • Expected behavior: retention GC should only delete terminal backups; Completed keeps the existing policy checks and Failed remains deletable.
  • setEncryptedSystemAccountsAnnotation now requires an explicit system-account identity before capturing credentials: component Secrets use the apps.kubeblocks.io/system-account label, and sharding Secrets must match the real generated <cluster>-<sharding>-<account> Secret name with sharding labels.
  • Trigger case: an ordinary cluster-labeled Secret carrying username/password was eligible for encrypted system-account backup annotation capture.
  • Previous behavior: unrelated credentials could be copied into the Backup annotation and restored as system-account Secrets.
  • Expected behavior: only real component or sharding system-account Secrets are captured.

Tests

  • GOFLAGS=-mod=mod GOCACHE= KUBEBUILDER_ASSETS= go test -short ./controllers/dataprotection -coverprofile=

@leon-ape leon-ape added the nopick Not auto cherry-pick when PR merged label Jun 15, 2026
@github-actions github-actions Bot added the size/XL Denotes a PR that changes 500-999 lines. label Jun 15, 2026
@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

@leon-ape
leon-ape force-pushed the support/coverage-controllers-dataprotection branch from 84930da to c6ffb32 Compare June 15, 2026 15:21
@leon-ape

Copy link
Copy Markdown
Collaborator Author

/approve

@leon-ape
leon-ape marked this pull request as ready for review June 15, 2026 15:37
@leon-ape
leon-ape requested review from a team, ldming and wangyelei as code owners June 15, 2026 15:37
@apecloud-bot apecloud-bot added the approved PR Approved Test label Jun 15, 2026
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.02%. Comparing base (93ec22c) to head (3e4abec).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10381      +/-   ##
==========================================
+ Coverage   53.26%   57.02%   +3.76%     
==========================================
  Files         533      533              
  Lines       63529    63541      +12     
==========================================
+ Hits        33839    36235    +2396     
+ Misses      26249    23864    -2385     
- Partials     3441     3442       +1     
Flag Coverage Δ
unittests 57.02% <100.00%> (+3.76%) ⬆️

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.


deletable, err := reconciler.isBackupDeletable(reqCtx, pendingBackup)
Expect(err).NotTo(HaveOccurred())
Expect(deletable).To(BeTrue())

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[P1] Do not codify running backups as GC-deletable. A normal Backup gets status.expiration when it moves to Running, so a run that exceeds its retention period can reach this path and be deleted by GC before it completes, including its backup Job/StatefulSet. Retention should only expire terminal backups, or an explicitly failed/stale state with a separate contract; this test locks in a data-protection behavior that can interrupt an in-flight backup.

@leon-ape
leon-ape force-pushed the support/coverage-controllers-dataprotection branch from c6ffb32 to bcd11a4 Compare June 16, 2026 00:51
@apecloud-bot apecloud-bot removed the approved PR Approved Test label Jun 16, 2026
ObjectMeta: metav1.ObjectMeta{
Namespace: cluster.Namespace,
Name: "component-account",
Labels: constant.GetCompLabels(cluster.Name, testdp.ComponentName),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[P1] This fixture only has generic cluster/component labels plus username/password data, but the test expects it to be captured as a system account. That locks in the current broad matching in setEncryptedSystemAccountsAnnotation: any cluster-labeled Secret with those two keys is copied into the Backup annotation and can later be restored as a system-account Secret, which can capture unrelated credentials. Please require an explicit system-account identity (for example the system-account label/name/spec) and add a negative case for ordinary cluster Secrets carrying username/password.

@leon-ape
leon-ape force-pushed the support/coverage-controllers-dataprotection branch from bcd11a4 to 0051608 Compare June 16, 2026 01:11
@leon-ape

Copy link
Copy Markdown
Collaborator Author

/approve

@apecloud-bot apecloud-bot added the approved PR Approved Test label Jun 16, 2026
usernameKey := constant.AccountNameForSecret
passwordKey := constant.AccountPasswdForSecret
isSystemAccountSecret := func(secret *corev1.Secret) bool {
if secret.Labels[systemAccountSecretLabel] == "" {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[P1] This label gate drops real sharding system-account Secrets from the backup annotation. The sharding account transformer creates those Secrets with the cluster/sharding labels and username/password data, but it does not add apps.kubeblocks.io/system-account; the new test manually adds that label to the sharding fixture, so it does not exercise the real generated Secret shape. As a result, sharded-cluster backups stop preserving sharding account passwords, and restore can recreate or leave system-account Secrets whose passwords do not match the restored database accounts. The fix needs to either add this explicit system-account identity in the real sharding account generation path before relying on the label, or keep a precise sharding-account identification path and cover that generated Secret shape in the test.

@leon-ape
leon-ape force-pushed the support/coverage-controllers-dataprotection branch from 0051608 to 3e4abec Compare June 16, 2026 01:23
@apecloud-bot apecloud-bot removed the approved PR Approved Test label Jun 16, 2026
@leon-ape

Copy link
Copy Markdown
Collaborator Author

/approve

@apecloud-bot apecloud-bot added the approved PR Approved Test label Jun 16, 2026
@leon-ape
leon-ape merged commit c121a23 into main Jun 16, 2026
50 of 52 checks passed
@leon-ape
leon-ape deleted the support/coverage-controllers-dataprotection branch June 16, 2026 02:04
@github-actions github-actions Bot added this to the Release 1.2.0 milestone Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved PR Approved Test 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.

2 participants