OADP-7869: Fix stale resourceAnnotations/resourceLabels after DPA patch#2176
Conversation
…fter DPA patch When resourceAnnotations or resourceLabels were removed from the DPA spec, stale values persisted on all managed resources (Velero deployment, BSL, ConfigMaps, Services, etc.) because the apply functions only merged annotations in and never removed previously-applied keys. This fix introduces tracking annotations (oadp.openshift.io/managed-resource- annotation-keys and oadp.openshift.io/managed-resource-label-keys) that record which keys were applied by resourceAnnotations/resourceLabels. On each reconcile, previously-tracked keys are removed before applying the current set, ensuring cleanup when the DPA spec changes. Additionally fixes a secondary issue where the Velero deployment incorrectly merged metadata annotations into the pod template via AppendUniqueKeyTOfTMaps, causing stale annotations to persist on the pod template. Aligned with the node-agent pattern of passing only PodConfig annotations to the install library. Fixes: OADP-7869 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@shubham-pampattiwar: This pull request references OADP-7869 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. |
WalkthroughThis PR consolidates how user-provided resource labels and annotations are applied across multiple controller files. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shubham-pampattiwar 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 |
openshift-ci
Bot
added
the
approved
|
/cherry-pick oadp-1.6 |
|
@shubham-pampattiwar: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/velero.go (1)
263-270:⚠️ Potential issue | 🔴 CriticalRe-assert Velero selector labels after applying
resourceLabels.
applyResourceLabelscan overwritek8s-app,component, anddeployon the pod template, butSpec.Selector.MatchLabelsstays unchanged. A DPA that sets any of those keys inspec.resourceLabelswill make the Deployment selector stop matching its pods.Proposed fix
// Apply user-provided resource labels (protected labels are filtered) // Note: NOT applied to Spec.Selector.MatchLabels as those are immutable after creation veleroDeployment.Labels, veleroDeployment.Annotations = applyResourceLabels(dpa, veleroDeployment.Labels, veleroDeployment.Annotations) veleroDeployment.Spec.Template.Labels, veleroDeployment.Spec.Template.Annotations = applyResourceLabels(dpa, veleroDeployment.Spec.Template.Labels, veleroDeployment.Spec.Template.Annotations) + + // Re-assert selector labels so user resourceLabels cannot break the Deployment selector. + if veleroDeployment.Spec.Selector != nil && veleroDeployment.Spec.Selector.MatchLabels != nil { + if veleroDeployment.Spec.Template.Labels == nil { + veleroDeployment.Spec.Template.Labels = make(map[string]string) + } + for k, v := range veleroDeployment.Spec.Selector.MatchLabels { + veleroDeployment.Spec.Template.Labels[k] = v + } + } // Apply user-provided resource annotations to both deployment and pod template veleroDeployment.Annotations = applyResourceAnnotations(dpa, veleroDeployment.Annotations) veleroDeployment.Spec.Template.Annotations = applyResourceAnnotations(dpa, veleroDeployment.Spec.Template.Annotations)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/velero.go` around lines 263 - 270, After applying applyResourceLabels to veleroDeployment.Spec.Template.Labels, re-assert the Deployment selector so it continues to match pods: copy the selector keys "k8s-app", "component", and "deploy" from veleroDeployment.Spec.Template.Labels into veleroDeployment.Spec.Selector.MatchLabels (upsert those keys only) so the selector remains consistent with the pod template; perform this update immediately after the applyResourceLabels calls on veleroDeployment.Spec.Template.Labels in the veleroDeployment handling code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/velero.go`:
- Around line 938-963: The stale-key cleanup currently deletes every key listed
in managedAnnotationKeysAnnotation from result before merging current spec
annotations, which removes controller-recreated annotations; instead, merge
dpa.Spec.ResourceAnnotations into result first (the block that iterates
dpa.Spec.ResourceAnnotations and sets keys/result when hasNewAnnotations is
true), then run the tracked-key cleanup: iterate the tracked list from
result[managedAnnotationKeysAnnotation] and only delete a key from result if
that key is NOT present in dpa.Spec.ResourceAnnotations; finally recompute
managedAnnotationKeysAnnotation (using keys and slices.Sort) as you already do.
Use the existing symbols managedAnnotationKeysAnnotation,
managedLabelKeysAnnotation, result, tracked, dpa.Spec.ResourceAnnotations,
hasNewAnnotations and keys to locate and reorder/adjust the logic.
---
Outside diff comments:
In `@internal/controller/velero.go`:
- Around line 263-270: After applying applyResourceLabels to
veleroDeployment.Spec.Template.Labels, re-assert the Deployment selector so it
continues to match pods: copy the selector keys "k8s-app", "component", and
"deploy" from veleroDeployment.Spec.Template.Labels into
veleroDeployment.Spec.Selector.MatchLabels (upsert those keys only) so the
selector remains consistent with the pod template; perform this update
immediately after the applyResourceLabels calls on
veleroDeployment.Spec.Template.Labels in the veleroDeployment handling code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0569ca88-e1e2-42fc-b8c6-9520dfb1b687
📒 Files selected for processing (12)
internal/controller/backup_repository.gointernal/controller/bsl.gointernal/controller/kubevirt_datamover_controller.gointernal/controller/monitor.gointernal/controller/nodeagent.gointernal/controller/nonadmin_controller.gointernal/controller/registry.gointernal/controller/repository_maintenance.gointernal/controller/velero.gointernal/controller/velero_test.gointernal/controller/vmfilerestore_controller.gointernal/controller/vsl.go
| if tracked, ok := result[managedAnnotationKeysAnnotation]; ok { | ||
| for _, k := range strings.Split(tracked, ",") { | ||
| if k = strings.TrimSpace(k); k != "" { | ||
| delete(result, k) | ||
| } | ||
| } | ||
| delete(result, managedAnnotationKeysAnnotation) | ||
| } | ||
|
|
||
| if !hasNewAnnotations { | ||
| return result | ||
| } | ||
|
|
||
| var keys []string | ||
| for k, v := range dpa.Spec.ResourceAnnotations { | ||
| if k == managedAnnotationKeysAnnotation || k == managedLabelKeysAnnotation { | ||
| continue | ||
| } | ||
| result[k] = v | ||
| keys = append(keys, k) | ||
| } | ||
|
|
||
| if len(keys) > 0 { | ||
| slices.Sort(keys) | ||
| result[managedAnnotationKeysAnnotation] = strings.Join(keys, ",") | ||
| } |
There was a problem hiding this comment.
Don't delete controller-owned annotations during stale-key cleanup.
This helper drops every key listed in managed-resource-annotation-keys before looking at the current annotation set. If the controller already rebuilt one of those annotations this reconcile, removing it from spec.resourceAnnotations now deletes the operator-owned value too. That breaks fallback to defaults for keys like Velero's Prometheus annotations or kdmDpaResourceVersionAnnotation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/velero.go` around lines 938 - 963, The stale-key cleanup
currently deletes every key listed in managedAnnotationKeysAnnotation from
result before merging current spec annotations, which removes
controller-recreated annotations; instead, merge dpa.Spec.ResourceAnnotations
into result first (the block that iterates dpa.Spec.ResourceAnnotations and sets
keys/result when hasNewAnnotations is true), then run the tracked-key cleanup:
iterate the tracked list from result[managedAnnotationKeysAnnotation] and only
delete a key from result if that key is NOT present in
dpa.Spec.ResourceAnnotations; finally recompute managedAnnotationKeysAnnotation
(using keys and slices.Sort) as you already do. Use the existing symbols
managedAnnotationKeysAnnotation, managedLabelKeysAnnotation, result, tracked,
dpa.Spec.ResourceAnnotations, hasNewAnnotations and keys to locate and
reorder/adjust the logic.
There was a problem hiding this comment.
This is a false positive. The tracking annotation only records keys that the user explicitly set via dpa.Spec.ResourceAnnotations. It never tracks operator-owned annotations like prometheus.io/scrape or deployment.kubernetes.io/revision -- those keys would only appear in the tracking list if the user explicitly added them to resourceAnnotations, in which case removing them on cleanup is the correct behavior.
The concern about Prometheus annotations or kdmDpaResourceVersionAnnotation being deleted doesn't apply because:
- Prometheus annotations are set on the pod template, not on deployment metadata where this function operates
- Operator-internal annotations are never added to the tracking list unless a user explicitly puts them in
resourceAnnotations
Both orderings (delete-then-merge vs merge-then-delete-unmatched) produce identical results since step 3 re-adds any key still in the current spec regardless.
There was a problem hiding this comment.
@shubham-pampattiwar, you're right — I've re-traced the logic and agree this is a false positive.
The invariant holds: the tracking annotation only ever records keys sourced from dpa.Spec.ResourceAnnotations, so:
- Operator-owned annotations (
prometheus.io/*,kdmDpaResourceVersionAnnotation, etc.) are never added to the tracking list unless the user explicitly puts them inresourceAnnotations. - In the delete-then-merge flow, any key that is still in the current spec is unconditionally re-added by the
dpa.Spec.ResourceAnnotationsloop, making both orderings equivalent. - The only keys permanently removed are those that were tracked but are absent from the current spec — which is precisely the stale-cleanup intent.
My concern was unfounded. The implementation is correct as-is.
|
@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. |
3 participants
Summary
resourceAnnotationsandresourceLabelsnot being removed from managed resources when cleared from the DPA specoadp.openshift.io/managed-resource-annotation-keys,oadp.openshift.io/managed-resource-label-keys) to record which keys were applied, enabling cleanup on subsequent reconcilesAppendUniqueKeyTOfTMaps, aligning with the node-agent pattern of passing only PodConfig annotations to the install libraryDetails
Problem
When
resourceAnnotationsorresourceLabelswere removed from the DPA spec, stale values persisted on all managed resources (Velero deployment, node-agent DaemonSet, BSL, ConfigMaps, Services, etc.). TheapplyResourceAnnotations()andapplyResourceLabels()functions only merged values in and never removed previously-applied keys.Additionally, the node-agent DaemonSet appeared unaffected at the pod level because its spec was rebuilt from scratch each reconcile, but its object metadata annotations were also stale. The Velero deployment pod template was affected because metadata annotations leaked into the pod template through an incorrect merge.
Fix
resourceAnnotations/resourceLabels. On each reconcile, previously-tracked keys are removed before applying the current set. This follows the same pattern used bykubectl apply(kubectl.kubernetes.io/last-applied-configuration).Affected resources
All resources using
applyResourceAnnotations/applyResourceLabels(12 call sites across 10 files): Velero Deployment, Node-Agent DaemonSet, Node-Agent ConfigMap, BSL, VSL, Monitor Service, Registry Secret, Backup Repository ConfigMap, Repository Maintenance ConfigMap, NonAdmin Controller, KubeVirt DataMover, VMFileRestore Controller.Fixes: OADP-7869
Test plan
make testpasses (unit tests + lint + go vet + api-isupdated)resourceAnnotations/resourceLabelsto DPA, verified they appeared on all managed resources, removed them, verified all stale values were cleaned up across Deployment metadata, DaemonSet metadata, pod templates, BSL, and ConfigMaps🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests