From 7f2da76c3bce63b3668bf925451d6ea769f7635c Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Wed, 29 Apr 2026 17:50:51 +0200 Subject: [PATCH] Fix NodeSelector nil-vs-pointer reconcile loop in all service reconcilers When the OSCP has no nodeSelector defined, instance.Spec.NodeSelector is a nil map. Taking its address (&instance.Spec.NodeSelector) creates a non-nil pointer to a nil map, which is structurally different from a nil pointer. controllerutil.CreateOrPatch detects this as a diff via reflect.DeepEqual and sends an update patch on every reconcile, bumping the sub-CR's Generation. The ObservedGeneration readiness check then reports the sub-CR as "in progress", triggering another OSCP reconcile and creating an infinite loop (~1 update/second). Guard all 25 NodeSelector inheritance assignments with len(instance.Spec.NodeSelector) > 0 so the assignment is skipped when the OSCP nodeSelector is nil or empty, avoiding the spurious diff. Two new functional tests verify the fix: - "does not set nodeSelector on sub-CRs": NodeSelector stays nil on Memcached, Galera, and RabbitMQ when the OSCP omits nodeSelector. - "does not cause spurious updates": sub-CR generation remains stable over 5 seconds (Consistently), catching the ~1/second spec mutation. Co-Authored-By: Claude Opus 4.6 --- internal/openstack/barbican.go | 2 +- internal/openstack/cinder.go | 2 +- internal/openstack/designate.go | 2 +- internal/openstack/dnsmasq.go | 2 +- internal/openstack/galera.go | 2 +- internal/openstack/glance.go | 2 +- internal/openstack/heat.go | 2 +- internal/openstack/horizon.go | 2 +- internal/openstack/ironic.go | 2 +- internal/openstack/keystone.go | 2 +- internal/openstack/manila.go | 2 +- internal/openstack/memcached.go | 2 +- internal/openstack/neutron.go | 2 +- internal/openstack/nova.go | 2 +- internal/openstack/octavia.go | 2 +- internal/openstack/openstackclient.go | 2 +- internal/openstack/ovn.go | 6 +- internal/openstack/placement.go | 2 +- internal/openstack/rabbitmq.go | 2 +- internal/openstack/redis.go | 2 +- internal/openstack/swift.go | 2 +- internal/openstack/telemetry.go | 2 +- internal/openstack/watcher.go | 2 +- .../openstackoperator_controller_test.go | 76 +++++++++++++++++++ 24 files changed, 101 insertions(+), 25 deletions(-) diff --git a/internal/openstack/barbican.go b/internal/openstack/barbican.go index 1c566ed738..5009308193 100644 --- a/internal/openstack/barbican.go +++ b/internal/openstack/barbican.go @@ -149,7 +149,7 @@ func ReconcileBarbican(ctx context.Context, instance *corev1beta1.OpenStackContr instance.Spec.Barbican.Template.BarbicanAPI.TLS.API.Internal.SecretName = endpointDetails.GetEndptCertSecret(service.EndpointInternal) } - if instance.Spec.Barbican.Template.NodeSelector == nil { + if instance.Spec.Barbican.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Barbican.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/cinder.go b/internal/openstack/cinder.go index 1d64d425a4..b880c5a83f 100644 --- a/internal/openstack/cinder.go +++ b/internal/openstack/cinder.go @@ -172,7 +172,7 @@ func ReconcileCinder(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Cinder.Template.CinderAPI.TLS.API.Internal.SecretName = endpointDetails.GetEndptCertSecret(service.EndpointInternal) } - if instance.Spec.Cinder.Template.NodeSelector == nil { + if instance.Spec.Cinder.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Cinder.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/designate.go b/internal/openstack/designate.go index 5ef092b0e0..d6e6c1e727 100644 --- a/internal/openstack/designate.go +++ b/internal/openstack/designate.go @@ -156,7 +156,7 @@ func ReconcileDesignate(ctx context.Context, instance *corev1beta1.OpenStackCont instance.Spec.Designate.Template.DesignateAPI.TLS.API.Internal.SecretName = endpointDetails.GetEndptCertSecret(service.EndpointInternal) } - if instance.Spec.Designate.Template.NodeSelector == nil { + if instance.Spec.Designate.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Designate.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/dnsmasq.go b/internal/openstack/dnsmasq.go index 3c1090d365..13858af189 100644 --- a/internal/openstack/dnsmasq.go +++ b/internal/openstack/dnsmasq.go @@ -40,7 +40,7 @@ func ReconcileDNSMasqs(ctx context.Context, instance *corev1beta1.OpenStackContr instance.Spec.DNS.Template = &networkv1.DNSMasqSpecCore{} } - if instance.Spec.DNS.Template.NodeSelector == nil { + if instance.Spec.DNS.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.DNS.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/galera.go b/internal/openstack/galera.go index 1b9175d2fe..dcf58753ef 100644 --- a/internal/openstack/galera.go +++ b/internal/openstack/galera.go @@ -266,7 +266,7 @@ func reconcileGalera( return galeraReady, galera, nil } - if spec.NodeSelector == nil { + if spec.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { spec.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/glance.go b/internal/openstack/glance.go index f94a4e4222..39b6eaddad 100644 --- a/internal/openstack/glance.go +++ b/internal/openstack/glance.go @@ -94,7 +94,7 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Glance.Template.NotificationBusInstance = nil } - if instance.Spec.Glance.Template.NodeSelector == nil { + if instance.Spec.Glance.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Glance.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/heat.go b/internal/openstack/heat.go index a9bad1d9b0..05e3320ad7 100644 --- a/internal/openstack/heat.go +++ b/internal/openstack/heat.go @@ -54,7 +54,7 @@ func ReconcileHeat(ctx context.Context, instance *corev1beta1.OpenStackControlPl // Note: Migration from rabbitMqClusterName to messagingBus.cluster is handled by the webhook // via annotation-based triggers. No direct spec mutation here to avoid GitOps conflicts. - if instance.Spec.Heat.Template.NodeSelector == nil { + if instance.Spec.Heat.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Heat.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/horizon.go b/internal/openstack/horizon.go index a0571dad1b..e9ae336bbd 100644 --- a/internal/openstack/horizon.go +++ b/internal/openstack/horizon.go @@ -50,7 +50,7 @@ func ReconcileHorizon(ctx context.Context, instance *corev1beta1.OpenStackContro instance.Spec.Horizon.Template = &horizonv1.HorizonSpecCore{} } - if instance.Spec.Horizon.Template.NodeSelector == nil { + if instance.Spec.Horizon.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Horizon.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/ironic.go b/internal/openstack/ironic.go index 37b3ff4222..56086a778d 100644 --- a/internal/openstack/ironic.go +++ b/internal/openstack/ironic.go @@ -58,7 +58,7 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl // via annotation-based triggers. No direct spec mutation here to avoid GitOps conflicts. // This applies to both Ironic main template and IronicNeutronAgent. - if instance.Spec.Ironic.Template.NodeSelector == nil { + if instance.Spec.Ironic.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Ironic.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/keystone.go b/internal/openstack/keystone.go index a7a7d8a774..a4ce61cbaa 100644 --- a/internal/openstack/keystone.go +++ b/internal/openstack/keystone.go @@ -106,7 +106,7 @@ func ReconcileKeystoneAPI(ctx context.Context, instance *corev1beta1.OpenStackCo instance.Spec.Keystone.Template.TLS.API.Internal.SecretName = endpointDetails.GetEndptCertSecret(service.EndpointInternal) } - if instance.Spec.Keystone.Template.NodeSelector == nil { + if instance.Spec.Keystone.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Keystone.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/manila.go b/internal/openstack/manila.go index dcba216762..7059f236dc 100644 --- a/internal/openstack/manila.go +++ b/internal/openstack/manila.go @@ -152,7 +152,7 @@ func ReconcileManila(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Manila.Template.ManilaAPI.TLS.API.Internal.SecretName = endpointDetails.GetEndptCertSecret(service.EndpointInternal) } - if instance.Spec.Manila.Template.NodeSelector == nil { + if instance.Spec.Manila.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Manila.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/memcached.go b/internal/openstack/memcached.go index 5223819e9e..b529b7f178 100644 --- a/internal/openstack/memcached.go +++ b/internal/openstack/memcached.go @@ -273,7 +273,7 @@ func reconcileMemcached( } } - if spec.NodeSelector == nil { + if spec.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { spec.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/neutron.go b/internal/openstack/neutron.go index 29418e4271..f59462fb1b 100644 --- a/internal/openstack/neutron.go +++ b/internal/openstack/neutron.go @@ -189,7 +189,7 @@ func ReconcileNeutron(ctx context.Context, instance *corev1beta1.OpenStackContro instance.Spec.Neutron.Template.TLS.API.Internal.SecretName = endpointDetails.GetEndptCertSecret(service.EndpointInternal) } - if instance.Spec.Neutron.Template.NodeSelector == nil { + if instance.Spec.Neutron.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Neutron.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/nova.go b/internal/openstack/nova.go index 7a5bbf2f3e..235e96de63 100644 --- a/internal/openstack/nova.go +++ b/internal/openstack/nova.go @@ -75,7 +75,7 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl // Note: Migration from apiMessageBusInstance and cellMessageBusInstance to messagingBus.cluster // is handled by the webhook via annotation-based triggers. No direct spec mutation here to avoid GitOps conflicts. - if instance.Spec.Nova.Template.NodeSelector == nil { + if instance.Spec.Nova.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Nova.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/octavia.go b/internal/openstack/octavia.go index 9d98b89f88..9c78e60325 100644 --- a/internal/openstack/octavia.go +++ b/internal/openstack/octavia.go @@ -73,7 +73,7 @@ func ReconcileOctavia(ctx context.Context, instance *corev1beta1.OpenStackContro // Note: Migration from rabbitMqClusterName to messagingBus.cluster is handled by the webhook // via annotation-based triggers. No direct spec mutation here to avoid GitOps conflicts. - if instance.Spec.Octavia.Template.NodeSelector == nil { + if instance.Spec.Octavia.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Octavia.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/openstackclient.go b/internal/openstack/openstackclient.go index ab4366197b..d265b882a6 100644 --- a/internal/openstack/openstackclient.go +++ b/internal/openstack/openstackclient.go @@ -47,7 +47,7 @@ func ReconcileOpenStackClient(ctx context.Context, instance *corev1.OpenStackCon return ctrl.Result{}, nil } - if instance.Spec.OpenStackClient.Template.NodeSelector == nil { + if instance.Spec.OpenStackClient.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.OpenStackClient.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/ovn.go b/internal/openstack/ovn.go index e18f8c6f6c..0e857c847f 100644 --- a/internal/openstack/ovn.go +++ b/internal/openstack/ovn.go @@ -208,7 +208,7 @@ func ReconcileOVNDbClusters(ctx context.Context, instance *corev1beta1.OpenStack dbcluster.MetricsTLS.CaBundleSecretName = instance.Status.TLS.CaBundleSecretName } - if dbcluster.NodeSelector == nil { + if dbcluster.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { dbcluster.NodeSelector = &instance.Spec.NodeSelector } @@ -349,7 +349,7 @@ func ReconcileOVNNorthd(ctx context.Context, instance *corev1beta1.OpenStackCont ovnNorthdSpec.MetricsTLS.CaBundleSecretName = instance.Status.TLS.CaBundleSecretName } - if ovnNorthdSpec.NodeSelector == nil { + if ovnNorthdSpec.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { ovnNorthdSpec.NodeSelector = &instance.Spec.NodeSelector } @@ -493,7 +493,7 @@ func ReconcileOVNController(ctx context.Context, instance *corev1beta1.OpenStack ovnControllerSpec.MetricsTLS.CaBundleSecretName = instance.Status.TLS.CaBundleSecretName } - if ovnControllerSpec.NodeSelector == nil { + if ovnControllerSpec.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { ovnControllerSpec.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/placement.go b/internal/openstack/placement.go index 60c17ad57d..03d2393cf9 100644 --- a/internal/openstack/placement.go +++ b/internal/openstack/placement.go @@ -45,7 +45,7 @@ func ReconcilePlacementAPI(ctx context.Context, instance *corev1beta1.OpenStackC instance.Spec.Placement.Template = &placementv1.PlacementAPISpecCore{} } - if instance.Spec.Placement.Template.NodeSelector == nil { + if instance.Spec.Placement.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Placement.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/rabbitmq.go b/internal/openstack/rabbitmq.go index c19b5cb6dd..40c7a958dc 100644 --- a/internal/openstack/rabbitmq.go +++ b/internal/openstack/rabbitmq.go @@ -245,7 +245,7 @@ func reconcileRabbitMQ( tlsCert = certSecret.Name } - if spec.NodeSelector == nil { + if spec.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { spec.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/redis.go b/internal/openstack/redis.go index a291f75a85..7f0fcafe2e 100644 --- a/internal/openstack/redis.go +++ b/internal/openstack/redis.go @@ -255,7 +255,7 @@ func reconcileRedis( tlsCert = certSecret.Name } - if spec.NodeSelector == nil { + if spec.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { spec.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/swift.go b/internal/openstack/swift.go index e7dc468a7f..67f6c54d7a 100644 --- a/internal/openstack/swift.go +++ b/internal/openstack/swift.go @@ -66,7 +66,7 @@ func ReconcileSwift(ctx context.Context, instance *corev1beta1.OpenStackControlP instance.Spec.Swift.Template.SwiftProxy.RabbitMqClusterName = "" } - if instance.Spec.Swift.Template.NodeSelector == nil { + if instance.Spec.Swift.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Swift.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/telemetry.go b/internal/openstack/telemetry.go index 0d302cb4cd..5a56ff19cf 100644 --- a/internal/openstack/telemetry.go +++ b/internal/openstack/telemetry.go @@ -71,7 +71,7 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont // notificationsBus.cluster (Aodh, Ceilometer) is handled by the webhook via annotation-based triggers. // No direct spec mutation here to avoid GitOps conflicts. - if instance.Spec.Telemetry.Template.NodeSelector == nil { + if instance.Spec.Telemetry.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Telemetry.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/internal/openstack/watcher.go b/internal/openstack/watcher.go index bfed839c50..2964b08d42 100644 --- a/internal/openstack/watcher.go +++ b/internal/openstack/watcher.go @@ -164,7 +164,7 @@ func ReconcileWatcher(ctx context.Context, instance *corev1beta1.OpenStackContro instance.Spec.Watcher.Template.APIServiceTemplate.TLS.API.Internal.SecretName = endpointDetails.GetEndptCertSecret(service.EndpointInternal) } - if instance.Spec.Watcher.Template.NodeSelector == nil { + if instance.Spec.Watcher.Template.NodeSelector == nil && len(instance.Spec.NodeSelector) > 0 { instance.Spec.Watcher.Template.NodeSelector = &instance.Spec.NodeSelector } diff --git a/test/functional/ctlplane/openstackoperator_controller_test.go b/test/functional/ctlplane/openstackoperator_controller_test.go index d65e71ea14..5fb14973b9 100644 --- a/test/functional/ctlplane/openstackoperator_controller_test.go +++ b/test/functional/ctlplane/openstackoperator_controller_test.go @@ -2681,6 +2681,82 @@ var _ = Describe("OpenStackOperator controller", func() { }) }) + When("An OpenStackControlplane instance is created without nodeSelector", func() { + BeforeEach(func() { + spec := GetDefaultOpenStackControlPlaneSpec() + spec["tls"] = GetTLSPublicSpec() + // Intentionally do NOT set spec["nodeSelector"] + DeferCleanup( + th.DeleteInstance, + CreateOpenStackControlPlane(names.OpenStackControlplaneName, spec), + ) + + Eventually(func(g Gomega) { + keystoneAPI := keystone.GetKeystoneAPI(names.KeystoneAPIName) + g.Expect(keystoneAPI).Should(Not(BeNil())) + }, timeout, interval).Should(Succeed()) + keystone.SimulateKeystoneAPIReady(names.KeystoneAPIName) + + Eventually(func(g Gomega) { + osversion := GetOpenStackVersion(names.OpenStackControlplaneName) + g.Expect(osversion).Should(Not(BeNil())) + + th.ExpectCondition( + names.OpenStackVersionName, + ConditionGetterFunc(OpenStackVersionConditionGetter), + corev1.OpenStackVersionInitialized, + k8s_corev1.ConditionTrue, + ) + }, timeout, interval).Should(Succeed()) + + th.CreateSecret(types.NamespacedName{Name: "openstack-config-secret", Namespace: namespace}, map[string][]byte{"secure.yaml": []byte("foo")}) + th.CreateConfigMap(types.NamespacedName{Name: "openstack-config", Namespace: namespace}, map[string]interface{}{"clouds.yaml": string("foo"), "OS_CLOUD": "default"}) + }) + + It("does not set nodeSelector on sub-CRs", func() { + Eventually(func(g Gomega) { + db := mariadb.GetGalera(names.DBName) + g.Expect(db.Spec.NodeSelector).To(BeNil()) + }, timeout, interval).Should(Succeed()) + Eventually(func(g Gomega) { + rmq := GetRabbitMQCluster(names.RabbitMQName) + g.Expect(rmq.Spec.NodeSelector).To(BeNil()) + }, timeout, interval).Should(Succeed()) + Eventually(func(g Gomega) { + mc := infra.GetMemcached(names.MemcachedName) + g.Expect(mc.Spec.NodeSelector).To(BeNil()) + }, timeout, interval).Should(Succeed()) + }) + + It("does not cause spurious updates on sub-CRs when nodeSelector is unset", func() { + // Wait for initial reconciliation to settle + Eventually(func(g Gomega) { + mc := infra.GetMemcached(names.MemcachedName) + g.Expect(mc).ShouldNot(BeNil()) + }, timeout, interval).Should(Succeed()) + + // Record the current generation of each sub-CR + mcGen := infra.GetMemcached(names.MemcachedName).Generation + dbGen := mariadb.GetGalera(names.DBName).Generation + rmqGen := GetRabbitMQCluster(names.RabbitMQName).Generation + + // Verify generation stays stable over multiple reconcile cycles. + // Before the fix, the nil-vs-pointer-to-nil-map mismatch in + // NodeSelector caused CreateOrPatch to detect a spurious diff and + // bump the generation on every reconcile (~1/second). + Consistently(func(g Gomega) { + mc := infra.GetMemcached(names.MemcachedName) + g.Expect(mc.Generation).To(Equal(mcGen)) + + db := mariadb.GetGalera(names.DBName) + g.Expect(db.Generation).To(Equal(dbGen)) + + rmq := GetRabbitMQCluster(names.RabbitMQName) + g.Expect(rmq.Generation).To(Equal(rmqGen)) + }, time.Second*5, interval).Should(Succeed()) + }) + }) + When("An OpenStackControlplane instance references a wrong topology", func() { BeforeEach(func() { spec := GetDefaultOpenStackControlPlaneSpec()