OADP filtering via labelSelector and orLabelSelectors#186
OADP filtering via labelSelector and orLabelSelectors#186michalskrivanek wants to merge 4 commits intoopenshift:mainfrom
Conversation
When multiple HostedClusters coexist in a shared namespace, the plugin previously paused all HCs/NPs found in the included namespaces regardless of which cluster the backup targeted. This passes the backup's LabelSelector through to the List calls so only matching resources are affected, preventing concurrent backups from interfering with each other. Requires someone to set the right labels and corresponding labelSelector. Signed-off-by: Michal Skrivanek <michal.skrivanek@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Michal Skrivanek <michal.skrivanek@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
noteit only suppots specific pattern
orLabelSelectors:
- matchExpressions: # "unlabeled resources"
- key: hypershift.openshift.io/hosted-control-plane
operator: DoesNotExist
- matchExpressions: # "this cluster's resources"
- key: hypershift.openshift.io/hosted-control-plane
operator: In
values: ["cluster-1"]
Signed-off-by: Michal Skrivanek <michal.skrivanek@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
WalkthroughAdds a ResourceFilter type (Velero-style AND/OR label selectors), a constructor and Matches method; threads the filter into UpdateHostedCluster, UpdateNodepools and updateHypershiftResource; applies client-side label filtering when listing resources and updates call sites in backup and restore to build and pass the filter. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: michalskrivanek 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 |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @michalskrivanek. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/common/utils_test.go (2)
212-213: Avoid silently ignoring the error fromNewResourceFilter.
NewResourceFilter(nil, nil)never errors today, but the_, _pattern is easily copied into cases where the selector could be invalid. Assign the error and assert it.♻️ Proposed fix (representative — apply consistently at all four call sites)
-filter, _ := NewResourceFilter(nil, nil) +filter, err := NewResourceFilter(nil, nil) +g.Expect(err).NotTo(HaveOccurred())Also applies to: 373-374, 455-455, 533-533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/utils_test.go` around lines 212 - 213, The tests currently call NewResourceFilter(nil, nil) and ignore its error return with the "_, _" pattern; change each call (including the ones around UpdateHostedCluster in the provided snippet and the other three occurrences) to capture the error (e.g., filter, err := NewResourceFilter(...)) and add an assertion/check that err is nil before proceeding (fail the test if non-nil) so invalid selector errors are not silently ignored; keep references to NewResourceFilter and UpdateHostedCluster to locate the call sites.
396-550: Missing test coverage fororLabelSelectors— the core new feature.The PR commits state support for
orLabelSelectors(e.g.,DoesNotExistto select unlabeled resources ORInto select a named cluster), butTestLabelSelectorHostedClusterandTestLabelSelectorNodepoolsonly exercise singlelabelSelector. AnorLabelSelectorstest case such as the one below would directly validate the OR-semantics path throughNewResourceFilterandMatches:// Example test case to add to TestLabelSelectorHostedCluster.tests { name: "orLabelSelectors selects unlabeled OR cluster dummy-1", orLabelSelectors: []*metav1.LabelSelector{ // arm 1: unlabeled (DoesNotExist) {MatchExpressions: []metav1.LabelSelectorRequirement{ {Key: "my-label/cluster-name", Operator: metav1.LabelSelectorOpDoesNotExist}, }}, // arm 2: cluster dummy-1 (In) {MatchExpressions: []metav1.LabelSelectorRequirement{ {Key: "my-label/cluster-name", Operator: metav1.LabelSelectorOpIn, Values: []string{"dummy-1"}}, }}, }, expectPaused: map[string]bool{"dummy-1": true, "dummy-2": false, "dummy-3": true}, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/utils_test.go` around lines 396 - 550, Tests for orLabelSelectors are missing; add cases exercising OR semantics so NewResourceFilter and its Matches path are validated. Update TestLabelSelectorHostedCluster and TestLabelSelectorNodepools to include a test case with an orLabelSelectors slice (e.g., one arm with MatchExpressions using LabelSelectorOpDoesNotExist for "my-label/cluster-name" and another arm using LabelSelectorOpIn with value "dummy-1"), call NewResourceFilter(...) with that orLabelSelectors, run UpdateHostedCluster/UpdateNodepools, and assert expectations per resource name to ensure or-logic selects unlabeled resources OR the named cluster/nodepool as intended.pkg/core/restore.go (1)
178-189: Consider hoisting the filter out of the per-itemExecutepath.The filter is reconstructed from the same (immutable)
input.Restore.Specon every invocation ofExecutefor aMainKindsresource. The cost is negligible today, but caching the compiled*common.ResourceFilteronRestorePlugin(analogous to howBackupPlugincould cache its filter) would make the intent clearer and avoid repeated parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/restore.go` around lines 178 - 189, The code repeatedly reconstructs the ResourceFilter inside Execute for MainKinds; hoist and cache it on RestorePlugin instead so it's built once from input.Restore.Spec.LabelSelector/OrLabelSelectors and reused. Add a field (e.g., resourceFilter *common.ResourceFilter) to RestorePlugin, initialize it when the plugin is created (or lazily on first Execute by calling common.NewResourceFilter with input.Restore.Spec.LabelSelector and OrLabelSelectors), and replace the local NewResourceFilter call in Execute with the cached RestorePlugin.resourceFilter before calling common.UpdateNodepools and common.UpdateHostedCluster.pkg/common/utils.go (1)
614-647: Add adefaultcase to thepausedswitch to avoid a silent no-op update.When
pausedis neither""nor"true"(e.g., an RFC3339 timestamp, which is a valid HyperShiftPausedUntilvalue),needsUpdatecan betruebutpausedUntilSetteris never invoked. The code then callsc.Updatewithout mutating the field — the resource appears perpetually stale, burns all 10 retry attempts, and surfaces an error.♻️ Proposed fix
switch paused { case "": log.Infof("setting PauseUntil to nil (unpause) in %s %s", resourceType, current.GetName()) pausedUntilSetter(current, nil) removePauseAuditAnnotations(current) case "true": log.Infof("setting PauseUntil to %s in %s %s", paused, resourceType, current.GetName()) pausedUntilSetter(current, ptr.To(paused)) addPauseAuditAnnotations(current) +default: + // Arbitrary date/time pause value; set the field but skip audit annotations. + log.Infof("setting PauseUntil to %s in %s %s", paused, resourceType, current.GetName()) + pausedUntilSetter(current, ptr.To(paused)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/utils.go` around lines 614 - 647, The switch on paused only handles "" and "true", causing a no-op when paused is an RFC3339 timestamp; add a default case in that switch (near the paused switch handling in the function that computes needsUpdate) that calls pausedUntilSetter(current, ptr.To(paused)) and invokes addPauseAuditAnnotations(current) (same behavior as the "true" branch) so the resource is actually mutated before calling c.Update; ensure this default is used when paused is neither "" nor "true" so needsUpdate results in a real field change and postUpdateHook/c.Update behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/utils.go`:
- Around line 536-547: ResourceFilter.Matches should guard against a nil
receiver to avoid a panic when callers like UpdateHostedCluster or
UpdateNodepools pass a nil *ResourceFilter; update the start of the Matches
method (ResourceFilter.Matches) to check if f == nil (and keep the existing
empty-selectors check) and return true for a nil/empty filter so callers that
pass nil will match safely without dereferencing f.selectors.
---
Nitpick comments:
In `@pkg/common/utils_test.go`:
- Around line 212-213: The tests currently call NewResourceFilter(nil, nil) and
ignore its error return with the "_, _" pattern; change each call (including the
ones around UpdateHostedCluster in the provided snippet and the other three
occurrences) to capture the error (e.g., filter, err := NewResourceFilter(...))
and add an assertion/check that err is nil before proceeding (fail the test if
non-nil) so invalid selector errors are not silently ignored; keep references to
NewResourceFilter and UpdateHostedCluster to locate the call sites.
- Around line 396-550: Tests for orLabelSelectors are missing; add cases
exercising OR semantics so NewResourceFilter and its Matches path are validated.
Update TestLabelSelectorHostedCluster and TestLabelSelectorNodepools to include
a test case with an orLabelSelectors slice (e.g., one arm with MatchExpressions
using LabelSelectorOpDoesNotExist for "my-label/cluster-name" and another arm
using LabelSelectorOpIn with value "dummy-1"), call NewResourceFilter(...) with
that orLabelSelectors, run UpdateHostedCluster/UpdateNodepools, and assert
expectations per resource name to ensure or-logic selects unlabeled resources OR
the named cluster/nodepool as intended.
In `@pkg/common/utils.go`:
- Around line 614-647: The switch on paused only handles "" and "true", causing
a no-op when paused is an RFC3339 timestamp; add a default case in that switch
(near the paused switch handling in the function that computes needsUpdate) that
calls pausedUntilSetter(current, ptr.To(paused)) and invokes
addPauseAuditAnnotations(current) (same behavior as the "true" branch) so the
resource is actually mutated before calling c.Update; ensure this default is
used when paused is neither "" nor "true" so needsUpdate results in a real field
change and postUpdateHook/c.Update behave correctly.
In `@pkg/core/restore.go`:
- Around line 178-189: The code repeatedly reconstructs the ResourceFilter
inside Execute for MainKinds; hoist and cache it on RestorePlugin instead so
it's built once from input.Restore.Spec.LabelSelector/OrLabelSelectors and
reused. Add a field (e.g., resourceFilter *common.ResourceFilter) to
RestorePlugin, initialize it when the plugin is created (or lazily on first
Execute by calling common.NewResourceFilter with
input.Restore.Spec.LabelSelector and OrLabelSelectors), and replace the local
NewResourceFilter call in Execute with the cached RestorePlugin.resourceFilter
before calling common.UpdateNodepools and common.UpdateHostedCluster.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
pkg/common/utils.gopkg/common/utils_test.gopkg/core/backup.gopkg/core/restore.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
What this PR does / why we need it
Which issue(s) this PR fixes
Fixes #
Checklist
Summary by CodeRabbit
New Features
Tests