Fix update after scaling (#807)

Author: thegridman

api/v1/coherenceresource_types.go

@@ -178,7 +178,13 @@ func (in *Coherence) GetHeadlessServiceIPFamily() []corev1.IPFamily {
 // return either the actual Replicas value or the default (DefaultReplicas const)
 // if the Replicas field is nil.
 func (in *Coherence) GetReplicas() int32 {
-	if in == nil || in.Spec.Replicas == nil {
+	if in == nil {
+		return DefaultReplicas
+	}
+	if in.Spec.Replicas == nil {
+		if in.Status.Replicas > 0 {
+			return in.Status.Replicas
+		}
 		return DefaultReplicas
 	}
 	return in.Spec.GetReplicas()

controllers/coherence_controller.go

@@ -158,7 +158,7 @@ func (in *CoherenceReconciler) Reconcile(ctx context.Context, request ctrl.Reque
 		return ctrl.Result{}, nil
 	}
 
-	// The request is an add or update
+	// This is an add request or update request
 
 	if deployment.Spec.AllowUnsafeDelete != nil && *deployment.Spec.AllowUnsafeDelete {
 		if controllerutil.ContainsFinalizer(deployment, coh.CoherenceFinalizer) {
@@ -174,8 +174,7 @@ func (in *CoherenceReconciler) Reconcile(ctx context.Context, request ctrl.Reque
 			log.Info("Finalizer not added to Coherence resource as AllowUnsafeDelete has been set to true")
 		}
 	} else {
-		// Add finalizer for this CR if required (it should have been added by the web-hook but may not have been if the
-		// Coherence resource was added when the Operator was uninstalled)
+		// Add a finalizer for this CR if required
 		if finalizerApplied, err := in.finalizerManager.EnsureFinalizerApplied(ctx, deployment); finalizerApplied || err != nil {
 			var msg string
 			if err != nil {
@@ -199,10 +198,9 @@ func (in *CoherenceReconciler) Reconcile(ctx context.Context, request ctrl.Reque
 	}
 
 	// Check whether the deployment has a replica count specified
-	// Ideally we'd do this with a validating/defaulting web-hook but maybe in a later version.
 	if deployment.Spec.Replicas == nil {
 		// No replica count, so we patch the deployment to have the default replicas value.
-		// The reason we do this, is because the kubectl scale command will fail if the replicas
+		// The reason we do this is the kubectl scale command will fail if the replicas
 		// field is not set to a non-nil value.
 		patch := &coh.Coherence{}
 		deployment.DeepCopyInto(patch)

test/certification/certifiy_deployment_test.go

@@ -84,9 +84,9 @@ func TestCertifyScaling(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 }
 
-// Test the scenario where we create a Coherence cluster without a replicas field, which will default to three Pods.
+// Test the scenario where we create a Coherence cluster without a "replicas" field, which will default to three Pods.
 // Then scale up the cluster to four.
-// The apply an update using the same Coherence resource with no replicas field.
+// Then apply an update using the same Coherence resource with no replicas field.
 // After the update is applied, the cluster should still be four and not revert to three.
 func TestCertifyScalingWithUpdate(t *testing.T) {
 	// Ensure that everything is cleaned up after the test!
@@ -118,6 +118,40 @@ func TestCertifyScalingWithUpdate(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 }
 
+// Test the scenario where we create a Coherence cluster with a "replicas" field.
+// Then scale up the cluster to four.
+// Then apply an update using the same Coherence resource with no replicas field.
+// After the update is applied, the cluster should still be four and not revert to three.
+func TestCertifyScalingClusterWithReplicasThenUpdate(t *testing.T) {
+	// Ensure that everything is cleaned up after the test!
+	testContext.CleanupAfterTest(t)
+	g := NewGomegaWithT(t)
+
+	ns := helper.GetTestClusterNamespace()
+
+	// the name of the cluster from scale-with-update-one.yaml and scale-with-update-two.yaml
+	name := "certify-scale-update"
+
+	// Start with the two replicas
+	err := apply(t, ns, "scale-with-update-with-replicas.yaml")
+	g.Expect(err).NotTo(HaveOccurred())
+	_, err = helper.WaitForPodsWithLabel(testContext, ns, "one=testOne", 2, time.Second*10, time.Minute*10)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// Scale Up to four
+	err = scale(t, ns, name, 4)
+	g.Expect(err).NotTo(HaveOccurred())
+	_, err = helper.WaitForStatefulSet(testContext, ns, name, 4, time.Second*10, time.Minute*5)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// apply the update
+	err = apply(t, ns, "scale-with-update-two.yaml")
+	g.Expect(err).NotTo(HaveOccurred())
+	// There should eventually be four Pods with the additional label
+	_, err = helper.WaitForPodsWithLabel(testContext, ns, "two=testTwo", 4, time.Second*10, time.Minute*10)
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func scale(t *testing.T, namespace, name string, replicas int32) error {
 	cmd := exec.Command("kubectl", "-n", namespace, "scale", fmt.Sprintf("--replicas=%d", replicas), "coherence/"+name)
 	cmd.Stdout = os.Stdout

test/certification/scale-with-update-with-replicas.yaml

@@ -0,0 +1,11 @@
+apiVersion: coherence.oracle.com/v1
+kind: Coherence
+metadata:
+  name: certify-scale-update
+spec:
+  replicas: 2
+  labels:
+    one: testOne
+  readinessProbe:
+    initialDelaySeconds: 10
+    periodSeconds: 10