Changed FCV handling when upgrading (#412)

Co-authored-by: Cian Hatton <cianhatton@gmail.com>

Author: Nikolas De Giorgis

api/v1/mongodbcommunity_types.go

@@ -483,18 +483,6 @@ func (m MongoDBCommunity) GetAgentScramCredentialsNamespacedName() types.Namespa
 	return types.NamespacedName{Name: fmt.Sprintf("%s-agent-scram-credentials", m.Name), Namespace: m.Namespace}
 }
 
-// GetFCV returns the feature compatibility version. If no FeatureCompatibilityVersion is specified.
-// It uses the major and minor version for whichever version of MongoDB is configured.
-func (m MongoDBCommunity) GetFCV() string {
-	versionToSplit := m.Spec.FeatureCompatibilityVersion
-	if versionToSplit == "" {
-		versionToSplit = m.Spec.Version
-	}
-	minorIndex := 1
-	parts := strings.Split(versionToSplit, ".")
-	return strings.Join(parts[:minorIndex+1], ".")
-}
-
 func (m MongoDBCommunity) DesiredReplicas() int {
 	return m.Spec.Members
 }

api/v1/mongodbcommunity_types_test.go

@@ -16,18 +16,6 @@ func TestMongoDB_MongoURI(t *testing.T) {
 	assert.Equal(t, mdb.MongoURI(), "mongodb://my-big-rs-0.my-big-rs-svc.my-big-namespace.svc.cluster.local:27017,my-big-rs-1.my-big-rs-svc.my-big-namespace.svc.cluster.local:27017,my-big-rs-2.my-big-rs-svc.my-big-namespace.svc.cluster.local:27017,my-big-rs-3.my-big-rs-svc.my-big-namespace.svc.cluster.local:27017,my-big-rs-4.my-big-rs-svc.my-big-namespace.svc.cluster.local:27017")
 }
 
