[#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.
artemiscloud · Oct 4, 2023 · a8e8e3e · a8e8e3e
1 parent 16f211f
commit a8e8e3e
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 30 deletions.
diff --git a/controllers/activemqartemis_controller.go b/controllers/activemqartemis_controller.go
@@ -599,35 +599,43 @@ 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, &current.Status) {
 		clog.Info("CR.status update", "Namespace", desired.Namespace, "Name", desired.Name, "Observed status", desired.Status)
 		return resources.UpdateStatus(client, desired)
 	}
 
 	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
@@ -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
@@ -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) {