Repository: submariner-io/submariner-operator
Pull request #1091 (2021-02-15 22:10:21)

Only update Submariner resource if changed
... when setting defaults on reconcile.

Fixes (hopefully)  https://github.com/submariner-io/submariner-operator/issues/1047


Patch:
diff --git a/apis/submariner/v1alpha1/submariner_types.go b/apis/submariner/v1alpha1/submariner_types.go
index 0dd787a4..9c66b46b 100644
--- a/apis/submariner/v1alpha1/submariner_types.go
+++ b/apis/submariner/v1alpha1/submariner_types.go
@@ -128,17 +128,24 @@ func init() {
 	SchemeBuilder.Register(&Submariner{}, &SubmarinerList{})
 }
 
-func (submariner *Submariner) SetDefaults() {
+func (submariner *Submariner) SetDefaults() bool {
+	updated := false
+
 	if submariner.Spec.Repository == "" {
 		// An empty field is converted to the default upstream submariner repository where all images live
 		submariner.Spec.Repository = versions.DefaultRepo
+		updated = true
 	}
 
 	if submariner.Spec.Version == "" {
 		submariner.Spec.Version = versions.DefaultSubmarinerVersion
+		updated = true
 	}
 
 	if submariner.Spec.ColorCodes == "" {
 		submariner.Spec.ColorCodes = DefaultColorCode
+		updated = true
 	}
+
+	return updated
 }
diff --git a/controllers/submariner/submariner_controller.go b/controllers/submariner/submariner_controller.go
index f79f57f9..3ef8b59e 100644
--- a/controllers/submariner/submariner_controller.go
+++ b/controllers/submariner/submariner_controller.go
@@ -123,10 +123,12 @@ func (r *SubmarinerReconciler) Reconcile(request reconcile.Request) (reconcile.R
 
 	initialStatus := instance.Status.DeepCopy()
 
-	instance.SetDefaults()
+	if instance.SetDefaults() {
+		reqLogger.Info("Updating Submariner instance with defaults")
 
-	if err := r.client.Update(context.TODO(), instance); err != nil {
-		return reconcile.Result{}, err
+		if err := r.client.Update(context.TODO(), instance); err != nil {
+			return reconcile.Result{}, err
+		}
 	}
 
 	// discovery is performed after Update to avoid storing the discovery in the
@@ -168,27 +170,7 @@ func (r *SubmarinerReconciler) Reconcile(request reconcile.Request) (reconcile.R
 		log.Error(err, "error retrieving gateways")
 	}
 
-	var gatewayStatuses = []submv1.GatewayStatus{}
-	if gateways != nil {
-		recordGateways(len(*gateways))
-		// Clear the connections so we don’t remember stale status information
-		recordNoConnections()
-		for _, gateway := range *gateways {
-			gatewayStatuses = append(gatewayStatuses, gateway.Status)
-			recordGatewayCreationTime(gateway.Status.LocalEndpoint, gateway.CreationTimestamp.Time)
-
-			for j := range gateway.Status.Connections {
-				recordConnection(
-					gateway.Status.LocalEndpoint,
-					gateway.Status.Connections[j].Endpoint,
-					string(gateway.Status.Connections[j].Status),
-				)
-			}
-		}
-	} else {
-		recordGateways(0)
-		recordNoConnections()
-	}
+	gatewayStatuses := buildGatewayStatusAndUpdateMetrics(gateways)
 
 	instance.Status.NatEnabled = instance.Spec.NatEnabled
 	instance.Status.ColorCodes = instance.Spec.ColorCodes
@@ -225,6 +207,33 @@ func (r *SubmarinerReconciler) Reconcile(request reconcile.Request) (reconcile.R
 	return reconcile.Result{}, nil
 }
 
+func buildGatewayStatusAndUpdateMetrics(gateways *[]submv1.Gateway) []submv1.GatewayStatus {
+	var gatewayStatuses = []submv1.GatewayStatus{}
+
+	if gateways != nil {
+		recordGateways(len(*gateways))
+		// Clear the connections so we don’t remember stale status information
+		recordNoConnections()
+		for _, gateway := range *gateways {
+			gatewayStatuses = append(gatewayStatuses, gateway.Status)
+			recordGatewayCreationTime(gateway.Status.LocalEndpoint, gateway.CreationTimestamp.Time)
+
+			for j := range gateway.Status.Connections {
+				recordConnection(
+					gateway.Status.LocalEndpoint,
+					gateway.Status.Connections[j].Endpoint,
+					string(gateway.Status.Connections[j].Status),
+				)
+			}
+		}
+	} else {
+		recordGateways(0)
+		recordNoConnections()
+	}
+
+	return gatewayStatuses
+}
+
 func (r *SubmarinerReconciler) updateDaemonSetStatus(daemonSet *appsv1.DaemonSet, status *submopv1a1.DaemonSetStatus,
 	namespace string) error {
 	if daemonSet != nil {


Test patch:
diff --git a/controllers/submariner/submariner_controller_test.go b/controllers/submariner/submariner_controller_test.go
index 63f21822..a8800002 100644
--- a/controllers/submariner/submariner_controller_test.go
+++ b/controllers/submariner/submariner_controller_test.go
@@ -73,7 +73,7 @@ func (c *failingClient) Get(ctx context.Context, key controllerClient.ObjectKey,
 
 func (c *failingClient) Update(ctx context.Context, obj runtime.Object, opts ...controllerClient.UpdateOption) error {
 	if c.onUpdate == reflect.TypeOf(obj) {
-		return fmt.Errorf("Mock Get error")
+		return fmt.Errorf("Mock Update error")
 	}
 
 	return c.Client.Update(ctx, obj, opts...)
@@ -311,8 +311,19 @@ func testReconciliation() {
 		})
 	})
 
+	When("all Submariner resource default values are set", func() {
+		BeforeEach(func() {
+			fakeClient = &failingClient{Client: newClient(), onUpdate: reflect.TypeOf(&submariner_v1.Submariner{})}
+		})
+
+		It("should not update the resource", func() {
+			Expect(reconcileErr).To(Succeed())
+		})
+	})
+
 	When("Submariner resource update fails", func() {
 		BeforeEach(func() {
+			submariner.Spec.Repository = ""
 			fakeClient = &failingClient{Client: newClient(), onUpdate: reflect.TypeOf(&submariner_v1.Submariner{})}
 		})