fix: added logic to remove custom labels from live objects if removed from argocd spec #1823
Conversation
alkakumari016
force-pushed
the
alkumari/rec-labels
branch
from
0614ce5 to
6051879
Compare
alkakumari016
force-pushed
the
alkumari/rec-labels
branch
2 times, most recently
from
8c0ef28 to
689fe12
Compare
alkakumari016
force-pushed
the
alkumari/rec-labels
branch
from
689fe12 to
e0c8672
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
controllers/argocd/repo_server.go (1)
523-541: Persist metadata updates when using UpdateMapValuesCalling
UpdateMapValuesonexisting.Spec.Template.{Annotations,Labels}before change detection mutates the in-memory object but leaveschangedfalse. We then return withoutr.Update, so any new spec labels/annotations never reach the cluster. Capture the return values once, use them to setchanged, and drop the second call.- // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. - UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) - UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) + // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. + labelsChanged := UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) + annotationsChanged := UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) @@ - if !reflect.DeepEqual(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) { + if !reflect.DeepEqual(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) { existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations - if changed { - explanation += ", " - } - explanation += "annotations" - changed = true + if !annotationsChanged { + if changed { + explanation += ", " + } + explanation += "annotations" + changed = true + } } @@ - if UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) { + if labelsChanged { if changed { explanation += ", " } explanation += "labels" changed = true } + if annotationsChanged { + if changed { + explanation += ", " + } + explanation += "annotations" + changed = true + }controllers/argocd/deployment.go (1)
1261-1278: Honor UpdateMapValues’ change flagWe call
UpdateMapValuesonce without looking at its return value, which mutatesexisting.Spec.Template.Annotations/Labelsbefore we checkchanged. The follow-up DeepEqual/second call never fires, so metadata-only drift (e.g., adding a label back into the spec) leaveschangedfalse and we skipr.Update, meaning the live Deployment never picks up the label/annotation change this PR is meant to ship. Please gate the mutation on the boolean result so we both preserve non-operator keys and still mark the Deployment dirty.- //Check if labels/annotations have changed - UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) - UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) - - if !reflect.DeepEqual(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) { - existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations - if changed { - explanation += ", " - } - explanation += "annotations" - changed = true - } - // Preserve non-operator labels in the existing deployment. - if UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) { + if UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) { + if changed { + explanation += ", " + } + explanation += "annotations" + changed = true + } + // Preserve non-operator labels in the existing deployment while still flagging changes. + if UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) {controllers/argocd/statefulset.go (1)
1027-1045: Propagate UpdateMapValues result to trigger updatesSame problem here as in the Deployment reconcile: the first
UpdateMapValuescall mutatesexistingbefore we check for differences, so the later DeepEqual sees no change andchangednever flips. Any metadata-only tweak (e.g., adding/removing a pod-template label) will now no-op and the StatefulSet never gets updated. Please rely on the helper’s boolean return to updateexplanation/changed.- //Check if labels/annotations have changed - UpdateMapValues(&existing.Spec.Template.Labels, ss.Spec.Template.Labels) - UpdateMapValues(&existing.Spec.Template.Annotations, ss.Spec.Template.Annotations) - - if !reflect.DeepEqual(ss.Spec.Template.Annotations, existing.Spec.Template.Annotations) { - existing.Spec.Template.Annotations = ss.Spec.Template.Annotations - if changed { - explanation += ", " - } - explanation += "annotations" - changed = true - } - - if !reflect.DeepEqual(ss.Spec.Template.Labels, existing.Spec.Template.Labels) { - existing.Spec.Template.Labels = ss.Spec.Template.Labels - if changed { - explanation += ", " - } - explanation += "labels" - changed = true - } + if UpdateMapValues(&existing.Spec.Template.Annotations, ss.Spec.Template.Annotations) { + if changed { + explanation += ", " + } + explanation += "annotations" + changed = true + } + + if UpdateMapValues(&existing.Spec.Template.Labels, ss.Spec.Template.Labels) { + if changed { + explanation += ", " + } + explanation += "labels" + changed = true + }controllers/argocd/applicationset.go (1)
279-296: Don’t normalise annotations before diffing
UpdateMapValuesis mutatingexisting.Spec.Template.Annotationsbefore we runidentifyDeploymentDifference, so if annotations are the only delta,deploymentsDifferentstays empty and we never reconcile—new annotations never land on the live Deployment. Capture the change status first, then apply the helper inside the update branch so the diff still sees the divergence.- //Check if annotations have changed - UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) - - // If the Deployment already exists, make sure the values we care about are up-to-date - deploymentsDifferent := identifyDeploymentDifference(*existing, *deploy) + annotationChanged := !reflect.DeepEqual(existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) + + // If the Deployment already exists, make sure the values we care about are up-to-date + deploymentsDifferent := identifyDeploymentDifference(*existing, *deploy) @@ - existing.Spec.Template.Spec.ServiceAccountName = podSpec.ServiceAccountName - UpdateMapValues(&existing.Labels, deploy.Labels) - UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) + existing.Spec.Template.Spec.ServiceAccountName = podSpec.ServiceAccountName + UpdateMapValues(&existing.Labels, deploy.Labels) + UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) existing.Spec.Selector = deploy.Spec.Selector existing.Spec.Template.Spec.NodeSelector = deploy.Spec.Template.Spec.NodeSelector existing.Spec.Template.Spec.Tolerations = deploy.Spec.Template.Spec.Tolerations existing.Spec.Template.Spec.Containers[0].SecurityContext = deploy.Spec.Template.Spec.Containers[0].SecurityContext - existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations + if annotationChanged { + UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
controllers/argocd/applicationset.go(1 hunks)controllers/argocd/argocd_controller.go(1 hunks)controllers/argocd/configmap.go(2 hunks)controllers/argocd/deployment.go(2 hunks)controllers/argocd/notifications.go(1 hunks)controllers/argocd/repo_server.go(2 hunks)controllers/argocd/role.go(2 hunks)controllers/argocd/statefulset.go(1 hunks)controllers/argocd/util.go(3 hunks)controllers/argocd/util_test.go(2 hunks)tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go(4 hunks)
🔇 Additional comments (8)
controllers/argocd/util.go (1)
1169-1248: Guard DropMetadataStore with synchronization
DropMetadataStoreis a package-level map that is mutated from both the predicate (viacalculateRemovedSpecLabels→addDropMetadata) and the reconcile loop (processDropMetadataForCleanup,removeDropMetadataForNamespace). These run on different controller-runtime worker goroutines, so we now have concurrent reads/writes to a plain Go map, which will intermittently panic withfatal error: concurrent map writesand crash the operator. Please protect the store with a mutex (or switch tosync.Map) and copy the slice when returning it to avoid holding the lock during cleanup.@@ - "strings" + "strings" + "sync" @@ -var DropMetadataStore = make(map[string][]DropMetadata) +var dropMetadataMu sync.RWMutex +var DropMetadataStore = make(map[string][]DropMetadata) @@ func addDropMetadata(namespace, resource string, keys []string) { + dropMetadataMu.Lock() + defer dropMetadataMu.Unlock() @@ func getDropMetadataForNamespace(namespace string) []DropMetadata { - return DropMetadataStore[namespace] + dropMetadataMu.RLock() + defer dropMetadataMu.RUnlock() + return append([]DropMetadata(nil), DropMetadataStore[namespace]...) } @@ func removeDropMetadataForNamespace(namespace string) { - delete(DropMetadataStore, namespace) + dropMetadataMu.Lock() + delete(DropMetadataStore, namespace) + dropMetadataMu.Unlock() } </blockquote></details> <details> <summary>controllers/argocd/role.go (1)</summary><blockquote> `517-543`: **Good use of UpdateMapValues** Thanks for wiring `UpdateMapValues` directly into the aggregated-role reconciliation; this keeps the live metadata intact while still letting us know when something changed. </blockquote></details> <details> <summary>controllers/argocd/util_test.go (1)</summary><blockquote> `1173-1254`: **Comprehensive coverage of UpdateMapValues** Nice job enumerating the edge cases around retained, overridden, and empty-map scenarios—this should keep regressions in check. </blockquote></details> <details> <summary>tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go (5)</summary><blockquote> `146-195`: **LGTM! Selective label removal verification is well-structured.** The test correctly verifies that partial label removal works as expected, with proper Eventually blocks fetching fresh data and checking both removal and retention of labels. --- `223-269`: **LGTM! Controller and appset selective removal logic is correct.** The test properly verifies selective label removal with appropriate Eventually blocks. --- `289-383`: **LGTM! Annotation removal tests are well-implemented.** The selective annotation removal verification logic correctly fetches fresh data in all Eventually blocks and properly validates both removal and retention. --- `408-442`: **LGTM! Function refactoring improves clarity.** The addition of the `componentName` parameter to `expectLabelsAndAnnotationsRemovedFromDepl` and the verification of operator-managed labels (lines 429-433) are valuable improvements that ensure the cleanup logic doesn't inadvertently remove system-managed metadata. --- `455-471`: **LGTM! New labels addition test validates the full lifecycle.** This test step appropriately validates that labels can be added after complete removal, ensuring the label management logic works bidirectionally. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go
Outdated
Show resolved
Hide resolved
tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go
Outdated
Show resolved
Hide resolved
tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go
Outdated
Show resolved
Hide resolved
…rator labels (argoproj-labs#1737) * GITOPS-6285: reconcile operator owned labels for configMap and ignore non operator labels Signed-off-by: Alka Kumari <alkumari@redhat.com> * GITOPS-6285: added reconcilialtion logic for labels Signed-off-by: Alka Kumari <alkumari@redhat.com> * Removed a typo that resulted in test case failures Signed-off-by: Alka Kumari <alkumari@redhat.com> * GITOPS-6285: error handling for r.Client.Update Signed-off-by: Alka Kumari <alkumari@redhat.com> * GITOPS-6285: use r.Update Signed-off-by: Alka Kumari <alkumari@redhat.com> * GITOPS-6285: changed func name to UpdateMapValues and added test cases Signed-off-by: Alka Kumari <alkumari@redhat.com> * GITOPS-6285: removed addKubernetesData and replaced the references with UpdateMapValues Signed-off-by: Alka Kumari <alkumari@redhat.com> 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 <alkumari@redhat.com> * GITOPS-6285: changed comments Signed-off-by: Alka Kumari <alkumari@redhat.com> --------- Signed-off-by: Alka Kumari <alkumari@redhat.com>
… removed from argocd spec Signed-off-by: Alka Kumari <alkumari@redhat.com>
Signed-off-by: Alka Kumari <alkumari@redhat.com>
Signed-off-by: Alka Kumari <alkumari@redhat.com>
Signed-off-by: Alka Kumari <alkumari@redhat.com>
Signed-off-by: Alka Kumari <alkumari@redhat.com>
Signed-off-by: Alka Kumari <alkumari@redhat.com>
…o ginkgo tests for checking custom labels and annotations Signed-off-by: Alka Kumari <alkumari@redhat.com>
Signed-off-by: Alka Kumari <alkumari@redhat.com>
alkakumari016
force-pushed
the
alkumari/rec-labels
branch
from
6ba2568 to
37fdd7f
Compare
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)
controllers/argocd/repo_server.go (1)
529-535: Critical: Labels removed from spec won't be deleted from live objects.The current logic uses
UpdateMapValues, which only adds or updates labels but does NOT remove labels present inexistingthat are absent fromdeploy. This violates the PR objective to remove custom labels when they're removed from the ArgoCD spec.This is inconsistent with the annotation handling (lines 520-527), which correctly uses wholesale replacement to ensure deleted annotations are removed.
Apply this diff to handle label removal correctly:
- if UpdateMapValues(&existing.Spec.Template.Labels, deploy.Spec.Template.Labels) { + if !reflect.DeepEqual(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) { + existing.Spec.Template.Labels = deploy.Spec.Template.Labels if changed { explanation += ", " } explanation += "labels" changed = true }This ensures parity with annotation handling and correctly removes deleted custom labels.
📜 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(2 hunks)controllers/argocd/repo_server.go(2 hunks)controllers/argocd/role.go(2 hunks)controllers/argocd/statefulset.go(1 hunks)controllers/argocd/util.go(4 hunks)controllers/argocd/util_test.go(2 hunks)tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- controllers/argocd/statefulset.go
- controllers/argocd/util_test.go
- controllers/argocd/deployment.go
- controllers/argocd/configmap.go
🧰 Additional context used
🧬 Code graph analysis (5)
controllers/argocd/role.go (1)
controllers/argocd/util.go (1)
UpdateMapValues(1891-1903)
tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go (2)
tests/ginkgo/fixture/argocd/fixture.go (1)
Update(23-44)tests/ginkgo/fixture/k8s/fixture.go (2)
Update(144-161)ExistByName(101-118)
controllers/argocd/applicationset.go (1)
controllers/argocd/util.go (1)
UpdateMapValues(1891-1903)
controllers/argocd/repo_server.go (1)
controllers/argocd/util.go (1)
UpdateMapValues(1891-1903)
controllers/argocd/util.go (3)
api/v1alpha1/argocd_types.go (1)
ArgoCD(61-67)api/v1beta1/argocd_types.go (1)
ArgoCD(61-67)controllers/argocd/argocd_controller.go (1)
ReconcileArgoCD(73-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
🔇 Additional comments (3)
controllers/argocd/util.go (1)
1248-1310: Protect DropMetadataStore with synchronization.Line 1256 makes
DropMetadataStorea package-level map that is written fromcalculateRemovedSpecLabels(predicate goroutine) and read/cleared inprocessDropMetadataForCleanupduring reconciliation. Controller-runtime can execute both paths concurrently, so this map will race and can panic under load. Please guard access with a lock (or move the store onto the reconciler instance) before release.@@ -import ( +import ( "bytes" "context" "crypto" "crypto/rand" "crypto/sha256" @@ "sort" "strconv" "strings" "text/template" "time" + "sync" @@ -var DropMetadataStore = make(map[string][]DropMetadata) +var ( + dropMetadataMu sync.RWMutex + DropMetadataStore = make(map[string][]DropMetadata) +) @@ func addDropMetadata(namespace, resource string, keys []string) { if len(keys) == 0 { return } + dropMetadataMu.Lock() + defer dropMetadataMu.Unlock() + dropMeta := DropMetadata{ Resource: resource, Keys: keys, } @@ func getDropMetadataForNamespace(namespace string) []DropMetadata { - return DropMetadataStore[namespace] + dropMetadataMu.RLock() + defer dropMetadataMu.RUnlock() + entries := DropMetadataStore[namespace] + copied := make([]DropMetadata, len(entries)) + copy(copied, entries) + return copied } @@ func removeDropMetadataForNamespace(namespace string) { - delete(DropMetadataStore, namespace) + dropMetadataMu.Lock() + defer dropMetadataMu.Unlock() + delete(DropMetadataStore, namespace) }controllers/argocd/applicationset.go (1)
279-299: Restore reconciliation for annotation-only changes.Line 281 calls
UpdateMapValuesbeforeidentifyDeploymentDifference, so when the only change is adding/updating a pod template annotation, the cache is mutated in place andidentifyDeploymentDifferencereturns an empty string. We then exit without callingr.Update, meaning the new annotation never reaches the cluster.- //Check if annotations have changed - UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) - - // If the Deployment already exists, make sure the values we care about are up-to-date - deploymentsDifferent := identifyDeploymentDifference(*existing, *deploy) - if len(deploymentsDifferent) > 0 { + annotationsChanged := UpdateMapValues(&existing.Spec.Template.Annotations, deploy.Spec.Template.Annotations) + + // If the Deployment already exists, make sure the values we care about are up-to-date + deploymentsDifferent := identifyDeploymentDifference(*existing, *deploy) + updateReason := deploymentsDifferent + if annotationsChanged && !strings.Contains(updateReason, ".Spec.Template.Annotations") { + if updateReason != "" { + updateReason += ", " + } + updateReason += ".Spec.Template.Annotations" + } + + if updateReason != "" { 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) @@ - existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations - - argoutil.LogResourceUpdate(log, existing, "due to difference in", deploymentsDifferent) + existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations + + argoutil.LogResourceUpdate(log, existing, "due to difference in", updateReason) return r.Update(context.TODO(), existing) } return nil // Deployment found with nothing to do, move along...controllers/argocd/repo_server.go (1)
520-527: Annotations handling correctly removes deleted keys.The wholesale replacement
existing.Spec.Template.Annotations = deploy.Spec.Template.Annotationsensures that annotations removed from the ArgoCD spec are properly deleted from the live object.
Signed-off-by: Alka Kumari <alkumari@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
controllers/argocd/deployment.go (1)
1260-1267: Clarify the comment to reflect actual behavior.The comment "Preserve non-operator labels in the existing deployment" may be misleading.
UpdateMapValuesadds or updates keys fromdeployintoexisting, potentially overwriting values. It doesn't remove keys absent fromdeploy, but it also doesn't "preserve" labels in the sense of protecting them from being overwritten.Consider revising to something like: "Merge labels from desired deployment into existing; removal of labels is handled separately by DropMetadata cleanup."
controllers/argocd/applicationset.go (1)
280-290: Update the comment to mention both annotations and labels.The comment on line 280 says "Check if annotations have changed", but the code also checks and updates labels on lines 289-290. Consider updating to "Check if annotations and labels have changed" for accuracy.
Otherwise, the logic is correct and consistent with the broader PR changes.
controllers/argocd/util.go (1)
1296-1320: Fix typo in comment.Line 1296: "thatefficiently" should be "that efficiently" (missing space).
Otherwise, the
mergeKeysfunction correctly uses a map for deduplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controllers/argocd/applicationset.go(1 hunks)controllers/argocd/deployment.go(1 hunks)controllers/argocd/repo_server.go(1 hunks)controllers/argocd/statefulset.go(1 hunks)controllers/argocd/util.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/argocd/repo_server.go
- controllers/argocd/statefulset.go
🧰 Additional context used
🧬 Code graph analysis (3)
controllers/argocd/applicationset.go (1)
controllers/argocd/util.go (1)
UpdateMapValues(1940-1952)
controllers/argocd/deployment.go (1)
controllers/argocd/util.go (1)
UpdateMapValues(1940-1952)
controllers/argocd/util.go (2)
api/v1alpha1/argocd_types.go (1)
ArgoCD(61-67)controllers/argocd/argocd_controller.go (1)
ReconcileArgoCD(73-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
- GitHub Check: build
🔇 Additional comments (11)
controllers/argocd/deployment.go (1)
1252-1259: LGTM!The use of
UpdateMapValuescorrectly merges annotations from the desired deployment into the existing deployment while tracking changes for the explanation message.controllers/argocd/util.go (10)
1033-1033: LGTM!Correctly registers the new predicate to watch for ArgoCD CR updates and track removed labels/annotations.
1259-1294: Logic is correct, but needs mutex protection.The
addDropMetadatafunction correctly merges removed labels/annotations and handles resource deduplication. However, it requires mutex protection when accessingDropMetadataStore(see previous comment).
1322-1330: LGTM, but add mutex protection.These functions are straightforward getters/setters for
DropMetadataStore. Ensure they use appropriate locking as noted in the earlier comment about the global map.
1397-1448: LGTM!The cleanup logic for deployments is correct:
- Fetches the deployment
- Skips if not found
- Removes specified labels and annotations from pod template metadata
- Updates only if changes were made
1450-1501: LGTM!The cleanup logic for StatefulSets mirrors the deployment cleanup and is correct.
1503-1522: LGTM!The predicate correctly watches for ArgoCD CR updates and triggers label/annotation removal tracking. It always returns
trueto ensure the reconciliation loop processes the changes.
1524-1575: Verify the empty string behavior for label removal.Lines 1538 and 1543 treat both
nilmaps and empty string values as indicators that a label/annotation should be removed:if newLabels == nil || newLabels[key] == ""This is likely correct since Kubernetes labels cannot have empty string values. However, consider adding a comment explaining this behavior for clarity.
1938-1952: LGTM!
UpdateMapValuescorrectly implements merge semantics: it adds or updates keys fromsourceintoexistingbut does not remove keys. This is the intended behavior, with label/annotation removal handled separately by the DropMetadata cleanup system.
2590-2604: LGTM!The usage of
UpdateMapValuesinreconcileDeploymentHelperis consistent with the pattern used throughout the codebase for merging labels into existing deployments.
1332-1395: Cleanup orchestration placement verified and approved.The
processDropMetadataForCleanupfunction is correctly integrated:
- Registered predicate
argoCDSpecLabelAndAnnotationCleanupPredicatedetects removed labels/annotations and populates the DropMetadataStoreprocessDropMetadataForCleanupis called at line 352, after predicate detection but beforereconcileResourcesat line 354- Execution order matches the intended flow: detect → store → cleanup → reconcile
controllers/argocd/util.go
Outdated
| // DropMetadata stores metadata about removed labels for a specific resource type | ||
| type DropMetadata struct { | ||
| Resource string `json:"resource"` | ||
| LabelKeys []string `json:"labelKeys"` | ||
| AnnotationKeys []string `json:"annotationKeys"` | ||
| } | ||
|
|
||
| // Global map to store removed labels per namespace | ||
| // Key: namespace, Value: slice of DropMetadata for different resources | ||
| var DropMetadataStore = make(map[string][]DropMetadata) |
There was a problem hiding this comment.
Add synchronization for concurrent access to DropMetadataStore.
DropMetadataStore is a global map that can be accessed concurrently by multiple reconciliation loops. Without synchronization (e.g., sync.Mutex or sync.RWMutex), this creates a race condition that can lead to data corruption or panics.
Apply this diff to add thread-safe access:
+var DropMetadataStoreMutex sync.RWMutex
+
// Global map to store removed labels per namespace
// Key: namespace, Value: slice of DropMetadata for different resources
var DropMetadataStore = make(map[string][]DropMetadata)Then wrap all accesses to DropMetadataStore with appropriate locks:
- Use
DropMetadataStoreMutex.Lock()/Unlock()for writes (inaddDropMetadataandremoveDropMetadataForNamespace) - Use
DropMetadataStoreMutex.RLock()/RUnlock()for reads (ingetDropMetadataForNamespace)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In controllers/argocd/util.go around lines 1248-1257, the global
DropMetadataStore map is not protected for concurrent access; add a
package-level sync.RWMutex named DropMetadataStoreMutex and use it to make
access thread-safe: use DropMetadataStoreMutex.Lock()/Unlock() around all write
sites (e.g., addDropMetadata and removeDropMetadataForNamespace) and use
DropMetadataStoreMutex.RLock()/RUnlock() around all read sites (e.g.,
getDropMetadataForNamespace); ensure the mutex is declared adjacent to the
DropMetadataStore variable and wrap every access path to the map accordingly.
Signed-off-by: Alka Kumari <alkumari@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controllers/argocd/util.go(2 hunks)tests/ginkgo/parallel/1-043_validate_custom_labels_annotations_test.go(1 hunks)tests/ginkgo/parallel/1-121_validate_external_labels_annotations_preserved_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/argocd/util.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-121_validate_external_labels_annotations_preserved_test.go (4)
tests/ginkgo/fixture/fixture.go (2)
EnsureParallelCleanSlate(45-55)CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)tests/ginkgo/fixture/utils/fixtureUtils.go (1)
GetE2ETestKubeClient(33-41)tests/ginkgo/fixture/argocd/fixture.go (1)
BeAvailable(47-49)tests/ginkgo/fixture/k8s/fixture.go (1)
ExistByName(101-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
- GitHub Check: build
alkakumari016
force-pushed
the
alkumari/rec-labels
branch
from
c1a899e to
9cd2b28
Compare
…e preserved Signed-off-by: Alka Kumari <alkumari@redhat.com>
alkakumari016
force-pushed
the
alkumari/rec-labels
branch
from
9cd2b28 to
c680642
Compare
2 participants
Assisted-by: < Cursor/Gemini etc>
What type of PR is this?
/kind bug
What does this PR do / why we need it:
When we remove custom labels from the ArgoCD spec (set them to {}), the new code iterates over an empty map, so it doesn't add any new keys, but it also doesn't remove the existing custom keys that should be cleaned up. This PR fixes this bug.
Fixes #?
GITOPS-7405
How to test changes / Special notes to the reviewer:
Summary by CodeRabbit
Bug Fixes
Tests