[artemiscloud#348] remove float32 fields from address settings in fav…

…our of brokerProperties

Makefile

@@ -125,7 +125,7 @@ test-mk-do: manifests generate fmt vet envtest generate-deploy ## Run tests agai
 
 ##@ Build
 
-build: generate fmt vet ## Build manager binary.
+build: generate fmt vet manifests ## Build manager binary.
 	go build -ldflags=$(LDFLAGS) -o bin/manager main.go
 
 run: manifests generate fmt vet ## Run a controller from your host.

api/v1beta1/activemqartemis_types.go

@@ -17,9 +17,6 @@ limitations under the License.
 package v1beta1
 
 import (
-	"encoding/json"
-	"strconv"
-
 	"github.com/RHsyseng/operator-utils/pkg/olm"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -60,40 +57,6 @@ type AddressSettingsType struct {
 	AddressSetting []AddressSettingType `json:"addressSetting,omitempty"`
 }
 
-// A Number represents a JSON number literal.
-type LiteralFloat32 string
-
-// String returns the literal text of the number.
-func (n LiteralFloat32) String() string { return string(n) }
-
-// Float64 returns the number as a float64.
-func (n LiteralFloat32) Float() (float64, error) {
-	return strconv.ParseFloat(string(n), 64)
-}
-
-func (n *LiteralFloat32) MarshalJSON() ([]byte, error) {
-	data, err := json.Marshal(n.String())
-
-	return data, err
-}
-
-func (n *LiteralFloat32) UnmarshalJSON(data []byte) error {
-
-	if _, err := strconv.ParseFloat(string(data), 32); err == nil {
-		*n = LiteralFloat32(data)
-	} else {
-		var v string
-
-		if err := json.Unmarshal(data, &v); err != nil {
-			return err
-		}
-
-		*n = LiteralFloat32(v)
-	}
-
-	return nil
-}
-
 type AddressSettingType struct {
 	// the address to send dead messages to
 	DeadLetterAddress *string `json:"deadLetterAddress,omitempty"`
@@ -119,10 +82,15 @@ type AddressSettingType struct {
 	MaxExpiryDelay *int32 `json:"maxExpiryDelay,omitempty"`
 	// the time (in ms) to wait before redelivering a cancelled message.
 	RedeliveryDelay *int32 `json:"redeliveryDelay,omitempty"`
+
+	// dropping these two fields due to historicatl incorrect conversion from *float32 to *string
+	// without conversion support. Existing CR's with these set cannot be reconciled
+	//
 	// multiplier to apply to the redelivery-delay
-	RedeliveryDelayMultiplier *LiteralFloat32 `json:"redeliveryDelayMultiplier,omitempty"`
+	//RedeliveryDelayMultiplier *string
 	// factor by which to modify the redelivery delay slightly to avoid collisions
-	RedeliveryCollisionAvoidanceFactor *LiteralFloat32 `json:"redeliveryCollisionAvoidanceFactor,omitempty"`
+	//RedeliveryCollisionAvoidanceFactor *string
+
 	// Maximum value for the redelivery-delay
 	MaxRedeliveryDelay *int32 `json:"maxRedeliveryDelay,omitempty"`
 	// how many times to attempt to deliver a message before sending to dead letter address

config/crd/bases/broker.amq.io_activemqartemises.yaml

@@ -394,18 +394,11 @@ spec:
                           description: The page size in bytes to use for an address.
                             Supports byte notation like K, Mb, GB, etc.
                           type: string
-                        redeliveryCollisionAvoidanceFactor:
-                          description: factor by which to modify the redelivery delay
-                            slightly to avoid collisions
-                          type: string
                         redeliveryDelay:
                           description: the time (in ms) to wait before redelivering
                             a cancelled message.
                           format: int32
                           type: integer
-                        redeliveryDelayMultiplier:
-                          description: multiplier to apply to the redelivery-delay
-                          type: string
                         redistributionDelay:
                           description: how long (in ms) to wait after the last consumer
                             is closed on a queue before redistributing messages.

controllers/activemqartemis_controller.go

@@ -18,8 +18,6 @@ package controllers
 
 import (
 	"context"
-	"encoding/json"
-	"fmt"
 	"reflect"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -141,11 +139,6 @@ func (r *ActiveMQArtemisReconciler) Reconcile(ctx context.Context, request ctrl.
 	// When deleting before creation reconcile won't be called
 	if err = r.Get(context.TODO(), request.NamespacedName, customResource); err == nil {
 
-		if err := UpdateCR(customResource, r.Client, request.NamespacedName); err != nil {
-			reqLogger.Error(err, "Error updating the CR", "ActiveMQArtemis", request.NamespacedName)
-			return ctrl.Result{}, err
-		}
-
 		namer := MakeNamers(customResource)
 		reconciler := ActiveMQArtemisReconcilerImpl{}
 
@@ -245,58 +238,6 @@ func (r *ActiveMQArtemisReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return err
 }
 
-func UpdateCR(cr *brokerv1beta1.ActiveMQArtemis, client rtclient.Client, namespacedName types.NamespacedName) error {
-
-	// The redeliveryDelayMultiplier and RedeliveryCollisionAvoidanceFactor fields are *float32
-	// in v2alpha5/v0.20.1 and they are *string in v2alpha5/v1.0.0.
-	// Those fields has been reverted to the original type to fix this backward compatibility issue
-	// but this reversion has caused a conversion issue from v2alpha5 to v1beta1.
-	// To fix this conversion issue the type of v1beta1 fields have been changed to v1beta1.LiteralFloat32.
-	// The v1beta1.LiteralFloat32 allows to unmarshal float and string values but it only allows to marshal string values.
-	// The following code block patches CR that contain float values and uses the string lenght to check
-	// if those values has been already patched.
-	patchPayload := []map[string]string{}
-	addressSettings := cr.Spec.AddressSettings.AddressSetting
-	for i := 0; i < len(addressSettings); i++ {
-		if addressSettings[i].RedeliveryDelayMultiplier != nil && len(addressSettings[i].RedeliveryDelayMultiplier.String()) < 9 {
-			redeliveryDelayMultiplier, err := addressSettings[i].RedeliveryDelayMultiplier.Float()
-			if err != nil {
-				clog.Error(err, "Error parsing RedeliveryDelayMultiplier", "ActiveMQArtemis", namespacedName)
-				return err
-			}
-			patchPayload = append(patchPayload, map[string]string{
-				"op":    "replace",
-				"path":  fmt.Sprintf("/spec/addressSettings/addressSetting/%d/redeliveryDelayMultiplier", i),
-				"value": fmt.Sprintf("%.9f", redeliveryDelayMultiplier),
-			})
-		}
-		if addressSettings[i].RedeliveryCollisionAvoidanceFactor != nil && len(addressSettings[i].RedeliveryCollisionAvoidanceFactor.String()) < 9 {
-			redeliveryCollisionAvoidanceFactor, err := addressSettings[i].RedeliveryCollisionAvoidanceFactor.Float()
-			if err != nil {
-				clog.Error(err, "Error parsing RedeliveryCollisionAvoidanceFactor", "ActiveMQArtemis", namespacedName)
-				return err
-			}
-			patchPayload = append(patchPayload, map[string]string{
-				"op":    "replace",
-				"path":  fmt.Sprintf("/spec/addressSettings/addressSetting/%d/redeliveryCollisionAvoidanceFactor", i),
-				"value": fmt.Sprintf("%.9f", redeliveryCollisionAvoidanceFactor),
-			})
-		}
-	}
-
-	if len(patchPayload) > 0 {
-		payloadBytes, _ := json.Marshal(patchPayload)
-
-		patchObj := rtclient.RawPatch(types.JSONPatchType, payloadBytes)
-		if err := client.Patch(context.TODO(), cr, patchObj); err != nil {
-			clog.Error(err, "unable to patch CR", "ActiveMQArtemis", namespacedName)
-			return err
-		}
-	}
-
-	return nil
-}
-
 func UpdateCRStatus(cr *brokerv1beta1.ActiveMQArtemis, client rtclient.Client, namespacedName types.NamespacedName) error {
 
 	common.SetReadyCondition(&cr.Status.Conditions)

controllers/activemqartemis_controller_test.go

@@ -259,8 +259,7 @@ var _ = Describe("artemis controller", func() {
 				},
 			}
 
-			By("deploying v2lpha5 with float32")
-
+			By("deploying v2lpha5 with float32, ignored")
 			Expect(k8sClient.Create(context.TODO(), &toCreate)).Should(Succeed())
 
 			key := types.NamespacedName{Name: namer.CrToSS(toCreate.Name), Namespace: defaultNamespace}
@@ -269,39 +268,63 @@ var _ = Describe("artemis controller", func() {
 				g.Expect(k8sClient.Get(ctx, key, createdSs)).Should(Succeed())
 			}, timeout, interval).Should(Succeed())
 
+			By("checking status - using original version - exercise in marshalling")
 			key = types.NamespacedName{Name: toCreate.Name, Namespace: toCreate.Namespace}
-			createdCrd := &v1beta1.ActiveMQArtemis{}
+			createdCrdv2alpha5 := &v2alpha5.ActiveMQArtemis{}
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(ctx, key, createdCrdv2alpha5)).Should(Succeed())
+				g.Expect(len(createdCrdv2alpha5.Status.PodStatus.Stopped)).Should(BeEquivalentTo(1))
+			}, timeout, interval).Should(Succeed())
+
+			By("checking status - using served version - with conditions")
+			key = types.NamespacedName{Name: toCreate.Name, Namespace: toCreate.Namespace}
+			createdCrd := &brokerv1beta1.ActiveMQArtemis{}
 			Eventually(func(g Gomega) {
 				g.Expect(k8sClient.Get(ctx, key, createdCrd)).Should(Succeed())
 				g.Expect(len(createdCrd.Status.PodStatus.Stopped)).Should(BeEquivalentTo(1))
-			}, existingClusterTimeout, existingClusterInterval).Should(Succeed())
+				g.Expect(meta.IsStatusConditionFalse(createdCrd.Status.Conditions, brokerv1beta1.DeployedConditionType)).Should(BeTrue())
+			}, timeout, interval).Should(Succeed())
 
-			crd := generateArtemisSpec(defaultNamespace)
-			var literalVar = brokerv1beta1.LiteralFloat32("3.5")
-			crd.Spec.AddressSettings = brokerv1beta1.AddressSettingsType{
-				ApplyRule: &ma,
-				AddressSetting: []brokerv1beta1.AddressSettingType{
-					{
-						Match:                              "#",
-						RedeliveryCollisionAvoidanceFactor: &literalVar,
+			Expect(k8sClient.Delete(ctx, createdCrd)).Should(Succeed())
+		})
+
+		It("can reconcile latest version with config in brokerProperties", func() {
+
+			toCreate := brokerv1beta1.ActiveMQArtemis{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "ActiveMQArtemis",
+					APIVersion: brokerv1beta1.GroupVersion.Identifier(),
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      nameFromTest(),
+					Namespace: defaultNamespace,
+				},
+				Spec: brokerv1beta1.ActiveMQArtemisSpec{
+					BrokerProperties: []string{
+						"addressesSettings.#.redeliveryDelayMultiplier=2.3",
+						"addressesSettings.#.redeliveryCollisionAvoidanceFactor=1.2",
 					},
 				},
 			}
 
-			By("deploying v1beta1 with LiteralFloat32")
-
-			Expect(k8sClient.Create(context.TODO(), &crd)).Should(Succeed())
+			Expect(k8sClient.Create(context.TODO(), &toCreate)).Should(Succeed())
 
-			key = types.NamespacedName{Name: namer.CrToSS(crd.Name), Namespace: defaultNamespace}
+			key := types.NamespacedName{Name: namer.CrToSS(toCreate.Name), Namespace: defaultNamespace}
+			createdSs := &appsv1.StatefulSet{}
 			Eventually(func(g Gomega) {
 				g.Expect(k8sClient.Get(ctx, key, createdSs)).Should(Succeed())
 			}, timeout, interval).Should(Succeed())
 
 			key = types.NamespacedName{Name: toCreate.Name, Namespace: toCreate.Namespace}
+			createdCrd := &v1beta1.ActiveMQArtemis{}
 			Eventually(func(g Gomega) {
 				g.Expect(k8sClient.Get(ctx, key, createdCrd)).Should(Succeed())
 				g.Expect(len(createdCrd.Status.PodStatus.Stopped)).Should(BeEquivalentTo(1))
+				g.Expect(meta.IsStatusConditionFalse(createdCrd.Status.Conditions, brokerv1beta1.DeployedConditionType)).Should(BeTrue())
 			}, existingClusterTimeout, existingClusterInterval).Should(Succeed())
+
+			Expect(k8sClient.Delete(ctx, createdCrd)).Should(Succeed())
+
 		})
 	})
 

pkg/utils/config/config.go

@@ -159,28 +159,6 @@ func IsEqualV1Beta1(currentAddressSetting []brokerv1beta1.AddressSettingType, ne
 		} else {
 			return false
 		}
-		if newSetting.RedeliveryDelayMultiplier == nil {
-			if curSetting.RedeliveryDelayMultiplier != nil {
-				return false
-			}
-		} else if curSetting.RedeliveryDelayMultiplier != nil {
-			if *curSetting.RedeliveryDelayMultiplier != *newSetting.RedeliveryDelayMultiplier {
-				return false
-			}
-		} else {
-			return false
-		}
-		if newSetting.RedeliveryCollisionAvoidanceFactor == nil {
-			if curSetting.RedeliveryCollisionAvoidanceFactor != nil {
-				return false
-			}
-		} else if curSetting.RedeliveryCollisionAvoidanceFactor != nil {
-			if *curSetting.RedeliveryCollisionAvoidanceFactor != *newSetting.RedeliveryCollisionAvoidanceFactor {
-				return false
-			}
-		} else {
-			return false
-		}
 		if newSetting.MaxRedeliveryDelay == nil {
 			if curSetting.MaxRedeliveryDelay != nil {
 				return false

pkg/utils/cr2jinja2/cr2jinja2.go

@@ -121,29 +121,6 @@ func checkFloat32(prop *float32) *string {
 	return &tmp
 }
 
-func checkFloat32AsliteralFloat32(prop *v1beta1.LiteralFloat32) *string {
-	if nil == prop {
-		return nil
-	}
-	tmp := fmt.Sprint(prop.Float())
-	return &tmp
-}
-
-func checkFloat32AsString(propstr *string) *string {
-	var prop *float32 = nil
-	var value float64
-	var err error
-	if nil != propstr {
-		if value, err = strconv.ParseFloat(*propstr, 32); err != nil {
-			cr2jinja2Log.Error(err, "failed to convert to float32 from string", "value", propstr)
-		} else {
-			tmpval := float32(value)
-			prop = &tmpval
-		}
-	}
-	return checkFloat32(prop)
-}
-
 /* return a yaml string and a map of special values that need to pass to yacfg */
 func MakeBrokerCfgOverrides(customResource interface{}, envVar *string, output *string) (string, map[string]string) {
 
@@ -681,12 +658,6 @@ func processAddressSettingsV1beta1(sb *strings.Builder, addressSettings *[]v1bet
 		if value := checkInt32(s.RedeliveryDelay); value != nil {
 			sb.WriteString("  redelivery_delay: " + *value + "\n")
 		}
-		if value := checkFloat32AsliteralFloat32(s.RedeliveryDelayMultiplier); value != nil {
-			sb.WriteString("  redelivery_delay_multiplier: " + *value + "\n")
-		}
-		if value := checkFloat32AsliteralFloat32(s.RedeliveryCollisionAvoidanceFactor); value != nil {
-			sb.WriteString("  redelivery_collision_avoidance_factor: " + *value + "\n")
-		}
 		if value := checkInt32(s.MaxRedeliveryDelay); value != nil {
 			sb.WriteString("  max_redelivery_delay: " + *value + "\n")
 		}