From d2f54b785aef6adf910bf514e97bd012199e1145 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Fri, 31 Oct 2025 10:28:07 -0400 Subject: [PATCH 1/5] fix: GitOps Operator hot loop issue Signed-off-by: Atif Ali --- controllers/consoleplugin.go | 57 +++++++++++++------------ controllers/gitopsservice_controller.go | 21 +++++++-- 2 files changed, 48 insertions(+), 30 deletions(-) diff --git a/controllers/consoleplugin.go b/controllers/consoleplugin.go index 85de4a528..9b2cc4fb7 100644 --- a/controllers/consoleplugin.go +++ b/controllers/consoleplugin.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os" - "reflect" argocommon "github.com/argoproj-labs/argocd-operator/common" argocdutil "github.com/argoproj-labs/argocd-operator/controllers/argoutil" @@ -16,6 +15,7 @@ import ( corev1 "k8s.io/api/core/v1" "github.com/redhat-developer/gitops-operator/common" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" resourcev1 "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -55,10 +55,12 @@ func getPluginPodSpec(crImagePullPolicy corev1.PullPolicy) corev1.PodSpec { podSpec := corev1.PodSpec{ Containers: []corev1.Container{ { - Env: util.ProxyEnvVars(), - Name: gitopsPluginName, - Image: consolePluginImage, - ImagePullPolicy: argocdutil.GetImagePullPolicy(crImagePullPolicy), + Env: util.ProxyEnvVars(), + Name: gitopsPluginName, + Image: consolePluginImage, + ImagePullPolicy: argocdutil.GetImagePullPolicy(crImagePullPolicy), + TerminationMessagePath: corev1.TerminationMessagePathDefault, + TerminationMessagePolicy: corev1.TerminationMessageReadFile, Ports: []corev1.ContainerPort{ { Name: "http", @@ -309,17 +311,18 @@ func (r *ReconcileGitopsService) reconcileDeployment(cr *pipelinesv1alpha1.Gitop } else { existingSpecTemplate := &existingPluginDeployment.Spec.Template newSpecTemplate := newPluginDeployment.Spec.Template - changed := !reflect.DeepEqual(existingPluginDeployment.Labels, newPluginDeployment.Labels) || - !reflect.DeepEqual(existingPluginDeployment.Spec.Replicas, newPluginDeployment.Spec.Replicas) || - !reflect.DeepEqual(existingPluginDeployment.Spec.Selector, newPluginDeployment.Spec.Selector) || - !reflect.DeepEqual(existingSpecTemplate.Labels, newSpecTemplate.Labels) || - !reflect.DeepEqual(existingSpecTemplate.Spec.Containers, newSpecTemplate.Spec.Containers) || - !reflect.DeepEqual(existingSpecTemplate.Spec.Volumes, newSpecTemplate.Spec.Volumes) || - !reflect.DeepEqual(existingSpecTemplate.Spec.RestartPolicy, newSpecTemplate.Spec.RestartPolicy) || - !reflect.DeepEqual(existingSpecTemplate.Spec.DNSPolicy, newSpecTemplate.Spec.DNSPolicy) || - !reflect.DeepEqual(existingPluginDeployment.Spec.Template.Spec.NodeSelector, newPluginDeployment.Spec.Template.Spec.NodeSelector) || - !reflect.DeepEqual(existingPluginDeployment.Spec.Template.Spec.Tolerations, newPluginDeployment.Spec.Template.Spec.Tolerations) || - !reflect.DeepEqual(existingSpecTemplate.Spec.SecurityContext, existingSpecTemplate.Spec.SecurityContext) || !reflect.DeepEqual(existingSpecTemplate.Spec.Containers[0].Resources, newSpecTemplate.Spec.Containers[0].Resources) + // Use semantic equality instead of reflect.DeepEqual to handle Kubernetes defaults properly + changed := !equality.Semantic.DeepEqual(existingPluginDeployment.Labels, newPluginDeployment.Labels) || + !equality.Semantic.DeepEqual(existingPluginDeployment.Spec.Replicas, newPluginDeployment.Spec.Replicas) || + !equality.Semantic.DeepEqual(existingPluginDeployment.Spec.Selector, newPluginDeployment.Spec.Selector) || + !equality.Semantic.DeepEqual(existingSpecTemplate.Labels, newSpecTemplate.Labels) || + !equality.Semantic.DeepEqual(existingSpecTemplate.Spec.Containers, newSpecTemplate.Spec.Containers) || + !equality.Semantic.DeepEqual(existingSpecTemplate.Spec.Volumes, newSpecTemplate.Spec.Volumes) || + !equality.Semantic.DeepEqual(existingSpecTemplate.Spec.RestartPolicy, newSpecTemplate.Spec.RestartPolicy) || + !equality.Semantic.DeepEqual(existingSpecTemplate.Spec.DNSPolicy, newSpecTemplate.Spec.DNSPolicy) || + !equality.Semantic.DeepEqual(existingPluginDeployment.Spec.Template.Spec.NodeSelector, newPluginDeployment.Spec.Template.Spec.NodeSelector) || + !equality.Semantic.DeepEqual(existingPluginDeployment.Spec.Template.Spec.Tolerations, newPluginDeployment.Spec.Template.Spec.Tolerations) || + !equality.Semantic.DeepEqual(existingSpecTemplate.Spec.SecurityContext, newSpecTemplate.Spec.SecurityContext) || !equality.Semantic.DeepEqual(existingSpecTemplate.Spec.Containers[0].Resources, newSpecTemplate.Spec.Containers[0].Resources) if changed { reqLogger.Info("Reconciling plugin deployment", "Namespace", existingPluginDeployment.Namespace, "Name", existingPluginDeployment.Name) @@ -364,10 +367,10 @@ func (r *ReconcileGitopsService) reconcileService(instance *pipelinesv1alpha1.Gi return reconcile.Result{}, err } } else { - changed := !reflect.DeepEqual(existingServiceRef.Annotations, pluginServiceRef.Annotations) || - !reflect.DeepEqual(existingServiceRef.Labels, pluginServiceRef.Labels) || - !reflect.DeepEqual(existingServiceRef.Spec.Selector, pluginServiceRef.Spec.Selector) || - !reflect.DeepEqual(existingServiceRef.Spec.Ports, pluginServiceRef.Spec.Ports) + changed := !equality.Semantic.DeepEqual(existingServiceRef.Annotations, pluginServiceRef.Annotations) || + !equality.Semantic.DeepEqual(existingServiceRef.Labels, pluginServiceRef.Labels) || + !equality.Semantic.DeepEqual(existingServiceRef.Spec.Selector, pluginServiceRef.Spec.Selector) || + !equality.Semantic.DeepEqual(existingServiceRef.Spec.Ports, pluginServiceRef.Spec.Ports) if changed { reqLogger.Info("Reconciling plugin service", "Namespace", existingServiceRef.Namespace, "Name", existingServiceRef.Name) @@ -375,7 +378,7 @@ func (r *ReconcileGitopsService) reconcileService(instance *pipelinesv1alpha1.Gi existingServiceRef.Labels = pluginServiceRef.Labels existingServiceRef.Spec.Selector = pluginServiceRef.Spec.Selector existingServiceRef.Spec.Ports = pluginServiceRef.Spec.Ports - return reconcile.Result{}, r.Client.Update(context.TODO(), pluginServiceRef) + return reconcile.Result{}, r.Client.Update(context.TODO(), existingServiceRef) } } return reconcile.Result{}, nil @@ -405,14 +408,14 @@ func (r *ReconcileGitopsService) reconcileConsolePlugin(instance *pipelinesv1alp return reconcile.Result{}, err } } else { - changed := !reflect.DeepEqual(existingPlugin.Spec.DisplayName, newConsolePlugin.Spec.DisplayName) || - !reflect.DeepEqual(existingPlugin.Spec.Backend.Service, newConsolePlugin.Spec.Backend.Service) + changed := !equality.Semantic.DeepEqual(existingPlugin.Spec.DisplayName, newConsolePlugin.Spec.DisplayName) || + !equality.Semantic.DeepEqual(existingPlugin.Spec.Backend.Service, newConsolePlugin.Spec.Backend.Service) if changed { reqLogger.Info("Reconciling Console Plugin", "Namespace", existingPlugin.Namespace, "Name", existingPlugin.Name) existingPlugin.Spec.DisplayName = newConsolePlugin.Spec.DisplayName existingPlugin.Spec.Backend.Service = newConsolePlugin.Spec.Backend.Service - return reconcile.Result{}, r.Client.Update(context.TODO(), newConsolePlugin) + return reconcile.Result{}, r.Client.Update(context.TODO(), existingPlugin) } } return reconcile.Result{}, nil @@ -440,13 +443,13 @@ func (r *ReconcileGitopsService) reconcileConfigMap(instance *pipelinesv1alpha1. return reconcile.Result{}, err } } else { - changed := !reflect.DeepEqual(existingPluginConfigMap.Data, newPluginConfigMap.Data) || - !reflect.DeepEqual(existingPluginConfigMap.Labels, newPluginConfigMap.Labels) + changed := !equality.Semantic.DeepEqual(existingPluginConfigMap.Data, newPluginConfigMap.Data) || + !equality.Semantic.DeepEqual(existingPluginConfigMap.Labels, newPluginConfigMap.Labels) if changed { reqLogger.Info("Reconciling plugin configMap", "Namespace", existingPluginConfigMap.Namespace, "Name", existingPluginConfigMap.Name) existingPluginConfigMap.Data = newPluginConfigMap.Data existingPluginConfigMap.Labels = newPluginConfigMap.Labels - return reconcile.Result{}, r.Client.Update(context.TODO(), newPluginConfigMap) + return reconcile.Result{}, r.Client.Update(context.TODO(), existingPluginConfigMap) } } return reconcile.Result{}, nil diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index 5bb7ffc6c..291e31f82 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -114,9 +114,24 @@ func (r *ReconcileGitopsService) SetupWithManager(mgr ctrl.Manager) error { })), ).Watches(&argoapp.ArgoCD{}, &handler.EnqueueRequestForObject{}, - builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool { - return obj.GetName() == "openshift-gitops" && obj.GetNamespace() == "openshift-gitops" - }))). + builder.WithPredicates(predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + // Only watch openshift-gitops ArgoCD + return e.Object.GetName() == "openshift-gitops" && e.Object.GetNamespace() == "openshift-gitops" + }, + UpdateFunc: func(e event.UpdateEvent) bool { + // Only watch openshift-gitops ArgoCD + if e.ObjectNew.GetName() != "openshift-gitops" || e.ObjectNew.GetNamespace() != "openshift-gitops" { + return false + } + // Ignore updates to CR status in which case metadata.Generation does not change + return e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() + }, + DeleteFunc: func(e event.DeleteEvent) bool { + // Only watch openshift-gitops ArgoCD + return e.Object.GetName() == "openshift-gitops" && e.Object.GetNamespace() == "openshift-gitops" + }, + })). Complete(r) } From aa44385bf44b113b01a4998ff4fc95bbe9dca4ca Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Fri, 31 Oct 2025 13:22:29 -0400 Subject: [PATCH 2/5] modify deep equal Signed-off-by: Atif Ali --- controllers/gitopsservice_controller.go | 30 ++++++++++++------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index 291e31f82..997cb00ff 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -21,7 +21,6 @@ import ( "fmt" "log" "os" - "reflect" "strings" argoapp "github.com/argoproj-labs/argocd-operator/api/v1beta1" @@ -39,6 +38,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" resourcev1 "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -103,7 +103,8 @@ func (r *ReconcileGitopsService) SetupWithManager(mgr ctrl.Manager) error { Owns(&rbacv1.ClusterRole{}). Owns(&corev1.ServiceAccount{}). Owns(&corev1.ConfigMap{}). - Owns(&appsv1.Deployment{}, builder.WithPredicates(pred)). + // Removed Owns(&appsv1.Deployment{}) watch to prevent hot loop from deployment updates + // Deployments are still reconciled when GitopsService or ArgoCD changes Owns(&corev1.Service{}, builder.WithPredicates(pred)). Owns(&routev1.Route{}, builder.WithPredicates(pred)). Watches( @@ -517,7 +518,7 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli // 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) { + if !equality.Semantic.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true } @@ -587,7 +588,7 @@ func (r *ReconcileGitopsService) reconcileBackend(gitopsserviceNamespacedName ty } else { return reconcile.Result{}, err } - } else if !reflect.DeepEqual(existingClusterRole.Rules, clusterRoleObj.Rules) { + } else if !equality.Semantic.DeepEqual(existingClusterRole.Rules, clusterRoleObj.Rules) { reqLogger.Info("Reconciling existing Cluster Role", "Name", clusterRoleObj.Name) existingClusterRole.Rules = clusterRoleObj.Rules err = r.Client.Update(context.TODO(), existingClusterRole) @@ -666,39 +667,36 @@ func (r *ReconcileGitopsService) reconcileBackend(gitopsserviceNamespacedName ty found.Spec.Template.Spec.Containers[0].Image = desiredImage changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.Containers[0].Env, deploymentObj.Spec.Template.Spec.Containers[0].Env) { + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Env, deploymentObj.Spec.Template.Spec.Containers[0].Env) { found.Spec.Template.Spec.Containers[0].Env = deploymentObj.Spec.Template.Spec.Containers[0].Env changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.Containers[0].ImagePullPolicy, deploymentObj.Spec.Template.Spec.Containers[0].ImagePullPolicy) { + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].ImagePullPolicy, deploymentObj.Spec.Template.Spec.Containers[0].ImagePullPolicy) { found.Spec.Template.Spec.Containers[0].ImagePullPolicy = deploymentObj.Spec.Template.Spec.Containers[0].ImagePullPolicy changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.Containers[0].Resources, deploymentObj.Spec.Template.Spec.Containers[0].Resources) { - found.Spec.Template.Spec.Containers[0].Resources = deploymentObj.Spec.Template.Spec.Containers[0].Resources - changed = true - } - if !reflect.DeepEqual(found.Spec.Template.Spec.Containers[0].Args, deploymentObj.Spec.Template.Spec.Containers[0].Args) { + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Args, deploymentObj.Spec.Template.Spec.Containers[0].Args) { found.Spec.Template.Spec.Containers[0].Args = deploymentObj.Spec.Template.Spec.Containers[0].Args changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.Containers[0].Resources, deploymentObj.Spec.Template.Spec.Containers[0].Resources) { + // Use semantic equality to handle Kubernetes defaults properly + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Resources, deploymentObj.Spec.Template.Spec.Containers[0].Resources) { found.Spec.Template.Spec.Containers[0].Resources = deploymentObj.Spec.Template.Spec.Containers[0].Resources changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.Containers[0].SecurityContext, deploymentObj.Spec.Template.Spec.Containers[0].SecurityContext) { + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].SecurityContext, deploymentObj.Spec.Template.Spec.Containers[0].SecurityContext) { found.Spec.Template.Spec.Containers[0].SecurityContext = deploymentObj.Spec.Template.Spec.Containers[0].SecurityContext changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.NodeSelector, deploymentObj.Spec.Template.Spec.NodeSelector) { + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.NodeSelector, deploymentObj.Spec.Template.Spec.NodeSelector) { found.Spec.Template.Spec.NodeSelector = deploymentObj.Spec.Template.Spec.NodeSelector changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.Tolerations, deploymentObj.Spec.Template.Spec.Tolerations) { + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Tolerations, deploymentObj.Spec.Template.Spec.Tolerations) { found.Spec.Template.Spec.Tolerations = deploymentObj.Spec.Template.Spec.Tolerations changed = true } - if !reflect.DeepEqual(found.Spec.Template.Spec.SecurityContext, deploymentObj.Spec.Template.Spec.SecurityContext) { + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.SecurityContext, deploymentObj.Spec.Template.Spec.SecurityContext) { found.Spec.Template.Spec.SecurityContext = deploymentObj.Spec.Template.Spec.SecurityContext changed = true } From 48f1a14f4ff55fc984db893a8de08e59121850ff Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Mon, 3 Nov 2025 13:49:38 -0500 Subject: [PATCH 3/5] remove leftover fields && duplicate comparison Signed-off-by: Atif Ali --- controllers/gitopsservice_controller.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index 997cb00ff..ecd2e0619 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -671,10 +671,6 @@ func (r *ReconcileGitopsService) reconcileBackend(gitopsserviceNamespacedName ty found.Spec.Template.Spec.Containers[0].Env = deploymentObj.Spec.Template.Spec.Containers[0].Env changed = true } - if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].ImagePullPolicy, deploymentObj.Spec.Template.Spec.Containers[0].ImagePullPolicy) { - found.Spec.Template.Spec.Containers[0].ImagePullPolicy = deploymentObj.Spec.Template.Spec.Containers[0].ImagePullPolicy - changed = true - } if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Args, deploymentObj.Spec.Template.Spec.Containers[0].Args) { found.Spec.Template.Spec.Containers[0].Args = deploymentObj.Spec.Template.Spec.Containers[0].Args changed = true From 134a3b57df5c3037c53c37b4fbb73d4ad2dd4e9a Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 10 Dec 2025 11:10:21 -0500 Subject: [PATCH 4/5] update only if changed is true Signed-off-by: Atif Ali --- controllers/gitopsservice_controller.go | 118 +++++++++++++++++++++++- 1 file changed, 113 insertions(+), 5 deletions(-) diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index ecd2e0619..807c89986 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -45,6 +45,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -483,10 +484,10 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli if existingArgoCD.Spec.SSO.Dex != nil { if existingArgoCD.Spec.SSO.Dex.Resources == nil { existingArgoCD.Spec.SSO.Dex.Resources = defaultArgoCDInstance.Spec.SSO.Dex.Resources + changed = true } } } - changed = true } //lint:ignore SA1019 known to be deprecated @@ -530,7 +531,67 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli if changed { reqLogger.Info("Reconciling ArgoCD", "Namespace", existingArgoCD.Namespace, "Name", existingArgoCD.Name) - err = r.Client.Update(context.TODO(), existingArgoCD) + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: existingArgoCD.Name, Namespace: existingArgoCD.Namespace}, existingArgoCD); err != nil { + return err + } + changed := false + if existingArgoCD.Spec.ApplicationSet != nil { + if existingArgoCD.Spec.ApplicationSet.Resources == nil { + existingArgoCD.Spec.ApplicationSet.Resources = defaultArgoCDInstance.Spec.ApplicationSet.Resources + changed = true + } + } + if existingArgoCD.Spec.Controller.Resources == nil { + existingArgoCD.Spec.Controller.Resources = defaultArgoCDInstance.Spec.Controller.Resources + changed = true + } + if argocdcontroller.UseDex(existingArgoCD) { + if existingArgoCD.Spec.SSO != nil && existingArgoCD.Spec.SSO.Provider == argoapp.SSOProviderTypeDex { + if existingArgoCD.Spec.SSO.Dex != nil { + if existingArgoCD.Spec.SSO.Dex.Resources == nil { + existingArgoCD.Spec.SSO.Dex.Resources = defaultArgoCDInstance.Spec.SSO.Dex.Resources + changed = true + } + } + } + } + //lint:ignore SA1019 known to be deprecated + if existingArgoCD.Spec.Grafana.Resources == nil { //nolint:staticcheck // SA1019: We must test deprecated fields. + //lint:ignore SA1019 known to be deprecated + existingArgoCD.Spec.Grafana.Resources = defaultArgoCDInstance.Spec.Grafana.Resources //nolint:staticcheck // SA1019: We must test deprecated fields. + changed = true + } + if existingArgoCD.Spec.HA.Resources == nil { + existingArgoCD.Spec.HA.Resources = defaultArgoCDInstance.Spec.HA.Resources + changed = true + } + if existingArgoCD.Spec.Redis.Resources == nil { + existingArgoCD.Spec.Redis.Resources = defaultArgoCDInstance.Spec.Redis.Resources + changed = true + } + if existingArgoCD.Spec.Repo.Resources == nil { + existingArgoCD.Spec.Repo.Resources = defaultArgoCDInstance.Spec.Repo.Resources + changed = true + } + if existingArgoCD.Spec.Server.Resources == nil { + existingArgoCD.Spec.Server.Resources = defaultArgoCDInstance.Spec.Server.Resources + changed = true + } + if defaultArgoCDInstance.Spec.NodePlacement != nil { + if !equality.Semantic.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 + } + if !changed { + return nil + } + return r.Client.Update(context.TODO(), existingArgoCD) + }) if err != nil { return reconcile.Result{}, err } @@ -590,8 +651,13 @@ func (r *ReconcileGitopsService) reconcileBackend(gitopsserviceNamespacedName ty } } else if !equality.Semantic.DeepEqual(existingClusterRole.Rules, clusterRoleObj.Rules) { reqLogger.Info("Reconciling existing Cluster Role", "Name", clusterRoleObj.Name) - existingClusterRole.Rules = clusterRoleObj.Rules - err = r.Client.Update(context.TODO(), existingClusterRole) + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: existingClusterRole.Name}, existingClusterRole); err != nil { + return err + } + existingClusterRole.Rules = clusterRoleObj.Rules + return r.Client.Update(context.TODO(), existingClusterRole) + }) if err != nil { return reconcile.Result{}, err } @@ -699,7 +765,49 @@ func (r *ReconcileGitopsService) reconcileBackend(gitopsserviceNamespacedName ty if changed { reqLogger.Info("Reconciling existing backend Deployment", "Namespace", deploymentObj.Namespace, "Name", deploymentObj.Name) - err = r.Client.Update(context.TODO(), found) + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: found.Name, Namespace: found.Namespace}, found); err != nil { + return err + } + changed := false + desiredImage := deploymentObj.Spec.Template.Spec.Containers[0].Image + if found.Spec.Template.Spec.Containers[0].Image != desiredImage { + found.Spec.Template.Spec.Containers[0].Image = desiredImage + changed = true + } + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Env, deploymentObj.Spec.Template.Spec.Containers[0].Env) { + found.Spec.Template.Spec.Containers[0].Env = deploymentObj.Spec.Template.Spec.Containers[0].Env + changed = true + } + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Args, deploymentObj.Spec.Template.Spec.Containers[0].Args) { + found.Spec.Template.Spec.Containers[0].Args = deploymentObj.Spec.Template.Spec.Containers[0].Args + changed = true + } + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Resources, deploymentObj.Spec.Template.Spec.Containers[0].Resources) { + found.Spec.Template.Spec.Containers[0].Resources = deploymentObj.Spec.Template.Spec.Containers[0].Resources + changed = true + } + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].SecurityContext, deploymentObj.Spec.Template.Spec.Containers[0].SecurityContext) { + found.Spec.Template.Spec.Containers[0].SecurityContext = deploymentObj.Spec.Template.Spec.Containers[0].SecurityContext + changed = true + } + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.NodeSelector, deploymentObj.Spec.Template.Spec.NodeSelector) { + found.Spec.Template.Spec.NodeSelector = deploymentObj.Spec.Template.Spec.NodeSelector + changed = true + } + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Tolerations, deploymentObj.Spec.Template.Spec.Tolerations) { + found.Spec.Template.Spec.Tolerations = deploymentObj.Spec.Template.Spec.Tolerations + changed = true + } + if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.SecurityContext, deploymentObj.Spec.Template.Spec.SecurityContext) { + found.Spec.Template.Spec.SecurityContext = deploymentObj.Spec.Template.Spec.SecurityContext + changed = true + } + if !changed { + return nil + } + return r.Client.Update(context.TODO(), found) + }) if err != nil { return reconcile.Result{}, err } From f5883ebc9d22f1e434973e94f6fcae0046d4a53b Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 17 Dec 2025 20:29:08 -0500 Subject: [PATCH 5/5] apply Siddhesh's review comments Signed-off-by: Atif Ali --- controllers/consoleplugin.go | 10 +- controllers/gitopsservice_controller.go | 174 ++++++++---------------- 2 files changed, 63 insertions(+), 121 deletions(-) diff --git a/controllers/consoleplugin.go b/controllers/consoleplugin.go index 9b2cc4fb7..58aa3f37a 100644 --- a/controllers/consoleplugin.go +++ b/controllers/consoleplugin.go @@ -55,12 +55,10 @@ func getPluginPodSpec(crImagePullPolicy corev1.PullPolicy) corev1.PodSpec { podSpec := corev1.PodSpec{ Containers: []corev1.Container{ { - Env: util.ProxyEnvVars(), - Name: gitopsPluginName, - Image: consolePluginImage, - ImagePullPolicy: argocdutil.GetImagePullPolicy(crImagePullPolicy), - TerminationMessagePath: corev1.TerminationMessagePathDefault, - TerminationMessagePolicy: corev1.TerminationMessageReadFile, + Env: util.ProxyEnvVars(), + Name: gitopsPluginName, + Image: consolePluginImage, + ImagePullPolicy: argocdutil.GetImagePullPolicy(crImagePullPolicy), Ports: []corev1.ContainerPort{ { Name: "http", diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index 807c89986..71cae8068 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -118,11 +118,9 @@ func (r *ReconcileGitopsService) SetupWithManager(mgr ctrl.Manager) error { &handler.EnqueueRequestForObject{}, builder.WithPredicates(predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { - // Only watch openshift-gitops ArgoCD return e.Object.GetName() == "openshift-gitops" && e.Object.GetNamespace() == "openshift-gitops" }, UpdateFunc: func(e event.UpdateEvent) bool { - // Only watch openshift-gitops ArgoCD if e.ObjectNew.GetName() != "openshift-gitops" || e.ObjectNew.GetNamespace() != "openshift-gitops" { return false } @@ -130,7 +128,6 @@ func (r *ReconcileGitopsService) SetupWithManager(mgr ctrl.Manager) error { return e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() }, DeleteFunc: func(e event.DeleteEvent) bool { - // Only watch openshift-gitops ArgoCD return e.Object.GetName() == "openshift-gitops" && e.Object.GetNamespace() == "openshift-gitops" }, })). @@ -466,135 +463,83 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli return reconcile.Result{}, err } } else { - changed := false - if existingArgoCD.Spec.ApplicationSet != nil { - if existingArgoCD.Spec.ApplicationSet.Resources == nil { - existingArgoCD.Spec.ApplicationSet.Resources = defaultArgoCDInstance.Spec.ApplicationSet.Resources - changed = true + updateArgoCDResources := func(existing *argoapp.ArgoCD, desired *argoapp.ArgoCD) bool { + changed := false + if existing.Spec.ApplicationSet != nil { + if existing.Spec.ApplicationSet.Resources == nil { + existing.Spec.ApplicationSet.Resources = desired.Spec.ApplicationSet.Resources + changed = true + } } - } - if existingArgoCD.Spec.Controller.Resources == nil { - existingArgoCD.Spec.Controller.Resources = defaultArgoCDInstance.Spec.Controller.Resources - changed = true - } + if existing.Spec.Controller.Resources == nil { + existing.Spec.Controller.Resources = desired.Spec.Controller.Resources + changed = true + } - if argocdcontroller.UseDex(existingArgoCD) { - if existingArgoCD.Spec.SSO != nil && existingArgoCD.Spec.SSO.Provider == argoapp.SSOProviderTypeDex { - if existingArgoCD.Spec.SSO.Dex != nil { - if existingArgoCD.Spec.SSO.Dex.Resources == nil { - existingArgoCD.Spec.SSO.Dex.Resources = defaultArgoCDInstance.Spec.SSO.Dex.Resources - changed = true + if argocdcontroller.UseDex(existing) { + if existing.Spec.SSO != nil && existing.Spec.SSO.Provider == argoapp.SSOProviderTypeDex { + if existing.Spec.SSO.Dex != nil { + if existing.Spec.SSO.Dex.Resources == nil { + existing.Spec.SSO.Dex.Resources = desired.Spec.SSO.Dex.Resources + changed = true + } } } } - } - //lint:ignore SA1019 known to be deprecated - if existingArgoCD.Spec.Grafana.Resources == nil { //nolint:staticcheck // SA1019: We must test deprecated fields. //lint:ignore SA1019 known to be deprecated - existingArgoCD.Spec.Grafana.Resources = defaultArgoCDInstance.Spec.Grafana.Resources //nolint:staticcheck // SA1019: We must test deprecated fields. - changed = true - } - - if existingArgoCD.Spec.HA.Resources == nil { - existingArgoCD.Spec.HA.Resources = defaultArgoCDInstance.Spec.HA.Resources - changed = true - } + if existing.Spec.Grafana.Resources == nil { //nolint:staticcheck // SA1019: We must test deprecated fields. + //lint:ignore SA1019 known to be deprecated + existing.Spec.Grafana.Resources = desired.Spec.Grafana.Resources //nolint:staticcheck // SA1019: We must test deprecated fields. + changed = true + } - if existingArgoCD.Spec.Redis.Resources == nil { - existingArgoCD.Spec.Redis.Resources = defaultArgoCDInstance.Spec.Redis.Resources - changed = true - } + if existing.Spec.HA.Resources == nil { + existing.Spec.HA.Resources = desired.Spec.HA.Resources + changed = true + } - if existingArgoCD.Spec.Repo.Resources == nil { - existingArgoCD.Spec.Repo.Resources = defaultArgoCDInstance.Spec.Repo.Resources - changed = true - } + if existing.Spec.Redis.Resources == nil { + existing.Spec.Redis.Resources = desired.Spec.Redis.Resources + changed = true + } - if existingArgoCD.Spec.Server.Resources == nil { - existingArgoCD.Spec.Server.Resources = defaultArgoCDInstance.Spec.Server.Resources - changed = true - } + if existing.Spec.Repo.Resources == nil { + existing.Spec.Repo.Resources = desired.Spec.Repo.Resources + changed = true + } - // if user is patching nodePlacement through GitopsService CR, then existingArgoCD NodePlacement is updated. - if defaultArgoCDInstance.Spec.NodePlacement != nil { - if !equality.Semantic.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { - existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement + if existing.Spec.Server.Resources == nil { + existing.Spec.Server.Resources = desired.Spec.Server.Resources changed = true } - // Handle the case where NodePlacement should be removed - } else if existingArgoCD.Spec.NodePlacement != nil { - existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement - changed = true - } - if changed { - reqLogger.Info("Reconciling ArgoCD", "Namespace", existingArgoCD.Namespace, "Name", existingArgoCD.Name) - err = retry.RetryOnConflict(retry.DefaultRetry, func() error { - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: existingArgoCD.Name, Namespace: existingArgoCD.Namespace}, existingArgoCD); err != nil { - return err - } - changed := false - if existingArgoCD.Spec.ApplicationSet != nil { - if existingArgoCD.Spec.ApplicationSet.Resources == nil { - existingArgoCD.Spec.ApplicationSet.Resources = defaultArgoCDInstance.Spec.ApplicationSet.Resources - changed = true - } - } - if existingArgoCD.Spec.Controller.Resources == nil { - existingArgoCD.Spec.Controller.Resources = defaultArgoCDInstance.Spec.Controller.Resources - changed = true - } - if argocdcontroller.UseDex(existingArgoCD) { - if existingArgoCD.Spec.SSO != nil && existingArgoCD.Spec.SSO.Provider == argoapp.SSOProviderTypeDex { - if existingArgoCD.Spec.SSO.Dex != nil { - if existingArgoCD.Spec.SSO.Dex.Resources == nil { - existingArgoCD.Spec.SSO.Dex.Resources = defaultArgoCDInstance.Spec.SSO.Dex.Resources - changed = true - } - } - } - } - //lint:ignore SA1019 known to be deprecated - if existingArgoCD.Spec.Grafana.Resources == nil { //nolint:staticcheck // SA1019: We must test deprecated fields. - //lint:ignore SA1019 known to be deprecated - existingArgoCD.Spec.Grafana.Resources = defaultArgoCDInstance.Spec.Grafana.Resources //nolint:staticcheck // SA1019: We must test deprecated fields. + if desired.Spec.NodePlacement != nil { + if !equality.Semantic.DeepEqual(existing.Spec.NodePlacement, desired.Spec.NodePlacement) { + existing.Spec.NodePlacement = desired.Spec.NodePlacement changed = true } - if existingArgoCD.Spec.HA.Resources == nil { - existingArgoCD.Spec.HA.Resources = defaultArgoCDInstance.Spec.HA.Resources - changed = true - } - if existingArgoCD.Spec.Redis.Resources == nil { - existingArgoCD.Spec.Redis.Resources = defaultArgoCDInstance.Spec.Redis.Resources - changed = true - } - if existingArgoCD.Spec.Repo.Resources == nil { - existingArgoCD.Spec.Repo.Resources = defaultArgoCDInstance.Spec.Repo.Resources - changed = true - } - if existingArgoCD.Spec.Server.Resources == nil { - existingArgoCD.Spec.Server.Resources = defaultArgoCDInstance.Spec.Server.Resources - changed = true - } - if defaultArgoCDInstance.Spec.NodePlacement != nil { - if !equality.Semantic.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 - } - if !changed { - return nil - } - return r.Client.Update(context.TODO(), existingArgoCD) - }) - if err != nil { - return reconcile.Result{}, err + } else if existing.Spec.NodePlacement != nil { + existing.Spec.NodePlacement = desired.Spec.NodePlacement + changed = true + } + + return changed + } + + reqLogger.Info("Reconciling ArgoCD", "Namespace", existingArgoCD.Namespace, "Name", existingArgoCD.Name) + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: existingArgoCD.Name, Namespace: existingArgoCD.Namespace}, existingArgoCD); err != nil { + return err + } + if !updateArgoCDResources(existingArgoCD, defaultArgoCDInstance) { + return nil } + return r.Client.Update(context.TODO(), existingArgoCD) + }) + if err != nil { + return reconcile.Result{}, err } } @@ -741,7 +686,6 @@ func (r *ReconcileGitopsService) reconcileBackend(gitopsserviceNamespacedName ty found.Spec.Template.Spec.Containers[0].Args = deploymentObj.Spec.Template.Spec.Containers[0].Args changed = true } - // Use semantic equality to handle Kubernetes defaults properly if !equality.Semantic.DeepEqual(found.Spec.Template.Spec.Containers[0].Resources, deploymentObj.Spec.Template.Spec.Containers[0].Resources) { found.Spec.Template.Spec.Containers[0].Resources = deploymentObj.Spec.Template.Spec.Containers[0].Resources changed = true