Add DR migration labels and annotations for ZTP resources#2388
Add DR migration labels and annotations for ZTP resources#2388ldpliu wants to merge 1 commit intostolostron:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ldpliu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Related Jira Issues: |
📝 WalkthroughWalkthroughSplit combined handling of BareMetalHost/DataImage into separate cases; BareMetalHost now ensures a cluster-backup label and both kinds remove pause annotation; ImageClusterInstall now sets Velero restore name and a velero restore-status annotation; three new constants were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/pkg/spec/migration/migration_from_syncer.go`:
- Around line 65-72: The const block with VeleroRestoreStatusAnnotation,
ClusterBackupLabel, GlobalHubRestoreName, and ClusterActivationBackup is not
formatted to gofumpt's rules; run gofumpt (or make strict-fmt) on
agent/pkg/spec/migration/migration_from_syncer.go to reformat that const block
so the constants and their values follow gofumpt alignment/spacing rules (or
manually adjust the spacing within the const (...) group to match gofumpt's
output). Ensure the names VeleroRestoreStatusAnnotation, ClusterBackupLabel,
GlobalHubRestoreName, and ClusterActivationBackup remain unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce1cb351-9a06-4623-abf1-21b25f95ec5b
📒 Files selected for processing (1)
agent/pkg/spec/migration/migration_from_syncer.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
agent/pkg/spec/migration/migration_from_syncer.go (2)
68-72: Reuse the shared backup label constants.
pkg/constants/constants.go:38-42already defines this backup key/value pair. Re-declaring it here creates a second contract to keep in sync in a path that now depends on exact string matches.♻️ Proposed cleanup
- // ClusterBackupLabel marks resources for cluster activation backup. - ClusterBackupLabel = "cluster.open-cluster-management.io/backup" - - GlobalHubRestoreName = "globalhub" - ClusterActivationBackup = "cluster-activation" + GlobalHubRestoreName = "globalhub"- if labels[ClusterBackupLabel] != ClusterActivationBackup { - labels[ClusterBackupLabel] = ClusterActivationBackup + if labels[constants.BackupKey] != constants.BackupActivationValue { + labels[constants.BackupKey] = constants.BackupActivationValue }Also applies to: 458-465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/pkg/spec/migration/migration_from_syncer.go` around lines 68 - 72, Remove the duplicate ClusterBackupLabel constant declaration in migration_from_syncer.go and any other re-declared backup key/value (e.g., ClusterBackupLabel and ClusterActivationBackup at the other location) and replace usages with the shared definitions from the package that holds the canonical constants (pkg/constants/constants.go); update imports to reference that constants package and use constants.ClusterBackupLabel (and the corresponding shared name for the activation backup constant) throughout so there is a single source of truth.
450-466: Add regression tests for the exact-value cases.This change is only different when labels/annotations are nil or when the key already exists with the wrong value. A small table-driven test around
processResourceByTypewould lock down the new BareMetalHost and ImageClusterInstall behavior.Also applies to: 474-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/pkg/spec/migration/migration_from_syncer.go` around lines 450 - 466, Add table-driven regression tests for processResourceByType covering BareMetalHost and ImageClusterInstall to lock behavior when annotations/labels are nil, when keys already exist with correct values, and when keys exist with wrong values; for each case create a resource object with initial Annotations and Labels (nil, correct, wrong), call processResourceByType (or the exposed wrapper that applies the migration), then assert that Metal3PauseAnnotation is removed from Annotations and that ClusterBackupLabel equals ClusterActivationBackup (or remains correct) and other keys are unchanged. Include cases where annotations/labels are nil to ensure SetAnnotations/SetLabels are called and existing wrong values are overwritten while correct values are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agent/pkg/spec/migration/migration_from_syncer.go`:
- Around line 68-72: Remove the duplicate ClusterBackupLabel constant
declaration in migration_from_syncer.go and any other re-declared backup
key/value (e.g., ClusterBackupLabel and ClusterActivationBackup at the other
location) and replace usages with the shared definitions from the package that
holds the canonical constants (pkg/constants/constants.go); update imports to
reference that constants package and use constants.ClusterBackupLabel (and the
corresponding shared name for the activation backup constant) throughout so
there is a single source of truth.
- Around line 450-466: Add table-driven regression tests for
processResourceByType covering BareMetalHost and ImageClusterInstall to lock
behavior when annotations/labels are nil, when keys already exist with correct
values, and when keys exist with wrong values; for each case create a resource
object with initial Annotations and Labels (nil, correct, wrong), call
processResourceByType (or the exposed wrapper that applies the migration), then
assert that Metal3PauseAnnotation is removed from Annotations and that
ClusterBackupLabel equals ClusterActivationBackup (or remains correct) and other
keys are unchanged. Include cases where annotations/labels are nil to ensure
SetAnnotations/SetLabels are called and existing wrong values are overwritten
while correct values are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03fd5fbd-79e9-4ff6-9851-ab1cae5f10e5
📒 Files selected for processing (1)
agent/pkg/spec/migration/migration_from_syncer.go
Enhance migration support for disaster recovery scenarios: - Add velero.io/restore-status annotation to ImageClusterInstall - Add cluster.open-cluster-management.io/backup label to BareMetalHost - Validate both key and value when setting labels/annotations The velero.io/restore-status annotation prevents the image-based-install-operator from reconciling ImageClusterInstall resources during DR recovery. The backup label ensures BareMetalHost resources are included in cluster activation backups. Both checks now validate the value matches expected constants, not just key existence, ensuring correct DR configuration even if resources have incorrect values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: daliu@redhat.com Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
agent/pkg/spec/migration/migration_from_syncer.go (1)
68-72: Use existing constants frompkg/constants/constants.goinstead of redefining them.
ClusterBackupLabelandClusterActivationBackupduplicateBackupKeyandBackupActivationValuealready defined inpkg/constants/constants.go. Having two sources of truth for the same values creates maintenance burden and risks drift if one is updated but not the other.Replace the local definitions with an import and use
constants.BackupKeyandconstants.BackupActivationValueat lines 463-464, consistent with how the rest of the codebase handles these constants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/pkg/spec/migration/migration_from_syncer.go` around lines 68 - 72, Remove the duplicated local constants ClusterBackupLabel and ClusterActivationBackup and replace all uses with the canonical constants from the shared package: import the constants package (e.g., import "pkg/constants" as constants) and use constants.BackupKey instead of ClusterBackupLabel and constants.BackupActivationValue instead of ClusterActivationBackup; also delete the local constant declarations so there is a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agent/pkg/spec/migration/migration_from_syncer.go`:
- Around line 68-72: Remove the duplicated local constants ClusterBackupLabel
and ClusterActivationBackup and replace all uses with the canonical constants
from the shared package: import the constants package (e.g., import
"pkg/constants" as constants) and use constants.BackupKey instead of
ClusterBackupLabel and constants.BackupActivationValue instead of
ClusterActivationBackup; also delete the local constant declarations so there is
a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 378bb1cc-b150-4e86-a900-2176d00dcb0b
📒 Files selected for processing (2)
agent/pkg/spec/migration/migration_from_syncer.goagent/pkg/spec/migration/migration_from_syncer_test.go
|
1 participant
Summary
velero.io/restore-statusannotation to ImageClusterInstall resourcescluster.open-cluster-management.io/backuplabel to BareMetalHost resourcesChanges
Agent (agent/pkg/spec/migration/migration_from_syncer.go)
Constants Added:
VeleroRestoreStatusAnnotation = "velero.io/restore-status"ClusterBackupLabel = "cluster.open-cluster-management.io/backup"ClusterActivationBackup = "cluster-activation"ImageClusterInstall Processing:
velero.io/restore-status: "true"annotation to prevent image-based-install-operator reconciliationvelero.io/restore-name: globalhublabel (existing, now validates value)BareMetalHost Processing:
cluster.open-cluster-management.io/backup: cluster-activationlabelWhy These Changes
velero.io/restore-status annotation:
The image-based-install-operator checks for this annotation to determine if a resource was restored by Velero during DR recovery. Without it, the operator may attempt to reconcile ImageClusterInstall resources inappropriately during migration, causing conflicts.
cluster.open-cluster-management.io/backup label:
This label marks BareMetalHost resources for inclusion in cluster activation backup workflows, ensuring they're properly captured during DR scenarios.
Value validation:
Previously, the code only checked if keys existed. Now it validates values match expected constants, preventing issues where resources have the annotation/label but with incorrect values.
Test plan
make unit-tests-agentRelated Issues
https://issues.redhat.com/browse/ACM-32454
🤖 Generated with Claude Code
Signed-off-by: daliu@redhat.com
Summary by CodeRabbit
Bug Fixes
Tests