From a8e8e3eaf107a18cb243ee46057b6d8eb9fb267b Mon Sep 17 00:00:00 2001 From: Domenico Francesco Bruscino Date: Wed, 4 Oct 2023 09:37:55 +0200 Subject: [PATCH] [#698] Sort labels in ScaleLabelSelector The labels in ScaleLabelSelector need to be sorted because they come from maps and their order could change causing infinite reconcile loops. --- controllers/activemqartemis_controller.go | 40 +++++++++++-------- .../activemqartemis_controller_test.go | 32 +++++++++++++++ controllers/activemqartemis_reconciler.go | 19 +++------ 3 files changed, 61 insertions(+), 30 deletions(-) diff --git a/controllers/activemqartemis_controller.go b/controllers/activemqartemis_controller.go index 1ce7f47aa..275266190 100644 --- a/controllers/activemqartemis_controller.go +++ b/controllers/activemqartemis_controller.go @@ -599,15 +599,7 @@ func UpdateCRStatus(desired *brokerv1beta1.ActiveMQArtemis, client rtclient.Clie return err } - if current.Status.DeploymentPlanSize != desired.Status.DeploymentPlanSize || - current.Status.ScaleLabelSelector != desired.Status.ScaleLabelSelector || - !reflect.DeepEqual(current.Status.Version, desired.Status.Version) || - len(desired.Status.ExternalConfigs) != len(current.Status.ExternalConfigs) || - externalConfigsModified(desired, current) || - !reflect.DeepEqual(current.Status.PodStatus, desired.Status.PodStatus) || - len(current.Status.Conditions) != len(desired.Status.Conditions) || - conditionsModified(desired, current) { - + if !EqualCRStatus(&desired.Status, ¤t.Status) { clog.Info("CR.status update", "Namespace", desired.Namespace, "Name", desired.Name, "Observed status", desired.Status) return resources.UpdateStatus(client, desired) } @@ -615,19 +607,35 @@ func UpdateCRStatus(desired *brokerv1beta1.ActiveMQArtemis, client rtclient.Clie return nil } -func conditionsModified(desired *brokerv1beta1.ActiveMQArtemis, current *brokerv1beta1.ActiveMQArtemis) bool { - for _, c := range desired.Status.Conditions { - if !common.IsConditionPresentAndEqual(current.Status.Conditions, c) { +func EqualCRStatus(s1, s2 *brokerv1beta1.ActiveMQArtemisStatus) bool { + if s1.DeploymentPlanSize != s2.DeploymentPlanSize || + s1.ScaleLabelSelector != s2.ScaleLabelSelector || + !reflect.DeepEqual(s1.Version, s2.Version) || + len(s2.ExternalConfigs) != len(s1.ExternalConfigs) || + externalConfigsModified(s2.ExternalConfigs, s1.ExternalConfigs) || + !reflect.DeepEqual(s1.PodStatus, s2.PodStatus) || + len(s1.Conditions) != len(s2.Conditions) || + conditionsModified(s2.Conditions, s1.Conditions) { + + return false + } + + return true +} + +func conditionsModified(desiredConditions []metav1.Condition, currentConditions []metav1.Condition) bool { + for _, c := range desiredConditions { + if !common.IsConditionPresentAndEqual(currentConditions, c) { return true } } return false } -func externalConfigsModified(desired *brokerv1beta1.ActiveMQArtemis, current *brokerv1beta1.ActiveMQArtemis) bool { - if len(desired.Status.ExternalConfigs) >= 0 { - for _, cfg := range desired.Status.ExternalConfigs { - for _, curCfg := range current.Status.ExternalConfigs { +func externalConfigsModified(desiredExternalConfigs []brokerv1beta1.ExternalConfigStatus, currentExternalConfigs []brokerv1beta1.ExternalConfigStatus) bool { + if len(desiredExternalConfigs) >= 0 { + for _, cfg := range desiredExternalConfigs { + for _, curCfg := range currentExternalConfigs { if curCfg.Name == cfg.Name && curCfg.ResourceVersion != cfg.ResourceVersion { return true } diff --git a/controllers/activemqartemis_controller_test.go b/controllers/activemqartemis_controller_test.go index 4fce88df7..0c5f36519 100644 --- a/controllers/activemqartemis_controller_test.go +++ b/controllers/activemqartemis_controller_test.go @@ -29,6 +29,7 @@ import ( "net/url" "os" "reflect" + "sort" "strconv" "strings" @@ -4016,6 +4017,37 @@ var _ = Describe("artemis controller", func() { Expect(k8sClient.Delete(ctx, createdCrd)) }) + + It("passing in 8 labels", func() { + By("Creating a crd with 8 labels") + crd := generateArtemisSpec(defaultNamespace) + crd.Spec.DeploymentPlan.Labels = make(map[string]string) + crd.Spec.DeploymentPlan.Labels["key1"] = "val1" + crd.Spec.DeploymentPlan.Labels["key2"] = "val2" + crd.Spec.DeploymentPlan.Labels["key3"] = "val3" + crd.Spec.DeploymentPlan.Labels["key4"] = "val4" + crd.Spec.DeploymentPlan.Labels["key5"] = "val5" + crd.Spec.DeploymentPlan.Labels["key6"] = "val6" + crd.Spec.DeploymentPlan.Labels["key7"] = "val7" + crd.Spec.DeploymentPlan.Labels["key8"] = "val8" + + namer := MakeNamers(&crd) + namespacedName := types.NamespacedName{Name: crd.Name, Namespace: crd.Namespace} + + crd0 := crd.DeepCopy() + By("Processing status 0") + ProcessStatus(crd0, k8sClient, namespacedName, *namer, nil) + Expect(crd0.Status.ScaleLabelSelector).ShouldNot(BeEmpty()) + Expect(sort.StringsAreSorted(strings.Split(crd0.Status.ScaleLabelSelector, ","))).Should(BeTrue()) + + crd1 := crd.DeepCopy() + By("Processing status 1") + ProcessStatus(crd1, k8sClient, namespacedName, *namer, nil) + Expect(crd1.Status.ScaleLabelSelector).ShouldNot(BeEmpty()) + Expect(sort.StringsAreSorted(strings.Split(crd1.Status.ScaleLabelSelector, ","))).Should(BeTrue()) + + Expect(EqualCRStatus(&crd0.Status, &crd1.Status)).Should(BeTrue()) + }) }) Context("Different namespace, deployed before start", func() { diff --git a/controllers/activemqartemis_reconciler.go b/controllers/activemqartemis_reconciler.go index fe4415ee3..c958da007 100644 --- a/controllers/activemqartemis_reconciler.go +++ b/controllers/activemqartemis_reconciler.go @@ -2421,24 +2421,15 @@ func ProcessStatus(cr *brokerv1beta1.ActiveMQArtemis, client rtclient.Client, na } func updateScaleStatus(cr *brokerv1beta1.ActiveMQArtemis, namer Namers) { - Selector := new(bytes.Buffer) - - var needsSep bool = false + labels := make([]string, 0, len(namer.LabelBuilder.Labels())+len(cr.Spec.DeploymentPlan.Labels)) for k, v := range namer.LabelBuilder.Labels() { - if needsSep { - fmt.Fprintf(Selector, ",") - } - fmt.Fprintf(Selector, "%s=%s", k, v) - needsSep = true + labels = append(labels, fmt.Sprintf("%s=%s", k, v)) } for k, v := range cr.Spec.DeploymentPlan.Labels { - if needsSep { - fmt.Fprintf(Selector, ",") - } - fmt.Fprintf(Selector, "%s=%s", k, v) - needsSep = true + labels = append(labels, fmt.Sprintf("%s=%s", k, v)) } - cr.Status.ScaleLabelSelector = Selector.String() + sort.Strings(labels) + cr.Status.ScaleLabelSelector = strings.Join(labels[:], ",") } func updateVersionStatus(cr *brokerv1beta1.ActiveMQArtemis) {