Fix conflicting Serverless scaling options (#15576)

* Fix conflicting scaling options * fix tests * change title * bump images * update proposal based on review comments * update the implementation * update tests * fix integration tests go.mod * integration tests * bump images * remove extra step in tests * Update components/function-controller/internal/controllers/serverless/scaling.go Co-authored-by: Damian Badura <45110612+dbadura@users.noreply.github.com> * review comments * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> * Update components/function-controller/design/scaling_functions.md Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com> Co-authored-by: Damian Badura <45110612+dbadura@users.noreply.github.com> Co-authored-by: Grzegorz Karaluch <grzegorz.karaluch@sap.com>
kyma-project · Oct 12, 2022 · f3e30e3 · f3e30e3
1 parent adfb3a0
commit f3e30e3
Show file tree

Hide file tree

Showing 18 changed files with 122 additions and 112 deletions.
diff --git a/components/function-controller/design/scaling_functions.md b/components/function-controller/design/scaling_functions.md
@@ -0,0 +1,40 @@
+# Serverless Functions Scaling Modes
+
+## Summary
+Initially, the Serverless API supported the `spec.ScaleConfig` field only. The workflow was as follows:
+- Function resources were defaulted to min/max replicas = 1
+- If you set a different scaling config, Function Controller created HPA resources with the user-defined min/max values. 
+- The HPA resource target ref was the Function runtime deployment.
+- The Function Controller did not enforce the runtime deployment `spec.Replicas` anymore and it was handled by the HPA resources.
+
+
+Later, the Serverless API was extended by the `Scale` subresource, which allows for direct scaling of the Function resources through the Kubernetes API.
+
+However, there are some implementation conflicts between the two features. This is a design and an implementation plan to unify the UX while using Functions as scaled resources.
+
+### Goals
+- Support Function scale subresource and `spec.ScaleConfig` without conflicts.
+- Provide frictionless UX for the feature.
+
+## Proposal
+Describe and implement two different scaling configuration. Both configurations already work to some extent. The point here is have an ergonomic UX and flow.
+
+### External scaling configuration
+This is managed and configured using `spec.Replicas`. It describes the scale subresource use case. It supports:
+- Manual scaling of the Function up and down through the API.
+- Configuring an HPA resource with the Function resources as a target.
+- Using an external scaler like [KEDA](https://keda.sh/).
+
+### Built-in scaling configuration
+This is managed and enabled by setting `spec.ScaleConfig`. It is configurable using Busola and it provides the most basic scaling configuration for the Function.
+
+This configuration is disabled by removing `spec.ScaleConfig`. The Busola UI can be extended to allow you to add or remove `spec.ScaleConfig` to manage built-in scaling with minimal effort.
+
+## Implementation details
+
+- The controller should support and accept both `spec.Replicas` and `spec.ScaleConfig`. Current validation rule to block this will be removed.
+- `spec.Replicas` is the only source of truth for scaling the Function/runtime deployment.
+- `spec.ScaleConfig` is only used to configure the controller internal HPA.
+- The internal HPA is removed if `spec.ScaleConfig == nil`
+- The HPA resource created by the controller still targets the Function resources.
+- The current Function status update logic must be fixed to reflect the Function's current scale. 
diff --git a/components/function-controller/internal/controllers/serverless/deployment.go b/components/function-controller/internal/controllers/serverless/deployment.go
@@ -46,7 +46,6 @@ func stateFnCheckDeployments(ctx context.Context, r *reconciler, s *systemState)
 	}
 
 	expectedDeployment := s.buildDeployment(args)
-
 	deploymentChanged := !s.deploymentEqual(expectedDeployment)
 
 	if !deploymentChanged {
@@ -61,7 +60,7 @@ func stateFnCheckDeployments(ctx context.Context, r *reconciler, s *systemState)
 		return stateFnDeleteDeployments
 	}
 
-	if !equalDeployments(s.deployments.Items[0], expectedDeployment, isScalingEnabled(&s.instance)) {
+	if !equalDeployments(s.deployments.Items[0], expectedDeployment) {
 		return buildStateFnUpdateDeployment(expectedDeployment.Spec, expectedDeployment.Labels)
 	}
 

diff --git a/components/function-controller/internal/controllers/serverless/deployment_test.go b/components/function-controller/internal/controllers/serverless/deployment_test.go
@@ -332,7 +332,7 @@ func TestFunctionReconciler_equalDeployments(t *testing.T) {
 				expected:       fixDeploymentWithReplicas(2),
 				scalingEnabled: true,
 			},
-			want: true,
+			want: false,
 		},
 		{
 			name: "scaling enabled and replicas match",
@@ -365,7 +365,7 @@ func TestFunctionReconciler_equalDeployments(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := gomega.NewGomegaWithT(t)
-			got := equalDeployments(tt.args.existing, tt.args.expected, tt.args.scalingEnabled)
+			got := equalDeployments(tt.args.existing, tt.args.expected)
 			g.Expect(got).To(gomega.Equal(tt.want))
 		})
 	}

