OCPBUGS-66146: Add audit annotations and unified pause/unpause logic for Hypershift backup operations #170
Conversation
openshift-ci-robot
added
jira/severity-important
|
@jparrill: This pull request references Jira Issue OCPBUGS-66146, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
WalkthroughDependency bumps; added OADP pause/resume constants; consolidated Hypershift resource update logic with pause-audit helpers; backup progress-throttled pause/unpause orchestration and tests; ShouldEndPluginExecution now returns (bool, error); DataMover handling/signatures updated; resource names normalized to singular; removed checked-in kubeconfig. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to ⛔ Files ignored due to path filters (116)
📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)pkg/core/backup.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![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
🧹 Nitpick comments (1)
pkg/common/utils.go (1)
572-577: Audit annotations only added forpaused == "true", not for other non-empty pause values.The annotation logic specifically checks for
paused == "true"(line 573), but theneedsUpdatelogic (lines 553-559) handles any non-empty pause value. If a future caller passes a timestamp (like"2024-12-31T23:59:59Z") as the pause value instead of"true", audit annotations won't be added.This appears intentional for the current implementation but could be a gotcha for future maintainers.
Consider documenting this behavior or unifying the logic if audit annotations should always accompany any pause operation:
- if paused == "true" { + if paused != "" { addPauseAuditAnnotations(current) } else if paused == "" { removePauseAuditAnnotations(current) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (10)
go.mod(3 hunks)pkg/common/types.go(1 hunks)pkg/common/utils.go(5 hunks)pkg/common/utils_test.go(2 hunks)pkg/core/backup.go(6 hunks)pkg/core/backup_test.go(1 hunks)pkg/core/restore.go(1 hunks)pkg/core/types/types.go(1 hunks)pkg/core/validation/backup.go(8 hunks)pkg/core/validation/backup_test.go(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/common/types.gopkg/core/backup_test.gogo.modpkg/core/types/types.gopkg/core/backup.gopkg/core/restore.gopkg/core/validation/backup.gopkg/common/utils_test.gopkg/core/validation/backup_test.gopkg/common/utils.go
🧬 Code graph analysis (4)
pkg/core/backup.go (2)
pkg/common/types.go (3)
HostedClusterKind(35-35)HostedClusterRestoredFromBackupAnnotation(27-27)ClusterDeploymentKind(40-40)pkg/common/utils.go (3)
AddAnnotation(716-723)UpdateHostedCluster(612-650)UpdateNodepools(653-687)
pkg/core/restore.go (1)
pkg/common/utils.go (2)
UpdateNodepools(653-687)UpdateHostedCluster(612-650)
pkg/common/utils_test.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
pkg/common/utils.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
🔇 Additional comments (21)
pkg/core/types/types.go (1)
8-14: LGTM - resource name variants support flexible matching.Including both singular and plural forms of resource names enables broader detection patterns across the codebase. The addition of
serviceaccounts,roles,rolebindings,priorityclasses, andpoddisruptionbudgetsextends backup coverage appropriately.go.mod (1)
7-9: LGTM - dependency updates align with PR objectives.The hypershift/api bump to a December 2025 version supports the enhanced validation mentioned in the PR description. The gomega and golang.org/x/* updates are routine maintenance.
pkg/core/validation/backup.go (2)
254-316: LGTM - conditional data mover reconciliation correctly implemented.The nil-check pattern
dataMover != nil && *dataMoverproperly guards the DataUpload reconciliation, ensuring it only runs whenSnapshotMoveDatais explicitly configured. Setting*p.DUFinished = truein the else branch correctly handles the case where data mover is not configured.
128-131: Consistent parameter passing across platform branches.The
backup.Spec.SnapshotMoveDatais correctly passed toreconcileStandardDataMoveracross all platform-specific branches (AWS, IBMCloud, Kubevirt, OpenStack, Agent/None), ensuring uniform behavior.pkg/core/validation/backup_test.go (2)
681-695: LGTM - test helper correctly extended for snapshotMoveData parameter.The
createTestBackuphelper properly handles the optionalsnapshotMoveDataparameter, only setting the field when non-nil. This enables testing both configured and unconfigured data mover scenarios.
707-788: Good test coverage for data mover conditional behavior.Test cases appropriately cover:
- Platforms with
snapshotMoveData: ptr.To(true)expecting DataUpload reconciliation- Platforms with
snapshotMoveData: nilexpectingDUFinished=trueset directly- Azure platform correctly configured without DataUpload expectations
pkg/common/types.go (1)
29-33: LGTM - audit annotation constants well-defined.The new constants follow the established pattern and use the appropriate
oadp.openshift.iodomain for annotations. This enables atomic annotation management for pause tracking as described in the PR objectives.pkg/common/utils_test.go (2)
4115-4181: LGTM - comprehensive test coverage for pause audit annotations.The tests properly verify:
- Annotations are added to objects with no existing annotations
- Existing annotations are preserved when adding audit annotations
- RFC3339 timestamp format is validated via
time.Parse
4183-4258: LGTM - removal tests correctly verify annotation cleanup.The tests appropriately verify that audit annotations are removed while preserving unrelated annotations, including the edge case of objects with no annotations.
pkg/core/backup_test.go (1)
11-134: LGTM - comprehensive tests for progress logging throttle.The test suite covers all key scenarios for the
shouldLogProgressfunction:
- 5% progress threshold triggering
- 30-second time-based fallback
- State update verification based on logging decision
- Edge case for progress decrease
The assertion at lines 123-131 correctly verifies that
lastLoggedProgressandlastLogTimeare updated only when logging occurs.pkg/core/restore.go (1)
180-187: Verify that empty string correctly triggers unpause behavior.The change from
"false"to""for thepausedparameter aligns with the unified pause/unpause logic. Ensure that theupdateHypershiftResourcehelper correctly interprets an empty string as an unpause operation, settingPausedUntiltoniland removing audit annotations.pkg/common/utils.go (5)
477-489: LGTM!The updated logic correctly handles both pause (
paused != "") and unpause (paused == "") scenarios with appropriate state verification.
611-687: LGTM - Good refactoring to reduce code duplication.The consolidation of
UpdateHostedClusterandUpdateNodepoolsinto the genericupdateHypershiftResourcehelper is clean. The use of function parameters for resource-specific behavior (getters, setters, post-update hooks) is idiomatic.
736-749: LGTM!The audit annotation helpers are clean and correctly use the constants from
types.go. RFC3339 timestamp format is appropriate for audit purposes.
808-815: LGTM!The expanded resource detection correctly identifies hypershift resources using substring matching, which handles both short and full resource names.
521-532: Only resources from the first matching namespace are processed.The loop breaks after finding resources in the first namespace (line 530), meaning resources in subsequent namespaces won't be updated. This may be intentional if only one namespace is expected to contain the target resources, but could cause issues if multiple namespaces need processing.
Verify this is the expected behavior by checking if backups can include multiple namespaces with Hypershift resources:
#!/bin/bash # Check how namespaces are used in backup operations rg -n -C3 "IncludedNamespaces" --type=gopkg/core/backup.go (5)
134-160: LGTM!The progress throttling logic is well-implemented with sensible thresholds (5% progress change or 30-second interval) to reduce log noise while maintaining visibility.
215-221: LGTM!Adding the restore annotation during backup to mark HostedClusters for restore processing is a reasonable pattern.
342-361: Last mile errors are logged but not returned - intentional tradeoff.The cleanup logs errors but doesn't propagate them, meaning the backup will succeed even if unpausing fails. This is a reasonable tradeoff to avoid failing a nearly-complete backup, but operators should monitor logs for these errors to ensure resources don't remain paused.
223-234: Verify FSBackup label logic - condition may be inverted.The FSBackup label is added when
DefaultVolumesToFsBackupis explicitly disabled (!*backup.Spec.DefaultVolumesToFsBackup). The comment says "In case of FSBackup" but the condition triggers when FSBackup is NOT the default.Please verify this is the intended behavior. If the label should be added when FSBackup is enabled, the condition should be:
- case kind == "Pod": - // In case of FSBackup, we need to add the label to the pod - if backup.Spec.DefaultVolumesToFsBackup != nil && !*backup.Spec.DefaultVolumesToFsBackup { + case kind == "Pod": + // In case of FSBackup, we need to add the label to the pod + if backup.Spec.DefaultVolumesToFsBackup != nil && *backup.Spec.DefaultVolumesToFsBackup {
236-242: LGTM!The simplified agent migration logic is cleaner - now directly checks platform type without additional flag checks.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-66146, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. In 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
force-pushed
the
OCPBUGS-66146
branch
from
6fca708 to
4143689
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/dependencies/dependencies_test.go (2)
23-73: Shared mutable state across subtests makes future parallelization harder
allDependenciesUpToDateandfailureMessagesare mutated from within eacht.Runbody. This is safe as long as subtests remain strictly sequential, but it becomes a data‑race and ordering issue the moment someone addst.Parallel().Given there are only a handful of watched modules and the per‑subtest error messages are already informative, you could simplify by removing the outer
allDependenciesUpToDate/failureMessagesaggregation and just letting subtests fail individually (the parent test will still fail if any subtest fails). That reduces coupling between subtests and keeps the test future‑proof if you ever decide to run them in parallel.
105-121: Leverage formattedgo listoutput to avoid manual parsing and keep behavior robustThis works as written, but you’re depending on the default
go list -m module@mainoutput being"path version"and separately hand‑parsinggo.modfor the current version.To reduce moving parts and make the test more resilient to formatting changes, consider:
- Using a formatted
go listhere, e.g.go list -m -f '{{.Version}}' module@main, and- Reusing the same
go listpattern ingetCurrentDependencyVersioninstead of scanninggo.modyourself.That way both “current” and “latest” versions come from Go’s own module resolution, and you avoid the
unexpected output formatbranch entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (2)
go.mod(3 hunks)tests/integration/dependencies/dependencies_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
tests/integration/dependencies/dependencies_test.go
jparrill
force-pushed
the
OCPBUGS-66146
branch
from
4143689 to
4e6af34
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/core/backup.go (1)
296-321: Fix comment-to-code inconsistencies (previously flagged).The comments do not match the actual thresholds:
- Line 297: Comment says "< 65%" but code checks
<= 65.0- Line 320: Comment says "> 65%" but code checks
> 90.0Apply these fixes:
- // PAUSE when progress > 0% and < 65% and BOTH not already paused + // PAUSE when progress > 0% and <= 65% and BOTH not already paused case progressPercent > 0.0 && progressPercent <= 65.0:- // UNPAUSE when progress > 65% and BOTH are paused + // UNPAUSE when progress > 90% and BOTH are paused case progressPercent > 90.0:
🧹 Nitpick comments (3)
tests/integration/dependencies/dependencies_test.go (2)
32-63: Subtest aggregation pattern is correct today but somewhat brittle for future parallelizationThe subtest loop and aggregation into
allDependenciesUpToDate/failureMessageswork fine as written because subtests are executed synchronously (not.Parallel()), so there’s no race on the shared state and each module is checked independently.If you later decide to parallelize these subtests, this pattern will start to race. A slightly more future-proof structure would be either:
- Keep the current structure but add a brief comment that subtests must not call
t.Parallel()because of the shared aggregating state; or- Drop the shared state and let each subtest fail on its own, relying on the per-module
t.Error(errorMsg)messages rather than aggregating into the parent.Very minor, but worth considering to avoid surprises if someone parallelizes the tests later.
105-121:getLatestDependencyVersionlogic is sound; consider richer error reportingUsing
go list -mod=mod -m module@mainand takingparts[1]as the version is a reasonable and robust simplification versus the previous commit-hash parsing, and it aligns with the new “latest available from main” semantics.For easier debugging when
go listfails or returns unexpected output, you might consider:
- Using
cmd.CombinedOutput()and including trimmed stdout/stderr in the error, so failures in CI clearly show whether this was a networking, auth, or module-resolution issue; and/or- Trimming whitespace from
outputbefore building the “unexpected output format” error to keep the message concise.These are quality-of-life improvements only; the current implementation is functionally correct.
pkg/core/backup.go (1)
342-361: LAST MILE cleanup is a good safety net but may trigger redundantly.The cleanup runs when
ItemsBackedUp == TotalItems, which could occur on multiple items if the backup processes items in parallel or if progress updates lag. SinceUpdateHostedClusterandUpdateNodepoolsare idempotent (unpausing already-unpaused resources is a no-op), this is safe but may cause extra API calls.Consider adding a guard flag (e.g.,
lastMileExecuted) to run cleanup exactly once:+ lastMileExecuted bool- if backup.Status.Progress.TotalItems > 0 && backup.Status.Progress.ItemsBackedUp == backup.Status.Progress.TotalItems { + if !p.lastMileExecuted && backup.Status.Progress.TotalItems > 0 && backup.Status.Progress.ItemsBackedUp == backup.Status.Progress.TotalItems { p.log.Info("Processing final object - ensuring all resources are unpaused") + p.lastMileExecuted = true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (11)
go.mod(3 hunks)pkg/common/types.go(1 hunks)pkg/common/utils.go(5 hunks)pkg/common/utils_test.go(2 hunks)pkg/core/backup.go(6 hunks)pkg/core/backup_test.go(1 hunks)pkg/core/restore.go(1 hunks)pkg/core/types/types.go(1 hunks)pkg/core/validation/backup.go(8 hunks)pkg/core/validation/backup_test.go(10 hunks)tests/integration/dependencies/dependencies_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/common/types.go
- pkg/core/types/types.go
- pkg/core/backup_test.go
- go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/core/validation/backup_test.gotests/integration/dependencies/dependencies_test.gopkg/core/restore.gopkg/common/utils_test.gopkg/core/backup.gopkg/common/utils.gopkg/core/validation/backup.go
🧬 Code graph analysis (4)
pkg/core/restore.go (1)
pkg/common/utils.go (2)
UpdateNodepools(653-687)UpdateHostedCluster(612-650)
pkg/common/utils_test.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
pkg/core/backup.go (2)
pkg/common/types.go (3)
HostedClusterKind(35-35)HostedClusterRestoredFromBackupAnnotation(27-27)ClusterDeploymentKind(40-40)pkg/common/utils.go (3)
AddAnnotation(716-723)UpdateHostedCluster(612-650)UpdateNodepools(653-687)
pkg/common/utils.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
🔇 Additional comments (17)
tests/integration/dependencies/dependencies_test.go (1)
23-26: Updated test description is clear and aligned with behaviorThe expanded comment accurately describes the intent (tracking watched dependencies against the latest main-branch versions) and matches the new implementation using
go list. No issues here.pkg/core/restore.go (1)
180-187: LGTM - Semantic change to unpause semantics.The change from
"false"to""aligns with the unified pause/unpause logic where an empty string signals removal of thePausedUntilfield (unpause), while"true"signals pause. This is consistent with the newupdateHypershiftResourcehelper inpkg/common/utils.go.pkg/core/validation/backup_test.go (2)
681-694: LGTM - Test helper properly handles optional snapshotMoveData.The updated
createTestBackuphelper correctly appliesSnapshotMoveDataonly when non-nil, which matches the conditional logic inreconcileStandardDataMover.
717-751: Good test case documentation for edge cases.The comments on lines 720-721 and 747-748 clearly explain the expected behavior when
snapshotMoveDatais nil (code setsDUFinished=true), making these test cases self-documenting.pkg/common/utils_test.go (2)
4115-4181: LGTM - Comprehensive test for addPauseAuditAnnotations.The test properly validates:
- Adding annotations to objects without existing annotations
- Preserving existing annotations
- RFC3339 timestamp format validation
4183-4258: LGTM - Thorough test for removePauseAuditAnnotations.The test covers important scenarios:
- Removing only audit annotations
- Preserving non-audit annotations
- Handling objects with no annotations
pkg/core/validation/backup.go (2)
254-313: LGTM - Data mover reconciliation properly guarded by SnapshotMoveData.The refactored logic correctly:
- Only performs
ReconcileDataUploadwhendataMoveris non-nil and true- Sets
DUFinished = truewhen data mover is not configured, preventing deadlocks- Handles HA mode correctly for data upload completion
33-33: Unused fieldDataMoverin BackupPluginValidator requires verification.The
DataMoverfield is added to the struct but verification of its usage throughout the codebase could not be completed due to repository access limitations. The review indicates the field is never assigned or read in this file, and the actual data mover behavior is controlled by passingbackup.Spec.SnapshotMoveDatadirectly toreconcileStandardDataMover. If this field is confirmed to be unused across the entire codebase, it should be removed.pkg/core/backup.go (3)
131-132: Verify intentional broad resource selector.
AppliesTo()returns an emptyResourceSelector, meaning this plugin receives every resource during backup. While this works for the plugin's logic (it filters internally viaShouldEndPluginExecution), it may have performance implications for non-Hypershift backups that trigger this plugin unnecessarily.
134-160: LGTM - Good progress logging throttle implementation.The
shouldLogProgressmethod effectively reduces log noise by:
- Logging on first invocation
- Logging on 5% progress increments
- Logging after 30-second intervals
This prevents log flooding during backup operations.
349-358: LAST MILE cleanup logs errors but continues - verify this is intended.The cleanup logs errors with
Errorfbut doesn't return them, allowing the backup to complete even if unpause fails. This "best effort" approach ensures the backup doesn't fail at the last moment, but leaves resources potentially paused.This design seems intentional based on the "safety net" comment, but consider if a failed unpause at this stage should be escalated differently (e.g., emit an event or metric).
pkg/common/utils.go (6)
477-488: PausedUntil propagation semantics look correctThe updated logic cleanly distinguishes unpause (
paused == ""→PausedUntil == nil) from pause (paused != ""→PausedUntil == paused) and matches howUpdateHostedClusternow setsPausedUntil. This should avoid stale empty-string values and makes the state transitions explicit.
611-650: HostedCluster wrapper over generic updater is clean
UpdateHostedClusterwires the generic updater correctly: list factory, item extraction,PausedUntilgetters/setters, and theWaitForPausedPropagatedhook all line up with the underlying HyperShift types. This removes duplication while preserving the previous propagation behavior.
652-687: NodePool wrapper correctly reuses generic pause/unpause logic
UpdateNodepoolsmirrors the HostedCluster wiring without a propagation hook, which matches the need to updatePausedUntiland audit annotations on NodePools without additional HCP polling. The list factory, extractor, and getter/setter are consistent withhyperv1.NodePool.
736-749: Audit annotation helpers correctly encapsulate paused-by/paused-at behavior
addPauseAuditAnnotationsandremovePauseAuditAnnotationscentralize theoadp.openshift.io/paused-byandoadp.openshift.io/paused-atmanagement and use RFC3339 timestamps viatime.Now().Format(time.RFC3339), which is appropriate for audit trails. Using the existingAddAnnotation/RemoveAnnotationhelpers keeps the implementation straightforward.
808-814: Expanded resource detection in ShouldEndPluginExecution is reasonableChecking
backup.Spec.IncludedResourcesforhostedcluster,hostedcontrolplane, ornodepoolsubstrings before falling back to the CRD existence check should reduce unnecessary work on non‑Hypershift clusters and correctly recognizes nodepool‑only scenarios.
502-609: Clarify audit-annotation condition to cover all pause valuesThe code shows an inconsistency in pause handling:
PausedUntilis set to any non-emptypausedvalue, but audit annotations are only added whenpaused == "true". If callers pass other non-empty values, those pauses will not be audited.Either broaden the audit-annotation condition to
paused != ""to audit all plugin-driven pauses:if paused != "" { addPauseAuditAnnotations(current) } else if paused == "" { removePauseAuditAnnotations(current) }or document and enforce at call-sites that
pausedis strictly""or"true"to prevent future bypass of auditing.
- Add audit annotation constants for tracking pause operations - Implement atomic annotation management (add on pause, remove on unpause) - Unify UpdateHostedCluster and UpdateNodepools with shared logic - Add progress-based pause/unpause: >1% pause, >90% unpause - Implement LAST MILE safety net for guaranteed resource cleanup - Fix data mover validation to only execute when defined in backup - Add comprehensive unit tests for new functionality - Fix validation tests with snapshotMoveData factor 🚀 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Juan Manuel Parrilla Madrid <[email protected]>
jparrill
force-pushed
the
OCPBUGS-66146
branch
from
4e6af34 to
04e69ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/common/utils.go (1)
502-609: updateHypershiftResource only handles the first non‑empty namespace
updateHypershiftResourcecurrently:
- Reuses a single list object and, inside the namespace loop, assigns
items = itemsExtractor(listObj)andbreaks as soon as it finds any items.- Then updates only those
items.If
namespacescontains more than one namespace with HostedClusters/NodePools, only the first namespace that happens to have resources will be paused/unpaused and annotated, leaving the rest untouched. Given the API accepts a slice of namespaces and callers passbackup.Spec.IncludedNamespaces, this looks like a behavioral gap rather than an intentional limitation.This can result in some Hypershift clusters/nodepools in the backup set never being paused (or never being unpaused / de‑annotated), which undermines both the safety and auditability this helper is meant to provide.
A minimal fix is to collect and process resources from all namespaces, e.g.:
func updateHypershiftResource( @@ - // Search through namespaces for resources - var items []crclient.Object - for _, ns := range namespaces { - if err := c.List(ctx, listObj, crclient.InNamespace(ns)); err != nil { - return fmt.Errorf("failed to list %s in namespace %s: %w", resourceType, ns, err) - } - - items = itemsExtractor(listObj) - if len(items) > 0 { - log.Debugf("found %s in namespace %s", resourceType, ns) - break - } - } - for _, item := range items { + // Search through all namespaces and aggregate resources + var items []crclient.Object + for _, ns := range namespaces { + nsListObj := listObjFactory() + if err := c.List(ctx, nsListObj, crclient.InNamespace(ns)); err != nil { + return fmt.Errorf("failed to list %s in namespace %s: %w", resourceType, ns, err) + } + + nsItems := itemsExtractor(nsListObj) + if len(nsItems) > 0 { + log.Debugf("found %d %s in namespace %s", len(nsItems), resourceType, ns) + items = append(items, nsItems...) + } + } + + for _, item := range items {This keeps the rest of the backoff, audit‑annotation, and post‑update behavior intact while ensuring all relevant resources across the provided namespaces are handled.
Also applies to: 611-687, 736-749
🧹 Nitpick comments (2)
pkg/core/validation/backup_test.go (1)
717-724: Consider adding a test case withsnapshotMoveDataexplicitly set tofalse.Current test cases cover
nil(not configured) andtrue(enabled), but there's no explicit test forsnapshotMoveData = ptr.To(false). This would validate behavior when data mover is explicitly disabled.tests/integration/dependencies/dependencies_test.go (1)
17-21: Consider simplifyingwatchedDependenciesto a slice.Since
repoURLis no longer used (ignored with_), the map could be simplified to a[]stringslice. However, keeping the map structure may be useful for future extensions or documentation purposes.-var watchedDependencies = map[string]string{ - "github.com/openshift/hypershift/api": "https://github.com/openshift/hypershift", - // Add more dependencies here as needed: - // "github.com/example/module": "https://github.com/example/repo", -} +var watchedDependencies = []string{ + "github.com/openshift/hypershift/api", + // Add more dependencies here as needed +}And update the iteration:
-for module, _ := range watchedDependencies { +for _, module := range watchedDependencies {Also applies to: 32-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (11)
go.mod(3 hunks)pkg/common/types.go(1 hunks)pkg/common/utils.go(5 hunks)pkg/common/utils_test.go(2 hunks)pkg/core/backup.go(6 hunks)pkg/core/backup_test.go(1 hunks)pkg/core/restore.go(1 hunks)pkg/core/types/types.go(1 hunks)pkg/core/validation/backup.go(8 hunks)pkg/core/validation/backup_test.go(10 hunks)tests/integration/dependencies/dependencies_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/core/validation/backup.go
- pkg/core/backup_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/core/types/types.gopkg/core/validation/backup_test.gopkg/common/utils_test.gogo.modpkg/core/restore.gopkg/common/utils.gotests/integration/dependencies/dependencies_test.gopkg/common/types.gopkg/core/backup.go
🧬 Code graph analysis (4)
pkg/common/utils_test.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
pkg/core/restore.go (1)
pkg/common/utils.go (2)
UpdateNodepools(653-687)UpdateHostedCluster(612-650)
pkg/common/utils.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
pkg/core/backup.go (2)
pkg/common/types.go (3)
HostedClusterKind(35-35)HostedClusterRestoredFromBackupAnnotation(27-27)ClusterDeploymentKind(40-40)pkg/common/utils.go (3)
AddAnnotation(716-723)UpdateHostedCluster(612-650)UpdateNodepools(653-687)
🔇 Additional comments (10)
pkg/core/validation/backup_test.go (1)
681-695: LGTM!The
createTestBackuphelper correctly handles the optionalsnapshotMoveDataparameter with proper nil checks before assignment. This enables flexible test case configuration for the data mover validation logic.pkg/common/types.go (1)
29-33: LGTM!The new audit annotation constants follow Kubernetes conventions and align with the PR objective of adding audit trail for pause operations. The
oadp.openshift.iodomain correctly scopes these annotations to the OADP plugin.tests/integration/dependencies/dependencies_test.go (2)
51-60: String equality comparison works for pseudo-version matching.The direct string comparison
currentVersion != latestVersionis appropriate here since both versions come from the same source format (go list). This simplification removes the complexity of commit hash extraction while achieving the same goal.
108-121: LGTM!Using
go list -mod=mod -m module@mainis the correct approach to resolve the latest version from the main branch. This properly handles submodule dependencies and returns consistent pseudo-version formats for comparison.go.mod (1)
9-9: Dependency update rationale needed for hypershift/api.The hypershift/api dependency has been updated to
v0.0.0-20251204074840-7ba3ed3ad9a3(2025-12-04 commit). While this aligns with the PR objective of enhanced validation, clarify why this specific commit was selected and whether it is from an official release tag or development commit.pkg/core/types/types.go (1)
9-13: Verify the need for both plural and singular resource name variants.The list includes both plural and singular forms (e.g.,
"hostedclusters", "hostedcluster"). Kubernetes APIs typically use plural forms for resource types. If this duplication is intentional for flexible matching logic (e.g., inShouldEndPluginExecutionusingstrings.Contains), clarify this in a comment. Otherwise, consider removing the singular forms to reduce redundancy.pkg/core/backup.go (1)
46-49: Progress throttling, pause/unpause orchestration, and early‑exit behavior look consistent
- New throttling state (
lastLoggedProgress,lastLogTime) andshouldLogProgresscorrectly limit log volume while still guaranteeing periodic updates.- The Execute early‑exit now logs and returns
(item, nil, nil)for non‑Hypershift backups, which aligns with Velero plugin expectations and avoids spurious backup failures.- HostedCluster and Pod cases integrate cleanly with existing helpers (
AddAnnotation, FS backup labeling, agent migration), without changing behavior for unrelated kinds.- The progress‑driven PAUSE (>0% and ≤65%) and UNPAUSE (>90%) logic is state‑guarded via
hcPaused/npPaused, and the “LAST MILE” cleanup is idempotent thanks to theneedsUpdatechecks inUpdateHostedCluster/UpdateNodepools.No blocking issues spotted here.
Also applies to: 130-132, 134-170, 216-224, 287-361
pkg/core/restore.go (1)
178-187: Unpause flag change aligns with new unified update helperSwitching the
pausedargument from"false"to""forUpdateNodepoolsandUpdateHostedClustermatches the newupdateHypershiftResourcecontract (empty string → unpause / clearPausedUntil) and keeps restore behavior correct.pkg/common/utils_test.go (1)
771-773: Tests correctly track new plugin‑end and audit‑annotation behavior
- The
ShouldEndPluginExecution“CRD does not exist” test now uses only non‑Hypershift resources, which matches the updated resource‑name check logic.TestAddPauseAuditAnnotationsandTestRemovePauseAuditAnnotationsthoroughly verify:
paused-byis set to the plugin name.paused-atis present and RFC3339‑parseable.- Existing annotations are preserved and only audit annotations are added/removed.
- Final annotation maps have the expected keys/length.
Looks solid; no issues from a correctness or maintainability standpoint.
Also applies to: 4114-4258
pkg/common/utils.go (1)
477-499: Pause propagation and plugin‑end heuristics updated coherently
WaitForPausedPropagatednow explicitly treatspaused == ""as the unpause case (expectingPausedUntil == nil) and non‑emptypausedas the pause case, which matches howUpdateHostedCluster/updateHypershiftResourceset the field.ShouldEndPluginExecution’s fast path that inspectsbackup.Spec.IncludedResourcesfor hostedcluster/hostedcontrolplane/nodepool names avoids unnecessary work on clearly non‑Hypershift backups while still falling back to the CRD existence check when needed.These changes are internally consistent and improve both clarity and behavior.
Also applies to: 808-815
jparrill
force-pushed
the
OCPBUGS-66146
branch
from
04e69ad to
41e98bd
Compare
|
/retest |
pkg/common/utils.go
Outdated
| needsUpdate = currentPausedUntil != nil | ||
| } else { | ||
| // For pause: we need to set pausedUntil to the paused value | ||
| needsUpdate = (currentPausedUntil == nil && paused != "") || (currentPausedUntil != nil && *currentPausedUntil != paused) |
There was a problem hiding this comment.
Nit: This paused != "" is redundant here. We're already in the else block where this is guaranteed.
There was a problem hiding this comment.
Makes sense. I've fixed that and also enhanced the next checks with a tagged switch
| if slices.Contains(backup.Spec.IncludedResources, "hostedcluster") || slices.Contains(backup.Spec.IncludedResources, "hostedcontrolplane") { | ||
| log.Debug("Hypershift resources found") | ||
| return false | ||
| // Check for both short and full resource names |
There was a problem hiding this comment.
Hmm, is this comment misleading? I see only one type of check.
There was a problem hiding this comment.
The check, checks both, because:
- If the resource is the long one (
hostedclusters.hypershift.io) it's enough with the short because is using string.Contains with the short one and i singularhostedcluster. - Same for the short one
pkg/core/backup.go
Outdated
| ctx := context.Context(p.ctx) | ||
|
|
||
| if returnEarly := common.ShouldEndPluginExecution(ctx, backup, p.client, p.log); returnEarly { | ||
| p.log.Info("Skipping hypershift plugin execution - not a hypershift backup") |
There was a problem hiding this comment.
After reading this log message for the first time, I thought that it is skipping only the plugin execution, not the hypershift backup.
The function ShouldEndPluginExecution returns true for several reasons - namespaces not specified, included resources don't include Hypershift resources, or HCP CRD doesn't exist. Maybe we could also log the right reason? The function ShouldEndPluginExecution logs the reason at Debug level but here we log with Info level, without the reason. I think it makes sense to return the message as well from ShouldEndPluginExecution and log it here with Info level only once.
If we want to keep the two levels of logging. I think it makes sense to make this single message shorter:
p.log.Info("Skipping hypershift-oadp-plugin execution")
There was a problem hiding this comment.
Maybe we could also log the right reason?
This is intended for the situation when someone wants to perform a backup using OADP, the DPA has the hypershift plugin set but they are not backing up a HostedCluster. Yeah I think we can add the reason to the log.
If we want to keep the two levels of logging.
We cannot do that, the plugin should adapt to the Veleros verbosity level. I've tried in the past to use my own logger to just put higher debuggability level just in the plugin and that blocked the whole plugin logger xD
jparrill
force-pushed
the
OCPBUGS-66146
branch
from
41e98bd to
10dda6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/core/backup.go (2)
167-169: Fix log formatting - missing argument for %v placeholder.Line 168 uses
%vin the format string but doesn't pass theerrvalue as an argument top.log.Info(). This will print the literal%vinstead of the error value.Apply this diff:
- if returnEarly, err := common.ShouldEndPluginExecution(ctx, backup, p.client, p.log); returnEarly { - p.log.Info("Skipping hypershift plugin execution - not a hypershift backup: %v", err) + if returnEarly, err := common.ShouldEndPluginExecution(ctx, backup, p.client, p.log); returnEarly { + p.log.Infof("Skipping hypershift plugin execution - not a hypershift backup: %v", err)
316-323: Pause retry logic won't recover from partial failures.The condition
!p.hcPaused && !p.npPausedrequires BOTH to be unpaused to enter the pause block. If one resource pauses successfully but the other fails, subsequent iterations won't retry the failed resource because the condition is false.Scenario:
- HC pauses successfully →
p.hcPaused = true- NP fails to pause →
p.npPaused = false- Next iteration: condition
!true && !false=false && true=false→ won't enter block- NP remains unpaused for the rest of the backup
Change to
||to retry if EITHER resource is not paused.Apply this diff:
- case progressPercent > 0.0 && progressPercent <= 65.0: - if !p.hcPaused && !p.npPaused { + case progressPercent > 0.0 && progressPercent <= 65.0: + if !p.hcPaused || !p.npPaused { p.log.Infof("Backup progress %.1f%% - executing PAUSE logic", progressPercent) if err := p.pauseAll(ctx, backup); err != nil { p.log.Errorf("error pausing HostedClusters/NodePools: %v", err) return item, nil, nil } }Based on learnings, this concern was previously raised in past review comments (lines 317-317) by mgencur.
🧹 Nitpick comments (1)
tests/integration/dependencies/dependencies_test.go (1)
75-121: Make version discovery less format-dependent and improve failure diagnosticsThe combination of manually parsing
go.modingetCurrentDependencyVersionand parsinggo list’s raw output ingetLatestDependencyVersionties this test fairly tightly to current file/CLI output formats. That’s workable now but a bit brittle if:
- A watched module is declared with a single-line
require <module> <version>(your prefix check won’t match), orgo listoutput ever includes extra tokens or formatting changes.Consider a small refactor along these lines:
- Use
go listfor both current and latest versions, relying on Go’s own module metadata instead of parsinggo.modby hand. For example, from the project root:
- Current:
go list -m -f '{{.Version}}' <module>- Latest:
go list -m -f '{{.Version}}' <module>@main- When
go listfails, capture and surface stderr in the error message so test failures are immediately diagnosable.This would reduce duplicate logic, tolerate more go.mod layouts, and make dependency failures easier to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/common/utils.go(5 hunks)pkg/common/utils_test.go(8 hunks)pkg/core/backup.go(5 hunks)pkg/core/backup_test.go(1 hunks)pkg/core/restore.go(2 hunks)tests/integration/dependencies/dependencies_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/core/backup_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
tests/integration/dependencies/dependencies_test.gopkg/core/restore.gopkg/common/utils.gopkg/core/backup.gopkg/common/utils_test.go
🧬 Code graph analysis (2)
pkg/core/restore.go (1)
pkg/common/utils.go (3)
ShouldEndPluginExecution(796-820)UpdateNodepools(647-681)UpdateHostedCluster(606-644)
pkg/common/utils.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
🔇 Additional comments (1)
tests/integration/dependencies/dependencies_test.go (1)
15-21: Version-based dependency check refactor looks solidSwitching to a simple
[]stringof module paths and comparing the go.mod version directly againstgetLatestDependencyVersion(module)makes this test much easier to read and reason about. The summary aggregation withfailureMessagesalso gives clear, actionable output for any outdated dependency. No functional issues spotted here.Also applies to: 32-63
jparrill
force-pushed
the
OCPBUGS-66146
branch
from
10dda6c to
e1145b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/dependencies/dependencies_test.go (2)
31-72: Summarizing flag can staytrueeven when version resolution failsRight now
allDependenciesUpToDateis only flipped tofalsewhencurrentVersion != latestVersion. IfgetCurrentDependencyVersionorgetLatestDependencyVersionfail (e.g., I/O issues,gotool problems), Gomega will correctly fail the subtest, butallDependenciesUpToDateremainstrue, so the final summary can misleadingly log that all dependencies are up to date.Consider also setting
allDependenciesUpToDate = falseand appending a message in the error paths (after you detect a non-nilerror an emptycurrentVersion), so the aggregate summary reflects any failure mode, not just mismatched versions.
75-121: Usego listfor current version as well to avoid fragile go.mod parsing
getCurrentDependencyVersionmanually scansgo.modlines and assumes they start with"<module> ". This can break or return incorrect data if:
- The dependency appears on a single-line
require(e.g.,require github.com/... vX.Y.Z), or- There are
replacedirectives for the same module (you may end up returning" =>"as the “version”).Since
getLatestDependencyVersionalready shells out togo list, you can make the whole check more robust and maintainable by usinggo listfor the current version too, for example:-func getCurrentDependencyVersion(module string) (string, error) { - // existing go.mod scanning logic... -} +func getCurrentDependencyVersion(module string) (string, error) { + cmd := exec.Command("go", "list", "-m", "-mod=mod", "-f", "{{.Version}}", module) + output, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("failed to get current version for %s: %w", module, err) + } + version := strings.TrimSpace(string(output)) + if version == "" { + return "", fmt.Errorf("empty version from go list for %s", module) + } + return version, nil +}This way both “current” and “latest from main” versions come from the same authoritative source and you avoid depending on go.mod’s exact formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (8)
go.mod(3 hunks)guestKubeconfig(0 hunks)pkg/common/utils.go(5 hunks)pkg/common/utils_test.go(8 hunks)pkg/core/backup.go(5 hunks)pkg/core/backup_test.go(1 hunks)pkg/core/restore.go(2 hunks)tests/integration/dependencies/dependencies_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- guestKubeconfig
✅ Files skipped from review due to trivial changes (1)
- pkg/core/backup_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/core/restore.go
- go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/common/utils_test.gopkg/core/backup.gotests/integration/dependencies/dependencies_test.gopkg/common/utils.go
🧬 Code graph analysis (3)
pkg/common/utils_test.go (2)
pkg/common/utils.go (1)
ShouldEndPluginExecution(796-820)pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
pkg/core/backup.go (2)
pkg/common/utils.go (4)
ShouldEndPluginExecution(796-820)AddAnnotation(710-717)UpdateHostedCluster(606-644)UpdateNodepools(647-681)pkg/common/types.go (3)
HostedClusterKind(35-35)HostedClusterRestoredFromBackupAnnotation(27-27)ClusterDeploymentKind(40-40)
pkg/common/utils.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
🔇 Additional comments (13)
tests/integration/dependencies/dependencies_test.go (1)
15-21: Slice-based watchedDependencies definition looks goodMoving to a simple
[]stringforwatchedDependencieswith clear comments improves readability and keeps the test intent obvious. No functional issues here.pkg/common/utils_test.go (3)
135-200: LGTM! Comprehensive unpause test coverage.The new test cases thoroughly validate unpause scenarios including edge cases (already unpaused, mixed states). This ensures the unified update logic in
updateHypershiftResourcehandles all pause/unpause transitions correctly.
222-227: Verification logic correctly enforces pause/unpause contract.The updated assertions properly validate that empty
pausedstring maps tonilPausedUntil (unpause) and non-empty maps to a pointer (pause), matching the implementation inupdateHypershiftResource.Also applies to: 382-387
4272-4415: LGTM! Thorough audit annotation test coverage.The new tests validate both adding and removing pause audit annotations, including RFC3339 timestamp format verification and preservation of existing annotations. This ensures the atomic annotation management works correctly.
pkg/common/utils.go (4)
477-489: LGTM! Correctly handles pause and unpause validation.The updated logic properly distinguishes between unpause (expecting
PausedUntilto benil) and pause (expectingPausedUntilto equal the paused value), with appropriate log messages for each case.
502-603: LGTM! Well-designed unified update logic.The generalized
updateHypershiftResourcefunction eliminates code duplication and provides a single, robust update path for both HostedCluster and NodePool resources. Key strengths:
- Callback-based design enables type-specific handling while sharing common logic
- Idempotent operations via
needsUpdatecheck prevent unnecessary updates- Atomic audit annotation management alongside
PausedUntilupdates- Proper conflict retry with exponential backoff (Steps: 10, Cap: 30s)
- Optional
postUpdateHookenables HostedCluster propagation validation
730-743: LGTM! Clean audit annotation helpers.The helpers ensure atomic management of both
paused-byandpaused-atannotations. Using RFC3339 timestamp format is appropriate for audit trails (standard, sortable, human-readable).
796-819: Signature change properly enhances error reporting.The updated signature
(bool, error)allows better diagnostics for configuration errors (no namespaces) and infrastructure issues (CRD check failures). The substring matching viastrings.Containscorrectly handles both short and long resource names (e.g., "hostedcluster" matches both "hostedcluster" and "hostedclusters.hypershift.openshift.io").Note: This is a breaking change, but tests and callers (backup.go:167) are updated accordingly.
pkg/core/backup.go (5)
47-49: LGTM! Progress throttling reduces log noise.The new fields and
shouldLogProgresshelper effectively throttle progress logs to every 5% change or 30-second interval, preventing excessive logging during backup operations while maintaining visibility.Also applies to: 135-160
217-222: Resource handling updates align with PR objectives.
- Adding
HostedClusterRestoredFromBackupAnnotationprepares the HostedCluster for restore operations- Pod FSBackup label logic correctly applies when
DefaultVolumesToFsBackupisfalse- Agent platform handling simplified by removing Migration flag check
Also applies to: 225-235, 238-243
287-335: Progress-driven pause/unpause logic with safety nets.The implementation introduces clear thresholds:
- Pause at progress >0% and ≤65% (both resources must be unpaused)
- Unpause at progress >85% (if either resource is paused)
- Force unpause on backup failure states
The thresholds (65%, 85%) are based on empirical observation that PV backups typically complete by mid-backup and provide a safety margin before completion, as noted in past review comments.
The condition logic correctly ensures atomic pause/unpause of both HostedCluster and NodePool resources.
338-350: LAST MILE safety net ensures resource cleanup.The final cleanup at backup completion guarantees that HostedCluster and NodePool resources are unpaused regardless of internal state tracking. This is the critical safety net mentioned in PR objectives.
Note: If unpause fails at this stage, the code returns
item, nil, nilto allow backup completion (since the backup data itself is complete), rather than failing the final item. This pragmatic choice prevents backup failure due to cleanup issues.
355-391: State management is safe via idempotency and safety net.The
pauseAllandunPauseAllhelpers update state flags only when both operations succeed. If one operation fails:
- Internal state (
p.hcPaused,p.npPaused) remains unchanged- Next iteration retries both operations
- Successful operations are no-ops (idempotent check in
updateHypershiftResource)This design is safe because:
- Operations are idempotent (skip update if already in desired state)
- LAST MILE cleanup (line 340) bypasses internal state and always attempts unpause
The use of
"true"for pause and""for unpause is clear from context but could be more explicit with constants.
jparrill
force-pushed
the
OCPBUGS-66146
branch
from
e1145b4 to
12a0c7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/core/restore.go (1)
144-147: Fix logging call –Infoignores%vformat verb.
logrus.Infodoesn’t interpolate format verbs, so the error is never printed. UseInfofor structured logging instead.- if returnEarly, err := common.ShouldEndPluginExecution(ctx, backup, p.client, p.log); returnEarly { - p.log.Info("Skipping hypershift plugin execution - not a hypershift backup: %v", err) + if returnEarly, err := common.ShouldEndPluginExecution(ctx, backup, p.client, p.log); returnEarly { + p.log.WithError(err).Info("Skipping hypershift-oadp-plugin execution") return velero.NewRestoreItemActionExecuteOutput(input.Item), nil }pkg/core/backup.go (1)
167-170: Fix logging call –Infoignores%vformat verb.Same as in restore:
logrus.Infowon’t interpolate%v, so the error is dropped. Switch to a formatting or structured call.- if returnEarly, err := common.ShouldEndPluginExecution(ctx, backup, p.client, p.log); returnEarly { - p.log.Info("Skipping hypershift plugin execution - not a hypershift backup: %v", err) - return item, nil, nil - } + if returnEarly, err := common.ShouldEndPluginExecution(ctx, backup, p.client, p.log); returnEarly { + p.log.WithError(err).Info("Skipping hypershift-oadp-plugin execution") + return item, nil, nil + }
🧹 Nitpick comments (4)
tests/integration/dependencies/dependencies_test.go (2)
24-63: Subtest pattern and aggregated summary are sound; watch for future parallelizationThe per-module
t.Runplus aggregatedallDependenciesUpToDate/failureMessagespattern is clear and makes failures easy to diagnose. This shared state is safe as long as subtests remain non-parallel; if you later addt.Parallel()in these subtests, you’ll want to refactor to avoid concurrent writes to those variables (e.g., by returning results from a helper instead of mutating outer scope).
105-121: Makego listparsing more robust and surface stderr for easier debuggingThis helper is reasonable, but you can harden it a bit:
- Avoid depending on the default
go listoutput shape by using a format string to return only the version (e.g.,-f '{{.Version}}').- When
go listfails, including stderr in the error message makes CI failures much easier to diagnose.For example:
-func getLatestDependencyVersion(module string) (string, error) { - cmd := exec.Command("go", "list", "-mod=mod", "-m", module+"@main") - output, err := cmd.Output() - if err != nil { - return "", fmt.Errorf("failed to get latest version: %w", err) - } - - parts := strings.Fields(string(output)) - if len(parts) >= 2 { - return parts[1], nil - } - - return "", fmt.Errorf("unexpected output format from go list: %s", string(output)) -} +func getLatestDependencyVersion(module string) (string, error) { + cmd := exec.Command("go", "list", "-mod=mod", "-m", "-f", "{{.Version}}", module+"@main") + output, err := cmd.CombinedOutput() + if err != nil { + return "", fmt.Errorf("failed to get latest version for %s: %w (output: %s)", module, err, strings.TrimSpace(string(output))) + } + + version := strings.TrimSpace(string(output)) + if version == "" { + return "", fmt.Errorf("unexpected empty version from go list for %s (output: %s)", module, strings.TrimSpace(string(output))) + } + return version, nil +}Please verify this matches the behavior of
go listwith the Go toolchain version you’re using in CI.pkg/core/backup.go (1)
216-223: Confirm whether “restored-from-backup” annotation should be set during backup.
HostedClusterRestoredFromBackupAnnotationis now added in the backup plugin as well as during restore. Given the name, this may be intended only for restored clusters; if other components key off this annotation, setting it at backup time could be misleading.Please double‑check that this annotation is meant to exist on the original HostedCluster before any restore occurs.
pkg/common/utils.go (1)
477-489: Unified pause/unpause utilities and audit annotations are consistent with callers and tests.
updateHypershiftResourceplusUpdateHostedCluster/UpdateNodepools, the updatedWaitForPausedPropagatedsemantics, and the new pause‑audit helpers (addPauseAuditAnnotations/removePauseAuditAnnotations) line up with the new backup/restore flows and the tests inutils_test.go. TheShouldEndPluginExecutionchange to(bool, error)also matches the updated call‑sites.If you expect additional paused states beyond
""and"true"in the future, consider validating or enumerating allowed values insideupdateHypershiftResourceto avoid silently doing a no‑op update for unexpected strings.Also applies to: 502-603, 605-681, 730-743, 793-820
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (8)
go.mod(3 hunks)guestKubeconfig(0 hunks)pkg/common/utils.go(5 hunks)pkg/common/utils_test.go(8 hunks)pkg/core/backup.go(5 hunks)pkg/core/backup_test.go(1 hunks)pkg/core/restore.go(2 hunks)tests/integration/dependencies/dependencies_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- guestKubeconfig
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/core/backup.gopkg/core/restore.gopkg/core/backup_test.gopkg/common/utils_test.gotests/integration/dependencies/dependencies_test.gopkg/common/utils.go
🧬 Code graph analysis (4)
pkg/core/backup.go (2)
pkg/common/utils.go (4)
ShouldEndPluginExecution(796-820)AddAnnotation(710-717)UpdateHostedCluster(606-644)UpdateNodepools(647-681)pkg/common/types.go (3)
HostedClusterKind(35-35)HostedClusterRestoredFromBackupAnnotation(27-27)ClusterDeploymentKind(40-40)
pkg/core/restore.go (1)
pkg/common/utils.go (3)
ShouldEndPluginExecution(796-820)UpdateNodepools(647-681)UpdateHostedCluster(606-644)
pkg/core/backup_test.go (1)
pkg/core/backup.go (1)
BackupPlugin(27-51)
pkg/common/utils_test.go (2)
pkg/common/utils.go (1)
ShouldEndPluginExecution(796-820)pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
🔇 Additional comments (5)
tests/integration/dependencies/dependencies_test.go (1)
15-21: Watched dependencies configuration is simple and extensibleUsing a
[]stringplus clear comments to list module paths keeps the configuration straightforward and easy to extend; no issues here.pkg/core/restore.go (1)
178-187: Unpause semantics viapaused=""look correct.Using
paused == ""for bothUpdateNodepoolsandUpdateHostedClusteris consistent with the new unpause behavior inupdateHypershiftResourceand is covered by the updated tests.pkg/common/utils_test.go (1)
135-201: New pause/unpause and audit annotation tests are solid and aligned with behavior.The added scenarios for HostedCluster/NodePool pause/unpause, ShouldEndPluginExecution error expectations, and the RFC3339 + preservation checks for pause audit annotations all match the implementation in
utils.goand materially improve safety around these flows.Also applies to: 222-227, 292-360, 382-387, 893-965, 4272-4415
pkg/core/backup_test.go (1)
19-142: BackupPlugin progress and pause/unpause tests cover critical paths well.The tests exercise throttled progress logging and the coordinated pause/unpause state transitions across realistic initial conditions using a fake client; they look correct and match the new backup logic.
Also applies to: 144-275, 277-398
pkg/core/backup.go (1)
47-49: Progress throttling and PAUSE/UNPAUSE orchestration look coherent.The
lastLoggedProgress/lastLogTimethrottling, error‑phase forced unpause, progress‑window PAUSE/UNPAUSE thresholds, and the “last mile” unpause safety net integrate cleanly with the underlying utilities and tests; once the small issues above are fixed, the overall flow should be robust.Also applies to: 134-160, 287-350
jparrill
force-pushed
the
OCPBUGS-66146
branch
2 times, most recently
from
e80b2af to
9804fcf
Compare
- Update hypershift/api dependency to latest version - Update associated Go dependencies and vendor files 🚀 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Juan Manuel Parrilla Madrid <[email protected]>
jparrill
force-pushed
the
OCPBUGS-66146
branch
from
9804fcf to
50acd97
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/core/backup_test.go (1)
277-398: LGTM with same suggestion as pauseAll tests.The unpause tests follow the same pattern as the pause tests. They verify internal state correctly but would benefit from also verifying that the actual resources were updated (i.e.,
PausedUntilisnilafter unpause).pkg/core/restore.go (1)
144-146: Correctly adapted to new ShouldEndPluginExecution signature.The code properly handles the updated signature that returns
(bool, error). The early return withitem, nilwhen not applicable correctly implements the Velero plugin contract.Note: The logging issue on line 145 was already flagged in previous reviews (should use
Infofinstead ofInfowith format specifier).pkg/core/backup.go (1)
355-391: Consider atomic flag updates in pauseAll/unPauseAll.Per past review comments (already flagged as issue), the current implementation updates
hcPausedafter the HC operation succeeds andnpPausedafter the NP operation succeeds. If the second operation fails, the plugin is left in a partial state.A safer approach is to only update flags after both operations succeed:
func (p *BackupPlugin) pauseAll(ctx context.Context, backup *velerov1.Backup) error { p.log.Info("Pausing HostedClusters with audit annotations...") if err := common.UpdateHostedCluster(ctx, p.client, p.log, "true", backup.Spec.IncludedNamespaces); err != nil { return fmt.Errorf("error pausing HostedClusters: %v", err) } - p.hcPaused = true p.log.Info("Pausing NodePools with audit annotations...") if err := common.UpdateNodepools(ctx, p.client, p.log, "true", backup.Spec.IncludedNamespaces); err != nil { return fmt.Errorf("error pausing NodePools: %v", err) } - p.npPaused = true + + // Only update flags after both operations succeed + p.hcPaused = true + p.npPaused = true return nil }Apply the same pattern to
unPauseAll.
🧹 Nitpick comments (3)
pkg/core/backup_test.go (1)
144-275: Consider verifying actual resource updates.The tests verify internal plugin state (
hcPaused,npPaused) but don't confirm that the actual Kubernetes resources were updated withPausedUntil = "true". This means the tests could pass even ifUpdateHostedCluster/UpdateNodepoolsfailed to set the field correctly.Consider adding assertions like:
// After pauseAll completes, verify actual resources were updated for _, hc := range tt.hcList.Items { updatedHC := &hyperv1.HostedCluster{} err := client.Get(context.TODO(), types.NamespacedName{Name: hc.Name, Namespace: hc.Namespace}, updatedHC) g.Expect(err).NotTo(HaveOccurred()) if tt.expectedHCPaused { g.Expect(updatedHC.Spec.PausedUntil).To(Equal(ptr.To("true"))) } }pkg/core/backup.go (2)
130-132: Consider resource filtering in AppliesTo for efficiency.Returning an empty
ResourceSelectormeans Velero will callExecute()for every resource in the backup. While the early return viaShouldEndPluginExecutionprevents processing non-Hypershift resources, this approach generates unnecessary overhead.Consider filtering to Hypershift-specific resources:
func (p *BackupPlugin) AppliesTo() (velero.ResourceSelector, error) { return velero.ResourceSelector{ IncludedResources: []string{ "hostedclusters", "hostedcontrolplanes", "nodepools", "clusterdeployments", // other Hypershift resources }, }, nil }This would prevent
Execute()from being called for unrelated resources.
297-312: Good safety net for failed backup states.The logic to unpause resources when the backup enters failed/partially-failed states (lines 297-312) is an important safety measure. This prevents resources from remaining paused if the backup fails before the normal unpause threshold.
One consideration: should this log at Warn level instead of Info to highlight the abnormal state?
-p.log.Infof("WARNING: Backup status is %s forcing UNPAUSE logic", backup.Status.Phase) +p.log.Warnf("Backup status is %s, forcing UNPAUSE logic", backup.Status.Phase)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (116)
go.sumis excluded by!**/*.sumvendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_cloudcredential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubecontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_kubestorageversionmigrator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_olm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_serviceca.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogapiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_servicecatalogcontrollermanager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/aws.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/groupversion_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/escape.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/parse.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/render.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go125.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_go126.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/config_pre_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/gotrack.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/timer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/fdset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ifreq_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/unicode/bidi/core.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (8)
go.mod(3 hunks)guestKubeconfig(0 hunks)pkg/common/utils.go(5 hunks)pkg/common/utils_test.go(8 hunks)pkg/core/backup.go(5 hunks)pkg/core/backup_test.go(1 hunks)pkg/core/restore.go(2 hunks)tests/integration/dependencies/dependencies_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- guestKubeconfig
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/core/restore.gopkg/core/backup_test.gotests/integration/dependencies/dependencies_test.gopkg/common/utils_test.gopkg/core/backup.gopkg/common/utils.go
🧬 Code graph analysis (5)
pkg/core/restore.go (1)
pkg/common/utils.go (3)
ShouldEndPluginExecution(796-820)UpdateNodepools(647-681)UpdateHostedCluster(606-644)
pkg/core/backup_test.go (1)
pkg/core/backup.go (1)
BackupPlugin(27-51)
pkg/common/utils_test.go (2)
pkg/common/utils.go (1)
ShouldEndPluginExecution(796-820)pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
pkg/core/backup.go (2)
pkg/common/utils.go (4)
ShouldEndPluginExecution(796-820)AddAnnotation(710-717)UpdateHostedCluster(606-644)UpdateNodepools(647-681)pkg/common/types.go (3)
HostedClusterKind(35-35)HostedClusterRestoredFromBackupAnnotation(27-27)ClusterDeploymentKind(40-40)
pkg/common/utils.go (1)
pkg/common/types.go (3)
OADPPausedByAnnotation(30-30)HypershiftOADPPluginName(32-32)OADPPausedAtAnnotation(31-31)
🔇 Additional comments (19)
pkg/core/backup_test.go (1)
19-142: LGTM! Comprehensive throttling logic tests.The test cases thoroughly cover the shouldLogProgress behavior including first-time logging, threshold-based logging (5% increments), time-based logging (30-second intervals), and the edge case of progress decrease. The verification of both return values and state mutations (lastLoggedProgress, lastLogTime) is well done.
tests/integration/dependencies/dependencies_test.go (1)
15-121: LGTM! Version-based dependency checks are clearer.The switch from commit-hash-based comparison to direct version comparison simplifies the test logic and makes it more maintainable. Using
go list -mod=mod -m module@mainto get the latest version is the right approach for checking if dependencies are up-to-date.pkg/common/utils.go (6)
477-489: LGTM! Correctly handles both pause and unpause.The updated logic properly distinguishes between unpause (expecting
PausedUntil == nil) and pause (expectingPausedUntil == paused). The debug log messages clearly indicate which state is being checked.
502-603: Excellent refactoring - unified update logic eliminates duplication.The
updateHypershiftResourcefunction is a well-designed abstraction that consolidates the previously separateUpdateHostedClusterandUpdateNodepoolslogic. Key improvements:
- Exponential backoff with conflict retry (10 steps, 500ms→30s cap) handles concurrent updates
- Atomic annotation management with audit trail (paused-by, paused-at)
- Conditional post-update hook allows HC-specific propagation validation
- Factory pattern enables type-safe reuse for both resource types
730-743: LGTM! Clean audit annotation management.The helper functions properly add/remove OADP audit annotations with RFC3339 timestamps. Using constants from
types.goensures consistency across the codebase.
605-644: LGTM! Clean delegation to unified update logic.The refactored
UpdateHostedClusternow delegates toupdateHypershiftResourcewith HostedCluster-specific callbacks. The inclusion of thepostUpdateHookfor propagation validation (viaWaitForPausedPropagated) is appropriate for HostedClusters.
646-681: LGTM! Appropriate to skip propagation check for NodePools.The refactored
UpdateNodepoolsfollows the same pattern asUpdateHostedClusterbut correctly setspostUpdateHooktonilsince NodePools don't require propagation validation to HostedControlPlane.
796-819: LGTM! Enhanced error reporting with (bool, error) signature.The updated signature provides better error context to callers. The broadened resource matching (line 803-805) using
strings.Containsappropriately handles both short and full resource names (including singular forms like "hostedcluster" and "nodepool").pkg/core/restore.go (1)
180-186: Correct use of empty string for unpause.The change from
"false"to""for unpausing resources aligns with the updated convention where empty string signals unpause (settingPausedUntiltonil), and"true"signals pause.pkg/common/utils_test.go (4)
135-200: Excellent test coverage for unpause scenarios.The addition of unpause test cases (lines 135-200) and mixed-state scenarios (lines 174-200) significantly improves coverage. The updated verification logic (lines 222-227) correctly distinguishes between unpaused (
nil) and paused states (ptr.To(paused)).Also applies to: 222-227
292-360: Good parallel test coverage for NodePool unpause.The NodePool tests follow the same comprehensive pattern as the HostedCluster tests, covering unpause, already-unpaused, and mixed-state scenarios. The verification logic correctly checks for
nilwhen unpaused.Also applies to: 382-387
893-967: Properly updated for new ShouldEndPluginExecution signature.The test expectations correctly include the
expectErrorfield to verify that errors are returned in appropriate scenarios (no namespaces, no resources, no CRD). Test case on line 920-922 correctly expects error when resources don't include Hypershift types.
4272-4415: Comprehensive audit annotation tests.The new tests for
addPauseAuditAnnotationsandremovePauseAuditAnnotationsthoroughly verify:
- Adding annotations to objects with/without existing annotations (lines 4281-4312)
- RFC3339 timestamp format validation (line 4327)
- Preservation of existing annotations (lines 4330-4335)
- Removal while preserving non-audit annotations (lines 4364-4382)
pkg/core/backup.go (6)
47-49: LGTM! Clean progress logging throttle implementation.The throttling logic (lines 134-160) effectively reduces log spam by only logging when:
- Progress increases by 5% or more, OR
- 30 seconds have elapsed since the last log
The state tracking via
lastLoggedProgressandlastLogTime(lines 47-49) is minimal and correctly updated only when logging occurs.Also applies to: 134-160
315-334: Partial-failure retry behavior needs verification.Per past review comments, if HostedCluster pauses successfully but NodePool fails (line 319-322),
hcPausedbecomestruewhilenpPausedremainsfalse. The gate condition!p.hcPaused || !p.npPaused(line 317) would prevent retry on the next iteration.Wait - looking at line 317, the condition is
!p.hcPaused || !p.npPaused. This uses OR, not AND. Let me recalculate:
- Initial: hcPaused=false, npPaused=false →
!false || !false=true || true=true✓ (enters block)- After HC succeeds, NP fails: hcPaused=true, npPaused=false →
!true || !false=false || true=true✓ (enters block on next call)Actually, the OR logic is correct - it will retry as long as either resource is not paused. However, there's still an issue: if HC pauses successfully and NP fails, the function returns an error, so both flags may not be set correctly.
Looking at pauseAll (lines 355-373), I see:
- Line 358-362: Tries to pause HC, returns error if it fails, then sets hcPaused=true
- Line 366-370: Tries to pause NP, returns error if it fails, then sets npPaused=true
So if HC succeeds but NP fails, hcPaused=true but the function returns error before setting npPaused=true. On retry, hcPaused=true, npPaused=false, so
!true || !false= true, enters block again, tries to pause HC (which is already paused but UpdateHostedCluster should handle this), then tries NP again.Actually, this could cause repeated pause attempts on already-paused HC. The UpdateHostedCluster function should skip the update if already paused, but that adds overhead.
The issue from the past review is valid - the partial state handling could be improved.
337-350: Last mile cleanup provides good safety net.The final cleanup at 100% completion (lines 337-350) ensures resources are always unpaused regardless of internal state tracking. This is a critical safety mechanism that prevents resources from being left paused if earlier unpause attempts failed.
The check
ItemsBackedUp == TotalItemscorrectly identifies the last object being processed.
216-222: Restored HostedCluster annotation logic looks correct.The annotation logic for HostedCluster has been properly restored. Adding the
HostedClusterRestoredFromBackupAnnotationis appropriate for tracking which clusters were restored from backup.
238-243: Removed Migration flag check - verify if intentional.The code now always runs
MigrationTasksforAgentPlatformregardless of the Migration flag. The AI summary notes this is intentional ("always run MigrationTasks for AgentPlatform case"), but this changes behavior.Was this change intentional? If the Migration flag should still gate the migration tasks, the condition should be:
case kind == common.ClusterDeploymentKind: - if p.hcp.Spec.Platform.Type == hyperv1.AgentPlatform { + if p.hcp.Spec.Platform.Type == hyperv1.AgentPlatform && p.Migration { if err := agent.MigrationTasks(ctx, item, p.client, p.log, p.config, backup); err != nil { return nil, nil, fmt.Errorf("error performing migration tasks for agent platform: %v", err) } }
287-335: Add comments explaining empirically-derived pause/unpause thresholds.The 0-65% pause and >85% unpause thresholds are based on observed PV backup behavior but lack inline documentation. Document the rationale for these empirical values and clarify that the 65-85% gap is intentional (resources remain paused during PV backup completion):
// PAUSE when progress > 0% and <= 65% // Rationale: PV backups typically start around 10% and complete by 65% case progressPercent > 0.0 && progressPercent <= 65.0:Also explain why the 65-85% range has no explicit handling to make the design intention clear to future maintainers.
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Hey @jparrill , I tested this with my custom branch where I delete Velero Pod when the progress exceeds 86%. The results are below. The HC sometimes doesn't get unpaused. I tested only the scenario with killing Velero pod when the progress is over 85%: Branch: https://github.com/mgencur/oadp-operator/tree/test_unpausing_safety_net Test log: Backup status: Velero Pod before fail logs only progress at 6.0% (not any more progress): Velero Pod after fail (the new Pod) logs: Note: Please note the hcPaused==false in the log above but the HC is actually paused: Suggestion: read the value of hcPaused / hcUnpaused from the HC itself at the beginning or do not check if it's already paused. Just force it. I have also tested deleting the BackupStorageLocation secret, with similar result. Velero logs that are relevant: There's only one progress check logged and never again, see the following grep: HC: |
| if err := common.UpdateHostedCluster(ctx, p.client, p.log, "false", backup.Spec.IncludedNamespaces); err != nil { | ||
| return nil, nil, fmt.Errorf("error updating HostedClusters: %v", err) | ||
| switch backup.Status.Phase { | ||
| case velerov1.BackupPhaseFinalizingPartiallyFailed, |
There was a problem hiding this comment.
I have one more question about this early check. Is it possible that there will be a failed backup from the past (e.g. when we use Schedule with periodic backups) and this code will encounter this failed backup and unpause HostedCluster/NodePool even if another (later) backup is currently in progress?
There was a problem hiding this comment.
no, the backup object is fresh from the velero current execution, I'm not querying the backup but using what velero provides in the framework.
Regarding pause/unpause, it cannot execute multiple backups at the same time so no issues without that older failed backup keep the HC in pause/unpause. If another backup was raised, the plugin will make sure the HC/NP stays in the right state.
|
@jparrill: This pull request references Jira Issue OCPBUGS-66146, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. In 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. |
|
/retest |
3 participants
Summary
Implements audit annotations and unified pause/unpause logic for Hypershift OADP backup operations to improve cluster state management during backup processes.
Related PRs
Changes
oadp.openshift.io/paused-by,oadp.openshift.io/paused-at)Test plan
Fixes
🚀 Generated with Claude Code