CLOUDP-226369: Fix instance size updates when autoscale ON (#1351)

Signed-off-by: jose.vazquez <[email protected]>

Author: josvazg

.github/workflows/lint.yaml

@@ -1,5 +1,4 @@
 # Check for every push
-
 name: Lint
 
 on:
@@ -21,20 +20,10 @@ jobs:
         uses: actions/setup-go@v4
         with:
           go-version-file: "${{ github.workspace }}/go.mod"
+          cache: false
 
-      - name: golangci-lint
-        uses: golangci/[email protected]
-        with:
-          # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
-          version: v1.53.3
-
-          # Optional: working directory, useful for monorepos
-          # working-directory:
-
-          args: --timeout 10m
-          # Optional: show only new issues if it's a pull request. The default value is `false`.
-          # only-new-issues: true
-
+      - name: lint
+        run: make lint
 
       - name: Run ShellCheck
         uses: bewuethr/shellcheck-action@v2

Makefile

@@ -79,6 +79,9 @@ OPERATOR_NAMESPACE = mongodb-atlas-system
 ATLAS_DOMAIN = https://cloud-qa.mongodb.com/
 ATLAS_KEY_SECRET_NAME = mongodb-atlas-operator-api-key
 
+# golangci-lint
+GOLANGCI_LINT_VERSION := v1.54.2
+
 .DEFAULT_GOAL := help
 .PHONY: help
 help: ## Show this help screen
@@ -155,9 +158,13 @@ $(TIMESTAMPS_DIR)/manifests: $(GO_SOURCES)
 manifests: CRD_OPTIONS ?= "crd:crdVersions=v1"
 manifests: fmt controller-gen $(TIMESTAMPS_DIR)/manifests ## Generate manifests e.g. CRD, RBAC etc.
 
+golangci-lint:
+	@which $@ >/dev/null 2>&1 || \
+	go install "github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION)"
+
 .PHONY: lint
-lint:
-	golangci-lint run
+lint: golangci-lint
+	golangci-lint run --timeout 10m
 
 $(TIMESTAMPS_DIR)/fmt: $(GO_SOURCES)
 	@echo "goimports -local github.com/mongodb/mongodb-atlas-kubernetes -l -w \$$(GO_SOURCES)"

major-version

@@ -0,0 +1 @@
+1

pkg/controller/atlasdeployment/advanced_deployment.go

@@ -83,7 +83,7 @@ func advancedDeploymentIdle(ctx *workflow.Context, project *mdbv1.AtlasProject,
 		return atlasDeploymentAsAtlas, workflow.Terminate(workflow.Internal, err.Error())
 	}
 
-	if areEqual, _ := AdvancedDeploymentsEqual(ctx.Log, specDeployment, atlasDeployment); areEqual {
+	if areEqual, _ := AdvancedDeploymentsEqual(ctx.Log, &specDeployment, &atlasDeployment); areEqual {
 		return atlasDeploymentAsAtlas, workflow.OK()
 	}
 
@@ -192,18 +192,27 @@ func convertDiskSizeField(result *mdbv1.AdvancedDeploymentSpec, atlas *mongodbat
 }
 
 // AdvancedDeploymentsEqual compares two Atlas Advanced Deployments
-func AdvancedDeploymentsEqual(log *zap.SugaredLogger, deploymentOperator mdbv1.AdvancedDeploymentSpec, deploymentAtlas mdbv1.AdvancedDeploymentSpec) (areEqual bool, diff string) {
-	deploymentAtlas = cleanupFieldsToCompare(deploymentAtlas, deploymentOperator)
-
-	d := cmp.Diff(deploymentAtlas, deploymentOperator, cmpopts.EquateEmpty(), cmpopts.SortSlices(mdbv1.LessAD))
+func AdvancedDeploymentsEqual(log *zap.SugaredLogger, deploymentOperator *mdbv1.AdvancedDeploymentSpec, deploymentAtlas *mdbv1.AdvancedDeploymentSpec) (areEqual bool, diff string) {
+	expected := deploymentOperator.DeepCopy()
+	actualCleaned := cleanupFieldsToCompare(deploymentAtlas.DeepCopy(), expected)
+
+	// Ignore differences on auto-scaled region configs
+	for _, rs := range expected.ReplicationSpecs {
+		for _, rc := range rs.RegionConfigs {
+			if isComputeAutoScalingEnabled(rc.AutoScaling) {
+				ignoreInstanceSize(rc)
+			}
+		}
+	}
+	d := cmp.Diff(actualCleaned, expected, cmpopts.EquateEmpty(), cmpopts.SortSlices(mdbv1.LessAD))
 	if d != "" {
 		log.Debugf("Deployments are different: %s", d)
 	}
 
 	return d == "", d
 }
 
-func cleanupFieldsToCompare(atlas, operator mdbv1.AdvancedDeploymentSpec) mdbv1.AdvancedDeploymentSpec {
+func cleanupFieldsToCompare(atlas, operator *mdbv1.AdvancedDeploymentSpec) *mdbv1.AdvancedDeploymentSpec {
 	if atlas.ReplicationSpecs == nil {
 		return atlas
 	}
@@ -231,6 +240,10 @@ func cleanupFieldsToCompare(atlas, operator mdbv1.AdvancedDeploymentSpec) mdbv1.
 					regionConfig.ReadOnlySpecs = operator.ReplicationSpecs[specIdx].RegionConfigs[configIdx].ReadOnlySpecs
 				}
 			}
+
+			if isComputeAutoScalingEnabled(regionConfig.AutoScaling) {
+				ignoreInstanceSize(regionConfig)
+			}
 		}
 	}
 

pkg/controller/atlasdeployment/advanced_deployment_test.go

@@ -50,15 +50,78 @@ func TestAdvancedDeploymentsEqual(t *testing.T) {
 	regionConfig := defaultAtlas.ReplicationSpecs[0].RegionConfigs[0]
 	fillInSpecs(regionConfig, "M10", "AWS")
 
-	t.Run("Test ", func(t *testing.T) {
+	t.Run("Test equal advanced deployments", func(t *testing.T) {
 		advancedCluster := mdbv1.DefaultAwsAdvancedDeployment("default", "my-project")
 
 		merged, atlas, err := MergedAdvancedDeployment(*defaultAtlas, *advancedCluster.Spec.AdvancedDeploymentSpec)
 		assert.NoError(t, err)
+		beforeSpec := merged.DeepCopy()
+		beforeAtlas := atlas.DeepCopy()
 
 		logger, _ := zap.NewProduction()
-		areEqual, _ := AdvancedDeploymentsEqual(logger.Sugar(), merged, atlas)
+		areEqual, _ := AdvancedDeploymentsEqual(logger.Sugar(), &merged, &atlas)
 		assert.True(t, areEqual, "Deployments should be equal")
+		assert.Equal(t, beforeSpec, &merged, "Comparison should not change original spec values")
+		assert.Equal(t, beforeAtlas, &atlas, "Comparison should not change original atlas values")
+	})
+
+	t.Run("Advanced deployments are equal when autoscaling is ON and only differ on instance sizes", func(t *testing.T) {
+		advancedCluster := mdbv1.DefaultAwsAdvancedDeployment("default", "my-project")
+		// set auto scaling ON
+		advancedCluster.Spec.AdvancedDeploymentSpec.ReplicationSpecs[0].RegionConfigs[0].AutoScaling = &mdbv1.AdvancedAutoScalingSpec{
+			DiskGB: &mdbv1.DiskGB{
+				Enabled: toptr.MakePtr(false),
+			},
+			Compute: &mdbv1.ComputeSpec{
+				Enabled:          toptr.MakePtr(true),
+				ScaleDownEnabled: toptr.MakePtr(true),
+				MinInstanceSize:  "M10",
+				MaxInstanceSize:  "M30",
+			},
+		}
+
+		merged, atlas, err := MergedAdvancedDeployment(*defaultAtlas, *advancedCluster.Spec.AdvancedDeploymentSpec)
+		// copy autoscaling to atlas
+		k8sRegion := advancedCluster.Spec.AdvancedDeploymentSpec.ReplicationSpecs[0].RegionConfigs[0]
+		atlas.ReplicationSpecs[0].RegionConfigs[0].AutoScaling = &mdbv1.AdvancedAutoScalingSpec{
+			DiskGB: &mdbv1.DiskGB{
+				Enabled: k8sRegion.AutoScaling.DiskGB.Enabled,
+			},
+			Compute: &mdbv1.ComputeSpec{
+				Enabled:          k8sRegion.AutoScaling.Compute.Enabled,
+				ScaleDownEnabled: k8sRegion.AutoScaling.Compute.ScaleDownEnabled,
+				MinInstanceSize:  k8sRegion.AutoScaling.Compute.MinInstanceSize,
+				MaxInstanceSize:  k8sRegion.AutoScaling.Compute.MaxInstanceSize,
+			},
+		}
+		// inject difference
+		atlas.ReplicationSpecs[0].RegionConfigs[0].ElectableSpecs.InstanceSize = "something-else"
+		assert.NoError(t, err)
+		beforeSpec := merged.DeepCopy()
+		beforeAtlas := atlas.DeepCopy()
+
+		logger, _ := zap.NewProduction()
+		areEqual, _ := AdvancedDeploymentsEqual(logger.Sugar(), &merged, &atlas)
+		assert.True(t, areEqual, "Deployments should be equal")
+		assert.Equal(t, beforeSpec, &merged, "Comparison should not change original spec values")
+		assert.Equal(t, beforeAtlas, &atlas, "Comparison should not change original atlas values")
+	})
+
+	t.Run("Advanced deployments are different when autoscaling is OFF and only differ on instance sizes", func(t *testing.T) {
+		advancedCluster := mdbv1.DefaultAwsAdvancedDeployment("default", "my-project")
+
+		merged, atlas, err := MergedAdvancedDeployment(*defaultAtlas, *advancedCluster.Spec.AdvancedDeploymentSpec)
+		// inject difference
+		atlas.ReplicationSpecs[0].RegionConfigs[0].ElectableSpecs.InstanceSize = "something-else"
+		assert.NoError(t, err)
+		beforeSpec := merged.DeepCopy()
+		beforeAtlas := atlas.DeepCopy()
+
+		logger, _ := zap.NewProduction()
+		areEqual, _ := AdvancedDeploymentsEqual(logger.Sugar(), &merged, &atlas)
+		assert.False(t, areEqual, "Deployments should be different")
+		assert.Equal(t, beforeSpec, &merged, "Comparison should not change original spec values")
+		assert.Equal(t, beforeAtlas, &atlas, "Comparison should not change original atlas values")
 	})
 }
 

pkg/controller/atlasdeployment/deployment.go

@@ -58,14 +58,14 @@ func convertLegacyReplicationSpecs(legacy *mdbv1.DeploymentSpec) ([]*mdbv1.Advan
 		fillDefaultReplicationSpec(legacy)
 	}
 
-	for _, legacyResplicationSpec := range legacy.ReplicationSpecs {
-		resplicationSpec := &mdbv1.AdvancedReplicationSpec{
-			NumShards:     *convertLegacyInt64(legacyResplicationSpec.NumShards),
-			ZoneName:      legacyResplicationSpec.ZoneName,
+	for _, legacyReplicationSpec := range legacy.ReplicationSpecs {
+		replicationSpec := &mdbv1.AdvancedReplicationSpec{
+			NumShards:     *convertLegacyInt64(legacyReplicationSpec.NumShards),
+			ZoneName:      legacyReplicationSpec.ZoneName,
 			RegionConfigs: []*mdbv1.AdvancedRegionConfig{},
 		}
 
-		for legacyRegion, legacyRegionConfig := range legacyResplicationSpec.RegionsConfig {
+		for legacyRegion, legacyRegionConfig := range legacyReplicationSpec.RegionsConfig {
 			regionConfig := mdbv1.AdvancedRegionConfig{
 				AnalyticsSpecs: &mdbv1.Specs{
 					DiskIOPS:      legacy.ProviderSettings.DiskIOPS,
@@ -92,10 +92,10 @@ func convertLegacyReplicationSpecs(legacy *mdbv1.DeploymentSpec) ([]*mdbv1.Advan
 				RegionName:          legacyRegion,
 			}
 
-			resplicationSpec.RegionConfigs = append(resplicationSpec.RegionConfigs, &regionConfig)
+			replicationSpec.RegionConfigs = append(replicationSpec.RegionConfigs, &regionConfig)
 		}
 
-		result = append(result, resplicationSpec)
+		result = append(result, replicationSpec)
 	}
 
 	return result, nil

pkg/controller/atlasdeployment/region_configuration.go

@@ -39,9 +39,20 @@ func syncRegionConfiguration(deploymentSpec *mdbv1.AdvancedDeploymentSpec, atlas
 		// When compute auto-scaling is enabled, unset instance size to avoid override production workload
 		if isComputeAutoScalingEnabled(regionSpec.AutoScaling) {
 			if !regionsHasChanged {
-				regionSpec.ElectableSpecs.InstanceSize = ""
-				regionSpec.ReadOnlySpecs.InstanceSize = ""
-				regionSpec.AnalyticsSpecs.InstanceSize = ""
+				for _, arc := range atlasCluster.ReplicationSpecs[0].RegionConfigs {
+					if arc.RegionName == regionSpec.RegionName {
+						if regionSpec.ElectableSpecs != nil && arc.ElectableSpecs != nil {
+							regionSpec.ElectableSpecs.InstanceSize = arc.ElectableSpecs.InstanceSize
+						}
+						if regionSpec.ReadOnlySpecs != nil && arc.ReadOnlySpecs != nil {
+							regionSpec.ReadOnlySpecs.InstanceSize = arc.ReadOnlySpecs.InstanceSize
+						}
+						if regionSpec.AnalyticsSpecs != nil && arc.AnalyticsSpecs != nil {
+							regionSpec.AnalyticsSpecs.InstanceSize = arc.AnalyticsSpecs.InstanceSize
+						}
+						break
+					}
+				}
 			}
 		} else {
 			if regionSpec.AutoScaling != nil {
@@ -86,28 +97,55 @@ func regionsConfigHasChanged(deploymentRegions []*mdbv1.AdvancedRegionConfig, at
 
 	mapDeploymentRegions := map[string]*mdbv1.AdvancedRegionConfig{}
 	for _, region := range deploymentRegions {
+		// do not compare instance sizes when autoscaling is ON
+		if isComputeAutoScalingEnabled(region.AutoScaling) {
+			region = ignoreInstanceSize(region.DeepCopy())
+		}
 		mapDeploymentRegions[region.RegionName] = region
 	}
 
 	for _, region := range atlasRegions {
-		if _, ok := mapDeploymentRegions[region.RegionName]; !ok {
+		k8sRegion, ok := mapDeploymentRegions[region.RegionName]
+		if !ok {
 			return true
 		}
 
 		var atlasAsOperatorRegion mdbv1.AdvancedRegionConfig
-		err := compat.JSONCopy(&atlasAsOperatorRegion, region)
+		var atlasRegion = &atlasAsOperatorRegion
+		err := compat.JSONCopy(atlasRegion, region)
 		if err != nil {
 			return true
 		}
 
-		if cmp.Diff(mapDeploymentRegions[region.RegionName], &atlasAsOperatorRegion) != "" {
+		// do not compare instance sizes when autoscaling is ON
+		if isComputeAutoScalingEnabled(k8sRegion.AutoScaling) {
+			atlasRegion = ignoreInstanceSize(atlasRegion.DeepCopy())
+		}
+
+		if diff := cmp.Diff(k8sRegion, atlasRegion); diff != "" {
 			return true
 		}
 	}
 
 	return false
 }
 
+func ignoreInstanceSize(rc *mdbv1.AdvancedRegionConfig) *mdbv1.AdvancedRegionConfig {
+	if rc == nil {
+		return rc
+	}
+	if rc.ElectableSpecs != nil {
+		rc.ElectableSpecs.InstanceSize = ""
+	}
+	if rc.ReadOnlySpecs != nil {
+		rc.ReadOnlySpecs.InstanceSize = ""
+	}
+	if rc.AnalyticsSpecs != nil {
+		rc.AnalyticsSpecs.InstanceSize = ""
+	}
+	return rc
+}
+
 func normalizeSpecs(regions []*mdbv1.AdvancedRegionConfig) {
 	for _, region := range regions {
 		if region == nil {