From 13ce14c8fe5d10cc81ada20927659047d661967b Mon Sep 17 00:00:00 2001 From: Vighnesh Shenoy Date: Thu, 7 Apr 2022 15:27:24 +0530 Subject: [PATCH 1/7] Fix metric name passed to external scaler in GetMetrics call. Signed-off-by: Vighnesh Shenoy --- pkg/scalers/external_scaler.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/scalers/external_scaler.go b/pkg/scalers/external_scaler.go index 384a93ca0a0..5dee3a0a0bb 100644 --- a/pkg/scalers/external_scaler.go +++ b/pkg/scalers/external_scaler.go @@ -197,6 +197,8 @@ func (s *externalScaler) GetMetrics(ctx context.Context, metricName string, metr } defer done() + // Remove the sX- prefix as the external scaler shouldn't have to know about it + metricName = strings.SplitN(metricName, "-", 2)[1] request := &pb.GetMetricsRequest{ MetricName: metricName, ScaledObjectRef: &s.scaledObjectRef, From 5784edbf1eb867a3b9d849214006642b60903178 Mon Sep 17 00:00:00 2001 From: Vighnesh Shenoy Date: Thu, 7 Apr 2022 16:32:38 +0530 Subject: [PATCH 2/7] Add e2e test for external scaler. Signed-off-by: Vighnesh Shenoy --- tests/scalers/external-scaler.test.ts | 193 ++++++++++++++++++++++++++ tests/scalers/helpers.ts | 6 + 2 files changed, 199 insertions(+) create mode 100644 tests/scalers/external-scaler.test.ts diff --git a/tests/scalers/external-scaler.test.ts b/tests/scalers/external-scaler.test.ts new file mode 100644 index 00000000000..15f76a1cee9 --- /dev/null +++ b/tests/scalers/external-scaler.test.ts @@ -0,0 +1,193 @@ +import * as sh from "shelljs" +import test from "ava" +import { createNamespace, createYamlFile, waitForDeploymentReplicaCount } from "./helpers" + +const testName = "test-external-scaler" +const testNamespace = `${testName}-ns` +const scalerName = `${testName}-scaler` +const serviceName = `${testName}-service` +const deploymentName = `${testName}-deployment` +const scaledObjectName = `${testName}-scaled-object` + +const idleReplicaCount = 0 +const minReplicaCount = 1 +const maxReplicaCount = 2 +const threshold = 10 + +test.before(async t => { + // TODO - Uncomment + // sh.config.silent = true + + // Create Kubernetes Namespace + createNamespace(testNamespace) + + // Create external scaler deployment + t.is( + sh.exec(`kubectl apply -f ${createYamlFile(scalerYaml)} -n ${testNamespace}`).code, + 0, + "Createing a external scaler deployment should work" + ) + + // Create service + t.is( + sh.exec(`kubectl apply -f ${createYamlFile(serviceYaml)} -n ${testNamespace}`).code, + 0, + "Createing a service should work" + ) + + // Create deployment + t.is( + sh.exec(`kubectl apply -f ${createYamlFile(deploymentYaml)} -n ${testNamespace}`).code, + 0, + "Createing a deployment should work" + ) + + // Create scaled object + t.is( + sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", "0"))} -n ${testNamespace}`).code, + 0, + "Creating a scaled object should work" + ) + + t.true(await waitForDeploymentReplicaCount(idleReplicaCount, deploymentName, testNamespace, 60, 1000), + `Replica count should be ${idleReplicaCount} after 1 minute`) +}) + +test.serial("Deployment should scale up to minReplicaCount", async t => { + // Modify scaled object's metricValue to induce scaling + t.is( + sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", `${threshold}`))} -n ${testNamespace}`).code, + 0, + "Modifying scaled object should work" + ) + + t.true(await waitForDeploymentReplicaCount(minReplicaCount, deploymentName, testNamespace, 60, 1000), + `Replica count should be ${minReplicaCount} after 1 minute`) +}) + +test.serial("Deployment should scale up to maxReplicaCount", async t => { + // Modify scaled object's metricValue to induce scaling + t.is( + sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", `${threshold * 2}`))} -n ${testNamespace}`).code, + 0, + "Modifying scaled object should work" + ) + + t.true(await waitForDeploymentReplicaCount(maxReplicaCount, deploymentName, testNamespace, 60, 1000), + `Replica count should be ${maxReplicaCount} after 1 minute`) +}) + +test.serial("Deployment should scale back down to idleReplicaCount", async t => { + // Modify scaled object's metricValue to induce scaling + t.is( + sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", "0"))} -n ${testNamespace}`).code, + 0, + "Modifying scaled object should work" + ) + + t.true(await waitForDeploymentReplicaCount(idleReplicaCount, deploymentName, testNamespace, 60, 1000), + `Replica count should be ${idleReplicaCount} after 1 minute`) +}) + +test.after.always("Clean up E2E K8s objects", async t => { + const resources = [ + `scaledobject.keda.sh/${scaledObjectName}`, + `deployments.apps/${deploymentName}`, + `service/${serviceName}`, + `deployments.apps/${scalerName}`, + ] + + for (const resource of resources) { + sh.exec(`kubectl delete ${resource} -n ${testNamespace}`) + } + + sh.exec(`kubectl delete ns ${testNamespace}`) +}) + +// YAML Definitions for Kubernetes resources +// External Scaler Deployment +const scalerYaml = +` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: ${scalerName} + namespace: ${testNamespace} +spec: + replicas: 1 + selector: + matchLabels: + app: ${scalerName} + template: + metadata: + labels: + app: ${scalerName} + spec: + containers: + - name: scaler + image: ghcr.io/kedacore/tests-external-scaler-e2e:latest + imagePullPolicy: Always + ports: + - containerPort: 6000 +` + +const serviceYaml = +` +apiVersion: v1 +kind: Service +metadata: + name: ${serviceName} + namespace: ${testNamespace} +spec: + ports: + - port: 6000 + targetPort: 6000 + selector: + app: ${scalerName} +` + +// Deployment +const deploymentYaml = +`apiVersion: apps/v1 +kind: Deployment +metadata: + name: ${deploymentName} + namespace: ${testNamespace} +spec: + replicas: 0 + selector: + matchLabels: + app: ${deploymentName} + template: + metadata: + labels: + app: ${deploymentName} + spec: + containers: + - name: nginx + image: nginx:1.16.1 +` + +// Scaled Object +const scaledObjectYaml = +` +apiVersion: keda.sh/v1alpha1 +kind: ScaledObject +metadata: + name: ${scaledObjectName} + namespace: ${testNamespace} +spec: + scaleTargetRef: + name: ${deploymentName} + pollingInterval: 5 + cooldownPeriod: 10 + idleReplicaCount: ${idleReplicaCount} + minReplicaCount: ${minReplicaCount} + maxReplicaCount: ${maxReplicaCount} + triggers: + - type: external + metadata: + scalerAddress: ${serviceName}.${testNamespace}:6000 + metricThreshold: "${threshold}" + metricValue: "{{VALUE}}" +` diff --git a/tests/scalers/helpers.ts b/tests/scalers/helpers.ts index 45ed5bceaa1..5af7677d0fe 100644 --- a/tests/scalers/helpers.ts +++ b/tests/scalers/helpers.ts @@ -31,6 +31,12 @@ export async function createNamespace(namespace: string) { sh.exec(`kubectl apply -f ${namespaceFile.name}`) } +export function createYamlFile(yaml: string) { + const tmpFile = tmp.fileSync() + fs.writeFileSync(tmpFile.name, yaml) + return tmpFile.name +} + const namespaceTemplate = `apiVersion: v1 kind: Namespace metadata: From 0370cd8081bf4e8b2438ded8c6ca365d75805d29 Mon Sep 17 00:00:00 2001 From: Vighnesh Shenoy Date: Thu, 7 Apr 2022 16:37:39 +0530 Subject: [PATCH 3/7] Update CHANGELOG. Signed-off-by: Vighnesh Shenoy --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f61d750aed..d4f58920057 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,8 @@ - **AWS DynamoDB:** Setup AWS DynamoDB test account ([#2803](https://github.com/kedacore/keda/issues/2803)) - **AWS Kinesis Stream:** Adding e2e test ([#1526](https://github.com/kedacore/keda/issues/1526)) - **AWS SQS Queue:** Adding e2e test ([#1527](https://github.com/kedacore/keda/issues/1527)) +- **External Scaler:** Adding e2e test. ([#2697](https://github.com/kedacore/keda/issues/2697)) +- **External Scaler:** Fix issue with internal KEDA core prefix being passed to external scaler. ([#2640](https://github.com/kedacore/keda/issues/2640)) - **GCP Pubsub Scaler:** Adding e2e test ([#1528](https://github.com/kedacore/keda/issues/1528)) - **Hashicorp Vault Secret Provider:** Adding e2e test ([#2842](https://github.com/kedacore/keda/issues/2842)) - **Memory Scaler:** Adding e2e test ([#2220](https://github.com/kedacore/keda/issues/2220)) From a02559698c3aa0a24b980985afeb7f2abc838a3b Mon Sep 17 00:00:00 2001 From: Vighnesh Shenoy Date: Thu, 7 Apr 2022 16:42:15 +0530 Subject: [PATCH 4/7] Remove TODO. Signed-off-by: Vighnesh Shenoy --- tests/scalers/external-scaler.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/scalers/external-scaler.test.ts b/tests/scalers/external-scaler.test.ts index 15f76a1cee9..450f932e3ad 100644 --- a/tests/scalers/external-scaler.test.ts +++ b/tests/scalers/external-scaler.test.ts @@ -15,8 +15,7 @@ const maxReplicaCount = 2 const threshold = 10 test.before(async t => { - // TODO - Uncomment - // sh.config.silent = true + sh.config.silent = true // Create Kubernetes Namespace createNamespace(testNamespace) From 905e6177d69023430822508a66b9f59c1a473552 Mon Sep 17 00:00:00 2001 From: Vighnesh Shenoy Date: Fri, 8 Apr 2022 13:52:46 +0530 Subject: [PATCH 5/7] Add tests. Signed-off-by: Vighnesh Shenoy --- pkg/scalers/external_scaler.go | 6 +++++- pkg/scalers/scaler.go | 16 ++++++++++++++++ pkg/scalers/scaler_test.go | 35 ++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/pkg/scalers/external_scaler.go b/pkg/scalers/external_scaler.go index 5dee3a0a0bb..980ffaabfc5 100644 --- a/pkg/scalers/external_scaler.go +++ b/pkg/scalers/external_scaler.go @@ -198,7 +198,11 @@ func (s *externalScaler) GetMetrics(ctx context.Context, metricName string, metr defer done() // Remove the sX- prefix as the external scaler shouldn't have to know about it - metricName = strings.SplitN(metricName, "-", 2)[1] + metricName, err = GenerateMetricNameWithoutIndex(s.metadata.scalerIndex, metricName) + if err != nil { + return metrics, err + } + request := &pb.GetMetricsRequest{ MetricName: metricName, ScaledObjectRef: &s.scaledObjectRef, diff --git a/pkg/scalers/scaler.go b/pkg/scalers/scaler.go index 049e54111d8..1e3958d2080 100644 --- a/pkg/scalers/scaler.go +++ b/pkg/scalers/scaler.go @@ -19,6 +19,7 @@ package scalers import ( "context" "fmt" + "strings" "time" "k8s.io/api/autoscaling/v2beta2" @@ -110,6 +111,21 @@ func GenerateMetricNameWithIndex(scalerIndex int, metricName string) string { return fmt.Sprintf("s%d-%s", scalerIndex, metricName) } +// GenerateMetricNameWithoutIndex removes the index prefix from the metric name +func GenerateMetricNameWithoutIndex(scalerIndex int, metricName string) (string, error) { + metricNameSplit := strings.SplitN(metricName, "-", 2) + if len(metricNameSplit) != 2 { + return "", fmt.Errorf("metric name without index prefix") + } + + indexPrefix, metricNameWithoutIndex := metricNameSplit[0], metricNameSplit[1] + if indexPrefix != fmt.Sprintf("s%d", scalerIndex) { + return "", fmt.Errorf("metric name contains incorrect index prefix") + } + + return metricNameWithoutIndex, nil +} + // GetMetricTargetType helps getting the metric target type of the scaler func GetMetricTargetType(config *ScalerConfig) (v2beta2.MetricTargetType, error) { switch config.MetricType { diff --git a/pkg/scalers/scaler_test.go b/pkg/scalers/scaler_test.go index 1a4b40f2e15..0f7278c3ef5 100644 --- a/pkg/scalers/scaler_test.go +++ b/pkg/scalers/scaler_test.go @@ -85,3 +85,38 @@ func TestGetMetricTarget(t *testing.T) { }) } } + +func TestGenerateMetricNameWithoutIndex(t *testing.T) { + cases := []struct { + scalerIndex int + metricName string + expectedMetricNameWithoutIndexPrefix string + isError bool + }{ + // Proper input + {scalerIndex: 0, metricName: "s0-metricName", expectedMetricNameWithoutIndexPrefix: "metricName", isError: false}, + // Incorrect index prefix + {scalerIndex: 1, metricName: "s0-metricName", expectedMetricNameWithoutIndexPrefix: "", isError: true}, + // Incorrect index prefix + {scalerIndex: 0, metricName: "0-metricName", expectedMetricNameWithoutIndexPrefix: "", isError: true}, + // No index prefix + {scalerIndex: 0, metricName: "metricName", expectedMetricNameWithoutIndexPrefix: "", isError: true}, + } + + for _, testCase := range cases { + metricName, err := GenerateMetricNameWithoutIndex(testCase.scalerIndex, testCase.metricName) + if err != nil && !testCase.isError { + t.Error("Expected success but got error", err) + } + + if testCase.isError && err == nil { + t.Error("Expected error but got success") + } + + if err == nil { + if metricName != testCase.expectedMetricNameWithoutIndexPrefix { + t.Errorf("Expected - %s, Got - %s", testCase.expectedMetricNameWithoutIndexPrefix, metricName) + } + } + } +} From 8a1fc4c1994b97eee3a9e68f2ad4325d45af339e Mon Sep 17 00:00:00 2001 From: Vighnesh Shenoy Date: Fri, 8 Apr 2022 14:34:47 +0530 Subject: [PATCH 6/7] Modify function name. Signed-off-by: Vighnesh Shenoy --- pkg/scalers/external_scaler.go | 2 +- pkg/scalers/scaler.go | 4 ++-- pkg/scalers/scaler_test.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/scalers/external_scaler.go b/pkg/scalers/external_scaler.go index 980ffaabfc5..a0cd8857c4e 100644 --- a/pkg/scalers/external_scaler.go +++ b/pkg/scalers/external_scaler.go @@ -198,7 +198,7 @@ func (s *externalScaler) GetMetrics(ctx context.Context, metricName string, metr defer done() // Remove the sX- prefix as the external scaler shouldn't have to know about it - metricName, err = GenerateMetricNameWithoutIndex(s.metadata.scalerIndex, metricName) + metricName, err = RemoveIndexFromMetricName(s.metadata.scalerIndex, metricName) if err != nil { return metrics, err } diff --git a/pkg/scalers/scaler.go b/pkg/scalers/scaler.go index 1e3958d2080..2176298ec67 100644 --- a/pkg/scalers/scaler.go +++ b/pkg/scalers/scaler.go @@ -111,8 +111,8 @@ func GenerateMetricNameWithIndex(scalerIndex int, metricName string) string { return fmt.Sprintf("s%d-%s", scalerIndex, metricName) } -// GenerateMetricNameWithoutIndex removes the index prefix from the metric name -func GenerateMetricNameWithoutIndex(scalerIndex int, metricName string) (string, error) { +// RemoveIndexFromMetricName removes the index prefix from the metric name +func RemoveIndexFromMetricName(scalerIndex int, metricName string) (string, error) { metricNameSplit := strings.SplitN(metricName, "-", 2) if len(metricNameSplit) != 2 { return "", fmt.Errorf("metric name without index prefix") diff --git a/pkg/scalers/scaler_test.go b/pkg/scalers/scaler_test.go index 0f7278c3ef5..ecebd73b79e 100644 --- a/pkg/scalers/scaler_test.go +++ b/pkg/scalers/scaler_test.go @@ -86,7 +86,7 @@ func TestGetMetricTarget(t *testing.T) { } } -func TestGenerateMetricNameWithoutIndex(t *testing.T) { +func TestRemoveIndexFromMetricName(t *testing.T) { cases := []struct { scalerIndex int metricName string @@ -104,7 +104,7 @@ func TestGenerateMetricNameWithoutIndex(t *testing.T) { } for _, testCase := range cases { - metricName, err := GenerateMetricNameWithoutIndex(testCase.scalerIndex, testCase.metricName) + metricName, err := RemoveIndexFromMetricName(testCase.scalerIndex, testCase.metricName) if err != nil && !testCase.isError { t.Error("Expected success but got error", err) } From 46cfd44c794122c1ba1678913674b55cbb21bf6d Mon Sep 17 00:00:00 2001 From: Vighnesh Shenoy Date: Fri, 8 Apr 2022 15:49:30 +0530 Subject: [PATCH 7/7] Add test case with scaler index > 9. Signed-off-by: Vighnesh Shenoy --- pkg/scalers/scaler_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/scalers/scaler_test.go b/pkg/scalers/scaler_test.go index ecebd73b79e..fced7eb5969 100644 --- a/pkg/scalers/scaler_test.go +++ b/pkg/scalers/scaler_test.go @@ -95,6 +95,8 @@ func TestRemoveIndexFromMetricName(t *testing.T) { }{ // Proper input {scalerIndex: 0, metricName: "s0-metricName", expectedMetricNameWithoutIndexPrefix: "metricName", isError: false}, + // Proper input with scalerIndex > 9 + {scalerIndex: 123, metricName: "s123-metricName", expectedMetricNameWithoutIndexPrefix: "metricName", isError: false}, // Incorrect index prefix {scalerIndex: 1, metricName: "s0-metricName", expectedMetricNameWithoutIndexPrefix: "", isError: true}, // Incorrect index prefix