Merge pull request #784 from 3scale/enhance-replica-reconciliation

THREESCALE-8754 Enhance replica reconciliation
3scale · Sep 30, 2022 · ca185cb · ca185cb
2 parents 2064fa9 + 9714119
commit ca185cb
Show file tree

Hide file tree

Showing 21 changed files with 409 additions and 256 deletions.
diff --git a/apis/apps/v1alpha1/apimanager_types.go b/apis/apps/v1alpha1/apimanager_types.go
@@ -760,24 +760,9 @@ func (apimanager *APIManager) setApicastSpecDefaults() bool {
 		changed = true
 	}
 
-	if spec.Apicast.StagingSpec.Replicas == nil {
-		spec.Apicast.StagingSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
-	if spec.Apicast.ProductionSpec.Replicas == nil {
-		spec.Apicast.ProductionSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
 	return changed
 }
 
-func (apimanager *APIManager) defaultReplicas() *int64 {
-	var defaultReplicas int64 = 1
-	return &defaultReplicas
-}
-
 func (apimanager *APIManager) setBackendSpecDefaults() bool {
 	changed := false
 	spec := &apimanager.Spec
@@ -802,21 +787,6 @@ func (apimanager *APIManager) setBackendSpecDefaults() bool {
 		changed = true
 	}
 
-	if spec.Backend.ListenerSpec.Replicas == nil {
-		spec.Backend.ListenerSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
-	if spec.Backend.CronSpec.Replicas == nil {
-		spec.Backend.CronSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
-	if spec.Backend.WorkerSpec.Replicas == nil {
-		spec.Backend.WorkerSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
 	return changed
 }
 
@@ -891,16 +861,6 @@ func (apimanager *APIManager) setSystemSpecDefaults() (bool, error) {
 		changed = true
 	}
 
-	if spec.System.AppSpec.Replicas == nil {
-		spec.System.AppSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
-	if spec.System.SidekiqSpec.Replicas == nil {
-		spec.System.SidekiqSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
 	return changed, nil
 }
 
@@ -957,16 +917,6 @@ func (apimanager *APIManager) setZyncDefaults() bool {
 		changed = true
 	}
 
-	if spec.Zync.AppSpec.Replicas == nil {
-		spec.Zync.AppSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
-	if spec.Zync.QueSpec.Replicas == nil {
-		spec.Zync.QueSpec.Replicas = apimanager.defaultReplicas()
-		changed = true
-	}
-
 	return changed
 }
 

diff --git a/apis/apps/v1alpha1/apimanager_types_test.go b/apis/apps/v1alpha1/apimanager_types_test.go
@@ -21,8 +21,6 @@ func TestSetDefaults(t *testing.T) {
 	tmpDefaultApicastResponseCodes := defaultApicastResponseCodes
 	tmpDefaultApicastRegistryURL := defaultApicastRegistryURL
 
-	var tmpDefaultReplicas int64 = 1
-
 	inputAPIManager := minimumAPIManagerTest()
 
 	expectedAPIManager := APIManager{
@@ -45,40 +43,22 @@ func TestSetDefaults(t *testing.T) {
 				ApicastManagementAPI: &tmpDefaultApicastManagementAPI,
 				OpenSSLVerify:        &tmpDefaultApicastOpenSSLVerify,
 				RegistryURL:          &tmpDefaultApicastRegistryURL,
-				ProductionSpec: &ApicastProductionSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
-				StagingSpec: &ApicastStagingSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
+				ProductionSpec:       &ApicastProductionSpec{},
+				StagingSpec:          &ApicastStagingSpec{},
 			},
 			Backend: &BackendSpec{
-				ListenerSpec: &BackendListenerSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
-				WorkerSpec: &BackendWorkerSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
-				CronSpec: &BackendCronSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
+				ListenerSpec: &BackendListenerSpec{},
+				WorkerSpec:   &BackendWorkerSpec{},
+				CronSpec:     &BackendCronSpec{},
 			},
 			System: &SystemSpec{
-				AppSpec: &SystemAppSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
-				SidekiqSpec: &SystemSidekiqSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
-				SphinxSpec: &SystemSphinxSpec{},
+				AppSpec:     &SystemAppSpec{},
+				SidekiqSpec: &SystemSidekiqSpec{},
+				SphinxSpec:  &SystemSphinxSpec{},
 			},
 			Zync: &ZyncSpec{
-				AppSpec: &ZyncAppSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
-				QueSpec: &ZyncQueSpec{
-					Replicas: &tmpDefaultReplicas,
-				},
+				AppSpec: &ZyncAppSpec{},
+				QueSpec: &ZyncQueSpec{},
 			},
 			PodDisruptionBudget: nil,
 		},

diff --git a/doc/apimanager-reference.md b/doc/apimanager-reference.md
@@ -93,11 +93,6 @@ Generated using [github-markdown-toc](https://github.com/ekalinin/github-markdow
 | **Annotations**  | **Name** | **Default value** | **Description** |
 | --- | --- | --- | --- |
 | `apps.3scale.net/disable-apicast-service-reconciler` | disableApicastPortReconcile | `false` | Can be `true` or `false` - will disable apicast service port reconcile when true |
-| `apps.3scale.net/disable-apicast-production-replica-reconciler` | disableApicastProductionReplicaReconciler | `false` | Can be `true` or `false` - will disable apicast production replicas reconcile when true |
-| `apps.3scale.net/disable-apicast-staging-replica-reconciler` | disableApicastStagingReplicaReconciler | `false` | Can be `true` or `false` - will disable apicast staging replicas reconcile when true |
-| `apps.3scale.net/disable-backend-listener-replica-reconciler` | disableBackendListenerReplicasReconciler | `false` | Can be `true` or `false` - will disable backend listener replicas reconcile when true |
-| `apps.3scale.net/disable-backend-worker-replica-reconciler` | disableBackendWorkerReplicasReconciler | `false` | Can be `true` or `false` - will disable backend worker replicas reconcile when true |
-| `apps.3scale.net/disable-cron-replica-reconciler` | disableCronReplicasReconciler | `false` | Can be `true` or `false` - will disable backend cron replicas reconcile when true |
 
 ### ApicastSpec
 
@@ -723,4 +718,4 @@ APIManager components are the following ones:
 | apicast-staging | 50m | 100m | 64Mi | 128Mi |
 | zync | 150m | 1 | 250M | 512Mi |
 | zync-que | 250m | 1 | 250M | 512Mi |
-| zync-database | 50m | 250m | 250M | 2G |
+| zync-database | 50m | 250m | 250M | 2G |
diff --git a/pkg/3scale/amp/component/system.go b/pkg/3scale/amp/component/system.go
@@ -572,7 +572,7 @@ func (system *System) AppDeploymentConfig() *appsv1.DeploymentConfig {
 							Kind: "ImageStreamTag",
 							Name: fmt.Sprintf("amp-system:%s", system.Options.ImageTag)}}},
 			},
-			Replicas: *system.Options.AppReplicas,
+			Replicas: system.Options.AppReplicas,
 			Selector: map[string]string{"deploymentConfig": SystemAppDeploymentName},
 			Template: &v1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{
@@ -807,7 +807,7 @@ func (system *System) SidekiqDeploymentConfig() *appsv1.DeploymentConfig {
 							Kind: "ImageStreamTag",
 							Name: fmt.Sprintf("amp-system:%s", system.Options.ImageTag)}}},
 			},
-			Replicas: *system.Options.SidekiqReplicas,
+			Replicas: system.Options.SidekiqReplicas,
 			Selector: map[string]string{"deploymentConfig": SystemSidekiqName},
 			Template: &v1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{

diff --git a/pkg/3scale/amp/component/system_options.go b/pkg/3scale/amp/component/system_options.go
@@ -48,8 +48,8 @@ type SystemOptions struct {
 	S3FileStorageOptions  *S3FileStorageOptions  `validate:"required_without=PvcFileStorageOptions"`
 	PvcFileStorageOptions *PVCFileStorageOptions `validate:"required_without=S3FileStorageOptions"`
 
-	AppReplicas     *int32 `validate:"required"`
-	SidekiqReplicas *int32 `validate:"required"`
+	AppReplicas     int32
+	SidekiqReplicas int32
 
 	AdminAccessToken    string  `validate:"required"`
 	AdminPassword       string  `validate:"required"`

diff --git a/pkg/3scale/amp/operator/apicast_options_provider.go b/pkg/3scale/amp/operator/apicast_options_provider.go
@@ -140,8 +140,15 @@ func (a *ApicastOptionsProvider) setNodeAffinityAndTolerationsOptions() {
 }
 
 func (a *ApicastOptionsProvider) setReplicas() {
-	a.apicastOptions.ProductionReplicas = int32(*a.apimanager.Spec.Apicast.ProductionSpec.Replicas)
-	a.apicastOptions.StagingReplicas = int32(*a.apimanager.Spec.Apicast.StagingSpec.Replicas)
+	a.apicastOptions.ProductionReplicas = 1
+	if a.apimanager.Spec.Apicast.ProductionSpec.Replicas != nil {
+		a.apicastOptions.ProductionReplicas = int32(*a.apimanager.Spec.Apicast.ProductionSpec.Replicas)
+	}
+
+	a.apicastOptions.StagingReplicas = 1
+	if a.apimanager.Spec.Apicast.StagingSpec.Replicas != nil {
+		a.apicastOptions.StagingReplicas = int32(*a.apimanager.Spec.Apicast.StagingSpec.Replicas)
+	}
 }
 
 func (a *ApicastOptionsProvider) commonLabels() map[string]string {

diff --git a/pkg/3scale/amp/operator/apicast_reconciler.go b/pkg/3scale/amp/operator/apicast_reconciler.go
@@ -17,11 +17,6 @@ import (
 	"github.com/3scale/3scale-operator/pkg/reconcilers"
 )
 
-const (
-	disableApicastProductionReplicaReconciler = "apps.3scale.net/disable-apicast-production-replica-reconciler"
-	disableApicastStagingReplicaReconciler    = "apps.3scale.net/disable-apicast-staging-replica-reconciler"
-)
-
 func ApicastEnvCMMutator(existingObj, desiredObj common.KubernetesObject) (bool, error) {
 	existing, ok := existingObj.(*v1.ConfigMap)
 	if !ok {
@@ -85,7 +80,7 @@ func (r *ApicastReconciler) Reconcile() (reconcile.Result, error) {
 		apicastPodTemplateEnvConfigMapAnnotationsMutator,
 	}
 
-	if value, found := r.apiManager.ObjectMeta.Annotations[disableApicastStagingReplicaReconciler]; !found || value != "true" {
+	if r.apiManager.Spec.Apicast.StagingSpec.Replicas != nil {
 		stagingMutators = append(stagingMutators, reconcilers.DeploymentConfigReplicasMutator)
 	}
 
@@ -117,7 +112,7 @@ func (r *ApicastReconciler) Reconcile() (reconcile.Result, error) {
 		apicastPodTemplateEnvConfigMapAnnotationsMutator,
 	}
 
-	if value, found := r.apiManager.ObjectMeta.Annotations[disableApicastProductionReplicaReconciler]; !found || value != "true" {
+	if r.apiManager.Spec.Apicast.ProductionSpec.Replicas != nil {
 		productionMutators = append(productionMutators, reconcilers.DeploymentConfigReplicasMutator)
 	}
 

diff --git a/pkg/3scale/amp/operator/apicast_reconciler_test.go b/pkg/3scale/amp/operator/apicast_reconciler_test.go
@@ -769,11 +769,13 @@ func TestApicastServicePortMutator(t *testing.T) {
 	}
 }
 
-func TestApicastReconcilerDisableReplicaSyncingAnnotations(t *testing.T) {
+func TestReplicaApicastReconciler(t *testing.T) {
 	var (
-		namespace       = "operator-unittest"
-		log             = logf.Log.WithName("operator_test")
-		twoValue  int32 = 2
+		namespace        = "operator-unittest"
+		log              = logf.Log.WithName("operator_test")
+		oneValue   int32 = 1
+		oneValue64 int64 = 1
+		twoValue   int32 = 2
 	)
 	ctx := context.TODO()
 	s := scheme.Scheme
@@ -794,22 +796,14 @@ func TestApicastReconcilerDisableReplicaSyncingAnnotations(t *testing.T) {
 	cases := []struct {
 		testName                 string
 		objName                  string
-		obj                      runtime.Object
 		apimanager               *appsv1alpha1.APIManager
-		annotation               string
-		annotationValue          string
 		expectedAmountOfReplicas int32
-		validatingFunction       func(*appsv1alpha1.APIManager, *appsv1.DeploymentConfig, string, string, int32) bool
 	}{
-		{"apicast-staging-DC-annotation not present", "apicast-staging", &appsv1.DeploymentConfig{}, apiManagerCreator("someAnnotation", "false"), disableApicastStagingReplicaReconciler, "dummy", int32(1), confirmReplicasWhenAnnotationIsNotPresent},
-		{"apicast-staging-DC-annotation false", "apicast-staging", &appsv1.DeploymentConfig{}, apiManagerCreator(disableApicastStagingReplicaReconciler, "false"), disableApicastStagingReplicaReconciler, "false", int32(1), confirmReplicasWhenAnnotationPresent},
-		{"apicast-staging-DC-annotation true", "apicast-staging", &appsv1.DeploymentConfig{}, apiManagerCreator(disableApicastStagingReplicaReconciler, "true"), disableApicastStagingReplicaReconciler, "true", int32(2), confirmReplicasWhenAnnotationPresent},
-		{"apicast-staging-DC-annotation true of dummy value", "apicast-staging", &appsv1.DeploymentConfig{}, apiManagerCreator(disableApicastStagingReplicaReconciler, "true"), disableApicastStagingReplicaReconciler, "someDummyValue", int32(1), confirmReplicasWhenAnnotationPresent},
+		{"apicast-staging replicas set", "apicast-staging", testApicastAPIManagerCreator(&oneValue64, nil), oneValue},
+		{"apicast-staging replicas not set", "apicast-staging", testApicastAPIManagerCreator(nil, nil), twoValue},
 
-		{"apicast-production-DC-annotation not present", "apicast-production", &appsv1.DeploymentConfig{}, apiManagerCreator("someAnnotation", "false"), disableApicastProductionReplicaReconciler, "dummy", int32(1), confirmReplicasWhenAnnotationIsNotPresent},
-		{"apicast-production-DC-annotation false", "apicast-production", &appsv1.DeploymentConfig{}, apiManagerCreator(disableApicastProductionReplicaReconciler, "false"), disableApicastProductionReplicaReconciler, "false", int32(1), confirmReplicasWhenAnnotationPresent},
-		{"apicast-production-DC-annotation true", "apicast-production", &appsv1.DeploymentConfig{}, apiManagerCreator(disableApicastProductionReplicaReconciler, "true"), disableApicastProductionReplicaReconciler, "true", int32(2), confirmReplicasWhenAnnotationPresent},
-		{"apicast-production-DC-annotation true of dummy value", "apicast-production", &appsv1.DeploymentConfig{}, apiManagerCreator(disableApicastProductionReplicaReconciler, "true"), disableApicastProductionReplicaReconciler, "someDummyValue", int32(1), confirmReplicasWhenAnnotationPresent},
+		{"apicast-production replicas set", "apicast-production", testApicastAPIManagerCreator(nil, &oneValue64), oneValue},
+		{"apicast-production replicas not set", "apicast-production", testApicastAPIManagerCreator(nil, nil), twoValue},
 	}
 
 	for _, tc := range cases {
@@ -858,31 +852,28 @@ func TestApicastReconcilerDisableReplicaSyncingAnnotations(t *testing.T) {
 				subT.Errorf("error fetching object %s: %v", tc.objName, err)
 			}
 
-			correct := tc.validatingFunction(tc.apimanager, dc, tc.annotation, tc.annotationValue, tc.expectedAmountOfReplicas)
-			if !correct {
-				subT.Errorf("value of expteced replicas does not match for %s. expected: %v actual: %v", tc.objName, tc.expectedAmountOfReplicas, dc.Spec.Replicas)
+			if tc.expectedAmountOfReplicas != dc.Spec.Replicas {
+				subT.Errorf("expected replicas do not match. expected: %d actual: %d", tc.expectedAmountOfReplicas, dc.Spec.Replicas)
 			}
 		})
 	}
 }
 
-func apiManagerCreator(disableSyncAnnotation string, disableSyncAnnotationValue string) *appsv1alpha1.APIManager {
+func testApicastAPIManagerCreator(stagingReplicas, productionReplicas *int64) *appsv1alpha1.APIManager {
 	var (
-		name                       = "example-apimanager"
-		namespace                  = "operator-unittest"
-		wildcardDomain             = "test.3scale.net"
-		appLabel                   = "someLabel"
-		tenantName                 = "someTenant"
-		trueValue                  = true
-		apicastManagementAPI       = "disabled"
-		oneValue             int64 = 1
+		name                 = "example-apimanager"
+		namespace            = "operator-unittest"
+		wildcardDomain       = "test.3scale.net"
+		appLabel             = "someLabel"
+		tenantName           = "someTenant"
+		trueValue            = true
+		apicastManagementAPI = "disabled"
 	)
 
 	return &appsv1alpha1.APIManager{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:        name,
-			Namespace:   namespace,
-			Annotations: map[string]string{disableSyncAnnotation: disableSyncAnnotationValue},
+			Name:      name,
+			Namespace: namespace,
 		},
 		Spec: appsv1alpha1.APIManagerSpec{
 			APIManagerCommonSpec: appsv1alpha1.APIManagerCommonSpec{
@@ -896,12 +887,8 @@ func apiManagerCreator(disableSyncAnnotation string, disableSyncAnnotationValue
 				ApicastManagementAPI: &apicastManagementAPI,
 				OpenSSLVerify:        &trueValue,
 				IncludeResponseCodes: &trueValue,
-				StagingSpec: &appsv1alpha1.ApicastStagingSpec{
-					Replicas: &oneValue,
-				},
-				ProductionSpec: &appsv1alpha1.ApicastProductionSpec{
-					Replicas: &oneValue,
-				},
+				StagingSpec:          &appsv1alpha1.ApicastStagingSpec{Replicas: stagingReplicas},
+				ProductionSpec:       &appsv1alpha1.ApicastProductionSpec{Replicas: productionReplicas},
 			},
 			PodDisruptionBudget: &appsv1alpha1.PodDisruptionBudgetSpec{Enabled: true},
 		},

diff --git a/pkg/3scale/amp/operator/backend_options_provider.go b/pkg/3scale/amp/operator/backend_options_provider.go
@@ -142,9 +142,20 @@ func (o *OperatorBackendOptionsProvider) setNodeAffinityAndTolerationsOptions()
 }
 
 func (o *OperatorBackendOptionsProvider) setReplicas() {
-	o.backendOptions.ListenerReplicas = int32(*o.apimanager.Spec.Backend.ListenerSpec.Replicas)
-	o.backendOptions.WorkerReplicas = int32(*o.apimanager.Spec.Backend.WorkerSpec.Replicas)
-	o.backendOptions.CronReplicas = int32(*o.apimanager.Spec.Backend.CronSpec.Replicas)
+	o.backendOptions.ListenerReplicas = 1
+	if o.apimanager.Spec.Backend.ListenerSpec.Replicas != nil {
+		o.backendOptions.ListenerReplicas = int32(*o.apimanager.Spec.Backend.ListenerSpec.Replicas)
+	}
+
+	o.backendOptions.WorkerReplicas = 1
+	if o.apimanager.Spec.Backend.WorkerSpec.Replicas != nil {
+		o.backendOptions.WorkerReplicas = int32(*o.apimanager.Spec.Backend.WorkerSpec.Replicas)
+	}
+
+	o.backendOptions.CronReplicas = 1
+	if o.apimanager.Spec.Backend.CronSpec.Replicas != nil {
+		o.backendOptions.CronReplicas = int32(*o.apimanager.Spec.Backend.CronSpec.Replicas)
+	}
 }
 
 func (o *OperatorBackendOptionsProvider) commonLabels() map[string]string {