From 1bf9ed2671d09e5e1c803c7e7daff6f535997bbb Mon Sep 17 00:00:00 2001 From: Pratik Langde Date: Mon, 7 Sep 2026 21:09:35 +0530 Subject: [PATCH 1/3] 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 d37b887aec1..ae8851962bf 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -588,15 +588,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 160b96c82ae..5102dafa9e1 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 300de7f5c3c79be2e1d75a170bb1411a57d565e1 Mon Sep 17 00:00:00 2001 From: Pratik Langde Date: Mon, 7 Sep 2026 21:09:35 +0530 Subject: [PATCH 2/3] 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 ae8851962bf..e2c8738894f 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -523,6 +523,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) @@ -590,7 +598,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 5102dafa9e1..3428df82be1 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) { From 169509d6594446f786573bb3419bb4854e3939a3 Mon Sep 17 00:00:00 2001 From: Pratik Langde Date: Tue, 13 Oct 2026 23:54:37 +0530 Subject: [PATCH 3/3] OCPBUGS-120842: apply default-instance node selectors on the namespace GitOps Service owns the default Argo CD instance. Node selection for that instance uses the openshift-gitops openshift.io/node-selector annotation (runOnInfra and GitopsService.nodeSelector), not spec.nodePlacement.NodeSelector on the ArgoCD CR. Tolerations still sync through GitopsService onto the default instance. --- common/common.go | 33 ++++- common/common_test.go | 36 +++++ controllers/gitopsservice_controller.go | 85 ++++-------- controllers/gitopsservice_controller_test.go | 128 +++--------------- .../1-071_validate_node_selectors_test.go | 41 ++++-- 5 files changed, 141 insertions(+), 182 deletions(-) create mode 100644 common/common_test.go diff --git a/common/common.go b/common/common.go index ada9bc7f3b3..1eb9372df4a 100644 --- a/common/common.go +++ b/common/common.go @@ -17,7 +17,11 @@ limitations under the License. package common import ( + "fmt" + "maps" "os" + "slices" + "strings" ) const ( @@ -48,10 +52,6 @@ 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 @@ -61,6 +61,31 @@ func InfraNodeSelector() map[string]string { } } +// NamespaceNodeSelectorAnnotation builds the openshift.io/node-selector value for the +// default GitOps namespace. runOnInfra uses the infra role; GitopsService nodeSelector +// entries are merged. An empty result means the annotation should be removed. +func NamespaceNodeSelectorAnnotation(runOnInfra bool, nodeSelector map[string]string) string { + if !runOnInfra && len(nodeSelector) == 0 { + return "" + } + if runOnInfra && len(nodeSelector) == 0 { + return InfraNodeSelectorAnnotationValue + } + sel := maps.Clone(nodeSelector) + if sel == nil { + sel = map[string]string{} + } + if runOnInfra { + sel[InfraNodeLabelSelector] = "" + } + keys := slices.Sorted(maps.Keys(sel)) + parts := make([]string, 0, len(keys)) + for _, k := range keys { + parts = append(parts, fmt.Sprintf("%s=%s", k, sel[k])) + } + return strings.Join(parts, ",") +} + func StringFromEnv(env string, defaultValue string) string { if str := os.Getenv(env); str != "" { return str diff --git a/common/common_test.go b/common/common_test.go new file mode 100644 index 00000000000..0d6b8252d14 --- /dev/null +++ b/common/common_test.go @@ -0,0 +1,36 @@ +package common + +import "testing" + +func TestNamespaceNodeSelectorAnnotation(t *testing.T) { + tests := map[string]struct { + runOnInfra bool + nodeSelector map[string]string + want string + }{ + "neither": { + want: "", + }, + "runOnInfra only keeps exact infra value": { + runOnInfra: true, + want: InfraNodeSelectorAnnotationValue, + }, + "custom selector": { + nodeSelector: map[string]string{"foo": "bar"}, + want: "foo=bar", + }, + "runOnInfra plus custom merges": { + runOnInfra: true, + nodeSelector: map[string]string{"foo": "bar"}, + want: "foo=bar,node-role.kubernetes.io/infra=", + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := NamespaceNodeSelectorAnnotation(tt.runOnInfra, tt.nodeSelector) + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index e2c8738894f..eb0f5c09844 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -264,7 +264,7 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil if err != nil { if errors.IsNotFound(err) { reqLogger.Info("Creating a new Namespace", "Name", namespace) - ensureInfraNodeSelectorAnnotation(namespaceRef, instance.Spec.RunOnInfra) + ensureInfraNodeSelectorAnnotation(namespaceRef, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) err = r.Client.Create(ctx, namespaceRef) if err != nil { return reconcile.Result{}, err @@ -273,7 +273,7 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil return reconcile.Result{}, err } } else { - if ensureNamespaceMetadata(namespaceRef, instance.Spec.RunOnInfra) { + if ensureNamespaceMetadata(namespaceRef, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) { err = r.Client.Update(context.TODO(), namespaceRef) if err != nil { return reconcile.Result{}, err @@ -460,7 +460,7 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli if err != nil { if errors.IsNotFound(err) { reqLogger.Info("Creating a new Namespace", "Name", argocdNS.Name) - ensureInfraNodeSelectorAnnotation(argocdNS, instance.Spec.RunOnInfra) + ensureInfraNodeSelectorAnnotation(argocdNS, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) err = r.Client.Create(context.TODO(), argocdNS) if err != nil { return reconcile.Result{}, err @@ -490,7 +490,7 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli } } - if ensureNamespaceMetadata(argocdNS, instance.Spec.RunOnInfra) { + if ensureNamespaceMetadata(argocdNS, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) { err = r.Client.Update(context.TODO(), argocdNS) if err != nil { return reconcile.Result{}, err @@ -504,15 +504,9 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli return reconcile.Result{}, err } - if len(instance.Spec.NodeSelector) > 0 { - if defaultArgoCDInstance.Spec.NodePlacement == nil { - defaultArgoCDInstance.Spec.NodePlacement = &argoapp.ArgoCDNodePlacementSpec{ - NodeSelector: instance.Spec.NodeSelector, - } - } else { - defaultArgoCDInstance.Spec.NodePlacement.NodeSelector = argocdutil.AppendStringMap(defaultArgoCDInstance.Spec.NodePlacement.NodeSelector, instance.Spec.NodeSelector) - } - } + // Node selectors for the default instance are applied on the openshift-gitops + // namespace (openshift.io/node-selector), not on ArgoCD spec.nodePlacement. + // GitopsService still owns NodePlacement.Tolerations for taints. if len(instance.Spec.Tolerations) > 0 { if defaultArgoCDInstance.Spec.NodePlacement == nil { defaultArgoCDInstance.Spec.NodePlacement = &argoapp.ArgoCDNodePlacementSpec{ @@ -523,14 +517,6 @@ 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) @@ -596,32 +582,15 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli changed = true } - // 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. - 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 { + // GitopsService owns the default ArgoCD instance. Sync NodePlacement from + // GitopsService (tolerations only; node selectors use the namespace annotation). + if defaultArgoCDInstance.Spec.NodePlacement != nil { + if !reflect.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true } - 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) + } else if existingArgoCD.Spec.NodePlacement != nil { + existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true } @@ -1082,11 +1051,11 @@ func policyRuleForBackendServiceClusterRole() []rbacv1.PolicyRule { } } -func ensureNamespaceMetadata(namespace *corev1.Namespace, runOnInfra bool) bool { +func ensureNamespaceMetadata(namespace *corev1.Namespace, runOnInfra bool, nodeSelector map[string]string) bool { changed := false labelUpdate, _ := ensurePodSecurityLabels(namespace) changed = changed || labelUpdate - annotationUpdate, _ := ensureInfraNodeSelectorAnnotation(namespace, runOnInfra) + annotationUpdate, _ := ensureInfraNodeSelectorAnnotation(namespace, runOnInfra, nodeSelector) changed = changed || annotationUpdate return changed } @@ -1102,20 +1071,22 @@ func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespa return false, namespace } -func ensureInfraNodeSelectorAnnotation(namespace *corev1.Namespace, runOnInfra bool) (bool, *corev1.Namespace) { +func ensureInfraNodeSelectorAnnotation(namespace *corev1.Namespace, runOnInfra bool, nodeSelector map[string]string) (bool, *corev1.Namespace) { if namespace.Annotations == nil { namespace.Annotations = make(map[string]string) } - if runOnInfra { - if namespace.Annotations[common.InfraNodeSelectorAnnotation] != common.InfraNodeSelectorAnnotationValue { - namespace.Annotations[common.InfraNodeSelectorAnnotation] = common.InfraNodeSelectorAnnotationValue - return true, namespace - } - } else { - if namespace.Annotations[common.InfraNodeSelectorAnnotation] == common.InfraNodeSelectorAnnotationValue { - delete(namespace.Annotations, common.InfraNodeSelectorAnnotation) - return true, namespace + want := common.NamespaceNodeSelectorAnnotation(runOnInfra, nodeSelector) + got := namespace.Annotations[common.InfraNodeSelectorAnnotation] + if want == "" { + if got == "" { + return false, namespace } + delete(namespace.Annotations, common.InfraNodeSelectorAnnotation) + return true, namespace } - return false, namespace + if got == want { + return false, namespace + } + namespace.Annotations[common.InfraNodeSelectorAnnotation] = want + return true, namespace } diff --git a/controllers/gitopsservice_controller_test.go b/controllers/gitopsservice_controller_test.go index 3428df82be1..7eb72d359d1 100644 --- a/controllers/gitopsservice_controller_test.go +++ b/controllers/gitopsservice_controller_test.go @@ -152,8 +152,6 @@ func TestReconcileDefaultForArgoCDNodeplacement(t *testing.T) { s := scheme.Scheme addKnownTypesToScheme(s) - var err error - gitopsService := &pipelinesv1alpha1.GitopsService{ ObjectMeta: v1.ObjectMeta{ Name: serviceName, @@ -189,80 +187,22 @@ func TestReconcileDefaultForArgoCDNodeplacement(t *testing.T) { }, } - err = fakeClient.Create(context.TODO(), existingArgoCD) + err := fakeClient.Create(context.TODO(), existingArgoCD) assertNoError(t, err) _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) assertNoError(t, err) - // verify whether existingArgoCD NodePlacement is updated when user is patching nodePlacement through GitopsService CR err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, existingArgoCD) 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) + // Node selectors are applied on the namespace, not on the default ArgoCD CR. + assert.Assert(t, existingArgoCD.Spec.NodePlacement == nil) - err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, - existingArgoCD) + ns := &corev1.Namespace{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, ns) assertNoError(t, err) - assert.DeepEqual(t, existingArgoCD.Spec.NodePlacement, directNodePlacement) - _, hasManagedAnnotation := existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] - assert.Assert(t, !hasManagedAnnotation) + assert.Equal(t, ns.Annotations[common.InfraNodeSelectorAnnotation], "foo=bar") } func TestReconcileDefaultArgoCDNodePlacementClearsWhenGitopsServiceCleared(t *testing.T) { @@ -278,6 +218,7 @@ func TestReconcileDefaultArgoCDNodePlacementClearsWhenGitopsServiceCleared(t *te NodeSelector: map[string]string{ "key1": "value1", }, + Tolerations: deploymentDefaultTolerations(), }, } @@ -311,55 +252,22 @@ func TestReconcileDefaultArgoCDNodePlacementClearsWhenGitopsServiceCleared(t *te _, 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) -} - -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) + assert.Check(t, existingArgoCD.Spec.NodePlacement != nil) + assert.Assert(t, existingArgoCD.Spec.NodePlacement.NodeSelector == nil) + assert.DeepEqual(t, existingArgoCD.Spec.NodePlacement.Tolerations, deploymentDefaultTolerations()) - existingArgoCD := &argoapp.ArgoCD{} - err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, - existingArgoCD) + ns := &corev1.Namespace{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, ns) assertNoError(t, err) - assert.Check(t, existingArgoCD.Spec.NodePlacement != nil) - assert.Equal(t, existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation], "true") + assert.Equal(t, ns.Annotations[common.InfraNodeSelectorAnnotation], "key1=value1") err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceName}, gitopsService) assertNoError(t, err) gitopsService.Spec.NodeSelector = nil + gitopsService.Spec.Tolerations = nil err = fakeClient.Update(context.TODO(), gitopsService) assertNoError(t, err) @@ -370,8 +278,12 @@ func TestReconcileDefaultArgoCDNodePlacementClearsWhenCreatedByReconcile(t *test existingArgoCD) assertNoError(t, err) assert.Assert(t, existingArgoCD.Spec.NodePlacement == nil) - _, hasManagedAnnotation := existingArgoCD.Annotations[common.NodePlacementManagedByGitopsServiceAnnotation] - assert.Assert(t, !hasManagedAnnotation) + + ns = &corev1.Namespace{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, ns) + assertNoError(t, err) + _, hasAnnotation := ns.Annotations[common.InfraNodeSelectorAnnotation] + assert.Assert(t, !hasAnnotation) } // If the DISABLE_DEFAULT_ARGOCD_INSTANCE is set, ensure that the default ArgoCD instance is not created. diff --git a/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go b/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go index 7e7556aa921..c2655091863 100644 --- a/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go +++ b/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go @@ -68,19 +68,27 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() { gs.Spec.NodeSelector["key1"] = "value1" }) - By("ensuring Deployments and StatefulSets pick up the change we made to nodeSelector in GitOpsService CR") + By("ensuring the openshift-gitops namespace annotation picks up GitOpsService nodeSelector") + openshiftGitopsNS := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops"}} + Eventually(openshiftGitopsNS).Should(namespaceFixture.HaveAnnotation(common.InfraNodeSelectorAnnotation, "key1=value1")) - for _, deploymentName := range deploymentNameList { + By("ensuring the GitOps Service backend Deployment still has nodeSelector on the pod template") + clusterDepl := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "openshift-gitops"}} + Eventually(clusterDepl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ + "kubernetes.io/os": "linux", + "key1": "value1", + })) + + By("ensuring Argo CD workload templates keep the default nodeSelector (namespace annotation schedules them)") + argoCDDeploymentNames := []string{"openshift-gitops-server", "openshift-gitops-repo-server", "openshift-gitops-dex-server", "openshift-gitops-redis"} + for _, deploymentName := range argoCDDeploymentNames { depl := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: deploymentName, Namespace: "openshift-gitops"}} Eventually(depl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) } - Eventually(appControllerSS).Should(statefulsetFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) By("enabling runOnInfra and setting various tolerations on GitOpsService") @@ -93,17 +101,26 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() { Value: "reserved"}} }) - By("ensuring the openshift-gitops namespace has the infra node-selector annotation") - openshiftGitopsNS := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops"}} - Eventually(openshiftGitopsNS).Should(namespaceFixture.HaveAnnotation(common.InfraNodeSelectorAnnotation, common.InfraNodeSelectorAnnotationValue)) + By("ensuring the openshift-gitops namespace annotation merges infra and custom selectors") + Eventually(openshiftGitopsNS).Should(namespaceFixture.HaveAnnotation(common.InfraNodeSelectorAnnotation, + common.NamespaceNodeSelectorAnnotation(true, map[string]string{"key1": "value1"}))) - By("ensuring Deployments and StatefulSets still have the custom nodeSelector and tolerations") + By("ensuring the GitOps Service backend Deployment still has the custom nodeSelector and tolerations") + Eventually(clusterDepl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ + "kubernetes.io/os": "linux", + "key1": "value1", + })) + Eventually(clusterDepl).Should(deploymentFixture.HaveTolerations([]corev1.Toleration{{ + Effect: "NoSchedule", + Key: "infra", + Value: "reserved", + }})) - for _, deploymentName := range deploymentNameList { + By("ensuring Argo CD workloads get GitopsService tolerations, with node selection via the namespace annotation") + for _, deploymentName := range argoCDDeploymentNames { depl := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: deploymentName, Namespace: "openshift-gitops"}} Eventually(depl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) Eventually(depl).Should(deploymentFixture.HaveTolerations([]corev1.Toleration{{ Effect: "NoSchedule", @@ -111,10 +128,8 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() { Value: "reserved", }})) } - Eventually(appControllerSS).Should(statefulsetFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) Eventually(appControllerSS).Should(statefulsetFixture.HaveTolerations([]corev1.Toleration{{ Effect: "NoSchedule",