diff --git a/...ts/function-controller/internal/controllers/serverless/function_reconcile_asserts_test.go b/...ts/function-controller/internal/controllers/serverless/function_reconcile_asserts_test.go
@@ -192,9 +192,9 @@ func assertSuccessfulFunctionDeployment(t *testing.T, resourceClient resource.Cl
 
 	hpaSpec := hpaList.Items[0].Spec
 
-	g.Expect(hpaSpec.ScaleTargetRef.Name).To(gomega.Equal(deployment.GetName()))
-	g.Expect(hpaSpec.ScaleTargetRef.Kind).To(gomega.Equal("Deployment"))
-	g.Expect(hpaSpec.ScaleTargetRef.APIVersion).To(gomega.Equal(appsv1.SchemeGroupVersion.String()))
+	g.Expect(hpaSpec.ScaleTargetRef.Name).To(gomega.Equal(function.GetName()))
+	g.Expect(hpaSpec.ScaleTargetRef.Kind).To(gomega.Equal(serverlessv1alpha2.FunctionKind))
+	g.Expect(hpaSpec.ScaleTargetRef.APIVersion).To(gomega.Equal(serverlessv1alpha2.GroupVersion.String()))
 
 	t.Log("deployment ready")
 	deployment.Status.Conditions = []appsv1.DeploymentCondition{

diff --git a/...nts/function-controller/internal/controllers/serverless/function_reconcile_gitops_test.go b/...nts/function-controller/internal/controllers/serverless/function_reconcile_gitops_test.go
@@ -357,7 +357,7 @@ func TestGitOpsWithContinuousGitCheckout(t *testing.T) {
 
 			g.Expect(svc.Spec.Selector).To(gomega.Equal(deployment.Spec.Selector.MatchLabels))
 
-			t.Log("hpa creation")
+			t.Log("HPA creation")
 			g.Expect(reconciler.Reconcile(ctx, request)).To(beOKReconcileResult)
 
 			function = &serverlessv1alpha2.Function{}
@@ -375,9 +375,9 @@ func TestGitOpsWithContinuousGitCheckout(t *testing.T) {
 
 			hpaSpec := hpaList.Items[0].Spec
 
-			g.Expect(hpaSpec.ScaleTargetRef.Name).To(gomega.Equal(deployment.GetName()))
-			g.Expect(hpaSpec.ScaleTargetRef.Kind).To(gomega.Equal("Deployment"))
-			g.Expect(hpaSpec.ScaleTargetRef.APIVersion).To(gomega.Equal(appsv1.SchemeGroupVersion.String()))
+			g.Expect(hpaSpec.ScaleTargetRef.Name).To(gomega.Equal(function.GetName()))
+			g.Expect(hpaSpec.ScaleTargetRef.Kind).To(gomega.Equal(serverlessv1alpha2.FunctionKind))
+			g.Expect(hpaSpec.ScaleTargetRef.APIVersion).To(gomega.Equal(serverlessv1alpha2.GroupVersion.String()))
 
 			t.Log("deployment ready")
 			deployment.Status.Conditions = []appsv1.DeploymentCondition{
@@ -650,9 +650,9 @@ func TestGitOpsWithoutContinuousGitCheckout(t *testing.T) {
 
 			hpaSpec := hpaList.Items[0].Spec
 
-			g.Expect(hpaSpec.ScaleTargetRef.Name).To(gomega.Equal(deployment.GetName()))
-			g.Expect(hpaSpec.ScaleTargetRef.Kind).To(gomega.Equal("Deployment"))
-			g.Expect(hpaSpec.ScaleTargetRef.APIVersion).To(gomega.Equal(appsv1.SchemeGroupVersion.String()))
+			g.Expect(hpaSpec.ScaleTargetRef.Name).To(gomega.Equal(function.GetName()))
+			g.Expect(hpaSpec.ScaleTargetRef.Kind).To(gomega.Equal(serverlessv1alpha2.FunctionKind))
+			g.Expect(hpaSpec.ScaleTargetRef.APIVersion).To(gomega.Equal(serverlessv1alpha2.GroupVersion.String()))
 
 			t.Log("Deployment is ready")
 			deployment.Status.Conditions = []appsv1.DeploymentCondition{

diff --git a/components/function-controller/internal/controllers/serverless/function_reconcile_test.go b/components/function-controller/internal/controllers/serverless/function_reconcile_test.go
@@ -96,14 +96,15 @@ func TestFunctionReconciler_Reconcile_Scaling(t *testing.T) {
 		assertSuccessfulFunctionBuild(t, resourceClient, reconciler, request, fnLabels, false)
 
 		assertSuccessfulFunctionDeployment(t, resourceClient, reconciler, request, fnLabels, "registry.kyma.local", false)
-
 		two := int32(2)
 		four := int32(4)
 
 		t.Log("updating function to use fixed replicas number")
 		g.Expect(resourceClient.Get(context.TODO(), request.NamespacedName, function)).To(gomega.Succeed())
-		function.Spec.ScaleConfig.MinReplicas = &two
 		function.Spec.ScaleConfig.MaxReplicas = &two
+		function.Spec.ScaleConfig.MinReplicas = &two
+		// TODO: This should be applied by the defaulting webhook
+		function.Spec.Replicas = &two
 		g.Expect(resourceClient.Update(context.TODO(), function)).To(gomega.Succeed())
 
 		t.Log("updating deployment with new number of replicas")
@@ -125,7 +126,7 @@ func TestFunctionReconciler_Reconcile_Scaling(t *testing.T) {
 		g.Expect(deployment).ToNot(gomega.BeNil())
 		g.Expect(deployment.Spec.Replicas).To(gomega.Equal(&two))
 
-		t.Log("removing hpa")
+		t.Log("HPA is removed")
 		result, err = reconciler.Reconcile(ctx, request)
 		g.Expect(err).To(gomega.BeNil())
 		g.Expect(result.Requeue).To(gomega.BeFalse())
@@ -149,12 +150,12 @@ func TestFunctionReconciler_Reconcile_Scaling(t *testing.T) {
 		g.Expect(getConditionStatus(function.Status.Conditions, serverlessv1alpha2.ConditionRunning)).To(gomega.Equal(corev1.ConditionTrue))
 		g.Expect(getConditionReason(function.Status.Conditions, serverlessv1alpha2.ConditionRunning)).To(gomega.Equal(serverlessv1alpha2.ConditionReasonDeploymentReady))
 
-		t.Log("updating function to use scalable replicas number")
+		t.Log("replicas increased by an external scaler")
 		g.Expect(resourceClient.Get(context.TODO(), request.NamespacedName, function)).To(gomega.Succeed())
-		function.Spec.ScaleConfig.MaxReplicas = &four
+		function.Spec.Replicas = &four
 		g.Expect(resourceClient.Update(context.TODO(), function)).To(gomega.Succeed())
 
-		t.Log("creating hpa")
+		t.Log("Updating deployment")
 		result, err = reconciler.Reconcile(ctx, request)
 		g.Expect(err).To(gomega.BeNil())
 		g.Expect(result.Requeue).To(gomega.BeFalse())
@@ -166,19 +167,17 @@ func TestFunctionReconciler_Reconcile_Scaling(t *testing.T) {
 		g.Expect(getConditionStatus(function.Status.Conditions, serverlessv1alpha2.ConditionBuildReady)).To(gomega.Equal(corev1.ConditionTrue))
 		g.Expect(getConditionStatus(function.Status.Conditions, serverlessv1alpha2.ConditionRunning)).To(gomega.Equal(corev1.ConditionUnknown))
 
-		g.Expect(getConditionReason(function.Status.Conditions, serverlessv1alpha2.ConditionRunning)).To(gomega.Equal(serverlessv1alpha2.ConditionReasonHorizontalPodAutoscalerCreated))
+		g.Expect(getConditionReason(function.Status.Conditions, serverlessv1alpha2.ConditionRunning)).To(gomega.Equal(serverlessv1alpha2.ConditionReasonDeploymentUpdated))
 
+		// we scale the deployment directly using spec.Replicas, a new HPA shouldn't be created
 		err = reconciler.client.ListByLabel(context.TODO(), function.GetNamespace(), fnLabels, hpaList)
 		g.Expect(err).To(gomega.BeNil())
-		g.Expect(hpaList.Items).To(gomega.HaveLen(1))
-
-		hpaSpec := hpaList.Items[0].Spec
-
-		g.Expect(hpaSpec.ScaleTargetRef.Name).To(gomega.Equal(deployment.GetName()))
-		g.Expect(hpaSpec.ScaleTargetRef.Kind).To(gomega.Equal("Deployment"))
-		g.Expect(hpaSpec.ScaleTargetRef.APIVersion).To(gomega.Equal(appsv1.SchemeGroupVersion.String()))
+		g.Expect(hpaList.Items).To(gomega.HaveLen(0))
 
 		t.Log("deployment ready")
+		g.Expect(resourceClient.ListByLabel(context.TODO(), request.Namespace, fnLabels, deployments)).To(gomega.Succeed())
+		g.Expect(len(deployments.Items)).To(gomega.Equal(1))
+		deployment = &deployments.Items[0]
 		deployment.Status.Conditions = []appsv1.DeploymentCondition{
 			{Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue, Reason: MinimumReplicasAvailable},
 			{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, Reason: NewRSAvailableReason},
@@ -705,7 +704,7 @@ func TestFunctionReconciler_Reconcile(t *testing.T) {
 		_, err = reconciler.Reconcile(ctx, request)
 		g.Expect(err).To(gomega.BeNil())
 
-		g.Expect(hpaList.Items[0].Spec.ScaleTargetRef.Name).To(gomega.Equal(deployList.Items[0].Name), "hpa should target proper deployment")
+		g.Expect(hpaList.Items[0].Spec.ScaleTargetRef.Name).To(gomega.Equal(inFunction.GetName()), "hpa should target the function")
 
 		t.Log("deleting deployment by 'accident' to check proper hpa-deployment reference")
 
@@ -731,7 +730,7 @@ func TestFunctionReconciler_Reconcile(t *testing.T) {
 		g.Expect(err).To(gomega.BeNil())
 		g.Expect(deployList.Items).To(gomega.HaveLen(1))
 
-		g.Expect(hpaList.Items[0].Spec.ScaleTargetRef.Name).To(gomega.Equal(deployList.Items[0].Name), "hpa should target proper deployment")
+		g.Expect(hpaList.Items[0].Spec.ScaleTargetRef.Name).To(gomega.Equal(function.GetName()), "hpa should target function")
 	})
 
 	t.Run("should requeue before creating a job", func(t *testing.T) {

diff --git a/components/function-controller/internal/controllers/serverless/scaling.go b/components/function-controller/internal/controllers/serverless/scaling.go
@@ -20,7 +20,7 @@ func stateFnCheckScaling(ctx context.Context, r *reconciler, s *systemState) sta
 		return nil
 	}
 
-	if s.instance.Spec.Replicas != nil {
+	if !isScalingEnabled(&s.instance) {
 		return stateFnCheckReplicas(ctx, r, s)
 	}
 
@@ -56,7 +56,7 @@ func stateFnCheckHPA(ctx context.Context, r *reconciler, s *systemState) stateFn
 		return stateFnDeleteAllHorizontalPodAutoscalers
 	}
 
-	if numHpa == 1 && equalInt32Pointer(s.instance.Spec.ScaleConfig.MinReplicas, s.instance.Spec.ScaleConfig.MaxReplicas) {
+	if numHpa == 1 && !isScalingEnabled(&s.instance) {
 		// this case is when we previously created HPA with maxReplicas > minReplicas, but now user changed
 		// function spec and NOW maxReplicas == minReplicas, so hpa is not needed anymore
 		return stateFnDeleteAllHorizontalPodAutoscalers

diff --git a/components/function-controller/internal/controllers/serverless/system_state.go b/components/function-controller/internal/controllers/serverless/system_state.go
@@ -495,20 +495,16 @@ func (s *systemState) buildDeployment(cfg buildDeploymentArgs) appsv1.Deployment
 }
 
 func (s *systemState) getReplicas(defaultVal int32) *int32 {
-	if s.instance.Spec.ScaleConfig != nil {
-		return s.instance.Spec.ScaleConfig.MinReplicas
-	}
 	if s.instance.Spec.Replicas != nil {
 		return s.instance.Spec.Replicas
 	}
-
 	return &defaultVal
 }
 
 // TODO do not negate
 func (s *systemState) deploymentEqual(d appsv1.Deployment) bool {
 	return len(s.deployments.Items) == 1 &&
-		equalDeployments(s.deployments.Items[0], d, isScalingEnabled(&s.instance))
+		equalDeployments(s.deployments.Items[0], d)
 }
 
 func (s *systemState) hasDeploymentConditionTrueStatusWithReason(conditionType appsv1.DeploymentConditionType, reason string) bool {
@@ -604,7 +600,6 @@ func (s *systemState) defaultReplicas() (int32, int32) {
 
 func (s *systemState) buildHorizontalPodAutoscaler(targetCPUUtilizationPercentage int32) autoscalingv1.HorizontalPodAutoscaler {
 	minReplicas, maxReplicas := s.defaultReplicas()
-	deploymentName := s.deployments.Items[0].GetName()
 	return autoscalingv1.HorizontalPodAutoscaler{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: fmt.Sprintf("%s-", s.instance.GetName()),
@@ -613,9 +608,9 @@ func (s *systemState) buildHorizontalPodAutoscaler(targetCPUUtilizationPercentag
 		},
 		Spec: autoscalingv1.HorizontalPodAutoscalerSpec{
 			ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
-				Kind:       "Deployment",
-				Name:       deploymentName,
-				APIVersion: appsv1.SchemeGroupVersion.String(),
+				Kind:       serverlessv1alpha2.FunctionKind,
+				Name:       s.instance.Name,
+				APIVersion: serverlessv1alpha2.GroupVersion.String(),
 			},
 			MinReplicas:                    &minReplicas,
 			MaxReplicas:                    maxReplicas,

diff --git a/components/function-controller/internal/controllers/serverless/utils.go b/components/function-controller/internal/controllers/serverless/utils.go
@@ -219,15 +219,15 @@ func mapsEqual(existing, expected map[string]string) bool {
 }
 
 // TODO refactor to make this code more readable
-func equalDeployments(existing appsv1.Deployment, expected appsv1.Deployment, scalingEnabled bool) bool {
+func equalDeployments(existing appsv1.Deployment, expected appsv1.Deployment) bool {
 	return len(existing.Spec.Template.Spec.Containers) == 1 &&
 		len(existing.Spec.Template.Spec.Containers) == len(expected.Spec.Template.Spec.Containers) &&
 		existing.Spec.Template.Spec.Containers[0].Image == expected.Spec.Template.Spec.Containers[0].Image &&
 		envsEqual(existing.Spec.Template.Spec.Containers[0].Env, expected.Spec.Template.Spec.Containers[0].Env) &&
 		mapsEqual(existing.GetLabels(), expected.GetLabels()) &&
 		mapsEqual(existing.Spec.Template.GetLabels(), expected.Spec.Template.GetLabels()) &&
 		equalResources(existing.Spec.Template.Spec.Containers[0].Resources, expected.Spec.Template.Spec.Containers[0].Resources) &&
-		(scalingEnabled || equalInt32Pointer(existing.Spec.Replicas, expected.Spec.Replicas))
+		equalInt32Pointer(existing.Spec.Replicas, expected.Spec.Replicas)
 }
 
 func equalServices(existing corev1.Service, expected corev1.Service) bool {

diff --git a/components/function-controller/internal/webhook/defaulting_webhook.go b/components/function-controller/internal/webhook/defaulting_webhook.go
@@ -66,7 +66,6 @@ func (w *DefaultingWebHook) handleFunctionDefaulting(req admission.Request) admi
 			}
 			fn.Default(w.configAlphaV2)
 			f = fn
-
 		}
 	default:
 		return admission.Errored(http.StatusBadRequest, errors.Errorf("Invalid resource version provided: %s", req.Kind.Version))

diff --git a/components/function-controller/pkg/apis/serverless/v1alpha2/function_defaults.go b/components/function-controller/pkg/apis/serverless/v1alpha2/function_defaults.go
@@ -58,30 +58,34 @@ type DefaultingConfig struct {
 }
 
 func (fn *Function) Default(config *DefaultingConfig) {
-	fn.Spec.defaultReplicas(config)
+	fn.Spec.defaultScaling(config)
 	fn.Spec.defaultFunctionResources(config, fn)
 	fn.Spec.defaultBuildResources(config, fn)
 }
 
-func (spec *FunctionSpec) defaultReplicas(config *DefaultingConfig) {
-	if spec.Replicas != nil {
-		return
-	}
-
+func (spec *FunctionSpec) defaultScaling(config *DefaultingConfig) {
 	defaultingConfig := config.Function.Replicas
 	replicasPreset := defaultingConfig.Presets[defaultingConfig.DefaultPreset]
 
+	if spec.Replicas == nil {
+		// TODO: The presets structure and docs should be updated to reflect the new behavior.
+		spec.Replicas = &replicasPreset.Min
+	}
+
 	if spec.ScaleConfig == nil {
-		spec.ScaleConfig = &ScaleConfig{}
+		return
 	}
+
+	// spec.ScaleConfig is SET, but not fully configured, for sanity, we will default MinReplicas, we will also use it as a default spec.Replica
 	if spec.ScaleConfig.MinReplicas == nil {
 		newMin := replicasPreset.Min
 		if spec.ScaleConfig.MaxReplicas != nil && *spec.ScaleConfig.MaxReplicas < newMin {
 			newMin = *spec.ScaleConfig.MaxReplicas
 		}
-
 		spec.ScaleConfig.MinReplicas = &newMin
 	}
+	spec.Replicas = spec.ScaleConfig.MinReplicas
+
 	if spec.ScaleConfig.MaxReplicas == nil {
 		newMax := replicasPreset.Max
 		if *spec.ScaleConfig.MinReplicas > newMax {