-func TestGetFCV(t *testing.T) {
-	mdb := newReplicaSet(3, "my-rs", "my-ns")
-	mdb.Spec.Version = "4.2.0"
-	assert.Equal(t, "4.2", mdb.GetFCV())
-
-	mdb.Spec.FeatureCompatibilityVersion = "4.0"
-	assert.Equal(t, "4.0", mdb.GetFCV())
-
-	mdb.Spec.FeatureCompatibilityVersion = ""
-	assert.Equal(t, "4.2", mdb.GetFCV())
-}
-
 func TestGetScramCredentialsSecretName(t *testing.T) {
 	testusers := []struct {
 		in  MongoDBUser

controllers/replica_set_controller.go

@@ -413,7 +413,6 @@ func buildAutomationConfig(mdb mdbv1.MongoDBCommunity, auth automationconfig.Aut
 	domain := getDomain(mdb.ServiceName(), mdb.Namespace, os.Getenv(clusterDNSName))
 	zap.S().Debugw("AutomationConfigMembersThisReconciliation", "mdb.AutomationConfigMembersThisReconciliation()", mdb.AutomationConfigMembersThisReconciliation())
 
-	fcv := mdb.GetFCV()
 	return automationconfig.NewBuilder().
 		SetTopology(automationconfig.ReplicaSetTopology).
 		SetName(mdb.Name).
@@ -422,7 +421,7 @@ func buildAutomationConfig(mdb mdbv1.MongoDBCommunity, auth automationconfig.Aut
 		SetReplicaSetHorizons(mdb.Spec.ReplicaSetHorizons).
 		SetPreviousAutomationConfig(currentAc).
 		SetMongoDBVersion(mdb.Spec.Version).
-		SetFCV(&fcv).
+		SetFCV(mdb.Spec.FeatureCompatibilityVersion).
 		SetOptions(automationconfig.Options{DownloadBase: "/var/lib/mongodb-mms-automation"}).
 		SetAuth(auth).
 		AddModifications(getMongodConfigModification(mdb)).

controllers/replicaset_controller_test.go

@@ -306,6 +306,32 @@ func TestAutomationConfig_versionIsNotBumpedWithNoChanges(t *testing.T) {
 	assert.Equal(t, currentAc.Version, 1)
 }
 
+func TestAutomationConfigFCVIsNotIncreasedWhenUpgradingMinorVersion(t *testing.T) {
+	mdb := newTestReplicaSet()
+	mgr := client.NewManager(&mdb)
+	r := NewReconciler(mgr)
+	res, err := r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
+	assertReconciliationSuccessful(t, res, err)
+
+	currentAc, err := automationconfig.ReadFromSecret(mgr.Client, types.NamespacedName{Name: mdb.AutomationConfigSecretName(), Namespace: mdb.Namespace})
+	assert.NoError(t, err)
+	assert.Len(t, currentAc.Processes, 3)
+	assert.Equal(t, currentAc.Processes[0].FeatureCompatibilityVersion, "4.2")
+
+	// Upgrading minor version does not change the FCV on the automationConfig
+	mdbRef := &mdb
+	mdbRef.Spec.Version = "4.4.0"
+	_ = mgr.Client.Update(context.TODO(), mdbRef)
+	res, err = r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
+	assertReconciliationSuccessful(t, res, err)
+
+	currentAc, err = automationconfig.ReadFromSecret(mgr.Client, types.NamespacedName{Name: mdb.AutomationConfigSecretName(), Namespace: mdb.Namespace})
+	assert.NoError(t, err)
+	assert.Len(t, currentAc.Processes, 3)
+	assert.Equal(t, currentAc.Processes[0].FeatureCompatibilityVersion, "4.2")
+
+}
+
 func TestAutomationConfig_CustomMongodConfig(t *testing.T) {
 	mdb := newTestReplicaSet()
 

dev_notes/RELEASE_NOTES.md

@@ -1,11 +1,16 @@
 *(Please use the [release template](release-notes-template.md) as the template for this document)*
 <!-- Next release -->
+# MongoDB Kubernetes Operator 0.5.3
+## Kubernetes Operator
+* Bug fixes
+  * Fixes an issue that prevented the agents from reaching goal state when upgrading minor version of MongoDB.
 
+<!-- Past Releases -->
 # MongoDB Kubernetes Operator 0.5.2
 ## Kubernetes Operator
 * Changes
   * Readiness probe has been moved into an init container from the Agent image.
-  * Security context is now added when the `MANAGED_SECURITY_CONTEXT` environment variable is not set. 
+  * Security context is now added when the `MANAGED_SECURITY_CONTEXT` environment variable is not set.
 * Bug fixes
   * Removed unnecessary environment variable configuration in the openshift samples.
   * Fixed an issue where the operator would perform unnecessary reconcilliations when Secrets were modified.
@@ -15,18 +20,16 @@
 
 ## MongoDBCommunity Resource
 * Changes
- * Added `spec.security.authentication.ignoreUnknownUsers` field. This value defaults to `true`. When enabled, 
+ * Added `spec.security.authentication.ignoreUnknownUsers` field. This value defaults to `true`. When enabled,
    any MongoDB users added through external sources will not be removed.
-   
-   
+
+
 ## Miscellaneous
 * Changes
   * Internal code refactorings to allow libraries to be imported into other projects.
- 
- 
+
+
  ## Updated Image Tags
  * mongodb-kubernetes-operator:0.5.2
  * mongodb-agent:10.27.0.6772-1
  * mongodb-kubernetes-readinessprobe:1.0.1 [new image]
-
-<!-- Past Releases -->

pkg/automationconfig/automation_config_builder.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"path"
 
+	"github.com/blang/semver"
 	"github.com/pkg/errors"
 
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/util/versions"
@@ -29,7 +30,7 @@ type Builder struct {
 	members            int
 	domain             string
 	name               string
-	fcv                *string
+	fcv                string
 	topology           Topology
 	mongodbVersion     string
 	previousAC         AutomationConfig
@@ -100,7 +101,7 @@ func (b *Builder) SetName(name string) *Builder {
 	return b
 }
 
-func (b *Builder) SetFCV(fcv *string) *Builder {
+func (b *Builder) SetFCV(fcv string) *Builder {
 	b.fcv = fcv
 	return b
 }
@@ -155,6 +156,33 @@ func (b *Builder) AddModifications(mod ...Modification) *Builder {
 	return b
 }
 
+func (b *Builder) setFeatureCompatibilityVersionIfUpgradeIsHappening() error {
+	// If we are upgrading, we can't increase featureCompatibilityVersion
+	// as that will make the agent never reach goal state
+	if len(b.previousAC.Processes) > 0 && b.fcv == "" {
+
+		// Create a x.y.0 version from FCV x.y
+		previousFCV := b.previousAC.Processes[0].FeatureCompatibilityVersion
+		previousFCVsemver, err := semver.Make(fmt.Sprintf("%s.0", previousFCV))
+		if err != nil {
+			return errors.Errorf("can't compute semver version from previous FeatureCompatibilityVersion %s", previousFCV)
+		}
+
+		currentVersionSemver, err := semver.Make(b.mongodbVersion)
+		if err != nil {
+			return errors.Errorf("current MongoDB version is not a valid semver version: %s", b.mongodbVersion)
+		}
+
+		// We would increase FCV here.
+		// Note: in theory this will also catch upgrade like 4.2.0 -> 4.2.1 but we don't care about those
+		// as they would not change the FCV
+		if currentVersionSemver.GT(previousFCVsemver) {
+			b.fcv = previousFCV
+		}
+	}
+	return nil
+}
+
 func (b *Builder) Build() (AutomationConfig, error) {
 	hostnames := make([]string, b.members)
 	for i := 0; i < b.members; i++ {
@@ -163,6 +191,10 @@ func (b *Builder) Build() (AutomationConfig, error) {
 
 	members := make([]ReplicaSetMember, b.members)
 	processes := make([]Process, b.members)
+
+	if err := b.setFeatureCompatibilityVersionIfUpgradeIsHappening(); err != nil {
+		return AutomationConfig{}, errors.Errorf("can't build the automation config: %s", err)
+	}
 	for i, h := range hostnames {
 
 		process := &Process{
@@ -178,8 +210,8 @@ func (b *Builder) Build() (AutomationConfig, error) {
 			Path:        path.Join(DefaultAgentLogPath, "/mongodb.log"),
 		})
 
-		if b.fcv != nil {
-			process.FeatureCompatibilityVersion = *b.fcv
+		if b.fcv != "" {
+			process.FeatureCompatibilityVersion = b.fcv
 		}
 
 		process.SetPort(27017)

pkg/automationconfig/automation_config_test.go

@@ -25,13 +25,12 @@ func defaultMongoDbVersion(version string) MongoDbVersionConfig {
 }
 
 func TestBuildAutomationConfig(t *testing.T) {
-	fcv := "4.0"
 	ac, err := NewBuilder().
 		SetName("my-rs").
 		SetDomain("my-ns.svc.cluster.local").
 		SetMongoDBVersion("4.2.0").
 		SetMembers(3).
-		SetFCV(&fcv).
+		SetFCV("4.0").
 		Build()
 
 	assert.NoError(t, err)

test/e2e/e2eutil.go

@@ -153,10 +153,9 @@ func NewTestMongoDB(name string, namespace string) (mdbv1.MongoDBCommunity, mdbv
 			Namespace: mongodbNamespace,
 		},
 		Spec: mdbv1.MongoDBCommunitySpec{
-			Members:                     3,
-			Type:                        "ReplicaSet",
-			Version:                     "4.4.0",
-			FeatureCompatibilityVersion: "4.4",
+			Members: 3,
+			Type:    "ReplicaSet",
+			Version: "4.4.0",
 			Security: mdbv1.Security{
 				Authentication: mdbv1.Authentication{
 					Modes: []mdbv1.AuthMode{"SCRAM"},

test/e2e/replica_set_change_version/replica_set_test.go

@@ -32,7 +32,7 @@ func TestReplicaSetUpgradeVersion(t *testing.T) {
 	}
 
 	mdb, user := e2eutil.NewTestMongoDB("mdb0", "")
-	mdb.Spec.Version = "4.4.0"
+	mdb.Spec.Version = "4.2.0"
 
 	_, err := setup.GeneratePasswordForUser(user, ctx, "")
 	if err != nil {
@@ -50,23 +50,23 @@ func TestReplicaSetUpgradeVersion(t *testing.T) {
 	t.Run("AutomationConfig has the correct version", mongodbtests.AutomationConfigVersionHasTheExpectedVersion(&mdb, 1))
 	t.Run("Ensure Authentication", tester.EnsureAuthenticationIsConfigured(3))
 
-	// Upgrade version to 4.4.1
-	t.Run("MongoDB is reachable while version is upgraded", func(t *testing.T) {
+	// Upgrade minor version to 4.4.0
+	t.Run("MongoDB is reachable while minor version is upgraded", func(t *testing.T) {
 		defer tester.StartBackgroundConnectivityTest(t, time.Second*10)()
-		t.Run("Test Version can be upgraded", mongodbtests.ChangeVersion(&mdb, "4.4.1"))
+		t.Run("Test Minor Version can be upgraded", mongodbtests.ChangeVersion(&mdb, "4.4.0"))
 		t.Run("StatefulSet has OnDelete update strategy", mongodbtests.StatefulSetHasUpdateStrategy(&mdb, appsv1.OnDeleteStatefulSetStrategyType))
 		t.Run("Stateful Set Reaches Ready State, after Upgrading", mongodbtests.StatefulSetBecomesReady(&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))
 
-	// Downgrade version back to 4.4.0
-	t.Run("MongoDB is reachable while version is downgraded", func(t *testing.T) {
+	// Upgrade patch version to 4.4.1
+	t.Run("MongoDB is reachable while patch version is upgraded", func(t *testing.T) {
 		defer tester.StartBackgroundConnectivityTest(t, time.Second*10)()
-		t.Run("Test Version can be downgraded", mongodbtests.ChangeVersion(&mdb, "4.4.0"))
+		t.Run("Test Patch Version can be upgraded", mongodbtests.ChangeVersion(&mdb, "4.4.1"))
 		t.Run("StatefulSet has OnDelete restart strategy", mongodbtests.StatefulSetHasUpdateStrategy(&mdb, appsv1.OnDeleteStatefulSetStrategyType))
-		t.Run("Stateful Set Reaches Ready State, after downgrading", mongodbtests.StatefulSetBecomesReady(&mdb))
+		t.Run("Stateful Set Reaches Ready State, after upgrading", mongodbtests.StatefulSetBecomesReady(&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))