From 5da83b17b7984abf7d11613ea58ca79a01fc49f1 Mon Sep 17 00:00:00 2001 From: antonlisovenko Date: Mon, 29 Mar 2021 17:04:45 +0100 Subject: [PATCH 1/5] wip --- test/int/cluster_test.go | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/test/int/cluster_test.go b/test/int/cluster_test.go index 5219be08b6..89f9500c32 100644 --- a/test/int/cluster_test.go +++ b/test/int/cluster_test.go @@ -24,7 +24,7 @@ import ( const ( // Set this to true if you are debugging cluster creation. // This may not help much if there was the update though... - ClusterDevMode = false + ClusterDevMode = true ) var _ = Describe("AtlasCluster", func() { @@ -242,7 +242,7 @@ var _ = Describe("AtlasCluster", func() { }) }) - Describe("Create/Update the cluster (more complex scenario)", func() { + FDescribe("Create/Update the cluster (more complex scenario)", func() { It("Should be created", func() { createdCluster = mdbv1.DefaultAWSCluster(namespace.Name, createdProject.Name) createdCluster.Spec.ClusterType = mdbv1.TypeReplicaSet @@ -280,10 +280,33 @@ 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() { + + // By("Creating the Cluster", func() { + // Expect(k8sClient.Create(context.Background(), createdCluster)).To(Succeed()) + // + // Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterCreatingFunc()), + // 1800, interval).Should(BeTrue()) + // + // doCommonStatusChecks() + // + // 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(5) + createdCluster.Spec.ReplicationSpecs[0].RegionsConfig["US_EAST_1"] = config + + createdCluster.Spec.ProviderSettings.AutoScaling.Compute.MaxInstanceSize = "M30" + Expect(k8sClient.Create(context.Background(), createdCluster)).To(Succeed()) - Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterCreatingFunc()), + // performUpdate(80 * time.Minute) + + Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterUpdatingFunc()), 1800, interval).Should(BeTrue()) doCommonStatusChecks() @@ -485,7 +508,7 @@ var _ = Describe("AtlasCluster", func() { Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterCreatingFunc()), 1800, interval).Should(BeTrue()) - doCommonChecks() + doCommonStatusChecks() checkAtlasState() }) From 6151bb2f072daf682f7bb8aeab5d83764f266d7f Mon Sep 17 00:00:00 2001 From: antonlisovenko Date: Mon, 29 Mar 2021 17:42:54 +0100 Subject: [PATCH 2/5] wip --- pkg/controller/atlascluster/cluster.go | 22 ++++++++--- pkg/controller/atlascluster/cluster_test.go | 42 +++++++++++++++++++++ test/int/cluster_test.go | 30 +++++++-------- 3 files changed, 73 insertions(+), 21 deletions(-) diff --git a/pkg/controller/atlascluster/cluster.go b/pkg/controller/atlascluster/cluster.go index 8ef83581d4..e34059efa7 100644 --- a/pkg/controller/atlascluster/cluster.go +++ b/pkg/controller/atlascluster/cluster.go @@ -109,15 +109,12 @@ 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 // Cluster with the replicationSpecs array." @@ -130,6 +127,21 @@ 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 { + 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..097bfa610f 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{ @@ -120,6 +125,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 89f9500c32..f5d2e92da3 100644 --- a/test/int/cluster_test.go +++ b/test/int/cluster_test.go @@ -24,7 +24,7 @@ import ( const ( // Set this to true if you are debugging cluster creation. // This may not help much if there was the update though... - ClusterDevMode = true + ClusterDevMode = false ) var _ = Describe("AtlasCluster", func() { @@ -242,7 +242,7 @@ var _ = Describe("AtlasCluster", func() { }) }) - FDescribe("Create/Update the cluster (more complex scenario)", func() { + Describe("Create/Update the cluster (more complex scenario)", func() { It("Should be created", func() { createdCluster = mdbv1.DefaultAWSCluster(namespace.Name, createdProject.Name) createdCluster.Spec.ClusterType = mdbv1.TypeReplicaSet @@ -281,30 +281,28 @@ var _ = Describe("AtlasCluster", func() { Expect(c.ReplicationSpecs).To(Equal(expectedReplicationSpecs)) } - // By("Creating the Cluster", func() { - // Expect(k8sClient.Create(context.Background(), createdCluster)).To(Succeed()) - // - // Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterCreatingFunc()), - // 1800, interval).Should(BeTrue()) - // - // doCommonStatusChecks() - // - // checkAtlasState(replicationSpecsCheckFunc) - // }) + By("Creating the Cluster", func() { + Expect(k8sClient.Create(context.Background(), createdCluster)).To(Succeed()) + + Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterCreatingFunc()), + 1800, interval).Should(BeTrue()) + + doCommonStatusChecks() + + 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(5) + config.Priority = int64ptr(7) createdCluster.Spec.ReplicationSpecs[0].RegionsConfig["US_EAST_1"] = config createdCluster.Spec.ProviderSettings.AutoScaling.Compute.MaxInstanceSize = "M30" - Expect(k8sClient.Create(context.Background(), createdCluster)).To(Succeed()) - - // performUpdate(80 * time.Minute) + performUpdate(80 * time.Minute) Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterUpdatingFunc()), 1800, interval).Should(BeTrue()) From a7c18d50faa6134f831244550d61fd6f377bced2 Mon Sep 17 00:00:00 2001 From: antonlisovenko Date: Tue, 30 Mar 2021 09:41:05 +0100 Subject: [PATCH 3/5] fix --- pkg/controller/atlascluster/cluster.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/controller/atlascluster/cluster.go b/pkg/controller/atlascluster/cluster.go index e34059efa7..af7303848e 100644 --- a/pkg/controller/atlascluster/cluster.go +++ b/pkg/controller/atlascluster/cluster.go @@ -133,6 +133,11 @@ func MergedCluster(atlasCluster mongodbatlas.Cluster, spec mdbv1.AtlasClusterSpe // 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 { From 63573609421286b95a22fdc2c60bbdb428b3f9ca Mon Sep 17 00:00:00 2001 From: antonlisovenko Date: Tue, 30 Mar 2021 17:03:55 +0100 Subject: [PATCH 4/5] wip --- pkg/controller/atlascluster/cluster.go | 8 ++++---- pkg/controller/atlascluster/cluster_test.go | 4 ++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/controller/atlascluster/cluster.go b/pkg/controller/atlascluster/cluster.go index af7303848e..b0f57c5b0e 100644 --- a/pkg/controller/atlascluster/cluster.go +++ b/pkg/controller/atlascluster/cluster.go @@ -108,10 +108,10 @@ func MergedCluster(atlasCluster mongodbatlas.Cluster, spec mdbv1.AtlasClusterSpe if err = compat.JSONCopy(&result, spec); err != nil { return } - - if err = compat.JSONSliceMerge(&result.ReplicationSpecs, spec.ReplicationSpecs); err != nil { - return - } + // + // if err = compat.JSONSliceMerge(&result.ReplicationSpecs, spec.ReplicationSpecs); err != nil { + // return + //} mergeRegionConfigs(result.ReplicationSpecs, spec.ReplicationSpecs) diff --git a/pkg/controller/atlascluster/cluster_test.go b/pkg/controller/atlascluster/cluster_test.go index 097bfa610f..45f73e2630 100644 --- a/pkg/controller/atlascluster/cluster_test.go +++ b/pkg/controller/atlascluster/cluster_test.go @@ -87,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) From 16fc2a6a6ce81eed27aeb2650731340210f4cd8e Mon Sep 17 00:00:00 2001 From: antonlisovenko Date: Tue, 30 Mar 2021 21:49:11 +0100 Subject: [PATCH 5/5] wip --- pkg/controller/atlascluster/cluster.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/controller/atlascluster/cluster.go b/pkg/controller/atlascluster/cluster.go index b0f57c5b0e..753d452a86 100644 --- a/pkg/controller/atlascluster/cluster.go +++ b/pkg/controller/atlascluster/cluster.go @@ -108,10 +108,6 @@ func MergedCluster(atlasCluster mongodbatlas.Cluster, spec mdbv1.AtlasClusterSpe if err = compat.JSONCopy(&result, spec); err != nil { return } - // - // if err = compat.JSONSliceMerge(&result.ReplicationSpecs, spec.ReplicationSpecs); err != nil { - // return - //} mergeRegionConfigs(result.ReplicationSpecs, spec.ReplicationSpecs)