Delete custom labels from live object when deleted from CR #1932
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
…rator labels (argoproj-labs#1737) * GITOPS-6285: reconcile operator owned labels for configMap and ignore non operator labels Signed-off-by: Alka Kumari <[email protected]> * GITOPS-6285: added reconcilialtion logic for labels Signed-off-by: Alka Kumari <[email protected]> * Removed a typo that resulted in test case failures Signed-off-by: Alka Kumari <[email protected]> * GITOPS-6285: error handling for r.Client.Update Signed-off-by: Alka Kumari <[email protected]> * GITOPS-6285: use r.Update Signed-off-by: Alka Kumari <[email protected]> * GITOPS-6285: changed func name to UpdateMapValues and added test cases Signed-off-by: Alka Kumari <[email protected]> * GITOPS-6285: removed addKubernetesData and replaced the references with UpdateMapValues Signed-off-by: Alka Kumari <[email protected]> GITOPS-6285: removed addKubernetesData and replaced the references with UpdateMapValues * GITOPS-6285: removed labels update logic from applicationset and statefulset Signed-off-by: Alka Kumari <[email protected]> * GITOPS-6285: changed comments Signed-off-by: Alka Kumari <[email protected]> --------- Signed-off-by: Alka Kumari <[email protected]>
… removed from argocd spec Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
…o ginkgo tests for checking custom labels and annotations Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
Signed-off-by: Alka Kumari <[email protected]>
WalkthroughThe PR refactors metadata synchronization across ArgoCD reconciliation by replacing wholesale map replacements with incremental updates via a new Changes
Sequence Diagram(s)sequenceDiagram
participant AC as ArgoCD Controller
participant Reconcile as Reconcile Pipeline
participant Cleanup as processDropMetadataForCleanup
participant KubeAPI as Kubernetes API
AC->>Reconcile: internalReconcile()
Reconcile->>Cleanup: processDropMetadataForCleanup (call 1)
Cleanup->>KubeAPI: Query DropMetadataStore
Cleanup->>KubeAPI: Remove labels/annotations from Deployments
Cleanup->>KubeAPI: Remove labels/annotations from StatefulSets
Reconcile->>Reconcile: Namespace Management cleanup
Reconcile->>Cleanup: processDropMetadataForCleanup (call 2)
Cleanup->>KubeAPI: Query DropMetadataStore
Cleanup->>KubeAPI: Remove labels/annotations from resources
Reconcile->>Reconcile: reconcileResources()
Note over Reconcile: UpdateMapValues used for incremental<br/>label/annotation updates
Reconcile->>KubeAPI: Update Deployments/StatefulSets with<br/>merged metadata changes
sequenceDiagram
participant ArgoCD as ArgoCD Spec Change
participant Watcher as Resource Watches
participant Predicate as argoCDSpecLabelAndAnnotationCleanupPredicate
participant Calculator as calculateRemovedSpecLabelsAndAnnotations
participant Store as DropMetadataStore
ArgoCD->>Watcher: Spec updated (labels/annotations removed)
Watcher->>Predicate: Evaluate update event
Predicate->>Calculator: Compare old vs new spec
Calculator->>Store: addDropMetadata() for each removed key
Store->>Store: Thread-safe store update (mutex locked)
Note over Store: Keys queued for cleanup in next<br/>reconcile cycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/argocd/applicationset.go (1)
279-299: Bug: annotation/label changes are not persistedYou mutate existing.Spec.Template.Annotations/Labels via UpdateMapValues but never set changed or call Update when these are the only diffs. identifyDeploymentDifference runs after the mutation, so it won’t detect that change either.
Apply this diff to track and persist these changes:
@@ - //Check if annotations have changed - UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) + // Check annotation/label changes up-front + annChanged := UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) @@ - deploymentsDifferent := identifyDeploymentDifference(*existing, *deploy) - if len(deploymentsDifferent) > 0 { + deploymentsDifferent := identifyDeploymentDifference(*existing, *deploy) + if len(deploymentsDifferent) > 0 { existing.Spec.Template.Spec.Containers = podSpec.Containers existing.Spec.Template.Spec.Volumes = podSpec.Volumes existing.Spec.Template.Spec.ServiceAccountName = podSpec.ServiceAccountName - UpdateMapValues(&existing.Labels, deploy.Labels) - UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) + lblChanged1 := UpdateMapValues(&existing.Labels, deploy.Labels) + lblChanged2 := UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) @@ - argoutil.LogResourceUpdate(log, existing, "due to difference in", deploymentsDifferent) - return r.Update(context.TODO(), existing) - } - return nil // Deployment found with nothing to do, move along... + argoutil.LogResourceUpdate(log, existing, "due to difference in", deploymentsDifferent) + return r.Update(context.TODO(), existing) + } + // No structural diffs, but annotations/labels may have changed + lblChanged1 := UpdateMapValues(&existing.Labels, deploy.Labels) + lblChanged2 := UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) + if annChanged || lblChanged1 || lblChanged2 { + explanation := "annotations" + if lblChanged1 || lblChanged2 { + explanation = explanation + ", labels" + } + argoutil.LogResourceUpdate(log, existing, "updating", explanation) + return r.Update(context.TODO(), existing) + } + return nil // nothing to do
🧹 Nitpick comments (1)
controllers/argocd/configmap.go (1)
346-364: Minor: simplify and fix log messageThe nested changed check is redundant, and the error says “service object”. Use “configmap object”.
Apply this diff:
@@ - if exists { - changed := false - //Check if labels have changed - if UpdateMapValues(&existingCM.Labels, cm.GetLabels()) { - argoutil.LogResourceUpdate(log, existingCM, "updating", "CAConfigMap labels") - changed = true - if changed { - if err := r.Update(context.TODO(), existingCM); err != nil { - log.Error(err, "failed to update service object") - } - } - } - } + if exists { + if UpdateMapValues(&existingCM.Labels, cm.GetLabels()) { + argoutil.LogResourceUpdate(log, existingCM, "updating", "CAConfigMap labels") + if err := r.Update(context.TODO(), existingCM); err != nil { + log.Error(err, "failed to update configmap object") + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
controllers/argocd/applicationset.go(1 hunks)controllers/argocd/argocd_controller.go(1 hunks)controllers/argocd/configmap.go(2 hunks)controllers/argocd/deployment.go(1 hunks)controllers/argocd/repo_server.go(1 hunks)controllers/argocd/role.go(2 hunks)controllers/argocd/statefulset.go(1 hunks)controllers/argocd/util.go(5 hunks)controllers/argocd/util_test.go(2 hunks)tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go(4 hunks)
🔇 Additional comments (13)
controllers/argocd/argocd_controller.go (1)
351-353: Wiring looks goodInvoking metadata cleanup once per reconcile before sub-resource reconciliation is reasonable and side‑effect free when there’s nothing queued.
controllers/argocd/role.go (1)
517-524: Good switch to incremental label updatesUsing UpdateMapValues prevents wholesale map replacement and avoids clobbering foreign labels.
Also applies to: 537-544
controllers/argocd/util_test.go (1)
1172-1256: Solid coverage for UpdateMapValues behaviorTests exercise retain/override/empty cases and match the new semantics.
controllers/argocd/util.go (1)
1948-1962: LGTM: helper is simple and safeMap is initialized defensively and updates are idempotent.
controllers/argocd/repo_server.go (1)
516-531: Good: incremental metadata updatesSwitching to UpdateMapValues for annotations and labels avoids clobbering unrelated keys.
controllers/argocd/configmap.go (1)
556-561: LGTM: labels reconciled incrementally on argocd-cmUsing UpdateMapValues avoids destructive replacement.
controllers/argocd/statefulset.go (1)
987-1002: LGTM! Consistent refactoring across StatefulSet reconciliation.The changes mirror the refactoring in
deployment.go, applying the same incremental metadata update pattern to StatefulSet resources. This ensures consistent behavior across Deployment and StatefulSet reconciliation paths.tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go (5)
146-294: Excellent test coverage for selective label removal.The test thoroughly validates:
- Selective removal of labels from individual components (server, repo, controller, appset)
- Cross-component isolation (changes to one component don't affect others)
- Preservation of remaining labels after selective removal
The use of
Eventuallywith detailedGinkgoWriteroutput provides good debugging information if tests fail.
295-389: LGTM! Comprehensive annotation removal testing.The annotation removal tests mirror the structure of the label removal tests, providing consistent coverage across both metadata types. This validates that
UpdateMapValuesworks correctly for both annotations and labels.
414-448: Good defensive testing with operator-managed label verification.The addition of the
componentNameparameter improves error messages, and the verification that operator-managed labels (likeapp.kubernetes.io/name) are preserved is an important safeguard. This ensures thatUpdateMapValuescorrectly distinguishes between custom and operator-managed metadata.
461-478: LGTM! Tests label re-addition after removal.This test validates the complete lifecycle (add → remove → add) and ensures that the removal mechanism doesn't leave the system in a state where labels cannot be added back.
480-744: Thread-safe DropMetadataStore implementation is correctly protected with proper mutex patterns.Verification confirms the
DropMetadataStorehas complete synchronization:
- Write operations (add/delete): Protected by
Lock()- Read operations: Protected by
RLock()- All three access functions (
addDropMetadata,getDropMetadataForNamespace,removeDropMetadataForNamespace) properly defer unlockThe test itself appropriately uses
Eventuallyfor retry logic to accommodate goroutine timing. The concurrent test at lines 480-744 effectively validates per-namespace isolation with the alternating removal pattern (even vs odd instances), and the verification confirms operator-managed labels remain intact under concurrent operations.No additional synchronization concerns identified in the test or implementation.
controllers/argocd/deployment.go (1)
1252-1267: Incorrect characterization of UpdateMapValues behavior.The review comment claims the code "allows removal of custom labels/annotations from the live object when they are deleted from the CR spec." However,
UpdateMapValues(controllers/argocd/util.go:1950–1962) is additive only—it updates and adds keys from the source map but does not remove keys from the existing map that are absent in the source.Keys existing in the live deployment but missing from the CR spec will be preserved, not removed. This aligns with the existing code comment at line 1260: "Preserve non-operator labels in the existing deployment."
The thread-safety concern in the review is valid:
UpdateMapValueslacks synchronization primitives and is unsafe for concurrent reconciliation loops.Likely an incorrect or invalid review comment.
Comment on lines
+1329
to
+1357
| func getDropMetadataForNamespace(namespace string) []DropMetadata { | ||
| DropMetadataStoreMutex.RLock() | ||
| defer DropMetadataStoreMutex.RUnlock() | ||
| return DropMetadataStore[namespace] | ||
| } | ||
|
|
||
| // removeDropMetadataForNamespace removes all DropMetadata for a specific namespace | ||
| func removeDropMetadataForNamespace(namespace string) { | ||
| DropMetadataStoreMutex.Lock() | ||
| defer DropMetadataStoreMutex.Unlock() | ||
| delete(DropMetadataStore, namespace) | ||
| } | ||
|
|
||
| // processDropMetadataForCleanup processes the DropMetadata store and cleans up removed labels and annotations from resources | ||
| func (r *ReconcileArgoCD) processDropMetadataForCleanup(argocd *argoproj.ArgoCD) { | ||
| namespace := argocd.GetNamespace() | ||
| dropMetadataList := getDropMetadataForNamespace(namespace) | ||
|
|
||
| for _, dropMeta := range dropMetadataList { | ||
| if err := r.cleanupLabelsAndAnnotationsFromResourceType(argocd, dropMeta); err != nil { | ||
| log.Error(err, "failed to cleanup labels and annotations from resource type", | ||
| "resource", dropMeta.Resource) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // Clear the DropMetadata for this namespace after cleanup | ||
| removeDropMetadataForNamespace(namespace) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix data race and event loss in DropMetadata processing
getDropMetadataForNamespace returns the backing slice without copying, then processDropMetadataForCleanup iterates after releasing the lock and finally deletes the entry. Writers can mutate the same slice concurrently, and new entries added during processing can be dropped when delete runs. Drain the namespace atomically under write lock and iterate over a copy.
Apply this diff:
@@
-func getDropMetadataForNamespace(namespace string) []DropMetadata {
- DropMetadataStoreMutex.RLock()
- defer DropMetadataStoreMutex.RUnlock()
- return DropMetadataStore[namespace]
-}
+func getDropMetadataForNamespace(namespace string) []DropMetadata {
+ DropMetadataStoreMutex.RLock()
+ defer DropMetadataStoreMutex.RUnlock()
+ // Return a shallow copy to avoid callers observing concurrent writes
+ src := DropMetadataStore[namespace]
+ dst := make([]DropMetadata, len(src))
+ copy(dst, src)
+ return dst
+}
// removeDropMetadataForNamespace removes all DropMetadata for a specific namespace
func removeDropMetadataForNamespace(namespace string) {
DropMetadataStoreMutex.Lock()
defer DropMetadataStoreMutex.Unlock()
delete(DropMetadataStore, namespace)
}
+// drainDropMetadataForNamespace atomically retrieves and clears the namespace entry.
+func drainDropMetadataForNamespace(namespace string) []DropMetadata {
+ DropMetadataStoreMutex.Lock()
+ defer DropMetadataStoreMutex.Unlock()
+ src := DropMetadataStore[namespace]
+ dst := make([]DropMetadata, len(src))
+ copy(dst, src)
+ delete(DropMetadataStore, namespace)
+ return dst
+}
@@
func (r *ReconcileArgoCD) processDropMetadataForCleanup(argocd *argoproj.ArgoCD) {
namespace := argocd.GetNamespace()
- dropMetadataList := getDropMetadataForNamespace(namespace)
+ // Atomically drain the queue for this namespace to avoid races and event loss
+ dropMetadataList := drainDropMetadataForNamespace(namespace)
for _, dropMeta := range dropMetadataList {
if err := r.cleanupLabelsAndAnnotationsFromResourceType(argocd, dropMeta); err != nil {
log.Error(err, "failed to cleanup labels and annotations from resource type",
"resource", dropMeta.Resource)
continue
}
}
-
- // Clear the DropMetadata for this namespace after cleanup
- removeDropMetadataForNamespace(namespace)
}📝 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
| func getDropMetadataForNamespace(namespace string) []DropMetadata { | |
| DropMetadataStoreMutex.RLock() | |
| defer DropMetadataStoreMutex.RUnlock() | |
| return DropMetadataStore[namespace] | |
| } | |
| // removeDropMetadataForNamespace removes all DropMetadata for a specific namespace | |
| func removeDropMetadataForNamespace(namespace string) { | |
| DropMetadataStoreMutex.Lock() | |
| defer DropMetadataStoreMutex.Unlock() | |
| delete(DropMetadataStore, namespace) | |
| } | |
| // processDropMetadataForCleanup processes the DropMetadata store and cleans up removed labels and annotations from resources | |
| func (r *ReconcileArgoCD) processDropMetadataForCleanup(argocd *argoproj.ArgoCD) { | |
| namespace := argocd.GetNamespace() | |
| dropMetadataList := getDropMetadataForNamespace(namespace) | |
| for _, dropMeta := range dropMetadataList { | |
| if err := r.cleanupLabelsAndAnnotationsFromResourceType(argocd, dropMeta); err != nil { | |
| log.Error(err, "failed to cleanup labels and annotations from resource type", | |
| "resource", dropMeta.Resource) | |
| continue | |
| } | |
| } | |
| // Clear the DropMetadata for this namespace after cleanup | |
| removeDropMetadataForNamespace(namespace) | |
| } | |
| func getDropMetadataForNamespace(namespace string) []DropMetadata { | |
| DropMetadataStoreMutex.RLock() | |
| defer DropMetadataStoreMutex.RUnlock() | |
| // Return a shallow copy to avoid callers observing concurrent writes | |
| src := DropMetadataStore[namespace] | |
| dst := make([]DropMetadata, len(src)) | |
| copy(dst, src) | |
| return dst | |
| } | |
| // removeDropMetadataForNamespace removes all DropMetadata for a specific namespace | |
| func removeDropMetadataForNamespace(namespace string) { | |
| DropMetadataStoreMutex.Lock() | |
| defer DropMetadataStoreMutex.Unlock() | |
| delete(DropMetadataStore, namespace) | |
| } | |
| // drainDropMetadataForNamespace atomically retrieves and clears the namespace entry. | |
| func drainDropMetadataForNamespace(namespace string) []DropMetadata { | |
| DropMetadataStoreMutex.Lock() | |
| defer DropMetadataStoreMutex.Unlock() | |
| src := DropMetadataStore[namespace] | |
| dst := make([]DropMetadata, len(src)) | |
| copy(dst, src) | |
| delete(DropMetadataStore, namespace) | |
| return dst | |
| } | |
| // processDropMetadataForCleanup processes the DropMetadata store and cleans up removed labels and annotations from resources | |
| func (r *ReconcileArgoCD) processDropMetadataForCleanup(argocd *argoproj.ArgoCD) { | |
| namespace := argocd.GetNamespace() | |
| // Atomically drain the queue for this namespace to avoid races and event loss | |
| dropMetadataList := drainDropMetadataForNamespace(namespace) | |
| for _, dropMeta := range dropMetadataList { | |
| if err := r.cleanupLabelsAndAnnotationsFromResourceType(argocd, dropMeta); err != nil { | |
| log.Error(err, "failed to cleanup labels and annotations from resource type", | |
| "resource", dropMeta.Resource) | |
| continue | |
| } | |
| } | |
| } |
Comment on lines
+1362
to
+1405
| switch dropMeta.Resource { | ||
| case "server-deployment": | ||
| // Clean up from ArgoCD server deployment only | ||
| deploymentName := fmt.Sprintf("%s-server", argocd.Name) | ||
| if err := r.cleanupLabelsAndAnnotationsFromDeployment(argocd, deploymentName, dropMeta.LabelKeys, dropMeta.AnnotationKeys); err != nil { | ||
| return fmt.Errorf("failed to cleanup labels from server deployment: %w", err) | ||
| } | ||
|
|
||
| case "repo-deployment": | ||
| // Clean up from ArgoCD repo server deployment only | ||
| deploymentName := fmt.Sprintf("%s-repo-server", argocd.Name) | ||
| if err := r.cleanupLabelsAndAnnotationsFromDeployment(argocd, deploymentName, dropMeta.LabelKeys, dropMeta.AnnotationKeys); err != nil { | ||
| return fmt.Errorf("failed to cleanup labels from repo deployment: %w", err) | ||
| } | ||
|
|
||
| case "applicationset-deployment": | ||
| // Clean up from ArgoCD applicationset controller deployment only | ||
| deploymentName := fmt.Sprintf("%s-applicationset-controller", argocd.Name) | ||
| if err := r.cleanupLabelsAndAnnotationsFromDeployment(argocd, deploymentName, dropMeta.LabelKeys, dropMeta.AnnotationKeys); err != nil { | ||
| return fmt.Errorf("failed to cleanup labels from applicationset deployment: %w", err) | ||
| } | ||
|
|
||
| case "notifications-deployment": | ||
| // Clean up from ArgoCD notifications controller deployment only | ||
| if argocd.Spec.Notifications.Enabled { | ||
| deploymentName := fmt.Sprintf("%s-notifications-controller", argocd.Name) | ||
| if err := r.cleanupLabelsAndAnnotationsFromDeployment(argocd, deploymentName, dropMeta.LabelKeys, dropMeta.AnnotationKeys); err != nil { | ||
| return fmt.Errorf("failed to cleanup labels from notifications deployment: %w", err) | ||
| } | ||
| } | ||
|
|
||
| case "controller-statefulset": | ||
| // Clean up from ArgoCD application controller StatefulSet | ||
| statefulSetName := fmt.Sprintf("%s-application-controller", argocd.Name) | ||
| if err := r.cleanupLabelsAndAnnotationsFromStatefulSet(argocd, statefulSetName, dropMeta.LabelKeys, dropMeta.AnnotationKeys); err != nil { | ||
| return fmt.Errorf("failed to cleanup labels from controller statefulset: %w", err) | ||
| } | ||
|
|
||
| default: | ||
| log.Info("Unknown resource type for cleanup", "resourceType", dropMeta.Resource) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Use nameWithSuffix(...) for resource names to avoid truncation mismatches
Using fmt.Sprintf("%s-...") with cr.Name breaks when resource names are derived via nameWithSuffix (truncated CR name). Cleanup will miss targets for long CR names.
Apply this diff:
@@
- case "server-deployment":
- deploymentName := fmt.Sprintf("%s-server", argocd.Name)
+ case "server-deployment":
+ deploymentName := nameWithSuffix("server", argocd)
@@
- case "repo-deployment":
- deploymentName := fmt.Sprintf("%s-repo-server", argocd.Name)
+ case "repo-deployment":
+ deploymentName := nameWithSuffix("repo-server", argocd)
@@
- case "applicationset-deployment":
- deploymentName := fmt.Sprintf("%s-applicationset-controller", argocd.Name)
+ case "applicationset-deployment":
+ deploymentName := nameWithSuffix("applicationset-controller", argocd)
@@
- case "notifications-deployment":
+ case "notifications-deployment":
// Clean up from ArgoCD notifications controller deployment only
if argocd.Spec.Notifications.Enabled {
- deploymentName := fmt.Sprintf("%s-notifications-controller", argocd.Name)
+ deploymentName := nameWithSuffix("notifications-controller", argocd)
if err := r.cleanupLabelsAndAnnotationsFromDeployment(argocd, deploymentName, dropMeta.LabelKeys, dropMeta.AnnotationKeys); err != nil {
return fmt.Errorf("failed to cleanup labels from notifications deployment: %w", err)
}
}
@@
- case "controller-statefulset":
- statefulSetName := fmt.Sprintf("%s-application-controller", argocd.Name)
+ case "controller-statefulset":
+ statefulSetName := nameWithSuffix("application-controller", argocd)Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+1407
to
+1458
| // cleanupLabelsFromDeployment removes labels from a specific deployment | ||
| func (r *ReconcileArgoCD) cleanupLabelsAndAnnotationsFromDeployment(argocd *argoproj.ArgoCD, deploymentName string, labelsToRemove, annotationsToRemove []string) error { | ||
| namespace := argocd.GetNamespace() | ||
| deployment := &appsv1.Deployment{} | ||
| key := types.NamespacedName{Namespace: namespace, Name: deploymentName} | ||
|
|
||
| if err := r.Get(context.TODO(), key, deployment); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| log.Info("Deployment not found, skipping label cleanup", "name", deploymentName) | ||
| return nil // Deployment doesn't exist, skip | ||
| } | ||
| return fmt.Errorf("failed to get deployment %s: %w", deploymentName, err) | ||
| } | ||
|
|
||
| // Work on pod template labels and annotations directly, not deployment metadata labels | ||
| currentLabels := deployment.Spec.Template.Labels | ||
| currentAnnotations := deployment.Spec.Template.Annotations | ||
| if currentLabels == nil && currentAnnotations == nil { | ||
| return nil // No labels or annotations to remove | ||
| } | ||
|
|
||
| modifiedLabels := false | ||
| modifiedAnnotations := false | ||
| for _, labelKey := range labelsToRemove { | ||
| if _, exists := currentLabels[labelKey]; exists { | ||
| delete(currentLabels, labelKey) | ||
| modifiedLabels = true | ||
| } | ||
| } | ||
|
|
||
| for _, annotationKey := range annotationsToRemove { | ||
| if _, exists := currentAnnotations[annotationKey]; exists { | ||
| delete(currentAnnotations, annotationKey) | ||
| modifiedAnnotations = true | ||
| } | ||
| } | ||
|
|
||
| if modifiedLabels { | ||
| deployment.Spec.Template.Labels = currentLabels | ||
| } | ||
| if modifiedAnnotations { | ||
| deployment.Spec.Template.Annotations = currentAnnotations | ||
| } | ||
|
|
||
| if modifiedLabels || modifiedAnnotations { | ||
| if err := r.Update(context.TODO(), deployment); err != nil { | ||
| log.Error(err, "failed to update deployment after removing labels and annotations", "name", deploymentName) | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Also remove labels/annotations from Deployment metadata, not only Pod template
Stale keys can persist on deployment.ObjectMeta.{Labels,Annotations}. Clean both places.
Apply this diff:
@@
- // Work on pod template labels and annotations directly, not deployment metadata labels
- currentLabels := deployment.Spec.Template.Labels
- currentAnnotations := deployment.Spec.Template.Annotations
+ // Clean both deployment metadata and pod template metadata
+ currentDepLabels := deployment.Labels
+ currentDepAnnotations := deployment.Annotations
+ currentLabels := deployment.Spec.Template.Labels
+ currentAnnotations := deployment.Spec.Template.Annotations
if currentLabels == nil && currentAnnotations == nil {
return nil // No labels or annotations to remove
}
- modifiedLabels := false
- modifiedAnnotations := false
+ modifiedDepLabels := false
+ modifiedDepAnnotations := false
+ modifiedLabels := false
+ modifiedAnnotations := false
for _, labelKey := range labelsToRemove {
+ if currentDepLabels != nil {
+ if _, exists := currentDepLabels[labelKey]; exists {
+ delete(currentDepLabels, labelKey)
+ modifiedDepLabels = true
+ }
+ }
if _, exists := currentLabels[labelKey]; exists {
delete(currentLabels, labelKey)
modifiedLabels = true
}
}
for _, annotationKey := range annotationsToRemove {
+ if currentDepAnnotations != nil {
+ if _, exists := currentDepAnnotations[annotationKey]; exists {
+ delete(currentDepAnnotations, annotationKey)
+ modifiedDepAnnotations = true
+ }
+ }
if _, exists := currentAnnotations[annotationKey]; exists {
delete(currentAnnotations, annotationKey)
modifiedAnnotations = true
}
}
+ if modifiedDepLabels {
+ deployment.Labels = currentDepLabels
+ }
+ if modifiedDepAnnotations {
+ deployment.Annotations = currentDepAnnotations
+ }
if modifiedLabels {
deployment.Spec.Template.Labels = currentLabels
}
if modifiedAnnotations {
deployment.Spec.Template.Annotations = currentAnnotations
}
- if modifiedLabels || modifiedAnnotations {
+ if modifiedDepLabels || modifiedDepAnnotations || modifiedLabels || modifiedAnnotations {
if err := r.Update(context.TODO(), deployment); err != nil {
log.Error(err, "failed to update deployment after removing labels and annotations", "name", deploymentName)
return err
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In controllers/argocd/util.go around lines 1407 to 1458, the function only
removes labels/annotations from deployment.Spec.Template but not from the
Deployment's metadata, so stale keys can remain; update the function to also
check and delete the same keys from deployment.ObjectMeta.Labels and
deployment.ObjectMeta.Annotations, track modifications for metadata separately
(or reuse the existing modified flags), guard against nil maps before
checking/deleting, set the deployment.ObjectMeta.Labels/Annotations back if
modified, and keep the existing update call so the Deployment is persisted when
either template or metadata changes.
Comment on lines
+1460
to
+1511
| // cleanupLabelsFromStatefulSet removes labels from a specific statefulset | ||
| func (r *ReconcileArgoCD) cleanupLabelsAndAnnotationsFromStatefulSet(argocd *argoproj.ArgoCD, statefulSetName string, labelsToRemove, annotationsToRemove []string) error { | ||
| namespace := argocd.GetNamespace() | ||
| statefulSet := &appsv1.StatefulSet{} | ||
| key := types.NamespacedName{Namespace: namespace, Name: statefulSetName} | ||
|
|
||
| if err := r.Get(context.TODO(), key, statefulSet); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| log.Info("StatefulSet not found, skipping label and annotation cleanup", "name", statefulSetName) | ||
| return nil // StatefulSet doesn't exist, nothing to clean | ||
| } | ||
| return fmt.Errorf("failed to get statefulset %s: %w", statefulSetName, err) | ||
| } | ||
|
|
||
| // Work on pod template labels directly, not StatefulSet metadata labels | ||
| currentLabels := statefulSet.Spec.Template.Labels | ||
| currentAnnotations := statefulSet.Spec.Template.Annotations | ||
| if currentLabels == nil && currentAnnotations == nil { | ||
| return nil // No labels or annotations to remove | ||
| } | ||
|
|
||
| modifiedLabels := false | ||
| modifiedAnnotations := false | ||
| for _, labelKey := range labelsToRemove { | ||
| if _, exists := currentLabels[labelKey]; exists { | ||
| delete(currentLabels, labelKey) | ||
| modifiedLabels = true | ||
| } | ||
| } | ||
| for _, annotationKey := range annotationsToRemove { | ||
| if _, exists := currentAnnotations[annotationKey]; exists { | ||
| delete(currentAnnotations, annotationKey) | ||
| modifiedAnnotations = true | ||
| } | ||
| } | ||
|
|
||
| if modifiedLabels { | ||
| statefulSet.Spec.Template.Labels = currentLabels | ||
| } | ||
| if modifiedAnnotations { | ||
| statefulSet.Spec.Template.Annotations = currentAnnotations | ||
| } | ||
|
|
||
| if modifiedLabels || modifiedAnnotations { | ||
| if err := r.Update(context.TODO(), statefulSet); err != nil { | ||
| log.Error(err, "failed to update statefulset after removing labels and annotations", "name", statefulSetName) | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Mirror cleanup on StatefulSet metadata as well
Same stale key risk on statefulSet.ObjectMeta.{Labels,Annotations}.
Apply this diff:
@@
- // Work on pod template labels directly, not StatefulSet metadata labels
- currentLabels := statefulSet.Spec.Template.Labels
- currentAnnotations := statefulSet.Spec.Template.Annotations
+ // Clean both statefulset metadata and pod template metadata
+ currentStsLabels := statefulSet.Labels
+ currentStsAnnotations := statefulSet.Annotations
+ currentLabels := statefulSet.Spec.Template.Labels
+ currentAnnotations := statefulSet.Spec.Template.Annotations
@@
- modifiedLabels := false
- modifiedAnnotations := false
+ modifiedStsLabels := false
+ modifiedStsAnnotations := false
+ modifiedLabels := false
+ modifiedAnnotations := false
for _, labelKey := range labelsToRemove {
+ if currentStsLabels != nil {
+ if _, exists := currentStsLabels[labelKey]; exists {
+ delete(currentStsLabels, labelKey)
+ modifiedStsLabels = true
+ }
+ }
if _, exists := currentLabels[labelKey]; exists {
delete(currentLabels, labelKey)
modifiedLabels = true
}
}
for _, annotationKey := range annotationsToRemove {
+ if currentStsAnnotations != nil {
+ if _, exists := currentStsAnnotations[annotationKey]; exists {
+ delete(currentStsAnnotations, annotationKey)
+ modifiedStsAnnotations = true
+ }
+ }
if _, exists := currentAnnotations[annotationKey]; exists {
delete(currentAnnotations, annotationKey)
modifiedAnnotations = true
}
}
+ if modifiedStsLabels {
+ statefulSet.Labels = currentStsLabels
+ }
+ if modifiedStsAnnotations {
+ statefulSet.Annotations = currentStsAnnotations
+ }
if modifiedLabels {
statefulSet.Spec.Template.Labels = currentLabels
}
if modifiedAnnotations {
statefulSet.Spec.Template.Annotations = currentAnnotations
}
- if modifiedLabels || modifiedAnnotations {
+ if modifiedStsLabels || modifiedStsAnnotations || modifiedLabels || modifiedAnnotations {
if err := r.Update(context.TODO(), statefulSet); err != nil {
log.Error(err, "failed to update statefulset after removing labels and annotations", "name", statefulSetName)
return err
}
}📝 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
| // cleanupLabelsFromStatefulSet removes labels from a specific statefulset | |
| func (r *ReconcileArgoCD) cleanupLabelsAndAnnotationsFromStatefulSet(argocd *argoproj.ArgoCD, statefulSetName string, labelsToRemove, annotationsToRemove []string) error { | |
| namespace := argocd.GetNamespace() | |
| statefulSet := &appsv1.StatefulSet{} | |
| key := types.NamespacedName{Namespace: namespace, Name: statefulSetName} | |
| if err := r.Get(context.TODO(), key, statefulSet); err != nil { | |
| if apierrors.IsNotFound(err) { | |
| log.Info("StatefulSet not found, skipping label and annotation cleanup", "name", statefulSetName) | |
| return nil // StatefulSet doesn't exist, nothing to clean | |
| } | |
| return fmt.Errorf("failed to get statefulset %s: %w", statefulSetName, err) | |
| } | |
| // Work on pod template labels directly, not StatefulSet metadata labels | |
| currentLabels := statefulSet.Spec.Template.Labels | |
| currentAnnotations := statefulSet.Spec.Template.Annotations | |
| if currentLabels == nil && currentAnnotations == nil { | |
| return nil // No labels or annotations to remove | |
| } | |
| modifiedLabels := false | |
| modifiedAnnotations := false | |
| for _, labelKey := range labelsToRemove { | |
| if _, exists := currentLabels[labelKey]; exists { | |
| delete(currentLabels, labelKey) | |
| modifiedLabels = true | |
| } | |
| } | |
| for _, annotationKey := range annotationsToRemove { | |
| if _, exists := currentAnnotations[annotationKey]; exists { | |
| delete(currentAnnotations, annotationKey) | |
| modifiedAnnotations = true | |
| } | |
| } | |
| if modifiedLabels { | |
| statefulSet.Spec.Template.Labels = currentLabels | |
| } | |
| if modifiedAnnotations { | |
| statefulSet.Spec.Template.Annotations = currentAnnotations | |
| } | |
| if modifiedLabels || modifiedAnnotations { | |
| if err := r.Update(context.TODO(), statefulSet); err != nil { | |
| log.Error(err, "failed to update statefulset after removing labels and annotations", "name", statefulSetName) | |
| return err | |
| } | |
| } | |
| return nil | |
| } | |
| // cleanupLabelsFromStatefulSet removes labels from a specific statefulset | |
| func (r *ReconcileArgoCD) cleanupLabelsAndAnnotationsFromStatefulSet(argocd *argoproj.ArgoCD, statefulSetName string, labelsToRemove, annotationsToRemove []string) error { | |
| namespace := argocd.GetNamespace() | |
| statefulSet := &appsv1.StatefulSet{} | |
| key := types.NamespacedName{Namespace: namespace, Name: statefulSetName} | |
| if err := r.Get(context.TODO(), key, statefulSet); err != nil { | |
| if apierrors.IsNotFound(err) { | |
| log.Info("StatefulSet not found, skipping label and annotation cleanup", "name", statefulSetName) | |
| return nil // StatefulSet doesn't exist, nothing to clean | |
| } | |
| return fmt.Errorf("failed to get statefulset %s: %w", statefulSetName, err) | |
| } | |
| // Clean both statefulset metadata and pod template metadata | |
| currentStsLabels := statefulSet.Labels | |
| currentStsAnnotations := statefulSet.Annotations | |
| currentLabels := statefulSet.Spec.Template.Labels | |
| currentAnnotations := statefulSet.Spec.Template.Annotations | |
| if currentLabels == nil && currentAnnotations == nil { | |
| return nil // No labels or annotations to remove | |
| } | |
| modifiedStsLabels := false | |
| modifiedStsAnnotations := false | |
| modifiedLabels := false | |
| modifiedAnnotations := false | |
| for _, labelKey := range labelsToRemove { | |
| if currentStsLabels != nil { | |
| if _, exists := currentStsLabels[labelKey]; exists { | |
| delete(currentStsLabels, labelKey) | |
| modifiedStsLabels = true | |
| } | |
| } | |
| if _, exists := currentLabels[labelKey]; exists { | |
| delete(currentLabels, labelKey) | |
| modifiedLabels = true | |
| } | |
| } | |
| for _, annotationKey := range annotationsToRemove { | |
| if currentStsAnnotations != nil { | |
| if _, exists := currentStsAnnotations[annotationKey]; exists { | |
| delete(currentStsAnnotations, annotationKey) | |
| modifiedStsAnnotations = true | |
| } | |
| } | |
| if _, exists := currentAnnotations[annotationKey]; exists { | |
| delete(currentAnnotations, annotationKey) | |
| modifiedAnnotations = true | |
| } | |
| } | |
| if modifiedStsLabels { | |
| statefulSet.Labels = currentStsLabels | |
| } | |
| if modifiedStsAnnotations { | |
| statefulSet.Annotations = currentStsAnnotations | |
| } | |
| if modifiedLabels { | |
| statefulSet.Spec.Template.Labels = currentLabels | |
| } | |
| if modifiedAnnotations { | |
| statefulSet.Spec.Template.Annotations = currentAnnotations | |
| } | |
| if modifiedStsLabels || modifiedStsAnnotations || modifiedLabels || modifiedAnnotations { | |
| if err := r.Update(context.TODO(), statefulSet); err != nil { | |
| log.Error(err, "failed to update statefulset after removing labels and annotations", "name", statefulSetName) | |
| return err | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In controllers/argocd/util.go around lines 1460 to 1511, the cleanup only
removes labels/annotations from statefulSet.Spec.Template but not from
statefulSet.ObjectMeta, leaving stale keys on the StatefulSet metadata; modify
the function to also check and remove each labelsToRemove/annotationsToRemove
key from statefulSet.ObjectMeta.Labels and statefulSet.ObjectMeta.Annotations
(ensure you initialize nil maps or guard nils), set
modifiedLabels/modifiedAnnotations if any metadata keys were deleted, assign
back to statefulSet.ObjectMeta.Labels/Annotations when changed, and then call
r.Update once if either spec/template or metadata were modified (preserving
existing error logging).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What type of PR is this?
What does this PR do / why we need it:
Delete custom labels from live object when deleted from CR
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
https://issues.redhat.com/browse/GITOPS-7405
How to test changes / Special notes to the reviewer:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests