diff --git a/pkg/controller/atlascluster/cluster.go b/pkg/controller/atlascluster/cluster.go index 8ef83581d4..753d452a86 100644 --- a/pkg/controller/atlascluster/cluster.go +++ b/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()) diff --git a/pkg/controller/atlascluster/cluster_test.go b/pkg/controller/atlascluster/cluster_test.go index d9e9c91947..45f73e2630 100644 --- a/pkg/controller/atlascluster/cluster_test.go +++ b/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 diff --git a/test/int/cluster_test.go b/test/int/cluster_test.go index 8192385522..f5d2e92da3 100644 --- a/test/int/cluster_test.go +++ b/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) + }) }) })