Preserve PVCs during disable DR - #2638
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (12)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extracts CephFS CG PVC disownership logic into package-level helpers, adds optional disownership control to ChangesPVC disownership control
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/cephfscg/cghandler_test.go (1)
91-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-skip disownership path.
These updated call sites only exercise
skipPVCDisownership=true. Please add a focused test with a VSHandler, an RS/RD-owned PVC, andskipPVCDisownership=falseto assert the PVC owner reference is removed before RGS/RGD deletion.Also applies to: 207-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/cephfscg/cghandler_test.go` at line 91, The current tests only cover the skipPVCDisownership=true path in the DeleteRGS/DeleteRGD call sites. Add a focused test around the existing cghandler_test coverage that uses a VSHandler with an RS/RD-owned PVC and passes skipPVCDisownership=false, then assert the PVC owner reference is cleared before the RGS/RGD deletion proceeds. Use the existing DeleteRGD (and matching DeleteRGS if applicable) test setup and verify the disownership behavior explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/cephfscg/cghandler.go`:
- Around line 781-790: The disownership flow in the cleanup path is being
silently skipped when skipPVCDisownership is false but vsHandler is nil, which
lets the delete continue without the requested owner-reference cleanup. Update
the logic in the cghandler cleanup routine (the block that iterates
rgsList.Items and calls disownRSManagedPVCs) to fail fast with an error when
disownership is requested but no VSHandler is available, and apply the same
guard to the related delete path noted in the companion block.
---
Nitpick comments:
In `@internal/controller/cephfscg/cghandler_test.go`:
- Line 91: The current tests only cover the skipPVCDisownership=true path in the
DeleteRGS/DeleteRGD call sites. Add a focused test around the existing
cghandler_test coverage that uses a VSHandler with an RS/RD-owned PVC and passes
skipPVCDisownership=false, then assert the PVC owner reference is cleared before
the RGS/RGD deletion proceeds. Use the existing DeleteRGD (and matching
DeleteRGS if applicable) test setup and verify the disownership behavior
explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 706f9001-be27-4952-b6f4-5c3da231d4c6
📒 Files selected for processing (3)
internal/controller/cephfscg/cghandler.gointernal/controller/cephfscg/cghandler_test.gointernal/controller/vrg_volsync.go
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: test
- GitHub Check: drenv (ubuntu-24.04, 3.10)
- GitHub Check: drenv (ubuntu-24.04, 3.12)
- GitHub Check: drenv (ubuntu-24.04, 3.11)
- GitHub Check: drenv (ubuntu-24.04, 3.13)
- GitHub Check: drenv (ubuntu-24.04, 3.14-dev)
- GitHub Check: Unit tests
- GitHub Check: Build image
- GitHub Check: Golangci Lint (.)
- GitHub Check: Go compatibility
🔇 Additional comments (2)
internal/controller/cephfscg/cghandler.go (1)
268-268: LGTM!Also applies to: 583-583, 819-885
internal/controller/vrg_volsync.go (1)
900-904: LGTM!Also applies to: 962-1000
| if !skipPVCDisownership && vsHandler != nil { | ||
| for i := range rgsList.Items { | ||
| rgs := &rgsList.Items[i] | ||
| if len(rgs.Spec.RSSpec) > 0 { | ||
| if err := disownRSManagedPVCs(ctx, k8sClient, vsHandler, rgs.Spec.RSSpec, logger); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail fast when disownership is requested without a VSHandler.
When skipPVCDisownership is false, the caller is explicitly requesting owner-reference cleanup before deleting RGS/RGD. The current && vsHandler != nil branch silently skips that cleanup and still deletes the group resources, which can preserve the same PVC GC race this PR is trying to fix.
Proposed fix
- if !skipPVCDisownership && vsHandler != nil {
+ if !skipPVCDisownership {
for i := range rgsList.Items {
rgs := &rgsList.Items[i]
if len(rgs.Spec.RSSpec) > 0 {
+ if vsHandler == nil {
+ return fmt.Errorf("VSHandler is required to disown RS-managed PVCs before deleting RGS")
+ }
if err := disownRSManagedPVCs(ctx, k8sClient, vsHandler, rgs.Spec.RSSpec, logger); err != nil {
return err
}
}
}
}- if !skipPVCDisownership && vsHandler != nil {
+ if !skipPVCDisownership {
for i := range rgdList.Items {
rgd := &rgdList.Items[i]
if len(rgd.Spec.RDSpecs) > 0 {
+ if vsHandler == nil {
+ return fmt.Errorf("VSHandler is required to disown RD-managed PVCs before deleting RGD")
+ }
if err := disownRDManagedPVCs(ctx, k8sClient, vsHandler, rgd.Spec.RDSpecs, logger); err != nil {
return err
}
}
}
}Also applies to: 805-814
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/cephfscg/cghandler.go` around lines 781 - 790, The
disownership flow in the cleanup path is being silently skipped when
skipPVCDisownership is false but vsHandler is nil, which lets the delete
continue without the requested owner-reference cleanup. Update the logic in the
cghandler cleanup routine (the block that iterates rgsList.Items and calls
disownRSManagedPVCs) to fail fast with an error when disownership is requested
but no VSHandler is available, and apply the same guard to the related delete
path noted in the companion block.
We don't check that the PVC is not deleted in e2e? I remember that we added a check for application health - after disable dr we wait until the workload is healthy, and this should fail quickly if the pvc is deleted. If this is not the case, please open an issue for e2e. We must verify this in the tests to avoid regressions. |
eebf653 to
0f4c3e8
Compare
PR RamenDR#2602 skipped PVC disownership whenever the VRG was deleting, which is correct for workload teardown but breaks disable DR: with do-not-delete-pvc set, PVCs must survive while VRG/RS/RD/RGS/RGD are removed. Fixes disable DR regression where PVCs entered Terminating because RS ownership was retained and deleted with the ReplicationSource. Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>
0f4c3e8 to
51522a8
Compare
Align VolSync unprotect error handling with VolRep so VRG finalize retries disown before ReplicationSource deletion. Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>
|
@raaizik Do you think the fix could simply be to remove this block that was added some time ago? It seems like that will fix the problem. ramen/internal/controller/volsync/vshandler.go Lines 3272 to 3276 in 0e17c0d |
@BenamarMk I added that logic in #2504 (428539a) because clearing all owners caused PVC leftovers when RS was deleted with skip=true. So just removing the block might fix disable DR but re-trigger that. This PR still matters for cleanup paths that don’t go through disownPVCs first. |
Thanks @raaizik for the explanation. However, the function The PVC
If you believe that this PR still matters, then it should be opened against the right bug. |
@BenamarMk, @ELENAGER hypothesized that the root cause for the bug in question is that #2602 introduced a regression where skipPVCDisownership was effectively always true during VRG delete, so cleanupRS/DeleteRGS skipped disown even with the annotation. You have a different take - that's fine. But it should be discussed with her as well as she's the one who identified it. |
3 participants
PR #2602 skipped PVC disownership whenever the VRG was deleting, which is correct for workload teardown but breaks disable DR: with do-not-delete-pvc set, PVCs must survive while VRG/RS/RD/RGS/RGD are removed.
Fixes disable DR regression where PVCs entered Terminating because RS ownership was retained and deleted with the ReplicationSource.
Test results
E2E
Manual checks shown for managed apps (same results for discovered apps)
Result
deletionTimestampon dr1 and dr2skipPVCDisownership=falseduring disable. QE's must gather logs pre-fix ignored annotation, skipped PVC disownership:TODO