CLOUDP-86187: cluster update (replicationSpecs) (#188)

Author: Anton

pkg/controller/atlascluster/cluster.go

@@ -109,14 +109,7 @@ func MergedCluster(atlasCluster mongodbatlas.Cluster, spec mdbv1.AtlasClusterSpe
 		return
 	}
 
-	// TODO: might need to do this with other slices
-	if err = compat.JSONSliceMerge(&result.ReplicationSpecs, atlasCluster.ReplicationSpecs); err != nil {
-		return
-	}
-
-	if err = compat.JSONSliceMerge(&result.ReplicationSpecs, spec.ReplicationSpecs); err != nil {
-		return
-	}
+	mergeRegionConfigs(result.ReplicationSpecs, spec.ReplicationSpecs)
 
 	// According to the docs for 'providerSettings.regionName' (https://docs.atlas.mongodb.com/reference/api/clusters-create-one/):
 	// "Don't specify this parameter when creating a multi-region cluster using the replicationSpec object or a Global
@@ -130,6 +123,26 @@ func MergedCluster(atlasCluster mongodbatlas.Cluster, spec mdbv1.AtlasClusterSpe
 	return
 }
 
+// mergeRegionConfigs removes replicationSpecs[i].RegionsConfigs[key] from Atlas Cluster that are absent in Operator.
+// Dev idea: this could have been added into some more generic method like `JSONCopy` or something wrapping it to make
+// sure any Atlas map get redundant keys removed. So far there's only one map in Cluster ('RegionsConfig') so we'll do this
+// explicitly - but may make sense to refactor this later if more maps are added (and all follow the same logic).
+func mergeRegionConfigs(atlasSpecs []mongodbatlas.ReplicationSpec, operatorSpecs []mdbv1.ReplicationSpec) {
+	for i, operatorSpec := range operatorSpecs {
+		if len(operatorSpec.RegionsConfig) == 0 {
+			// Edge case: if the operator doesn't specify regions configs - Atlas will put the default ones. We shouldn't
+			// remove it in this case.
+			continue
+		}
+		atlasSpec := atlasSpecs[i]
+		for key := range atlasSpec.RegionsConfig {
+			if _, ok := operatorSpec.RegionsConfig[key]; !ok {
+				delete(atlasSpec.RegionsConfig, key)
+			}
+		}
+	}
+}
+
 // ClustersEqual compares two Atlas Clusters
 func ClustersEqual(log *zap.SugaredLogger, clusterAtlas mongodbatlas.Cluster, clusterOperator mongodbatlas.Cluster) bool {
 	d := cmp.Diff(clusterAtlas, clusterOperator, cmpopts.EquateEmpty())

pkg/controller/atlascluster/cluster_test.go

@@ -10,6 +10,11 @@ import (
 	mdbv1 "github.com/mongodb/mongodb-atlas-kubernetes/pkg/api/v1"
 )
 
+func init() {
+	logger, _ := zap.NewDevelopment()
+	zap.ReplaceGlobals(logger)
+}
+
 func TestClusterMatchesSpec(t *testing.T) {
 	t.Run("Clusters match (enums)", func(t *testing.T) {
 		atlasCluster := mongodbatlas.Cluster{
@@ -82,6 +87,10 @@ func TestClusterMatchesSpec(t *testing.T) {
 				"US_EAST": {AnalyticsNodes: int64ptr(0), ElectableNodes: int64ptr(3), Priority: int64ptr(7), ReadOnlyNodes: int64ptr(0)}}},
 		}
 		operatorCluster := mdbv1.DefaultAWSCluster("test-ns", "project-name")
+		operatorCluster.Spec.ReplicationSpecs = []mdbv1.ReplicationSpec{{
+			NumShards: int64ptr(1),
+			ZoneName:  "zone1",
+		}}
 
 		merged, err := MergedCluster(*atlasCluster, operatorCluster.Spec)
 		assert.NoError(t, err)
@@ -120,6 +129,43 @@ func TestClusterMatchesSpec(t *testing.T) {
 		equal := ClustersEqual(zap.S(), *atlasCluster, merged)
 		assert.False(t, equal)
 	})
+
+	t.Run("Clusters don't match - Operator removed the region", func(t *testing.T) {
+		atlasCluster, err := mdbv1.DefaultAWSCluster("test-ns", "project-name").Spec.Cluster()
+		assert.NoError(t, err)
+		atlasCluster.ReplicationSpecs = []mongodbatlas.ReplicationSpec{{
+			ID:        "id",
+			NumShards: int64ptr(1),
+			ZoneName:  "zone1",
+			RegionsConfig: map[string]mongodbatlas.RegionsConfig{
+				"US_EAST": {AnalyticsNodes: int64ptr(0), ElectableNodes: int64ptr(3), Priority: int64ptr(7), ReadOnlyNodes: int64ptr(0)},
+				"US_WEST": {AnalyticsNodes: int64ptr(2), ElectableNodes: int64ptr(5), Priority: int64ptr(6), ReadOnlyNodes: int64ptr(0)},
+			}},
+		}
+		operatorCluster := mdbv1.DefaultAWSCluster("test-ns", "project-name")
+		operatorCluster.Spec.ReplicationSpecs = []mdbv1.ReplicationSpec{{
+			NumShards: int64ptr(1),
+			ZoneName:  "zone1",
+			RegionsConfig: map[string]mdbv1.RegionsConfig{
+				"US_EAST": {AnalyticsNodes: int64ptr(0), ElectableNodes: int64ptr(3), Priority: int64ptr(7), ReadOnlyNodes: int64ptr(0)},
+			}},
+		}
+
+		merged, err := MergedCluster(*atlasCluster, operatorCluster.Spec)
+		assert.NoError(t, err)
+
+		expectedReplicationSpecs := []mongodbatlas.ReplicationSpec{{
+			ID:        "id",
+			NumShards: int64ptr(1),
+			ZoneName:  "zone1",
+			RegionsConfig: map[string]mongodbatlas.RegionsConfig{
+				"US_EAST": {AnalyticsNodes: int64ptr(0), ElectableNodes: int64ptr(3), Priority: int64ptr(7), ReadOnlyNodes: int64ptr(0)}}},
+		}
+		assert.Equal(t, expectedReplicationSpecs, merged.ReplicationSpecs)
+
+		equal := ClustersEqual(zap.S(), *atlasCluster, merged)
+		assert.False(t, equal)
+	})
 }
 func int64ptr(i int64) *int64 {
 	return &i

test/int/cluster_test.go

@@ -280,6 +280,7 @@ var _ = Describe("AtlasCluster", func() {
 				// Apart from 'ID' all other fields are equal to the ones sent by the Operator
 				Expect(c.ReplicationSpecs).To(Equal(expectedReplicationSpecs))
 			}
+
 			By("Creating the Cluster", func() {
 				Expect(k8sClient.Create(context.Background(), createdCluster)).To(Succeed())
 
@@ -290,6 +291,26 @@ var _ = Describe("AtlasCluster", func() {
 
 				checkAtlasState(replicationSpecsCheckFunc)
 			})
+
+			By("Updating the cluster (multiple operations)", func() {
+				delete(createdCluster.Spec.ReplicationSpecs[0].RegionsConfig, "US_WEST_2")
+				createdCluster.Spec.ReplicationSpecs[0].RegionsConfig["US_WEST_1"] = mdbv1.RegionsConfig{AnalyticsNodes: int64ptr(0), ElectableNodes: int64ptr(2), Priority: int64ptr(6), ReadOnlyNodes: int64ptr(0)}
+				config := createdCluster.Spec.ReplicationSpecs[0].RegionsConfig["US_EAST_1"]
+				// Note, that Atlas has strict requirements to priorities - they must start with 7 and be in descending order over the regions
+				config.Priority = int64ptr(7)
+				createdCluster.Spec.ReplicationSpecs[0].RegionsConfig["US_EAST_1"] = config
+
+				createdCluster.Spec.ProviderSettings.AutoScaling.Compute.MaxInstanceSize = "M30"
+
+				performUpdate(80 * time.Minute)
+
+				Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterUpdatingFunc()),
+					1800, interval).Should(BeTrue())
+
+				doCommonStatusChecks()
+
+				checkAtlasState(replicationSpecsCheckFunc)
+			})
 		})
 	})