diff --git a/pkg/reconciler/common/ha.go b/pkg/reconciler/common/ha.go index 3d1004e099..9686086586 100644 --- a/pkg/reconciler/common/ha.go +++ b/pkg/reconciler/common/ha.go @@ -18,32 +18,21 @@ package common import ( mf "github.com/manifestival/manifestival" - "go.uber.org/zap" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/sets" "knative.dev/operator/pkg/apis/operator/base" ) -func haUnSupported(obj base.KComponent) sets.String { +func haUnSupported(name string) bool { return sets.NewString( "pingsource-mt-adapter", - ) -} - -// When Deployment has HPA, the replicas should be controlled by HPA's minReplicas instead of operator. -// Hence, skip changing the spec.replicas in deployment directory for these Deployments. -func hasHorizontalPodAutoscaler(obj base.KComponent) sets.String { - return sets.NewString( - "webhook", - "activator", - "3scale-kourier-gateway", - ) + ).Has(name) } // HighAvailabilityTransform mutates configmaps and replicacounts of certain // controllers when HA control plane is specified. -func HighAvailabilityTransform(obj base.KComponent, log *zap.SugaredLogger) mf.Transformer { +func HighAvailabilityTransform(obj base.KComponent) mf.Transformer { return func(u *unstructured.Unstructured) error { // Use spec.deployments.replicas for the deployment instead of spec.high-availability. for _, override := range obj.GetSpec().GetWorkloadOverrides() { @@ -61,39 +50,14 @@ func HighAvailabilityTransform(obj base.KComponent, log *zap.SugaredLogger) mf.T replicas := int64(*ha.Replicas) // Transform deployments that support HA. - if u.GetKind() == "Deployment" && !haUnSupported(obj).Has(u.GetName()) && !hasHorizontalPodAutoscaler(obj).Has(u.GetName()) { + if u.GetKind() == "Deployment" && !haUnSupported(u.GetName()) && !hasHorizontalPodAutoscaler(u.GetName()) { if err := unstructured.SetNestedField(u.Object, replicas, "spec", "replicas"); err != nil { return err } } if u.GetKind() == "HorizontalPodAutoscaler" { - min, _, err := unstructured.NestedInt64(u.Object, "spec", "minReplicas") - if err != nil { - return err - } - // Do nothing if the HPA ships with even more replicas out of the box. - if min >= replicas { - return nil - } - - if err := unstructured.SetNestedField(u.Object, replicas, "spec", "minReplicas"); err != nil { - return err - } - - max, found, err := unstructured.NestedInt64(u.Object, "spec", "maxReplicas") - if err != nil { - return err - } - - // Do nothing if maxReplicas is not defined. - if !found { - return nil - } - - // Increase maxReplicas to the amount that we increased, - // because we need to avoid minReplicas > maxReplicas happenning. - if err := unstructured.SetNestedField(u.Object, max+(replicas-min), "spec", "maxReplicas"); err != nil { + if err := hpaTransform(u, replicas); err != nil { return err } } diff --git a/pkg/reconciler/common/ha_test.go b/pkg/reconciler/common/ha_test.go index a3ef3ed3c6..a9ae53aa15 100644 --- a/pkg/reconciler/common/ha_test.go +++ b/pkg/reconciler/common/ha_test.go @@ -23,7 +23,6 @@ import ( "knative.dev/operator/pkg/apis/operator/v1beta1" appsv1 "k8s.io/api/apps/v1" - autoscalingv2beta1 "k8s.io/api/autoscaling/v2beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/kubernetes/scheme" @@ -74,7 +73,7 @@ func TestHighAvailabilityTransform(t *testing.T) { in: makeUnstructuredHPA(t, "activator", 2, 5), expected: makeUnstructuredHPA(t, "activator", 2, 5), }, { - name: "HA; adjust hpa when replicas is lerger than maxReplicas", + name: "HA; adjust hpa when replicas is larger than maxReplicas", config: makeHa(6), in: makeUnstructuredHPA(t, "activator", 2, 5), expected: makeUnstructuredHPA(t, "activator", 6, 9), // maxReplicas is increased by max+(replicas-min) to avoid minReplicas > maxReplicas happenning. @@ -107,7 +106,7 @@ func TestHighAvailabilityTransform(t *testing.T) { }, }, } - haTransform := HighAvailabilityTransform(instance, log) + haTransform := HighAvailabilityTransform(instance) err := haTransform(tc.in) util.AssertDeepEqual(t, err, tc.err) @@ -143,23 +142,3 @@ func makeUnstructuredDeploymentReplicas(t *testing.T, name string, replicas int3 return result } - -func makeUnstructuredHPA(t *testing.T, name string, minReplicas, maxReplicas int32) *unstructured.Unstructured { - hpa := &autoscalingv2beta1.HorizontalPodAutoscaler{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - Spec: autoscalingv2beta1.HorizontalPodAutoscalerSpec{ - MinReplicas: &minReplicas, - MaxReplicas: maxReplicas, - }, - } - - result := &unstructured.Unstructured{} - err := scheme.Scheme.Convert(hpa, result, nil) - if err != nil { - t.Fatalf("Could not create unstructured HPA: %v, err: %v", hpa, err) - } - - return result -} diff --git a/pkg/reconciler/common/hpa.go b/pkg/reconciler/common/hpa.go new file mode 100644 index 0000000000..3c592db625 --- /dev/null +++ b/pkg/reconciler/common/hpa.go @@ -0,0 +1,71 @@ +/* +Copyright 2023 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package common + +import ( + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" +) + +// When Deployment has HPA, the replicas should be controlled by HPA's minReplicas instead of operator. +// Hence, skip changing the spec.replicas in deployment directory for these Deployments. +func hasHorizontalPodAutoscaler(name string) bool { + return sets.NewString( + "webhook", + "activator", + "3scale-kourier-gateway", + ).Has(name) +} + +// hpaTransform sets the minReplicas and maxReplicas of an HPA based on a replica override value. +// If minReplica needs to be increased, the maxReplica is increased by the same value. +func hpaTransform(u *unstructured.Unstructured, replicas int64) error { + if u.GetKind() != "HorizontalPodAutoscaler" { + return nil + } + + min, _, err := unstructured.NestedInt64(u.Object, "spec", "minReplicas") + if err != nil { + return err + } + + // Do nothing if the HPA ships with even more replicas out of the box. + if min >= replicas { + return nil + } + + if err := unstructured.SetNestedField(u.Object, replicas, "spec", "minReplicas"); err != nil { + return err + } + + max, found, err := unstructured.NestedInt64(u.Object, "spec", "maxReplicas") + if err != nil { + return err + } + + // Do nothing if maxReplicas is not defined. + if !found { + return nil + } + + // Increase maxReplicas to the amount that we increased, + // because we need to avoid minReplicas > maxReplicas happenning. + if err := unstructured.SetNestedField(u.Object, max+(replicas-min), "spec", "maxReplicas"); err != nil { + return err + } + return nil +} diff --git a/pkg/reconciler/common/hpa_test.go b/pkg/reconciler/common/hpa_test.go new file mode 100644 index 0000000000..8d6fce4fb0 --- /dev/null +++ b/pkg/reconciler/common/hpa_test.go @@ -0,0 +1,85 @@ +/* +Copyright 2023 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package common + +import ( + "testing" + + "k8s.io/api/autoscaling/v2beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/kubernetes/scheme" + util "knative.dev/operator/pkg/reconciler/common/testing" +) + +func TestHpaTransform(t *testing.T) { + cases := []struct { + name string + in *unstructured.Unstructured + replicas int64 + expected *unstructured.Unstructured + err error + }{{ + name: "Object is not a HPA", + in: makeUnstructuredDeployment(t, "not-a-hpa"), + replicas: 5, + expected: makeUnstructuredDeployment(t, "not-a-hpa"), + err: nil, + }, { + name: "minReplicas same as override", + in: makeUnstructuredHPA(t, "hpa", 1, 2), + replicas: 1, + expected: makeUnstructuredHPA(t, "hpa", 1, 2), + err: nil, + }, { + name: "minReplicas lower than override", + in: makeUnstructuredHPA(t, "hpa", 1, 2), + replicas: 5, + expected: makeUnstructuredHPA(t, "hpa", 5, 6), + err: nil, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + + err := hpaTransform(tc.in, tc.replicas) + + util.AssertDeepEqual(t, err, tc.err) + util.AssertDeepEqual(t, tc.in, tc.expected) + }) + } +} + +func makeUnstructuredHPA(t *testing.T, name string, minReplicas, maxReplicas int32) *unstructured.Unstructured { + hpa := &v2beta1.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: v2beta1.HorizontalPodAutoscalerSpec{ + MinReplicas: &minReplicas, + MaxReplicas: maxReplicas, + }, + } + + result := &unstructured.Unstructured{} + err := scheme.Scheme.Convert(hpa, result, nil) + if err != nil { + t.Fatalf("Could not create unstructured HPA: %v, err: %v", hpa, err) + } + + return result +} diff --git a/pkg/reconciler/common/transformers.go b/pkg/reconciler/common/transformers.go index 00ff77d592..a5923a90fe 100644 --- a/pkg/reconciler/common/transformers.go +++ b/pkg/reconciler/common/transformers.go @@ -31,7 +31,7 @@ func transformers(ctx context.Context, obj base.KComponent) []mf.Transformer { injectOwner(obj), mf.InjectNamespace(obj.GetNamespace()), JobTransform(obj), - HighAvailabilityTransform(obj, logger), + HighAvailabilityTransform(obj), ImageTransform(obj.GetSpec().GetRegistry(), logger), ConfigMapTransform(obj.GetSpec().GetConfig(), logger), ResourceRequirementsTransform(obj, logger), diff --git a/pkg/reconciler/common/workload_override.go b/pkg/reconciler/common/workload_override.go index 8f334f6a75..14bf45614d 100644 --- a/pkg/reconciler/common/workload_override.go +++ b/pkg/reconciler/common/workload_override.go @@ -47,7 +47,9 @@ func OverridesTransform(overrides []base.WorkloadOverride, log *zap.SugaredLogge } obj = deployment ps = &deployment.Spec.Template - if override.Replicas != nil { + + // Do not set replicas, if this resource is controlled by a HPA + if override.Replicas != nil && !hasHorizontalPodAutoscaler(override.Name) { deployment.Spec.Replicas = override.Replicas } } @@ -58,7 +60,9 @@ func OverridesTransform(overrides []base.WorkloadOverride, log *zap.SugaredLogge } obj = ss ps = &ss.Spec.Template - if override.Replicas != nil { + + // Do not set replicas, if this resource is controlled by a HPA + if override.Replicas != nil && !hasHorizontalPodAutoscaler(override.Name) { ss.Spec.Replicas = override.Replicas } } @@ -71,6 +75,13 @@ func OverridesTransform(overrides []base.WorkloadOverride, log *zap.SugaredLogge ps = &job.Spec.Template } + if u.GetKind() == "HorizontalPodAutoscaler" && u.GetName() == override.Name && override.Replicas != nil { + overrideReplicas := int64(*override.Replicas) + if err := hpaTransform(u, overrideReplicas); err != nil { + return err + } + } + if obj == nil { continue } diff --git a/pkg/reconciler/common/workload_override_test.go b/pkg/reconciler/common/workload_override_test.go index 91d9a75d49..1185b571b6 100644 --- a/pkg/reconciler/common/workload_override_test.go +++ b/pkg/reconciler/common/workload_override_test.go @@ -24,6 +24,7 @@ import ( mf "github.com/manifestival/manifestival" "google.golang.org/api/googleapi" appsv1 "k8s.io/api/apps/v1" + "k8s.io/api/autoscaling/v2beta1" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" @@ -56,6 +57,11 @@ type expDeployments struct { expDNSPolicy *corev1.DNSPolicy } +type expHorizontalPodAutoscalers struct { + expMinReplicas int32 + expMaxReplicas int32 +} + type expJobs struct { expNodeSelector map[string]string expTolerations []corev1.Toleration @@ -65,12 +71,13 @@ func TestComponentsTransform(t *testing.T) { var four int32 = 4 var five int32 = 5 var defaultDnsPolicy = corev1.DNSPolicy("") - var dnsClusterFirstWithHostNet corev1.DNSPolicy = corev1.DNSClusterFirstWithHostNet + var dnsClusterFirstWithHostNet = corev1.DNSClusterFirstWithHostNet tests := []struct { - name string - override []base.WorkloadOverride - globalReplicas int32 - expDeployment map[string]expDeployments + name string + override []base.WorkloadOverride + globalReplicas int32 + expDeployment map[string]expDeployments + expHorizontalPodAutoscaler map[string]expHorizontalPodAutoscalers }{{ name: "no override", override: nil, @@ -544,7 +551,7 @@ func TestComponentsTransform(t *testing.T) { }, }}, }, { - name: "neither replicas in deploymentoverride nor global replicas", + name: "neither replicas in deployment override nor global replicas", override: []base.WorkloadOverride{ {Name: "controller"}, }, @@ -672,7 +679,7 @@ func TestComponentsTransform(t *testing.T) { expTemplateLabels: map[string]string{"serving.knative.dev/release": "v0.13.0", "app": "webhook", "role": "webhook", "e": "f"}, expAnnotations: map[string]string{"g": "h"}, expTemplateAnnotations: map[string]string{"cluster-autoscaler.kubernetes.io/safe-to-evict": "false", "g": "h"}, - expReplicas: 4, + expReplicas: 0, expNodeSelector: map[string]string{"env": "prod"}, expTopologySpreadConstraints: []corev1.TopologySpreadConstraint{{ MaxSkew: 1, @@ -705,6 +712,29 @@ func TestComponentsTransform(t *testing.T) { expDNSPolicy: &dnsClusterFirstWithHostNet, }, }, + }, { + name: "HPA activator override minReplicas", + override: []base.WorkloadOverride{ + { + Name: "activator", + Replicas: &four, + }, + }, + expDeployment: map[string]expDeployments{ + "activator": { + expLabels: map[string]string{"serving.knative.dev/release": "v0.13.0"}, + expTemplateLabels: map[string]string{"serving.knative.dev/release": "v0.13.0", "app": "activator", "role": "activator"}, + expAnnotations: nil, + expTemplateAnnotations: map[string]string{"cluster-autoscaler.kubernetes.io/safe-to-evict": "false"}, + expReplicas: 0, + }, + }, + expHorizontalPodAutoscaler: map[string]expHorizontalPodAutoscalers{ + "activator": { + expMinReplicas: four, + expMaxReplicas: 23, // in manifest.yaml maxReplicas=20 +3 (difference between existing min and overwritten min) + }, + }, }} for _, test := range tests { @@ -739,8 +769,7 @@ func TestComponentsTransform(t *testing.T) { for key, ks := range kss { t.Run(key, func(t *testing.T) { - //manifest, err = manifest.Transform(OverridesTransform(ks, log), HighAvailabilityTransform(ks, log)) - manifest, err = manifest.Transform(HighAvailabilityTransform(ks, log), OverridesTransform(ks.GetSpec().GetWorkloadOverrides(), log)) + manifest, err = manifest.Transform(HighAvailabilityTransform(ks), OverridesTransform(ks.GetSpec().GetWorkloadOverrides(), log)) if err != nil { t.Fatalf("Failed to transform manifest: %v", err) } @@ -797,12 +826,12 @@ func TestComponentsTransform(t *testing.T) { } r, l := getProbes(got.Spec.Template.Spec.Containers, expName) if d.expReadinessProbe != nil { - if diff := cmp.Diff(*r, (*d.expReadinessProbe)); diff != "" { + if diff := cmp.Diff(*r, *d.expReadinessProbe); diff != "" { t.Fatalf("Unexpected readiness probe in pod template: %v", diff) } } if d.expLivenessProbe != nil { - if diff := cmp.Diff(*l, (*d.expLivenessProbe)); diff != "" { + if diff := cmp.Diff(*l, *d.expLivenessProbe); diff != "" { t.Fatalf("Unexpected liveness probe in pod template: %v", diff) } } @@ -823,6 +852,29 @@ func TestComponentsTransform(t *testing.T) { } } } + + for expName, d := range test.expHorizontalPodAutoscaler { + for _, u := range manifest.Resources() { + if u.GetKind() == "HorizontalPodAutoscaler" && u.GetName() == expName { + got := &v2beta1.HorizontalPodAutoscaler{} + if err := scheme.Scheme.Convert(&u, got, nil); err != nil { + t.Fatalf("Failed to convert unstructured to deployment: %v", err) + } + + minReplicas := int32(0) + if got.Spec.MinReplicas != nil { + minReplicas = *got.Spec.MinReplicas + } + if diff := cmp.Diff(minReplicas, d.expMinReplicas); diff != "" { + t.Fatalf("Unexpected minReplicas: %v", diff) + } + + if diff := cmp.Diff(got.Spec.MaxReplicas, d.expMaxReplicas); diff != "" { + t.Fatalf("Unexpected maxReplicas: %v", diff) + } + } + } + } }) } })