OCPBUGS-97592: skip plugin execution when backup does not target HCP resources#293
OCPBUGS-97592: skip plugin execution when backup does not target HCP resources#293Alcamech wants to merge 1 commit into
Conversation
…resources Signed-off-by: Lawton Mizell <lmizell@redhat.com>
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@Alcamech: This pull request references Jira Issue OCPBUGS-97592, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
WalkthroughThe ChangesIncludedResources-based execution gating
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Alcamech The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
212-221: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat
*as a match here.Backup.Spec.IncludedResourcescan legally be["*"], but this loop only recognizes explicit HCP resource names, so those backups fall through and get skipped. Short-circuit on*(or otherwise handle the all-resources case) so HCP restores still run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/common/utils.go` around lines 212 - 221, The resource filter in the IncludedResources check currently only matches explicit hostedcluster, hostedcontrolplane, and nodepool names, so backups configured with "*" are incorrectly skipped. Update the logic in the loop inside the relevant utils helper to treat "*" as a valid match for all resources and short-circuit to allow the restore path, while keeping the existing explicit resource-name checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/common/utils.go`:
- Around line 212-221: The resource filter in the IncludedResources check
currently only matches explicit hostedcluster, hostedcontrolplane, and nodepool
names, so backups configured with "*" are incorrectly skipped. Update the logic
in the loop inside the relevant utils helper to treat "*" as a valid match for
all resources and short-circuit to allow the restore path, while keeping the
existing explicit resource-name checks intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a6b9ffa-d7bc-4df2-9ee3-9a79cf7e776d
📒 Files selected for processing (3)
pkg/common/utils.gopkg/core/backup_test.gopkg/core/restore_test.go
|
/uncc @enxebre |
|
/uncc @sjenning |
|
/cc @jparrill |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| @@ -222,16 +218,7 @@ func ShouldEndPluginExecution(ctx context.Context, backup *veleroapiv1.Backup, c | |||
| } | |||
| } | |||
There was a problem hiding this comment.
[blocking] The for loop above doesn't handle Velero's wildcard includedResources: ["*"]. The string "*" doesn't contain "hostedcluster", "hostedcontrolplane", or "nodepool", so a backup configured with ["*"] scoped to HC namespaces would be silently skipped.
for _, resource := range backup.Spec.IncludedResources {
if resource == "*" ||
strings.Contains(resource, "hostedcluster") ||
strings.Contains(resource, "hostedcontrolplane") ||
strings.Contains(resource, "nodepool") {
return false, nil
}
}| } | ||
|
|
||
| return true, fmt.Errorf("no HostedControlPlane CRD found") | ||
| return true, fmt.Errorf("backup does not include hostedcontrolplane resources") |
There was a problem hiding this comment.
[suggestion] This returns an error for a normal operational path — an MC-wide backup not targeting HCP resources is expected behavior, not an error condition. Both callers already swallow the error and return nil to Velero, so this doesn't cause PartiallyFailed, but semantically it's cleaner to return nil:
return true, nilThe callers at backup.go:142 and restore.go:167 already log the skip at Info level, so observability is preserved.
Also, after removing the CRD check, ctx, c, and log are unused in this function. Worth simplifying the signature to func ShouldEndPluginExecution(backup *veleroapiv1.Backup) (bool, error) and updating the two callers.
| // Returns true if the plugin should end execution (i.e., if this is not a Hypershift cluster). | ||
| // ShouldEndPluginExecution checks whether this backup targets a hosted control | ||
| // plane by looking for HCP resource types in the backup's includedResources. | ||
| // Returns true (skip) when the backup does not target an HCP. |
There was a problem hiding this comment.
[nit] The updated comment doesn't mention the namespace-empty check (first guard). Suggestion:
// ShouldEndPluginExecution checks whether this backup targets a hosted control
// plane. It returns true (skip) when no namespaces are provided or when the
// backup's includedResources does not contain HCP resource types.| ObjectMeta: metav1.ObjectMeta{Name: "test-backup", Namespace: "openshift-adp"}, | ||
| Spec: velerov1.BackupSpec{ | ||
| IncludedNamespaces: []string{"clusters", "clusters-test"}, | ||
| IncludedResources: []string{"hostedcontrolplane", "hostedcluster", "nodepool"}, |
There was a problem hiding this comment.
[suggestion] The dedicated tests in utils_test.go:TestShouldEndPluginExecution aren't updated by this PR. The existing test names ("CRD exists", "CRD does not exist") reference removed behavior, and the key scenario — CRD present but includedResources has no HCP types, which is the actual bug — has no coverage.
Worth adding cases for:
includedResourceswith no HCP types (the fix) → returnstrueincludedResources: ["*"](wildcard) → returnsfalseincludedResources: ["nodepool"]alone → returnsfalseincludedResources: ["hostedcontrolplanes.hypershift.openshift.io"](full GVR) → returnsfalse
Also, the []string{"hostedcontrolplane", "hostedcluster", "nodepool"} slice is repeated 4 times across backup and restore tests — a shared test-level var would reduce duplication.
3 participants
Summary
ShouldEndPluginExecution. The plugin now requiresincludedResourcesto list HCP types (hostedcontrolplane,hostedcluster, ornodepool) to proceed.includedResources: null) are correctly skipped, eliminating ~16K spurious errors and the misleadingPartiallyFailedstatus. Backup data was never affected.Context
ShouldEndPluginExecutionfalls back to a CRD-existence check whenincludedResourcesis empty. On management clusters the HCP CRD is always present, so MC-wide backups (daily-full/weekly-full) are never skipped. The plugin runs on every item (~16K), errors on each because no HostedControlPlane can be associated, and marks the backupPartiallyFailed.Every HC backup schedule on the staging MC (240 schedules) and every example in this repo specifies
includedResourceswith HCP types. MC-wide schedules haveincludedResources: null.Open question for the team: Can we require
includedResourcesto list HCP types for the plugin to run? If an HC backup omitsincludedResources, the plugin would silently skip. All current schedules and examples already include them.Test plan
make verifypassesCompleted(notPartiallyFailed)includedResourcescontaining HCP types proceeds normallyincludedResourcesproceeds normallySummary by CodeRabbit