OADP-6846: Add resourceLabels and resourceAnnotations to DPA spec #2053
+536
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add support for propagating user-defined labels and annotations to all resources managed by the DataProtectionApplication. This allows users to set annotations like `argocd.argoproj.io/ignore-resource-updates` once at the DPA level instead of per-resource. Resources affected: - BackupStorageLocations - VolumeSnapshotLocations - Velero Deployment - NodeAgent DaemonSet and ConfigMap - BackupRepository ConfigMap - RepositoryMaintenance ConfigMap - VeleroMetrics Service - Registry Secrets - NonAdmin Controller Deployment - VMFileRestore Controller Deployment Protected labels (app.kubernetes.io/*, openshift.io/oadp) cannot be overridden by user-provided resourceLabels. Fixes: OADP-6846 Co-Authored-By: Claude <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
|
@shubham-pampattiwar: This pull request references OADP-6846 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
WalkthroughAdds ResourceLabels and ResourceAnnotations to DataProtectionApplicationSpec, updates CRD and deepcopy support, and propagates filtered/merged labels and annotations across multiple controllers and resources with helper functions and unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing touches
Comment |
openshift-ci
bot
added
the
approved
Contributor
|
/retest |
Contributor
|
/test unit-test |
Contributor
|
@shubham-pampattiwar unit test |
Filter out resourceLabels from deployment matchLabels to prevent immutable selector errors on subsequent reconciles. When resourceLabels are applied to the velero deployment, they should only be added to the deployment's metadata.labels and pod template labels, not to the spec.selector.matchLabels which are immutable after creation. This fix ensures that: 1. resourceLabels are filtered before passing to install.Deployment 2. resourceLabels are filtered before adding to matchLabels 3. The velero deployment can be reconciled without triggering immutable selector errors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/controller/nodeagent.go`:
- Around line 508-516: The current merge of user resource labels can change the
DaemonSet's selector alignment; after calling applyResourceLabels(dpa, ...)
ensure the selector's immutable labels are preserved by re-asserting
ds.Spec.Template.Labels to include the original selector entries
(ds.Spec.Selector.MatchLabels) or by filtering protected keys (e.g.,
"component", "name") out of the result of applyResourceLabels before assigning
to ds.Spec.Template.Labels; update the code around ds.Labels,
ds.Spec.Template.Labels, ds.Spec.Selector.MatchLabels and
applyResourceLabels(dpa, ...) so the selector labels remain exactly as in
ds.Spec.Selector.MatchLabels after merging.
In `@internal/controller/nonadmin_controller.go`:
- Around line 137-145: The user-supplied resource labels may overwrite
controller-managed selector labels and break Deployment selector alignment;
after calling applyResourceLabels on deploymentObject.Labels and
deploymentObject.Spec.Template.Labels, re-assert the selector labels from
deploymentObject.Spec.Selector.MatchLabels onto both
deploymentObject.Spec.Template.Labels (and optionally deploymentObject.Labels)
to ensure template labels always include the selector keys, or filter out those
selector keys before applying applyResourceLabels; update the logic around
applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels) to preserve or
restore deploymentObject.Spec.Selector.MatchLabels so the selector and pod
template remain identical.
In `@internal/controller/velero.go`:
- Around line 846-863: filterOutResourceLabels currently deletes every key found
in dpa.Spec.ResourceLabels which allows protected labels to be removed; change
the loop that deletes labels to skip protected keys by checking each key against
the protected set before calling delete. Concretely, update
filterOutResourceLabels to consult the canonical protected-label checker (e.g.,
an isProtectedLabel(key) function or a protectedLabelSet/map) and only call
delete(result, k) when isProtectedLabel(k) returns false, leaving protected keys
intact while still removing non-protected resourceLabels.
In `@internal/controller/vmfilerestore_controller.go`:
- Around line 138-146: After applying user resource labels via
applyResourceLabels to deploymentObject.Labels and
deploymentObject.Spec.Template.Labels, ensure the Deployment's selector labels
are not overridden: re-assert the original selector values back onto
deploymentObject.Spec.Selector.MatchLabels (or remove protected keys from the
applied map) so the controller label (e.g., "control-plane") remains identical
between deploymentObject.Spec.Selector.MatchLabels and
deploymentObject.Spec.Template.Labels; locate and update the code around
deploymentObject, applyResourceLabels, and r.dpa to perform this re-assertion or
filtering immediately after the two applyResourceLabels calls.
Comment on lines
138
to
146
|
|
||
| // Apply user-provided resource labels (protected labels are filtered) | ||
| // Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation | ||
| deploymentObject.Labels = applyResourceLabels(r.dpa, deploymentObject.Labels) | ||
| deploymentObject.Spec.Template.Labels = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels) | ||
|
|
||
| // Apply user-provided resource annotations to both deployment and pod template | ||
| deploymentObject.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Annotations) | ||
| deploymentObject.Spec.Template.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Spec.Template.Annotations) |
Contributor
There was a problem hiding this comment.
Guard selector labels against resourceLabels overrides.
User-provided labels can overwrite the control-plane selector label, making spec.selector and pod template labels inconsistent and causing Deployment validation failures. Re-assert selector labels (or filter them out) after applying resource labels.
✅ Suggested fix (re-assert selector labels)
deploymentObject.Labels = applyResourceLabels(r.dpa, deploymentObject.Labels)
deploymentObject.Spec.Template.Labels = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels)
+
+// Ensure selector labels remain on the pod template.
+if deploymentObject.Spec.Template.Labels == nil {
+ deploymentObject.Spec.Template.Labels = map[string]string{}
+}
+for key, value := range vmFileRestoreControlPlaneLabel {
+ deploymentObject.Spec.Template.Labels[key] = value
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Apply user-provided resource labels (protected labels are filtered) | |
| // Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation | |
| deploymentObject.Labels = applyResourceLabels(r.dpa, deploymentObject.Labels) | |
| deploymentObject.Spec.Template.Labels = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels) | |
| // Apply user-provided resource annotations to both deployment and pod template | |
| deploymentObject.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Annotations) | |
| deploymentObject.Spec.Template.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Spec.Template.Annotations) | |
| // Apply user-provided resource labels (protected labels are filtered) | |
| // Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation | |
| deploymentObject.Labels = applyResourceLabels(r.dpa, deploymentObject.Labels) | |
| deploymentObject.Spec.Template.Labels = applyResourceLabels(r.dpa, deploymentObject.Spec.Template.Labels) | |
| // Ensure selector labels remain on the pod template. | |
| if deploymentObject.Spec.Template.Labels == nil { | |
| deploymentObject.Spec.Template.Labels = map[string]string{} | |
| } | |
| for key, value := range vmFileRestoreControlPlaneLabel { | |
| deploymentObject.Spec.Template.Labels[key] = value | |
| } | |
| // Apply user-provided resource annotations to both deployment and pod template | |
| deploymentObject.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Annotations) | |
| deploymentObject.Spec.Template.Annotations = applyResourceAnnotations(r.dpa, deploymentObject.Spec.Template.Annotations) |
🤖 Prompt for AI Agents
In `@internal/controller/vmfilerestore_controller.go` around lines 138 - 146,
After applying user resource labels via applyResourceLabels to
deploymentObject.Labels and deploymentObject.Spec.Template.Labels, ensure the
Deployment's selector labels are not overridden: re-assert the original selector
values back onto deploymentObject.Spec.Selector.MatchLabels (or remove protected
keys from the applied map) so the controller label (e.g., "control-plane")
remains identical between deploymentObject.Spec.Selector.MatchLabels and
deploymentObject.Spec.Template.Labels; locate and update the code around
deploymentObject, applyResourceLabels, and r.dpa to perform this re-assertion or
filtering immediately after the two applyResourceLabels calls.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/nodeagent.go`:
- Around line 508-515: After applying user resource labels via
applyResourceLabels to ds.Labels and ds.Spec.Template.Labels, ensure the
DaemonSet template cannot diverge from its selector by re-asserting the selector
labels into the pod template: read ds.Spec.Selector.MatchLabels and
copy/overwrite those key/value pairs into ds.Spec.Template.Labels (so selector
keys like "component" or "name" always win), leaving any other user-provided
labels intact; perform this step immediately after the existing
applyResourceLabels calls for ds.Spec.Template.Labels to guarantee
selector/template consistency.
In `@internal/controller/velero_test.go`:
- Around line 3407-3671: The file fails gci formatting (import grouping/order)
causing CI to fail; locate the test file by the test functions
TestIsProtectedLabel, TestApplyResourceLabels, and TestApplyResourceAnnotations
and reformat it using the repo's formatting target (or run gci to fix import
groups and then gofmt/gofumpt to rewrite the file), committing only the
formatting changes so gci no longer flags the file.
Fix gci linting error by reorganizing imports and aligning comments. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/velero_test.go`:
- Around line 24-27: The import block currently mixes internal imports with
other third-party imports; separate the internal imports (oadpv1alpha1,
oadpclient, common, stsflow) into the configured gci prefix group by adding a
blank line between external imports and the github.com/openshift/oadp-operator
imports and reordering them per gci rules; you can run make lint-fix or gci
write to auto-fix, or manually move those four imports into the internal section
so imports follow: standard, external third-party, then
prefix(github.com/openshift/oadp-operator).
- filterOutResourceLabels now skips protected labels to preserve core operator labels in matchLabels - Added selector label re-assertion in nodeagent.go after applyResourceLabels to prevent template/selector divergence - Added selector label re-assertion in nonadmin_controller.go - Added selector label re-assertion in vmfilerestore_controller.go - Fixed import ordering via gci Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/controller/nodeagent.go`:
- Around line 11-15: Reorder the imports in nodeagent.go to satisfy
gci/goimports grouping rules: ensure standard library imports come first, then
external modules (e.g., github.com/openshift/api/config/v1 and
github.com/openshift/oadp-operator/...), and finally internal package imports if
any; you can simply run gci or goimports to automatically fix the ordering for
imports like configv1, oadpv1alpha1, common, credentials, and stsflow so the
file passes static analysis.
In `@internal/controller/nonadmin_controller.go`:
- Around line 10-12: Reorder and format the import block in
nonadmin_controller.go to satisfy gci/goimports canonical ordering: group
standard library, third-party (e.g., "github.com/go-logr/logr"), and local
modules (e.g., "github.com/openshift/oadp-operator/api/v1alpha1" and
"github.com/openshift/oadp-operator/pkg/common") correctly, then run gci or
goimports to automatically fix spacing and ordering so the imports match the
repository's import style.
In `@internal/controller/velero.go`:
- Around line 11-16: The imports in internal/controller/velero.go are out of
order; run gci or goimports to reorder and group standard-library, third-party,
and project-local imports correctly (e.g., ensure "github.com/google/go-cmp/cmp"
and third‑party imports are grouped separately from local modules like
oadpv1alpha1, common, credentials, stsflow, and veleroserver). Reformat the
import block (or re-run gci/goimports) so the file passes static analysis and
gofmt checks without changing any identifiers or functionality.
In `@internal/controller/vmfilerestore_controller.go`:
- Around line 9-11: Reorder the import block in vmfilerestore_controller.go to
satisfy gci grouping (standard library first, then external modules, then
internal project imports) and re-run the formatting tools; either run gci -w .
or goimports -w . (or manually reorder the imports so github.com/go-logr/logr
and github.com/openshift/oadp-operator/pkg/common are grouped correctly with
oadpv1alpha1 in the appropriate section) to fix the import ordering reported by
CI.
Reorder imports to match .golangci.yaml sections: 1. standard library 2. third-party (default) 3. project-local (github.com/openshift/oadp-operator) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
weshayutin
approved these changes
Jan 21, 2026
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
Member
|
Can you clarify in pr description if it's scoped to things oadp manager create? Or also user created or velero created of those resources in the namespace. |
|
@shubham-pampattiwar: This pull request references OADP-6846 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Member
Author
|
@kaovilai I've updated the PR description to clarify the scope. This feature is scoped ONLY to resources that the OADP operator creates and reconciles. It does NOT affect:
The labels and annotations are applied during the DPA reconciliation loop to resources that OADP directly manages (BSL, VSL, Velero Deployment, NodeAgent DaemonSet, etc.). |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
|
/lgtm |
|
@shubham-pampattiwar: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
resourceLabelsandresourceAnnotationsfields to the DataProtectionApplication specargocd.argoproj.io/ignore-resource-updates: 'true'to prevent ArgoCD from reconciling on BSL status updatesScope
This feature is scoped ONLY to resources that the OADP operator creates and reconciles. It does NOT affect:
The labels and annotations are applied during the DPA reconciliation loop to resources that the OADP operator directly manages.
Resources Affected
Usage Example
Test plan
TestIsProtectedLabel,TestApplyResourceLabels,TestApplyResourceAnnotations)Fixes: https://issues.redhat.com/browse/OADP-6846
🤖 Generated with Claude Code