Fixing HPA creation when multiple metrics specs are specified

Signed-off-by: Vishesh Tanksale <[email protected]>

internal/controller/platform/standalone/nimservice.go

@@ -28,7 +28,7 @@ import (
 	"github.com/go-logr/logr"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
 	appsv1 "k8s.io/api/apps/v1"
-	autoscalingv1 "k8s.io/api/autoscaling/v1"
+	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -125,7 +125,7 @@ func (r *NIMServiceReconciler) reconcileNIMService(ctx context.Context, nimServi
 
 	// Sync HPA
 	if nimService.IsAutoScalingEnabled() {
-		err = r.renderAndSyncResource(ctx, nimService, &renderer, &autoscalingv1.HorizontalPodAutoscaler{}, func() (client.Object, error) {
+		err = r.renderAndSyncResource(ctx, nimService, &renderer, &autoscalingv2.HorizontalPodAutoscaler{}, func() (client.Object, error) {
 			return renderer.HPA(nimService.GetHPAParams())
 		}, "hpa", conditions.ReasonHPAFailed)
 		if err != nil {

internal/controller/platform/standalone/nimservice_test.go

@@ -32,7 +32,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
-	autoscalingv1 "k8s.io/api/autoscaling/v1"
+	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -79,7 +79,7 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
 		Expect(appsv1alpha1.AddToScheme(scheme)).To(Succeed())
 		Expect(appsv1.AddToScheme(scheme)).To(Succeed())
 		Expect(rbacv1.AddToScheme(scheme)).To(Succeed())
-		Expect(autoscalingv1.AddToScheme(scheme)).To(Succeed())
+		Expect(autoscalingv2.AddToScheme(scheme)).To(Succeed())
 		Expect(networkingv1.AddToScheme(scheme)).To(Succeed())
 		Expect(corev1.AddToScheme(scheme)).To(Succeed())
 
@@ -99,6 +99,7 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
 			renderer: render.NewRenderer(path.Join(strings.TrimSuffix(cwd, "internal/controller/platform/standalone"), "manifests")),
 		}
 		pvcName := "test-pvc"
+		minReplicas := int32(1)
 		nimService = &appsv1alpha1.NIMService{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "test-nimservice",
@@ -172,7 +173,31 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
 						Effect:   corev1.TaintEffectNoSchedule,
 					},
 				},
-				Scale: appsv1alpha1.Autoscaling{Enabled: ptr.To[bool](false)},
+				Scale: appsv1alpha1.Autoscaling{
+					Enabled: ptr.To[bool](true),
+					HPA: appsv1alpha1.HorizontalPodAutoscalerSpec{
+						MinReplicas: &minReplicas,
+						MaxReplicas: 10,
+						Metrics: []autoscalingv2.MetricSpec{
+							{
+								Type: autoscalingv2.ResourceMetricSourceType,
+								Resource: &autoscalingv2.ResourceMetricSource{
+									Target: autoscalingv2.MetricTarget{
+										Type: autoscalingv2.UtilizationMetricType,
+									},
+								},
+							},
+							{
+								Type: autoscalingv2.PodsMetricSourceType,
+								Pods: &autoscalingv2.PodsMetricSource{
+									Target: autoscalingv2.MetricTarget{
+										Type: autoscalingv2.UtilizationMetricType,
+									},
+								},
+							},
+						},
+					},
+				},
 				ReadinessProbe: appsv1alpha1.Probe{
 					Enabled: &boolTrue,
 					Probe: &corev1.Probe{
@@ -323,6 +348,15 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
 			Expect(service.Name).To(Equal(nimService.GetName()))
 			Expect(service.Namespace).To(Equal(nimService.GetNamespace()))
 
+			// HPA should be deployed
+			hpa := &autoscalingv2.HorizontalPodAutoscaler{}
+			err = client.Get(context.TODO(), namespacedName, hpa)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(hpa.Name).To(Equal(nimService.GetName()))
+			Expect(hpa.Namespace).To(Equal(nimService.GetNamespace()))
+			Expect(*hpa.Spec.MinReplicas).To(Equal(int32(1)))
+			Expect(hpa.Spec.MaxReplicas).To(Equal(int32(10)))
+
 			// Deployment should be created
 			deployment := &appsv1.Deployment{}
 			err = client.Get(context.TODO(), namespacedName, deployment)

internal/render/render.go

@@ -348,6 +348,8 @@ func (r *textTemplateRenderer) HPA(params *types.HPAParams) (*autoscalingv2.Hori
 	if err != nil {
 		return nil, fmt.Errorf("error converting unstructured object to HPA: %w", err)
 	}
+	// Sorting to avoid unnecessary object updates
+	hpa.Spec.Metrics = utils.SortHPAMetricsSpec(hpa.Spec.Metrics)
 	return hpa, nil
 }
 

internal/render/render_test.go

@@ -360,5 +360,50 @@ var _ = Describe("K8s Resources Rendering", func() {
 			Expect(*hpa.Spec.MinReplicas).To(Equal(int32(1)))
 			Expect(hpa.Spec.MaxReplicas).To(Equal(int32(10)))
 		})
+
+		It("should render HPA template and sort metrics spec correctly", func() {
+			minRep := int32(1)
+			params := types.HPAParams{
+				Enabled:   true,
+				Name:      "test-hpa",
+				Namespace: "default",
+				HPASpec: autoscalingv2.HorizontalPodAutoscalerSpec{
+					ScaleTargetRef: autoscalingv2.CrossVersionObjectReference{
+						Name:       "test-deployment",
+						Kind:       "Deployment",
+						APIVersion: "apps/v1",
+					},
+					MinReplicas: &minRep,
+					MaxReplicas: 10,
+					Metrics: []autoscalingv2.MetricSpec{
+						{
+							Type: autoscalingv2.ResourceMetricSourceType,
+							Resource: &autoscalingv2.ResourceMetricSource{
+								Target: autoscalingv2.MetricTarget{
+									Type: autoscalingv2.UtilizationMetricType,
+								},
+							},
+						},
+						{
+							Type: autoscalingv2.PodsMetricSourceType,
+							Pods: &autoscalingv2.PodsMetricSource{
+								Target: autoscalingv2.MetricTarget{
+									Type: autoscalingv2.UtilizationMetricType,
+								},
+							},
+						},
+					},
+				},
+			}
+			r := render.NewRenderer(templatesDir)
+			hpa, err := r.HPA(&params)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(hpa.Name).To(Equal("test-hpa"))
+			Expect(hpa.Namespace).To(Equal("default"))
+			Expect(*hpa.Spec.MinReplicas).To(Equal(int32(1)))
+			Expect(hpa.Spec.MaxReplicas).To(Equal(int32(10)))
+			Expect(hpa.Spec.Metrics[0].Type).To(Equal(autoscalingv2.PodsMetricSourceType))
+			Expect(hpa.Spec.Metrics[1].Type).To(Equal(autoscalingv2.ResourceMetricSourceType))
+		})
 	})
 })

internal/utils/utils.go

@@ -23,12 +23,13 @@ import (
 	"hash/fnv"
 	"os"
 	"path/filepath"
+	"reflect"
 	"sort"
 	"strings"
 
+	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/rand"
-	"reflect"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -131,13 +132,30 @@ func SortKeys(obj interface{}) interface{} {
 		}
 		return sortedMap
 	case []interface{}:
-		// Check if the slice contains maps and sort them if so
+		// Check if the slice contains maps and sort them by the "name" field or the first available field
 		if len(obj) > 0 {
+
 			if _, ok := obj[0].(map[string]interface{}); ok {
-				sort.Slice(obj, func(i, j int) bool {
-					iName := obj[i].(map[string]interface{})["name"].(string)
-					jName := obj[j].(map[string]interface{})["name"].(string)
-					return iName < jName
+				sort.SliceStable(obj, func(i, j int) bool {
+					iMap, iOk := obj[i].(map[string]interface{})
+					jMap, jOk := obj[j].(map[string]interface{})
+					if iOk && jOk {
+						// Try to sort by "name" if present
+						iName, iNameOk := iMap["name"].(string)
+						jName, jNameOk := jMap["name"].(string)
+						if iNameOk && jNameOk {
+							return iName < jName
+						}
+
+						// If "name" is not available, sort by the first key in each map
+						if len(iMap) > 0 && len(jMap) > 0 {
+							iFirstKey := firstKey(iMap)
+							jFirstKey := firstKey(jMap)
+							return iFirstKey < jFirstKey
+						}
+					}
+					// If no valid comparison is possible, maintain the original order
+					return false
 				})
 			}
 		}
@@ -148,6 +166,16 @@ func SortKeys(obj interface{}) interface{} {
 	return obj
 }
 
+// Helper function to get the first key of a map (alphabetically sorted)
+func firstKey(m map[string]interface{}) string {
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys[0]
+}
+
 // GetResourceHash returns a consistent hash for the given object spec
 func GetResourceHash(obj client.Object) string {
 	// Convert obj to a map[string]interface{}
@@ -242,3 +270,12 @@ func IsEqual[T client.Object](existing, desired T, fieldsToCompare ...string) bo
 
 	return true
 }
+
+func SortHPAMetricsSpec(metrics []autoscalingv2.MetricSpec) []autoscalingv2.MetricSpec {
+	sort.Slice(metrics, func(i, j int) bool {
+		iMetricsType := metrics[i].Type
+		jMetricsType := metrics[j].Type
+		return iMetricsType < jMetricsType
+	})
+	return metrics
+}