CNTRLPLANE-3244: add --use-etcd-snapshot flag to OADP CLI commands - #8232
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request references CNTRLPLANE-3244 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Tech Preview etcd snapshot mode toggled by Sequence Diagram(s)mermaid CLI->>Opts: invoke backup/schedule/restore (with flags) 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
approved
|
@jparrill: This pull request references CNTRLPLANE-3244 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8232 +/- ##
==========================================
+ Coverage 34.63% 35.70% +1.07%
==========================================
Files 767 767
Lines 93186 93398 +212
==========================================
+ Hits 32277 33350 +1073
+ Misses 58236 57346 -890
- Partials 2673 2702 +29
🚀 New features to boost your workflow:
|
12e33b6 to
ecb29a7
Compare
|
@jparrill: This pull request references CNTRLPLANE-3244 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jparrill: This pull request references CNTRLPLANE-3244 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![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 the current code and only fix it if needed.
Inline comments:
In `@cmd/oadp/restore_test.go`:
- Around line 80-126: The tests currently ignore errors from
unstructured.NestedString/NestedStringSlice/NestedBool calls (e.g., the calls
reading "spec" fields from restore.Object), which can hide type-mismatch
failures; update each call (bn, policy, namespaces, rpv, cbr, val, er) to
capture the error (e.g., bn, found, err := unstructured.NestedString(...)) and
add assertions that err is nil (using g.Expect(err).To(BeNil()) or similar)
before asserting found/values, and when expecting a field to be absent assert
err is not nil or err indicates a type error as appropriate so type-mismatch
failures fail the test. Ensure you change all occurrences of
unstructured.NestedString, NestedStringSlice, and NestedBool in this block.
In `@cmd/oadp/schedule_test.go`:
- Around line 993-1019: The test creates an unstructured DPA object (`dpa`) but
doesn't set its GVK, so the fake client can't find it during `VerifyDPAStatus`;
call `dpa.SetGroupVersionKind(schema.GroupVersionKind{Group:"oadp.openshift.io",
Version:"v1alpha1", Kind:"DataProtectionApplication"})` after constructing `dpa`
and add the import for "k8s.io/apimachinery/pkg/runtime/schema" so the object is
indexed correctly by `fake.NewClientBuilder().WithObjects(...)` and will be
returned by the `List` used in `VerifyDPAStatus`.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1112cbfb-5e85-4806-8720-e62094bcf7d9
📒 Files selected for processing (4)
cmd/oadp/backup_test.gocmd/oadp/restore_test.gocmd/oadp/schedule.gocmd/oadp/schedule_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/oadp/backup_test.go
ecb29a7 to
7022cf9
Compare
|
@jparrill: This pull request references CNTRLPLANE-3244 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/oadp/restore_test.go (1)
125-127:⚠️ Potential issue | 🟡 MinorAssert
excludedResourcespresence explicitly before membership checks.Line 126 still discards
foundfromunstructured.NestedStringSlice. Capturing and asserting it makes failures clearer whenspec.excludedResourcesis missing.Suggested patch
- er, _, err := unstructured.NestedStringSlice(restore.Object, "spec", "excludedResources") + er, found, err := unstructured.NestedStringSlice(restore.Object, "spec", "excludedResources") g.Expect(err).NotTo(HaveOccurred()) + g.Expect(found).To(BeTrue()) for _, expected := range tt.expectedExcludedContains { g.Expect(er).To(ContainElement(expected)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/oadp/restore_test.go` around lines 125 - 127, The test currently ignores the `found` boolean returned by unstructured.NestedStringSlice, so if spec.excludedResources is missing the membership assertions become confusing; modify the call to capture the second return (e.g., er, found, err := unstructured.NestedStringSlice(restore.Object, "spec", "excludedResources")), assert err is nil as before and then add an explicit assertion that found is true (e.g., using g.Expect(found).To(BeTrue())) before performing any checks against `er` to make failures clearer; reference the call to unstructured.NestedStringSlice, the `er` variable and `restore.Object`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/oadp/schedule.go`:
- Around line 121-124: The render branch is hard-coding "AWS" when calling
GenerateScheduleObject which drives resolveIncludedResources and
platform-specific policy generation; change the call in the o.Render block to
use the actual platform (e.g., o.Platform or another explicit platform flag) or
return an error when platform/resource set isn't supplied so we don't emit AWS
manifests for Azure/KubeVirt users—update the o.Render logic to validate a
provided platform value and pass that into GenerateScheduleObject instead of the
literal "AWS", or fail fast if none is available.
- Line 89: When opts.UseEtcdSnapshot is set, reject the mutually incompatible
flags --snapshot-move-data and --default-volumes-to-fs-backup instead of
silently ignoring them: add a validation step (e.g., in the command run/PreRun
or right after flag parsing in the schedule command) that checks
opts.UseEtcdSnapshot and if either opts.SnapshotMoveData or
opts.DefaultVolumesToFsBackup is true, return/exit with an error explaining the
conflict; note that buildBackupSpec in cmd/oadp/common.go forces
snapshotMoveData=false and defaultVolumesToFsBackup=false, so enforce the
failure there to prevent user input from being dropped.
---
Duplicate comments:
In `@cmd/oadp/restore_test.go`:
- Around line 125-127: The test currently ignores the `found` boolean returned
by unstructured.NestedStringSlice, so if spec.excludedResources is missing the
membership assertions become confusing; modify the call to capture the second
return (e.g., er, found, err := unstructured.NestedStringSlice(restore.Object,
"spec", "excludedResources")), assert err is nil as before and then add an
explicit assertion that found is true (e.g., using g.Expect(found).To(BeTrue()))
before performing any checks against `er` to make failures clearer; reference
the call to unstructured.NestedStringSlice, the `er` variable and
`restore.Object`.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 514c2817-3088-4300-a521-86de54d9104f
📒 Files selected for processing (4)
cmd/oadp/backup_test.gocmd/oadp/restore_test.gocmd/oadp/schedule.gocmd/oadp/schedule_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/oadp/backup_test.go
- cmd/oadp/schedule_test.go
7022cf9 to
63e2d8f
Compare
openshift-ci
Bot
added
the
area/documentation
|
@jparrill: This pull request references CNTRLPLANE-3244 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/oadp/restore.go (1)
83-83: Clarify restore flag help text to avoid backup/restore ambiguity.Line 83 currently says etcd “is backed up,” but this is the restore command. Tightening wording will reduce CLI confusion.
Suggested wording tweak
- cmd.Flags().BoolVar(&opts.UseEtcdSnapshot, "use-etcd-snapshot", false, "Use etcd snapshot mode: etcd is backed up via HCPEtcdBackup CRD snapshots instead of PV volume snapshots") + cmd.Flags().BoolVar(&opts.UseEtcdSnapshot, "use-etcd-snapshot", false, "Use etcd snapshot restore mode: restore from backups created via HCPEtcdBackup CRD snapshots instead of PV volume snapshots")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/oadp/restore.go` at line 83, The help text for the Bool flag defined in the cmd.Flags().BoolVar call (variable opts.UseEtcdSnapshot) is misleading for the restore command because it says etcd “is backed up”; change the description to clearly state this flag controls using etcd snapshot mode for restore operations (e.g., "Restore etcd from HCPEtcdBackup CRD snapshots instead of PV volume snapshots" or similar) so the text correctly references restore rather than backup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/oadp/schedule_test.go`:
- Around line 378-383: The test currently ignores the "found" boolean returned
by unstructured.NestedBool, so missing keys would default to false and tests
would incorrectly pass; update the two checks to capture the found flag (e.g.,
sv, svFound, err := unstructured.NestedBool(schedule.Object, "spec", "template",
"snapshotVolumes") and smd, smdFound, err :=
unstructured.NestedBool(schedule.Object, "spec", "template",
"snapshotMoveData")) and add assertions that svFound and smdFound are true (and
that sv and smd are false) so the test fails if those fields are absent; keep
references to schedule.Object and the same key paths and error handling.
In `@docs/content/how-to/disaster-recovery/dr-cli.md`:
- Around line 743-758: The doc claims that in `--use-etcd-snapshot` mode the
fields `veleroManagedClustersBackupName`, `veleroCredentialsBackupName`, and
`veleroResourcesBackupName` are always "set to backup name", but the
implementation only sets these when `--from-backup` is used (not
`--from-schedule`); update the wording under `--use-etcd-snapshot` to scope
those three backup-name fields to the `--from-backup` case (e.g., "set to backup
name when restoring from `--from-backup`") and ensure the doc mentions the
distinction between `--from-backup` and `--from-schedule`.
---
Nitpick comments:
In `@cmd/oadp/restore.go`:
- Line 83: The help text for the Bool flag defined in the cmd.Flags().BoolVar
call (variable opts.UseEtcdSnapshot) is misleading for the restore command
because it says etcd “is backed up”; change the description to clearly state
this flag controls using etcd snapshot mode for restore operations (e.g.,
"Restore etcd from HCPEtcdBackup CRD snapshots instead of PV volume snapshots"
or similar) so the text correctly references restore rather than backup.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c6e0507e-312e-4e9f-8a49-34648ddb4e50
📒 Files selected for processing (10)
cmd/oadp/backup.gocmd/oadp/backup_test.gocmd/oadp/common.gocmd/oadp/common_test.gocmd/oadp/restore.gocmd/oadp/restore_test.gocmd/oadp/schedule.gocmd/oadp/schedule_test.gocmd/oadp/types.godocs/content/how-to/disaster-recovery/dr-cli.md
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/oadp/restore_test.go
- cmd/oadp/backup.go
- cmd/oadp/backup_test.go
- cmd/oadp/common.go
- cmd/oadp/types.go
63e2d8f to
afe4bd6
Compare
afe4bd6 to
3191582
Compare
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aks
e2e-aws
|
|
/test e2e-aks |
…test Add feature gate check, BackupCompleted condition verification, etcd-init log validation, and etcd-snapshot-specific manifest helpers to the BackupRestoreEtcdSnapshot e2e test per reviewer feedback. - Skip test early in BeforeAll when HCPEtcdBackup CRD is not installed (feature gate disabled), preventing opaque CRD-not-found failures - Add BackupCompleted=True condition check on HCPEtcdBackup to ensure backup actually succeeded before verifying snapshotURL - Verify etcd-0 init container logs after restore contain expected snapshot download and restore traces - Extract EtcdSnapshotBackupOptions and EtcdSnapshotRestoreOptions helper functions with etcd-snapshot-specific flag values (snapshotMoveData=false, restorePVs=false) and TODOs to integrate --use-etcd-snapshot flag from PR openshift#8232 once merged Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/test e2e-azure-self-managed |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-azure-self-managed |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
I now have complete evidence. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 13 test failures share an identical root cause: the ExternalDNS pod on the management cluster failed to create DNS A/CNAME records in the Azure DNS zone Root CauseThe ExternalDNS pod ( This means the external-dns process either:
The consequence is that no DNS records were created for any of the 6 HostedClusters' API endpoints ( The proof that this is an infrastructure issue and not a code regression:
Recommendations
Evidence
|
| | `csiSnapshotTimeout` | `10m0s` | not set | | ||
| | `itemOperationTimeout` | not set | `4h0m0s` | | ||
| | `excludedResources` | not set | `[]` (empty) | | ||
| | Included resources | FQDNs (e.g. `hostedclusters.hypershift.openshift.io`) | Short names (e.g. `hostedcluster`) | |
There was a problem hiding this comment.
Based on my findings this line is wrong:
The code itself uses FQDNs in baseResourcesEtcdSnapshot
(types.go:116-125). The docs are wrong when they say etcd snapshot
mode uses short names. The actual implementation uses FQDNs just
like the default mode — the only difference between baseResources
and baseResourcesEtcdSnapshot is which resources are included, not
how they're named.
There was a problem hiding this comment.
Fixed. The doc table no longer mentions short names vs FQDNs. Updated to: "Base + platform resources (without PVs, deployments, statefulsets; adds namespaces)" which reflects what the code actually does.
| | `snapshotMoveData` | user-configurable | `false` (forced) | | ||
| | `defaultVolumesToFsBackup` | user-configurable | `false` (forced) | | ||
| | `dataMover` | `velero` | not set | | ||
| | `csiSnapshotTimeout` | `10m0s` | not set | |
There was a problem hiding this comment.
csiSnapshotTimeout is still there:
csiSnapshotTimeout: 10m0s
In "render" mode it is not, but Velero adds this to the resource in the end.
There was a problem hiding this comment.
Good catch. Updated to "not set by CLI (Velero may add a default)" to clarify that the CLI doesn't set it, but Velero may populate it at runtime.
| | `veleroResourcesBackupName` | not set | set to backup name (when `--from-backup` is used) | | ||
| | Excluded `csinodes.storage.k8s.io` | yes | no | | ||
| | Excluded `volumeattachments.storage.k8s.io` | yes | no | | ||
| | Excluded `backuprepositories.velero.io` | yes | no | |
There was a problem hiding this comment.
This resource looks different when I try that with Velero:
ᐅ oc get restore -A -oyaml
apiVersion: v1
items:
- apiVersion: velero.io/v1
kind: Restore
metadata:
creationTimestamp: "2026-04-17T09:54:49Z"
finalizers:
- restores.velero.io/external-resources-finalizer
generation: 16
name: mgencur-hc1-clusters-wqk4lg
namespace: openshift-adp
resourceVersion: "1228053"
uid: 6bdc1ab4-9621-4900-9bf2-87687e434b5e
spec:
backupName: mgencur-hc1-clusters-82jk9j
excludedResources:
- nodes
- events
- events.events.k8s.io
- backups.velero.io
- restores.velero.io
- resticrepositories.velero.io
- csinodes.storage.k8s.io
- volumeattachments.storage.k8s.io
- backuprepositories.velero.io
existingResourcePolicy: update
includedNamespaces:
- clusters
- clusters-mgencur-hc1
itemOperationTimeout: 4h0m0s
preserveNodePorts: true
restorePVs: false
status:
completionTimestamp: "2026-04-17T09:55:01Z"
hookStatus: {}
phase: Completed
progress:
itemsRestored: 337
totalItems: 337
startTimestamp: "2026-04-17T09:54:49Z"
warnings: 9
kind: List
metadata:
resourceVersion: ""
In "render" mode it matches what these docs describe.
There was a problem hiding this comment.
Good point. Added a disclaimer to the restore table header: "as generated by the CLI; Velero may add additional fields at runtime". The tables document what the CLI produces (i.e. what --render outputs), not what the final Velero resource looks like after the controller reconciles it.
Add Tech Preview documentation for the etcd snapshot mode including flag details, comparison tables and usage scenarios. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
27f56a8 to
ace1934
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by @mgencur |
openshift-ci-robot
added
the
verified
|
@mgencur: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-azure-self-managed |
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…test Add feature gate check, BackupCompleted condition verification, etcd-init log validation, and etcd-snapshot-specific manifest helpers to the BackupRestoreEtcdSnapshot e2e test per reviewer feedback. - Skip test early in BeforeAll when HCPEtcdBackup CRD is not installed (feature gate disabled), preventing opaque CRD-not-found failures - Add BackupCompleted=True condition check on HCPEtcdBackup to ensure backup actually succeeded before verifying snapshotURL - Verify etcd-0 init container logs after restore contain expected snapshot download and restore traces - Extract EtcdSnapshotBackupOptions and EtcdSnapshotRestoreOptions helper functions with etcd-snapshot-specific flag values (snapshotMoveData=false, restorePVs=false) and TODOs to integrate --use-etcd-snapshot flag from PR openshift#8232 once merged Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test Add feature gate check, BackupCompleted condition verification, etcd-init log validation, and etcd-snapshot-specific manifest helpers to the BackupRestoreEtcdSnapshot e2e test per reviewer feedback. - Skip test early in BeforeAll when HCPEtcdBackup CRD is not installed (feature gate disabled), preventing opaque CRD-not-found failures - Add BackupCompleted=True condition check on HCPEtcdBackup to ensure backup actually succeeded before verifying snapshotURL - Verify etcd-0 init container logs after restore contain expected snapshot download and restore traces - Extract EtcdSnapshotBackupOptions and EtcdSnapshotRestoreOptions helper functions with etcd-snapshot-specific flag values (snapshotMoveData=false, restorePVs=false) and TODOs to integrate --use-etcd-snapshot flag from PR openshift#8232 once merged Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test Add feature gate check, BackupCompleted condition verification, etcd-init log validation, and etcd-snapshot-specific manifest helpers to the BackupRestoreEtcdSnapshot e2e test per reviewer feedback. - Skip test early in BeforeAll when HCPEtcdBackup CRD is not installed (feature gate disabled), preventing opaque CRD-not-found failures - Add BackupCompleted=True condition check on HCPEtcdBackup to ensure backup actually succeeded before verifying snapshotURL - Verify etcd-0 init container logs after restore contain expected snapshot download and restore traces - Extract EtcdSnapshotBackupOptions and EtcdSnapshotRestoreOptions helper functions with etcd-snapshot-specific flag values (snapshotMoveData=false, restorePVs=false) and TODOs to integrate --use-etcd-snapshot flag from PR openshift#8232 once merged Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 participants
Summary
--use-etcd-snapshotflag tooadp-backup,oadp-restoreandoadp-schedulecommandsTest plan
RunSchedulewith fake client: render fallback, non-render happy path, and error paths (platform detection, OADP validation, DPA verification)--renderflag for both default and etcd snapshot modes🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests