Refactor CPU/Memory scaler config (#6229)

* Refactor cpu memory scaler Signed-off-by: rickbrouwer <[email protected]> * Fix Signed-off-by: rickbrouwer <[email protected]> --------- Signed-off-by: rickbrouwer <[email protected]>
kedacore · Oct 16, 2024 · 00f4e51 · 00f4e51
1 parent a103204
commit 00f4e51
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 42 deletions.
diff --git a/pkg/scalers/cpu_memory_scaler.go b/pkg/scalers/cpu_memory_scaler.go
@@ -15,25 +15,38 @@ import (
 )
 
 type cpuMemoryScaler struct {
-	metadata     *cpuMemoryMetadata
+	metadata     cpuMemoryMetadata
 	resourceName v1.ResourceName
 	logger       logr.Logger
 }
 
 type cpuMemoryMetadata struct {
-	Type               v2.MetricTargetType
+	Type               string `keda:"name=type,          order=triggerMetadata, enum=Utilization;AverageValue, optional"`
+	Value              string `keda:"name=value,         order=triggerMetadata"`
+	ContainerName      string `keda:"name=containerName, order=triggerMetadata, optional"`
 	AverageValue       *resource.Quantity
 	AverageUtilization *int32
-	ContainerName      string
+	MetricType         v2.MetricTargetType
+}
+
+func (m *cpuMemoryMetadata) Validate() error {
+	return nil
 }
 
 // NewCPUMemoryScaler creates a new cpuMemoryScaler
 func NewCPUMemoryScaler(resourceName v1.ResourceName, config *scalersconfig.ScalerConfig) (Scaler, error) {
 	logger := InitializeLogger(config, "cpu_memory_scaler")
 
-	meta, parseErr := parseResourceMetadata(config, logger)
-	if parseErr != nil {
-		return nil, fmt.Errorf("error parsing %s metadata: %w", resourceName, parseErr)
+	meta, err := parseResourceMetadata(config, logger)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing %s metadata: %w", resourceName, err)
+	}
+
+	if err := meta.Validate(); err != nil {
+		if meta.MetricType == "" {
+			return nil, fmt.Errorf("metricType is required")
+		}
+		return nil, fmt.Errorf("validation error: %w", err)
 	}
 
 	return &cpuMemoryScaler{
@@ -43,62 +56,71 @@ func NewCPUMemoryScaler(resourceName v1.ResourceName, config *scalersconfig.Scal
 	}, nil
 }
 
-func parseResourceMetadata(config *scalersconfig.ScalerConfig, logger logr.Logger) (*cpuMemoryMetadata, error) {
-	meta := &cpuMemoryMetadata{}
-	var value string
-	var ok bool
-	value, ok = config.TriggerMetadata["type"]
-	switch {
-	case ok && value != "" && config.MetricType != "":
-		return nil, fmt.Errorf("only one of trigger.metadata.type or trigger.metricType should be defined")
-	case ok && value != "":
-		logger.V(0).Info("trigger.metadata.type is deprecated in favor of trigger.metricType")
-		meta.Type = v2.MetricTargetType(value)
-	case config.MetricType != "":
-		meta.Type = config.MetricType
-	default:
-		return nil, fmt.Errorf("no type given in neither trigger.metadata.type or trigger.metricType")
+func parseResourceMetadata(config *scalersconfig.ScalerConfig, logger logr.Logger) (cpuMemoryMetadata, error) {
+	meta := cpuMemoryMetadata{}
+	err := config.TypedConfig(&meta)
+	if err != nil {
+		return meta, err
+	}
+
+	if config.MetricType != "" {
+		meta.MetricType = config.MetricType
 	}
 
-	if value, ok = config.TriggerMetadata["value"]; !ok || value == "" {
-		return nil, fmt.Errorf("no value given")
+	// This is deprecated and can be removed later
+	if meta.Type != "" {
+		logger.Info("The 'type' setting is DEPRECATED and will be removed in v2.18 - Use 'metricType' instead.")
+		switch meta.Type {
+		case "AverageValue":
+			meta.MetricType = v2.AverageValueMetricType
+		case "Utilization":
+			meta.MetricType = v2.UtilizationMetricType
+		default:
+			return meta, fmt.Errorf("unknown metric type: %s, allowed values are 'Utilization' or 'AverageValue'", meta.Type)
+		}
 	}
-	switch meta.Type {
+
+	switch meta.MetricType {
 	case v2.AverageValueMetricType:
-		averageValueQuantity := resource.MustParse(value)
+		averageValueQuantity := resource.MustParse(meta.Value)
 		meta.AverageValue = &averageValueQuantity
 	case v2.UtilizationMetricType:
-		valueNum, err := strconv.ParseInt(value, 10, 32)
+		utilizationNum, err := parseUtilization(meta.Value)
 		if err != nil {
-			return nil, err
+			return meta, err
 		}
-		utilizationNum := int32(valueNum)
-		meta.AverageUtilization = &utilizationNum
+		meta.AverageUtilization = utilizationNum
 	default:
-		return nil, fmt.Errorf("unsupported metric type, allowed values are 'Utilization' or 'AverageValue'")
-	}
-
-	if value, ok = config.TriggerMetadata["containerName"]; ok && value != "" {
-		meta.ContainerName = value
+		return meta, fmt.Errorf("unknown metric type: %s, allowed values are 'Utilization' or 'AverageValue'", string(meta.MetricType))
 	}
 
 	return meta, nil
 }
 
+func parseUtilization(value string) (*int32, error) {
+	valueNum, err := strconv.ParseInt(value, 10, 32)
+	if err != nil {
+		return nil, err
+	}
+	utilizationNum := int32(valueNum)
+	return &utilizationNum, nil
+}
+
 // Close no need for cpuMemory scaler
 func (s *cpuMemoryScaler) Close(context.Context) error {
 	return nil
 }
 
 // GetMetricSpecForScaling returns the metric spec for the HPA
 func (s *cpuMemoryScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSpec {
-	var metricSpec v2.MetricSpec
+	metricType := s.metadata.MetricType
 
+	var metricSpec v2.MetricSpec
 	if s.metadata.ContainerName != "" {
 		containerCPUMemoryMetric := &v2.ContainerResourceMetricSource{
 			Name: s.resourceName,
 			Target: v2.MetricTarget{
-				Type:               s.metadata.Type,
+				Type:               metricType,
 				AverageUtilization: s.metadata.AverageUtilization,
 				AverageValue:       s.metadata.AverageValue,
 			},
@@ -109,7 +131,7 @@ func (s *cpuMemoryScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSp
 		cpuMemoryMetric := &v2.ResourceMetricSource{
 			Name: s.resourceName,
 			Target: v2.MetricTarget{
-				Type:               s.metadata.Type,
+				Type:               metricType,
 				AverageUtilization: s.metadata.AverageUtilization,
 				AverageValue:       s.metadata.AverageValue,
 			},

diff --git a/pkg/scalers/cpu_memory_scaler_test.go b/pkg/scalers/cpu_memory_scaler_test.go
@@ -18,7 +18,6 @@ type parseCPUMemoryMetadataTestData struct {
 	isError    bool
 }
 
-// A complete valid metadata example for reference
 var validCPUMemoryMetadata = map[string]string{
 	"type":  "Utilization",
 	"value": "50",
@@ -44,17 +43,18 @@ var testCPUMemoryMetadata = []parseCPUMemoryMetadataTestData{
 }
 
 func TestCPUMemoryParseMetadata(t *testing.T) {
-	for _, testData := range testCPUMemoryMetadata {
+	logger := logr.Discard()
+	for i, testData := range testCPUMemoryMetadata {
 		config := &scalersconfig.ScalerConfig{
 			TriggerMetadata: testData.metadata,
 			MetricType:      testData.metricType,
 		}
-		_, err := parseResourceMetadata(config, logr.Discard())
+		_, err := parseResourceMetadata(config, logger)
 		if err != nil && !testData.isError {
-			t.Error("Expected success but got error", err)
+			t.Errorf("Test case %d: Expected success but got error: %v", i, err)
 		}
 		if testData.isError && err == nil {
-			t.Error("Expected error but got success")
+			t.Errorf("Test case %d: Expected error but got success", i)
 		}
 	}
 }