e2e: assert PVC state after Disable DR - #2675
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 (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (13)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe PR adds PVC validation to deployment and VM health checks. It classifies missing, lost, or deleting PVCs as unrecoverable and stops retries for those errors. PVC health validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WaitWorkloadHealth
participant WorkloadHealth
participant checkPVCHealth
participant PVCAPI
WaitWorkloadHealth->>WorkloadHealth: check workload health
WorkloadHealth->>checkPVCHealth: validate workload PVC
checkPVCHealth->>PVCAPI: retrieve PVC
PVCAPI-->>checkPVCHealth: return PVC state
checkPVCHealth-->>WorkloadHealth: return health result
WorkloadHealth-->>WaitWorkloadHealth: return success or classified error
WaitWorkloadHealth-->>WaitWorkloadHealth: stop retrying unrecoverable errors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
c6373b6 to
2336d62
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@e2e/dractions/retry.go`:
- Around line 237-240: Update the error path after util.Sleep in the retry flow
to return or wrap the sleep error when cancellation or timeout occurs, rather
than using assertErr as the primary error. Preserve the existing workload,
namespace, and cluster context, and optionally include assertErr as supplemental
context.
In `@e2e/workloads/pvc.go`:
- Around line 38-54: Update isDRManagedOwnerReference to require both the owner
reference APIVersion and Kind: match VolumeReplicationGroup only with
ramendr.openshift.io/v1alpha1, and ReplicationSource or ReplicationDestination
only with volsync.backube/v1alpha1. Preserve returning false for all other
version/Kind combinations.
🪄 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: 1150baaa-8a38-45b0-a00b-ceb1c73cc77c
📒 Files selected for processing (7)
e2e/dractions/actions.goe2e/dractions/retry.goe2e/recipes/recipes_test.goe2e/types/types.goe2e/workloads/deploy.goe2e/workloads/pvc.goe2e/workloads/vm.go
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: test
- GitHub Check: ramendev (ubuntu-24.04, 3.10)
- GitHub Check: drenv (ubuntu-24.04, 3.12)
- GitHub Check: drenv (ubuntu-24.04, 3.13)
- GitHub Check: drenv (ubuntu-24.04, 3.10)
- GitHub Check: drenv (ubuntu-24.04, 3.11)
- GitHub Check: ramendev (ubuntu-24.04, 3.13)
- GitHub Check: drenv (ubuntu-24.04, 3.14-dev)
- GitHub Check: Build image
- GitHub Check: E2E unit tests
- GitHub Check: Golangci Lint (api)
- GitHub Check: Go compatibility
- GitHub Check: Golangci Lint (.)
- GitHub Check: Unit tests
- GitHub Check: Golangci Lint (e2e)
- GitHub Check: drenv (macos-latest, 3.13)
- GitHub Check: Linters
🔇 Additional comments (7)
e2e/types/types.go (1)
96-98: LGTM!e2e/workloads/pvc.go (1)
15-36: LGTM!e2e/workloads/deploy.go (1)
164-167: LGTM!e2e/workloads/vm.go (1)
130-133: LGTM!e2e/recipes/recipes_test.go (1)
186-188: LGTM!e2e/dractions/retry.go (1)
219-236: LGTM!Also applies to: 241-242
e2e/dractions/actions.go (1)
134-145: LGTM!
| if err := util.Sleep(ctx.Context(), util.RetryInterval); err != nil { | ||
| return fmt.Errorf("workload \"%s/%s\" PVCs are not preserved in cluster %q: %w", | ||
| ctx.AppNamespace(), w.GetAppName(), cluster.Name, assertErr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve retry cancellation errors.
When util.Sleep fails because the context is canceled or times out, the returned error wraps the previous assertion error instead of the sleep error. Return the sleep error while optionally retaining the last assertion as context.
Proposed fix
if err := util.Sleep(ctx.Context(), util.RetryInterval); err != nil {
- return fmt.Errorf("workload \"%s/%s\" PVCs are not preserved in cluster %q: %w",
- ctx.AppNamespace(), w.GetAppName(), cluster.Name, assertErr)
+ return fmt.Errorf("waiting for workload \"%s/%s\" PVC preservation in cluster %q was interrupted: %w (last assertion: %v)",
+ ctx.AppNamespace(), w.GetAppName(), cluster.Name, err, assertErr)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := util.Sleep(ctx.Context(), util.RetryInterval); err != nil { | |
| return fmt.Errorf("workload \"%s/%s\" PVCs are not preserved in cluster %q: %w", | |
| ctx.AppNamespace(), w.GetAppName(), cluster.Name, assertErr) | |
| } | |
| if err := util.Sleep(ctx.Context(), util.RetryInterval); err != nil { | |
| return fmt.Errorf("waiting for workload \"%s/%s\" PVC preservation in cluster %q was interrupted: %w (last assertion: %v)", | |
| ctx.AppNamespace(), w.GetAppName(), cluster.Name, err, assertErr) | |
| } |
🤖 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 `@e2e/dractions/retry.go` around lines 237 - 240, Update the error path after
util.Sleep in the retry flow to return or wrap the sleep error when cancellation
or timeout occurs, rather than using assertErr as the primary error. Preserve
the existing workload, namespace, and cluster context, and optionally include
assertErr as supplemental context.
| for _, ref := range pvc.OwnerReferences { | ||
| if isDRManagedOwnerReference(ref) { | ||
| return fmt.Errorf("pvc \"%s/%s\" still owned by %s %q in cluster %q", | ||
| namespace, pvcName, ref.Kind, ref.Name, cluster.Name) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func isDRManagedOwnerReference(ref metav1.OwnerReference) bool { | ||
| switch ref.Kind { | ||
| case "VolumeReplicationGroup", "ReplicationSource", "ReplicationDestination": | ||
| return true | ||
| default: | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate relevant files and symbols"
git ls-files | rg '(^|/)pvc\.go$|pvc' | sed -n '1,120p'
echo
echo "Find OwnerReference helpers/API group/DR constants"
rg -n 'isDRManagedOwnerReference|OwnerRef|VolumeReplicationGroup|ReplicationSource|ReplicationDestination|replication' . --glob '!vendor/**' --glob '!*.sum' --glob '!*.lock' | sed -n '1,200p'
echo
echo "Inspect e2e/workloads/pvc.go around helper"
sed -n '1,120p' e2e/workloads/pvc.go
echo
echo "Check imports/API constants files"
rg -n 'replicationv1alpha1|replication.*Group|GroupVersion|VolumeReplicationGroup|ReplicationSource|ReplicationDestination|rclone|ReplicationDestination|ReplicationSource' . --glob '!vendor/**' --glob '!*.sum' --glob '!*.lock' | sed -n '1,240p'Repository: RamenDR/ramen
Length of output: 47146
🌐 Web query:
VolSync VolumeReplicationGroup ReplicationSource ReplicationDestination APIVersion groupKind
💡 Result:
In VolSync, the API version and groupKind for the relevant custom resources are as follows: The API group for VolSync is volsync.backube [1][2][3]. The supported API version for both ReplicationSource and ReplicationDestination is v1alpha1 [1][4][2]. Therefore, the groupKind and APIVersion configuration for these resources are: ReplicationSource - apiVersion: volsync.backube/v1alpha1 [2][3][5] - Kind: ReplicationSource [1][2][6] ReplicationDestination - apiVersion: volsync.backube/v1alpha1 [2][3][5] - Kind: ReplicationDestination [4][2] Note that while early design documentation may have referenced different API groups [7], the established and current implementation uses volsync.backube [1][4][2][3]. These resources are used to configure asynchronous volume replication between persistent volumes in Kubernetes environments [8][9]. When using the VolSync volume populator, a PersistentVolumeClaim may reference a ReplicationDestination via its dataSourceRef field, where the apiGroup must be specified as volsync.backube and the kind as ReplicationDestination [10][11].
Citations:
- 1: https://github.com/backube/volsync/blob/main/config/crd/bases/volsync.backube_replicationsources.yaml
- 2: https://github.com/backube/volsync/blob/f13b9b82/bundle/manifests/volsync.clusterserviceversion.yaml
- 3: https://volsync.readthedocs.io/en/stable/usage/cli/replication.html
- 4: https://github.com/backube/volsync/blob/f13b9b82/helm/volsync/templates/volsync.backube_replicationdestinations.yaml
- 5: https://volsync.readthedocs.io/en/latest/usage/cli/replication.html
- 6: https://github.com/backube/volsync/blob/f13b9b82/api/v1alpha1/replicationsource_types.go
- 7: https://volsync.readthedocs.io/en/stable/design/custom-resources.html
- 8: https://volsync.readthedocs.io/
- 9: https://github.com/backube/volsync
- 10: https://volsync.readthedocs.io/en/stable/usage/volume-populator/index.html
- 11: https://volsync.readthedocs.io/en/latest/usage/volume-populator/index.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Ramen GroupVersion definition"
rg -n 'var GroupVersion|GroupVersion.*=|GroupVersion\.String' internal/api internal ramendr -g '*.go' --glob '!vendor/**' | sed -n '1,120p'
echo
echo "VolSync owner references in VRG VolSync code"
rg -n 'OwnerReference|APIVersion:.*ramendr|ReplicationSource|ReplicationDestination|volsync' internal/controller/vrg_vol*.go internal/controller/vrg_volrep.go --glob '!vendor/**' | sed -n '1,240p'
echo
echo "pvc.go imports and helper context"
cat -n e2e/workloads/pvc.goRepository: RamenDR/ramen
Length of output: 1562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "List relevant controller Go files"
fd -e go '^(cmd|internal|e2e)' . | sed -n '1,120p'
echo
echo "Find ramen package import and GroupType definitions"
rg -n 'GroupVersion \( = schema\.GroupVersion\{|= scheme\.BuildGroupVersion|name.*volumereplicationgroup|ramendrv1alpha1' . --glob '*.go' --glob '!vendor/**' | sed -n '1,200p'
echo
echo "Inspect VRG PVC owner reference construction"
rg -n 'OwnerReference|Kind: "VolumeReplicationGroup"|Kind: "ReplicationSource"|Kind: "ReplicationDestination"' internal e2e --glob '*.go' --glob '!vendor/**' -A3 -B5 | sed -n '1,260p'
echo
echo "Inspect e2e/workloads/pvc.go"
cat -n e2e/workloads/pvc.goRepository: RamenDR/ramen
Length of output: 43891
Match DR-managed owners by API version and Kind.
ReplicationSource and ReplicationDestination are VolSync CRDs under volsync.backube/v1alpha1, while VRG is ramendr.openshift.io/v1alpha1; matching by bare Kind can reject PVCs owned by a non-DR kind that reuses the same names. Compare both APIVersion and Kind.
🤖 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 `@e2e/workloads/pvc.go` around lines 38 - 54, Update isDRManagedOwnerReference
to require both the owner reference APIVersion and Kind: match
VolumeReplicationGroup only with ramendr.openshift.io/v1alpha1, and
ReplicationSource or ReplicationDestination only with volsync.backube/v1alpha1.
Preserve returning false for all other version/Kind combinations.
3b2f2bb to
63e3d95
Compare
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Issues
-
PVC health is checked only during Disable DR. PVC health matters after deployment, failover, and relocate too. The existing
Health()method is already called in all these flows but does not check PVCs. A PVC being Bound is part of application health. -
Deleted PVCs cause the test to wait until timeout.
WaitPVCPreservedretries until the context times out. If the PVC is being deleted or Lost, retrying is pointless. -
New interface method is unnecessary and inconsistent. Adding
AssertPVCPreserved()to theWorkloadinterface creates a parallel health-checking path, requires stub implementations in unrelated code (NoHooks), and duplicates theWaitWorkloadHealthretry structure. The name is also inconsistent —HealthandStatusare short nouns, whileAssertPVCPreservedis an imperative with implementation details. Interface methods should match the existing style. -
Health check changed from assert to wait.
DisableProtectioncallsctx.Workload().Health()once — fail immediately if unhealthy. This change replaces it withdeployers.WaitWorkloadHealth()which retries until timeout. After Disable DR, the workload was never touched — it should already be healthy. Waiting will not help. -
Owner reference check is redundant. By the time we check, the VRG and VolSync resources are already deleted. If they had owner references on the PVC, garbage collection would have deleted the PVC. Checking PVC phase is sufficient. If the test fails, we gather the PVC which will reveal the owner reference as the root cause.
-
Owner reference check prevents reuse in other flows. Checking owner references means the check cannot be used after failover or relocate, where PVCs may legitimately have DR owner references.
Suggested approach
Add PVC health check inside Deployment.Health() and VM.Health(). This way every caller that already checks workload health (deploy, enable protection, failover, relocate, disable protection) also validates PVC health.
Handle the different PVC states:
- Bound — healthy, return no error.
- Lost or DeletionTimestamp set — unrecoverable, return an unrecoverable error so the retry loop stops immediately.
- Pending — temporary, return a regular error so the retry loop retries. This also means
DisableProtection, which callsHealth()once, will fail immediately on a Pending PVC — the right behavior since the PVC should already be Bound at that point.
Introduce an unrecoverable error sentinel so WaitWorkloadHealth can stop immediately instead of waiting for the test timeout:
var ErrUnrecoverable = errors.New("unrecoverable error")When detecting an unrecoverable error, wrap the sentinel:
return fmt.Errorf("pvc %q is being deleted in cluster %q: %w",
pvcName, cluster.Name, ErrUnrecoverable)In WaitWorkloadHealth, stop retrying on unrecoverable errors:
for {
err := w.Health(ctx, cluster)
if err == nil {
return nil
}
if errors.Is(err, ErrUnrecoverable) {
return err
}
if err := util.Sleep(ctx.Context(), util.RetryInterval); err != nil {
// ...
}
}With this approach:
- No new method on the
Workloadinterface. - No new retry function or
pvc.gofile. - No stub implementations in unrelated code.
- PVC health is validated everywhere, not just after Disable DR.
- Unrecoverable PVC states fail fast instead of waiting for the test timeout.
DisableProtectionkeeps the singleHealth()call — no need to wait since the workload was not touched.
| // If the cluster is not nil, the workload exists and its health and PVC state are validated. | ||
| if cluster != nil { | ||
| if err := ctx.Workload().Health(ctx, cluster); err != nil { | ||
| if err := deployers.WaitWorkloadHealth(ctx, cluster); err != nil { |
There was a problem hiding this comment.
When we disable protection the workload must be healthy. If not we should fail immediately. The original code was correct.
| log.Debugf("Workload \"%s/%s\" is healthy in cluster %q", | ||
| if err := WaitPVCPreserved(ctx, cluster); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
At this point the PVC must be Bound and not deleted. A single call is enough to validate this, and we don't need to wait.
| } | ||
| } | ||
|
|
||
| func WaitPVCPreserved(ctx types.TestContext, cluster *types.Cluster) error { |
There was a problem hiding this comment.
This waits for no benefit. If the PVC is deleted, we already found unrecoverable state, so single check is good enough in disable DR flow.
|
|
||
| func (w *NoHooks) AssertPVCPreserved(ctx types.TestContext, cluster *types.Cluster) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This is unwanted side effect of making the workload interface bigger.
| Health(ctx TestContext, cluster *Cluster) error | ||
| // AssertPVCPreserved verifies application PVCs survived Disable DR (Bound, not | ||
| // deleting, no DR-managed VolSync/VRG owner references). | ||
| AssertPVCPreserved(ctx TestContext, cluster *Cluster) error |
There was a problem hiding this comment.
This has several issues:
- Health already covers PVC health check - and application with deleted PVC is not healthy.
- The name is not consistent with other names. When we modify existing code want to keep the code consistent, as if it was written by the same person.
26547ac to
1b312a1
Compare
f2c1cad to
7cdafd2
Compare
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Looks so much simpler now!
Can you share logs with this change? Did we fix the ownership issue with volsync so this check pass now, or (better) this check reveal the ownership bug?
| if err != nil { | ||
| if k8serrors.IsNotFound(err) { | ||
| return fmt.Errorf("pvc \"%s/%s\" not found in cluster %q", | ||
| namespace, pvcName, cluster.Name) |
There was a problem hiding this comment.
If the PVC is not found the application is not healthy and cannot be healthy - unless the PVC will be created soon. This is most likely unrecoverable error but it depends on when we check application health. If we check only after deployment is healthy, then this should never happen and should be unrecoverable error.
| } | ||
|
|
||
| return fmt.Errorf("pvc \"%s/%s\" not available in cluster %q: %w", | ||
| namespace, pvcName, cluster.Name, err) |
There was a problem hiding this comment.
This is unexpected error meaning that we could not get the PVC. We try to handle such errors first before handling expected errors - this decrase teh chance to miss an uxpected errror.
So this should be:
if !k8serrors.IsNotFound(err) {
return err
}
// handle expected error of missing PVC...| namespace, pvcName, cluster.Name, err) | ||
| } | ||
|
|
||
| if pvc.DeletionTimestamp != nil { |
There was a problem hiding this comment.
In ramen we check !obj.GetDeletionTimestamp().IsZero(). Can we have a zero value time value in the DeletionTimestamp?
There was a problem hiding this comment.
I switched to that. I think it sets a real time when deletion starts so a zero value shouldn’t show up, but IsZero() also covers a nil timestamp and keeps us consistent with the controllers
| namespace, pvcName, pvc.Status.Phase, cluster.Name, util.ErrUnrecoverable) | ||
| default: | ||
| return fmt.Errorf("pvc \"%s/%s\" phase is %q, expected %q in cluster %q", | ||
| namespace, pvcName, pvc.Status.Phase, corev1.ClaimBound, cluster.Name) |
There was a problem hiding this comment.
The default case is for handling unexpected phases added in the future?
There was a problem hiding this comment.
Yes, default covers empty status and any future phases
7487c2d to
5557361
Compare
5557361 to
7c3a175
Compare
Fail when application PVCs are Lost or being deleted, and treat Pending as retryable so wait loops can recover during deploy. Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>
e495371 to
c7767dd
Compare
2 participants
Summary
Catch regressions where PVCs end up Terminating or still owned by VolSync/VRG while the workload still looks healthy.
Fixes #2649
Verification
Manual Deploy → Enable → Disable on VolSync CephFS
disapp-deploy-cephfs.Post Enable (
dr1)DRPC
Deployed, VRGPrimary. PVC while protected:{ "phase": "Bound", "deletionTimestamp": null, "ownerReferences": [ { "kind": "ReplicationSource", "name": "busybox-pvc" } ] }Post Disable
TestDR/disapp-deploy-cephfs/DisablePASS. DRPC/VRG removed. PVC after disable:{ "phase": "Bound", "deletionTimestamp": null, "ownerReferences": [] }Expected: PVC remains Bound, not Terminating, and VolSync/VRG owners are cleared (
do-not-delete-pvcpath).