Fixed OnDelete strategy (#83)

Author: chatton

pkg/apis/mongodb/v1/mongodb_types.go

@@ -21,12 +21,6 @@ const (
 	Running Phase = "Running"
 )
 
-const (
-	// LastVersionAnnotationKey should indicate which version of MongoDB was last
-	// configured
-	LastVersionAnnotationKey = "lastVersion"
-)
-
 // MongoDBSpec defines the desired state of MongoDB
 type MongoDBSpec struct {
 	// Members is the number of members in the replica set
@@ -70,13 +64,6 @@ func (m *MongoDB) UpdateSuccess() {
 	m.Status.Phase = Running
 }
 
-func (m MongoDB) IsChangingVersion() bool {
-	if lastVersion, ok := m.Annotations[LastVersionAnnotationKey]; ok {
-		return (m.Spec.Version != lastVersion) && lastVersion != ""
-	}
-	return false
-}
-
 // MongoURI returns a mongo uri which can be used to connect to this deployment
 func (m MongoDB) MongoURI() string {
 	members := make([]string, m.Spec.Members)

pkg/controller/mongodb/mongodb_controller.go

@@ -57,6 +57,11 @@ const (
 	clusterFilePath                = "/var/lib/automation/config/automation-config"
 	operatorServiceAccountName     = "mongodb-kubernetes-operator"
 	agentHealthStatusFilePathValue = "/var/log/mongodb-mms-automation/healthstatus/agent-health-status.json"
+
+	// lastVersionAnnotationKey should indicate which version of MongoDB was last
+	// configured
+	lastVersionAnnotationKey       = "mongodb.com/v1.lastVersion"
+	hasLeftReadyStateAnnotationKey = "mongodb.com/v1.hasLeftReadyStateAnnotationKey"
 )
 
 // Add creates a new MongoDB Controller and adds it to the Manager. The Manager will set fields on the Controller
@@ -176,8 +181,13 @@ func (r *ReplicaSetReconciler) Reconcile(request reconcile.Request) (reconcile.R
 	}
 
 	r.log.Debug("Setting MongoDB Annotations")
-	if err := r.setAnnotation(types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, mdbv1.LastVersionAnnotationKey, mdb.Spec.Version); err != nil {
-		r.log.Warnf("Error setting annotation: %+v", err)
+
+	annotations := map[string]string{
+		lastVersionAnnotationKey:       mdb.Spec.Version,
+		hasLeftReadyStateAnnotationKey: "false",
+	}
+	if err := r.setAnnotations(mdb.NamespacedName(), annotations); err != nil {
+		r.log.Warnf("Error setting annotations: %+v", err)
 		return reconcile.Result{}, err
 	}
 
@@ -195,7 +205,7 @@ func (r *ReplicaSetReconciler) Reconcile(request reconcile.Request) (reconcile.R
 // resetStatefulSetUpdateStrategy ensures the stateful set is configured back to using RollingUpdateStatefulSetStrategyType
 // and does not keep using OnDelete
 func (r *ReplicaSetReconciler) resetStatefulSetUpdateStrategy(mdb mdbv1.MongoDB) error {
-	if !mdb.IsChangingVersion() {
+	if !isChangingVersion(mdb) {
 		return nil
 	}
 	// if we changed the version, we need to reset the UpdatePolicy back to OnUpdate
@@ -220,10 +230,28 @@ func (r *ReplicaSetReconciler) isStatefulSetReady(mdb mdbv1.MongoDB, existingSta
 		return false, err
 	}
 
-	// comparison is done with bytes instead of reflect.DeepEqual as there are
-	// some issues with nil/empty maps not being compared correctly otherwise
-	areEqual := bytes.Compare(stsCopyBytes, stsBytes) == 0
+	//comparison is done with bytes instead of reflect.DeepEqual as there are
+	//some issues with nil/empty maps not being compared correctly otherwise
+	areEqual := bytes.Equal(stsCopyBytes, stsBytes)
+
 	isReady := statefulset.IsReady(*existingStatefulSet, mdb.Spec.Members)
+	if existingStatefulSet.Spec.UpdateStrategy.Type == appsv1.OnDeleteStatefulSetStrategyType && !isReady {
+		r.log.Info("StatefulSet has left ready state, version upgrade in progress")
+		annotations := map[string]string{
+			hasLeftReadyStateAnnotationKey: "true",
+		}
+		if err := r.setAnnotations(mdb.NamespacedName(), annotations); err != nil {
+			return false, fmt.Errorf("failed setting %s annotation to true: %s", hasLeftReadyStateAnnotationKey, err)
+		}
+	}
+
+	hasPerformedUpgrade := mdb.Annotations[hasLeftReadyStateAnnotationKey] == "true"
+	r.log.Infow("StatefulSet Readiness", "isReady", isReady, "hasPerformedUpgrade", hasPerformedUpgrade, "areEqual", areEqual)
+
+	if existingStatefulSet.Spec.UpdateStrategy.Type == appsv1.OnDeleteStatefulSetStrategyType {
+		return areEqual && isReady && hasPerformedUpgrade, nil
+	}
+
 	return areEqual && isReady, nil
 }
 
@@ -251,15 +279,17 @@ func (r *ReplicaSetReconciler) createOrUpdateStatefulSet(mdb mdbv1.MongoDB) erro
 	return nil
 }
 
-// setAnnotation updates the monogdb resource with the given namespaced name and sets the annotation
-// "key" with the provided value "val"
-func (r ReplicaSetReconciler) setAnnotation(nsName types.NamespacedName, key, val string) error {
+// setAnnotations updates the monogdb resource annotations by applying the provided annotations
+// on top of the existing ones
+func (r ReplicaSetReconciler) setAnnotations(nsName types.NamespacedName, annotations map[string]string) error {
 	mdb := mdbv1.MongoDB{}
 	return r.client.GetAndUpdate(nsName, &mdb, func() {
 		if mdb.Annotations == nil {
 			mdb.Annotations = map[string]string{}
 		}
-		mdb.Annotations[key] = val
+		for key, val := range annotations {
+			mdb.Annotations[key] = val
+		}
 	})
 }
 
@@ -383,7 +413,7 @@ func (r ReplicaSetReconciler) buildAutomationConfigConfigMap(mdb mdbv1.MongoDB)
 // getUpdateStrategyType returns the type of RollingUpgradeStrategy that the StatefulSet
 // should be configured with
 func getUpdateStrategyType(mdb mdbv1.MongoDB) appsv1.StatefulSetUpdateStrategyType {
-	if !mdb.IsChangingVersion() {
+	if !isChangingVersion(mdb) {
 		return appsv1.RollingUpdateStatefulSetStrategyType
 	}
 	return appsv1.OnDeleteStatefulSetStrategyType
@@ -397,6 +427,13 @@ func buildStatefulSet(mdb mdbv1.MongoDB) (appsv1.StatefulSet, error) {
 	return sts, nil
 }
 
+func isChangingVersion(mdb mdbv1.MongoDB) bool {
+	if lastVersion, ok := mdb.Annotations[lastVersionAnnotationKey]; ok {
+		return (mdb.Spec.Version != lastVersion) && lastVersion != ""
+	}
+	return false
+}
+
 func mongodbAgentContainer(volumeMounts []corev1.VolumeMount) container.Modification {
 	return container.Apply(
 		container.WithName(agentName),

pkg/controller/mongodb/replicaset_controller_test.go

@@ -5,6 +5,9 @@ import (
 	"os"
 	"reflect"
 	"testing"
+	"time"
+
+	k8sClient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/probes"
 
@@ -111,11 +114,11 @@ func TestChangingVersion_ResultsInRollingUpdateStrategyType(t *testing.T) {
 	mgr := client.NewManager(&mdb)
 	mgrClient := mgr.GetClient()
 	r := newReconciler(mgr, mockManifestProvider(mdb.Spec.Version))
-	res, err := r.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
+	res, err := r.Reconcile(reconcile.Request{NamespacedName: mdb.NamespacedName()})
 	assertReconciliationSuccessful(t, res, err)
 
 	// fetch updated resource after first reconciliation
-	_ = mgrClient.Get(context.TODO(), types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, &mdb)
+	_ = mgrClient.Get(context.TODO(), mdb.NamespacedName(), &mdb)
 
 	sts := appsv1.StatefulSet{}
 	err = mgrClient.Get(context.TODO(), types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, &sts)
@@ -127,6 +130,26 @@ func TestChangingVersion_ResultsInRollingUpdateStrategyType(t *testing.T) {
 
 	_ = mgrClient.Update(context.TODO(), &mdb)
 
+	// agents start the upgrade, they are not all ready
+	sts.Status.UpdatedReplicas = 1
+	sts.Status.ReadyReplicas = 2
+	err = mgrClient.Update(context.TODO(), &sts)
+
+	_ = mgrClient.Get(context.TODO(), mdb.NamespacedName(), &sts)
+
+	// the request is requeued as the agents are still doing the upgrade
+	res, err = r.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
+	assert.NoError(t, err)
+	assert.Equal(t, res.RequeueAfter, time.Second*10)
+
+	_ = mgrClient.Get(context.TODO(), mdb.NamespacedName(), &sts)
+	assert.Equal(t, appsv1.OnDeleteStatefulSetStrategyType, sts.Spec.UpdateStrategy.Type)
+	// upgrade is now complete
+	sts.Status.UpdatedReplicas = 3
+	sts.Status.ReadyReplicas = 3
+	err = mgrClient.Update(context.TODO(), &sts)
+
+	// reconcilliation is successful
 	res, err = r.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
 	assertReconciliationSuccessful(t, res, err)
 
@@ -142,23 +165,23 @@ func TestBuildStatefulSet_ConfiguresUpdateStrategyCorrectly(t *testing.T) {
 	t.Run("On No Version Change, Same Version", func(t *testing.T) {
 		mdb := newTestReplicaSet()
 		mdb.Spec.Version = "4.0.0"
-		mdb.Annotations[mdbv1.LastVersionAnnotationKey] = "4.0.0"
+		mdb.Annotations[lastVersionAnnotationKey] = "4.0.0"
 		sts, err := buildStatefulSet(mdb)
 		assert.NoError(t, err)
 		assert.Equal(t, appsv1.RollingUpdateStatefulSetStrategyType, sts.Spec.UpdateStrategy.Type)
 	})
 	t.Run("On No Version Change, First Version", func(t *testing.T) {
 		mdb := newTestReplicaSet()
 		mdb.Spec.Version = "4.0.0"
-		delete(mdb.Annotations, mdbv1.LastVersionAnnotationKey)
+		delete(mdb.Annotations, lastVersionAnnotationKey)
 		sts, err := buildStatefulSet(mdb)
 		assert.NoError(t, err)
 		assert.Equal(t, appsv1.RollingUpdateStatefulSetStrategyType, sts.Spec.UpdateStrategy.Type)
 	})
 	t.Run("On Version Change", func(t *testing.T) {
 		mdb := newTestReplicaSet()
 		mdb.Spec.Version = "4.0.0"
-		mdb.Annotations[mdbv1.LastVersionAnnotationKey] = "4.2.0"
+		mdb.Annotations[lastVersionAnnotationKey] = "4.2.0"
 		sts, err := buildStatefulSet(mdb)
 		assert.NoError(t, err)
 		assert.Equal(t, appsv1.OnDeleteStatefulSetStrategyType, sts.Spec.UpdateStrategy.Type)
@@ -198,6 +221,7 @@ func TestAutomationConfig_versionIsBumpedOnChange(t *testing.T) {
 	assert.Equal(t, 1, currentAc.Version)
 
 	mdb.Spec.Members += 1
+	makeStatefulSetReady(mgr.GetClient(), mdb)
 
 	_ = mgr.GetClient().Update(context.TODO(), &mdb)
 	res, err = r.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
@@ -228,3 +252,13 @@ func TestAutomationConfig_versionIsNotBumpedWithNoChanges(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, currentAc.Version, 1)
 }
+
+// makeStatefulSetReady updates the StatefulSet corresponding to the
+// provided MongoDB resource to mark it as ready for the case of `statefulset.IsReady`
+func makeStatefulSetReady(c k8sClient.Client, mdb mdbv1.MongoDB) {
+	sts := appsv1.StatefulSet{}
+	_ = c.Get(context.TODO(), mdb.NamespacedName(), &sts)
+	sts.Status.ReadyReplicas = int32(mdb.Spec.Members)
+	sts.Status.UpdatedReplicas = int32(mdb.Spec.Members)
+	_ = c.Update(context.TODO(), &sts)
+}

pkg/kube/client/mocked_client.go

@@ -86,10 +86,6 @@ func (m *mockedClient) Update(_ context.Context, obj runtime.Object, _ ...k8sCli
 	if err != nil {
 		return err
 	}
-	switch v := obj.(type) {
-	case *appsv1.StatefulSet:
-		makeStatefulSetReady(v)
-	}
 	relevantMap[objKey] = obj
 	return nil
 }

test/e2e/e2eutil.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/statefulset"
+
 	mdbv1 "github.com/mongodb/mongodb-kubernetes-operator/pkg/apis/mongodb/v1"
 	f "github.com/operator-framework/operator-sdk/pkg/test"
 	appsv1 "k8s.io/api/apps/v1"
@@ -77,15 +79,7 @@ func WaitForStatefulSetToHaveUpdateStrategy(t *testing.T, mdb *mdbv1.MongoDB, st
 // have reached the ready status
 func WaitForStatefulSetToBeReady(t *testing.T, mdb *mdbv1.MongoDB, retryInterval, timeout time.Duration) error {
 	return waitForStatefulSetCondition(t, mdb, retryInterval, timeout, func(sts appsv1.StatefulSet) bool {
-		return sts.Status.ReadyReplicas == int32(mdb.Spec.Members)
-	})
-}
-
-// waitForStatefulSetToBeUpdated waits until all replicas of the StatefulSet with the given name
-// are updated.
-func WaitForStatefulSetToBeUpdated(t *testing.T, mdb *mdbv1.MongoDB, retryInterval, timeout time.Duration) error {
-	return waitForStatefulSetCondition(t, mdb, retryInterval, timeout, func(sts appsv1.StatefulSet) bool {
-		return sts.Status.UpdatedReplicas == int32(mdb.Spec.Members)
+		return statefulset.IsReady(sts, mdb.Spec.Members)
 	})
 }
 

test/e2e/feature_compatibility_version/feature_compatibility_version_test.go

@@ -31,7 +31,7 @@ func TestFeatureCompatibilityVersion(t *testing.T) {
 	t.Run("MongoDB is reachable while version is upgraded", mongodbtests.IsReachableDuring(&mdb, time.Second*10,
 		func() {
 			t.Run("Test Version can be upgraded", mongodbtests.ChangeVersion(&mdb, "4.2.6"))
-			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsUpdated(&mdb))
+			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsReady(&mdb))
 		},
 	))
 	t.Run("Test Basic Connectivity after upgrade has completed", mongodbtests.BasicConnectivity(&mdb))
@@ -41,7 +41,7 @@ func TestFeatureCompatibilityVersion(t *testing.T) {
 	t.Run("MongoDB is reachable while version is downgraded", mongodbtests.IsReachableDuring(&mdb, time.Second*10,
 		func() {
 			t.Run("Test Version can be downgraded", mongodbtests.ChangeVersion(&mdb, "4.0.6"))
-			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsUpdated(&mdb))
+			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsReady(&mdb))
 		},
 	))
 	t.Run("Test FeatureCompatibilityVersion, after downgrade, is 4.0", mongodbtests.HasFeatureCompatibilityVersion(&mdb, "4.0", 3))

test/e2e/feature_compatibility_version_upgrade/feature_compatibility_version_upgrade_test.go

@@ -33,7 +33,7 @@ func TestFeatureCompatibilityVersionUpgrade(t *testing.T) {
 	t.Run("MongoDB is reachable", mongodbtests.IsReachableDuring(&mdb, time.Second*10,
 		func() {
 			t.Run("Test Version can be upgraded", mongodbtests.ChangeVersion(&mdb, "4.2.6"))
-			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsUpdated(&mdb))
+			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsReady(&mdb))
 			t.Run("Test Basic Connectivity after upgrade has completed", mongodbtests.BasicConnectivity(&mdb))
 		},
 	))
@@ -47,7 +47,7 @@ func TestFeatureCompatibilityVersionUpgrade(t *testing.T) {
 				})
 				assert.NoError(t, err)
 			})
-			t.Run("Stateful Set Reaches Ready State", mongodbtests.StatefulSetIsUpdated(&mdb))
+			t.Run("Stateful Set Reaches Ready State", mongodbtests.StatefulSetIsReady(&mdb))
 			t.Run("MongoDB Reaches Running Phase", mongodbtests.MongoDBReachesRunningPhase(&mdb))
 		},
 	))

test/e2e/mongodbtests/mongodbtests.go

@@ -60,23 +60,11 @@ func StatefulSetHasOwnerReference(mdb *mdbv1.MongoDB, expectedOwnerReference met
 	}
 }
 
-// StatefulSetIsUpdated ensures that all the Pods of the StatefulSet are
-// in "Updated" condition.
-func StatefulSetIsUpdated(mdb *mdbv1.MongoDB) func(t *testing.T) {
-	return func(t *testing.T) {
-		err := e2eutil.WaitForStatefulSetToBeUpdated(t, mdb, time.Second*15, time.Minute*5)
-		if err != nil {
-			t.Fatal(err)
-		}
-		t.Logf("StatefulSet %s/%s is updated!", mdb.Namespace, mdb.Name)
-	}
-}
-
 // StatefulSetHasUpdateStrategy verifies that the StatefulSet holding this MongoDB
 // resource has the correct Update Strategy
 func StatefulSetHasUpdateStrategy(mdb *mdbv1.MongoDB, strategy appsv1.StatefulSetUpdateStrategyType) func(t *testing.T) {
 	return func(t *testing.T) {
-		err := e2eutil.WaitForStatefulSetToHaveUpdateStrategy(t, mdb, appsv1.RollingUpdateStatefulSetStrategyType, time.Second*15, time.Minute*5)
+		err := e2eutil.WaitForStatefulSetToHaveUpdateStrategy(t, mdb, strategy, time.Second*15, time.Minute*5)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -142,8 +130,6 @@ func HasFeatureCompatibilityVersion(mdb *mdbv1.MongoDB, fcv string, tries int) f
 			case <-time.After(10 * time.Second):
 				var result bson.M
 				err = database.RunCommand(ctx, runCommand).Decode(&result)
-				assert.NoError(t, err)
-
 				expected := primitive.M{"version": fcv}
 				if reflect.DeepEqual(expected, result["featureCompatibilityVersion"]) {
 					found = true

test/e2e/replica_set_change_version/replica_set_test.go

@@ -43,9 +43,8 @@ func TestReplicaSetUpgradeVersion(t *testing.T) {
 		func() {
 			t.Run("Test Version can be upgraded", mongodbtests.ChangeVersion(&mdb, "4.0.8"))
 			t.Run("StatefulSet has OnDelete update strategy", mongodbtests.StatefulSetHasUpdateStrategy(&mdb, appsv1.OnDeleteStatefulSetStrategyType))
-			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsUpdated(&mdb))
+			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsReady(&mdb))
 			t.Run("AutomationConfig's version has been increased", mongodbtests.AutomationConfigVersionHasTheExpectedVersion(&mdb, 2))
-
 		},
 	))
 	t.Run("StatefulSet has RollingUpgrade restart strategy", mongodbtests.StatefulSetHasUpdateStrategy(&mdb, appsv1.RollingUpdateStatefulSetStrategyType))
@@ -55,9 +54,8 @@ func TestReplicaSetUpgradeVersion(t *testing.T) {
 		func() {
 			t.Run("Test Version can be downgraded", mongodbtests.ChangeVersion(&mdb, "4.0.6"))
 			t.Run("StatefulSet has OnDelete restart strategy", mongodbtests.StatefulSetHasUpdateStrategy(&mdb, appsv1.OnDeleteStatefulSetStrategyType))
-			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsUpdated(&mdb))
+			t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetIsReady(&mdb))
 			t.Run("AutomationConfig's version has been increased", mongodbtests.AutomationConfigVersionHasTheExpectedVersion(&mdb, 3))
-
 		},
 	))
 	t.Run("StatefulSet has RollingUpgrade restart strategy", mongodbtests.StatefulSetHasUpdateStrategy(&mdb, appsv1.RollingUpdateStatefulSetStrategyType))