From c8aeb97a1df8b3bac71f25b1818bd75de9dcda84 Mon Sep 17 00:00:00 2001 From: Pratik Langde Date: Mon, 7 Sep 2026 21:09:35 +0530 Subject: [PATCH 1/2] Fix GitopsService controller wiping admin NodePlacement on default ArgoCD CR When GitopsService does not configure nodeSelector or tolerations, the reconcile loop was clearing spec.nodePlacement on the default ArgoCD CR. Admins who set nodePlacement directly on the ArgoCD CR (documented pattern for dedicated infra nodes) saw scheduling config removed every reconcile. Only sync NodePlacement when GitopsService explicitly configures placement fields. Track GitopsService-managed placement with an annotation so clearing GitopsService fields still removes operator-applied placement. Fixes redhat-developer/gitops-operator#572 Signed-off-by: Pratik Langde --- common/common.go | 4 + controllers/gitopsservice_controller.go | 30 ++++- controllers/gitopsservice_controller_test.go | 125 +++++++++++++++++++ 3 files changed, 153 insertions(+), 6 deletions(-) diff --git a/common/common.go b/common/common.go index 65e7347e82c..ada9bc7f3b3 100644 --- a/common/common.go +++ b/common/common.go @@ -48,6 +48,10 @@ const ( InfraNodeSelectorAnnotation = "openshift.io/node-selector" // InfraNodeSelectorAnnotationValue is the value for the infra node selector annotation InfraNodeSelectorAnnotationValue = "node-role.kubernetes.io/infra=" + // NodePlacementManagedByGitopsServiceAnnotation marks NodePlacement on the default ArgoCD CR + // that was applied by the GitopsService controller. Used to distinguish GitopsService-managed + // placement from admin edits on the ArgoCD CR directly. + NodePlacementManagedByGitopsServiceAnnotation = "gitops.openshift.io/node-placement-managed-by-gitopsservice" ) // InfraNodeSelector returns openshift label for infrastructure nodes diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index ade2d0792b2..601a059b1ac 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -560,15 +560,33 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli changed = true } - // if user is patching nodePlacement through GitopsService CR, then existingArgoCD NodePlacement is updated. - if defaultArgoCDInstance.Spec.NodePlacement != nil { - if !reflect.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { + // Sync NodePlacement when GitopsService configures placement fields. Do not wipe NodePlacement + // that admins set directly on the ArgoCD CR when GitopsService placement fields are empty. + gitopsServiceConfiguresNodePlacement := len(instance.Spec.NodeSelector) > 0 || len(instance.Spec.Tolerations) > 0 + if gitopsServiceConfiguresNodePlacement { + if defaultArgoCDInstance.Spec.NodePlacement != nil { + if !reflect.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { + existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement + changed = true + } + } else if existingArgoCD.Spec.NodePlacement != nil { existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true } - // Handle the case where NodePlacement should be removed - } else if existingArgoCD.Spec.NodePlacement != nil { - existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement + if existingArgoCD.Annotations == nil { + existingArgoCD.Annotations = map[string]string{} + } + if existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] != "true" { + existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] = "true" + changed = true + } + } else if existingArgoCD.Annotations != nil && + existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] == "true" { + if existingArgoCD.Spec.NodePlacement != nil { + existingArgoCD.Spec.NodePlacement = nil + changed = true + } + delete(existingArgoCD.Annotations, common.NodePlacementManagedByGitopsServiceAnnotation) changed = true } diff --git a/controllers/gitopsservice_controller_test.go b/controllers/gitopsservice_controller_test.go index f499d9a5c75..af8aec365ef 100644 --- a/controllers/gitopsservice_controller_test.go +++ b/controllers/gitopsservice_controller_test.go @@ -201,6 +201,131 @@ func TestReconcileDefaultForArgoCDNodeplacement(t *testing.T) { assertNoError(t, err) assert.Check(t, existingArgoCD.Spec.NodePlacement != nil) assert.DeepEqual(t, existingArgoCD.Spec.NodePlacement.NodeSelector, gitopsService.Spec.NodeSelector) + assert.Equal(t, existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation], "true") +} + +func TestReconcileDefaultArgoCDNodePlacementPreservesDirectEdit(t *testing.T) { + logf.SetLogger(argocd.ZapLogger(true)) + s := scheme.Scheme + addKnownTypesToScheme(s) + + gitopsService := &pipelinesv1alpha1.GitopsService{ + ObjectMeta: v1.ObjectMeta{ + Name: serviceName, + }, + } + + directNodePlacement := &argoapp.ArgoCDNodePlacementSpec{ + NodeSelector: map[string]string{ + "machine.openshift.io/cluster-api-machineset": "cl01-infra-0", + }, + Tolerations: []corev1.Toleration{{ + Effect: corev1.TaintEffectNoSchedule, + Key: "infra", + Value: "reserved", + }}, + } + + fakeClient := fake.NewFakeClient(gitopsService) + reconciler := newReconcileGitOpsService(fakeClient, s) + + existingArgoCD := &argoapp.ArgoCD{ + ObjectMeta: v1.ObjectMeta{ + Name: serviceNamespace, + Namespace: serviceNamespace, + }, + Spec: argoapp.ArgoCDSpec{ + NodePlacement: directNodePlacement, + Server: argoapp.ArgoCDServerSpec{ + Route: argoapp.ArgoCDRouteSpec{ + Enabled: true, + }, + }, + ApplicationSet: &argoapp.ArgoCDApplicationSet{}, + SSO: &argoapp.ArgoCDSSOSpec{ + Provider: "dex", + Dex: &argoapp.ArgoCDDexSpec{ + Config: "test-config", + }, + }, + }, + } + + err := fakeClient.Create(context.TODO(), existingArgoCD) + assertNoError(t, err) + + _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, + existingArgoCD) + assertNoError(t, err) + assert.DeepEqual(t, existingArgoCD.Spec.NodePlacement, directNodePlacement) + _, hasManagedAnnotation := existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] + assert.Assert(t, !hasManagedAnnotation) +} + +func TestReconcileDefaultArgoCDNodePlacementClearsWhenGitopsServiceCleared(t *testing.T) { + logf.SetLogger(argocd.ZapLogger(true)) + s := scheme.Scheme + addKnownTypesToScheme(s) + + gitopsService := &pipelinesv1alpha1.GitopsService{ + ObjectMeta: v1.ObjectMeta{ + Name: serviceName, + }, + Spec: pipelinesv1alpha1.GitopsServiceSpec{ + NodeSelector: map[string]string{ + "key1": "value1", + }, + }, + } + + fakeClient := fake.NewFakeClient(gitopsService) + reconciler := newReconcileGitOpsService(fakeClient, s) + + existingArgoCD := &argoapp.ArgoCD{ + ObjectMeta: v1.ObjectMeta{ + Name: serviceNamespace, + Namespace: serviceNamespace, + }, + Spec: argoapp.ArgoCDSpec{ + Server: argoapp.ArgoCDServerSpec{ + Route: argoapp.ArgoCDRouteSpec{ + Enabled: true, + }, + }, + ApplicationSet: &argoapp.ArgoCDApplicationSet{}, + SSO: &argoapp.ArgoCDSSOSpec{ + Provider: "dex", + Dex: &argoapp.ArgoCDDexSpec{ + Config: "test-config", + }, + }, + }, + } + + err := fakeClient.Create(context.TODO(), existingArgoCD) + assertNoError(t, err) + + _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceName}, gitopsService) + assertNoError(t, err) + gitopsService.Spec.NodeSelector = nil + err = fakeClient.Update(context.TODO(), gitopsService) + assertNoError(t, err) + + _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, + existingArgoCD) + assertNoError(t, err) + assert.Assert(t, existingArgoCD.Spec.NodePlacement == nil) + _, hasManagedAnnotation := existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] + assert.Assert(t, !hasManagedAnnotation) } // If the DISABLE_DEFAULT_ARGOCD_INSTANCE is set, ensure that the default ArgoCD instance is not created. From bc936c645117bbf2311c489c7dfa09ed3010a9af Mon Sep 17 00:00:00 2001 From: Pratik Langde Date: Mon, 7 Sep 2026 21:09:35 +0530 Subject: [PATCH 2/2] Mark GitopsService-managed NodePlacement on ArgoCD create path Set the managed-by annotation on defaultArgoCDInstance before create so clearing GitopsService placement later removes operator-applied placement for newly created instances too. Signed-off-by: Pratik Langde --- controllers/gitopsservice_controller.go | 9 +++- controllers/gitopsservice_controller_test.go | 46 ++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index 601a059b1ac..d3b7a007392 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -495,6 +495,14 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli } } + gitopsServiceConfiguresNodePlacement := len(instance.Spec.NodeSelector) > 0 || len(instance.Spec.Tolerations) > 0 + if gitopsServiceConfiguresNodePlacement { + if defaultArgoCDInstance.Annotations == nil { + defaultArgoCDInstance.Annotations = map[string]string{} + } + defaultArgoCDInstance.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] = "true" + } + // Get or create ArgoCD instance in default namespace existingArgoCD := &argoapp.ArgoCD{} err = r.Client.Get(context.TODO(), types.NamespacedName{Name: defaultArgoCDInstance.Name, Namespace: defaultArgoCDInstance.Namespace}, existingArgoCD) @@ -562,7 +570,6 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli // Sync NodePlacement when GitopsService configures placement fields. Do not wipe NodePlacement // that admins set directly on the ArgoCD CR when GitopsService placement fields are empty. - gitopsServiceConfiguresNodePlacement := len(instance.Spec.NodeSelector) > 0 || len(instance.Spec.Tolerations) > 0 if gitopsServiceConfiguresNodePlacement { if defaultArgoCDInstance.Spec.NodePlacement != nil { if !reflect.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { diff --git a/controllers/gitopsservice_controller_test.go b/controllers/gitopsservice_controller_test.go index af8aec365ef..3402d697bcf 100644 --- a/controllers/gitopsservice_controller_test.go +++ b/controllers/gitopsservice_controller_test.go @@ -328,6 +328,52 @@ func TestReconcileDefaultArgoCDNodePlacementClearsWhenGitopsServiceCleared(t *te assert.Assert(t, !hasManagedAnnotation) } +func TestReconcileDefaultArgoCDNodePlacementClearsWhenCreatedByReconcile(t *testing.T) { + logf.SetLogger(argocd.ZapLogger(true)) + s := scheme.Scheme + addKnownTypesToScheme(s) + + gitopsService := &pipelinesv1alpha1.GitopsService{ + ObjectMeta: v1.ObjectMeta{ + Name: serviceName, + }, + Spec: pipelinesv1alpha1.GitopsServiceSpec{ + NodeSelector: map[string]string{ + "key1": "value1", + }, + }, + } + + fakeClient := fake.NewFakeClient(gitopsService) + reconciler := newReconcileGitOpsService(fakeClient, s) + + _, err := reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + existingArgoCD := &argoapp.ArgoCD{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, + existingArgoCD) + assertNoError(t, err) + assert.Check(t, existingArgoCD.Spec.NodePlacement != nil) + assert.Equal(t, existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation], "true") + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceName}, gitopsService) + assertNoError(t, err) + gitopsService.Spec.NodeSelector = nil + err = fakeClient.Update(context.TODO(), gitopsService) + assertNoError(t, err) + + _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, + existingArgoCD) + assertNoError(t, err) + assert.Assert(t, existingArgoCD.Spec.NodePlacement == nil) + _, hasManagedAnnotation := existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] + assert.Assert(t, !hasManagedAnnotation) +} + // If the DISABLE_DEFAULT_ARGOCD_INSTANCE is set, ensure that the default ArgoCD instance is not created. func TestReconcileDisableDefault(t *testing.T) {