From 27db984a08bab61797ee6bf07a3fd8847d92b2e1 Mon Sep 17 00:00:00 2001 From: Guilherme Vicentin <79584418+gvicentin@users.noreply.github.com> Date: Wed, 27 Mar 2024 17:52:18 -0300 Subject: [PATCH] Enable RpaasInstance shutting down using Shutdown flag (#149) * Enable instance shuting down using Shutdown flag * Fix doc string * Remove unnecessary pointer * Remove pointer in manifest files * Changing isAutoscaleEnabled param * Testing Shutdown flag * Code review --------- Co-authored-by: Guilherme Vicentin --- api/v1alpha1/rpaasinstance_types.go | 6 + api/v1alpha1/zz_generated.deepcopy.go | 40 +++++- .../extensions.tsuru.io_rpaasflavors.yaml | 6 + .../extensions.tsuru.io_rpaasinstances.yaml | 6 + controllers/controller.go | 21 ++- controllers/controller_test.go | 126 ++++++++++++++++++ 6 files changed, 198 insertions(+), 7 deletions(-) diff --git a/api/v1alpha1/rpaasinstance_types.go b/api/v1alpha1/rpaasinstance_types.go index 09d9a4659..7cb43c671 100644 --- a/api/v1alpha1/rpaasinstance_types.go +++ b/api/v1alpha1/rpaasinstance_types.go @@ -134,6 +134,12 @@ type RpaasInstanceSpec struct { // +optional // +kubebuilder:default:=false Suspend *bool `json:"suspend,omitempty"` + + // Shutdown flag tells whether controller should scale down Nginx instances. + // Any assosciated HorizontalPodAutoscaler is remove/created when this flag is toggled. + // +optional + // +kubebuilder:default:=false + Shutdown bool `json:"shutdown,omitempty"` } type DynamicCertificates struct { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 0e83bf711..e99e4e95c 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -285,6 +285,13 @@ func (in *NginxConfig) DeepCopyInto(out *NginxConfig) { *out = new(bool) **out = **in } + if in.TemplateExtraVars != nil { + in, out := &in.TemplateExtraVars, &out.TemplateExtraVars + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NginxConfig. @@ -363,6 +370,11 @@ func (in *RpaasFlavorSpec) DeepCopyInto(out *RpaasFlavorSpec) { *out = new(RpaasInstanceSpec) (*in).DeepCopyInto(*out) } + if in.IncompatibleFlavors != nil { + in, out := &in.IncompatibleFlavors, &out.IncompatibleFlavors + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RpaasFlavorSpec. @@ -380,7 +392,7 @@ func (in *RpaasInstance) DeepCopyInto(out *RpaasInstance) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) in.Spec.DeepCopyInto(&out.Spec) } @@ -447,6 +459,31 @@ func (in *RpaasInstanceAutoscaleSpec) DeepCopy() *RpaasInstanceAutoscaleSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RpaasInstanceExternalAddressesStatus) DeepCopyInto(out *RpaasInstanceExternalAddressesStatus) { + *out = *in + if in.IPs != nil { + in, out := &in.IPs, &out.IPs + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Hostnames != nil { + in, out := &in.Hostnames, &out.Hostnames + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RpaasInstanceExternalAddressesStatus. +func (in *RpaasInstanceExternalAddressesStatus) DeepCopy() *RpaasInstanceExternalAddressesStatus { + if in == nil { + return nil + } + out := new(RpaasInstanceExternalAddressesStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RpaasInstanceList) DeepCopyInto(out *RpaasInstanceList) { *out = *in @@ -606,6 +643,7 @@ func (in *RpaasInstanceSpec) DeepCopy() *RpaasInstanceSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RpaasInstanceStatus) DeepCopyInto(out *RpaasInstanceStatus) { *out = *in + in.ExternalAddresses.DeepCopyInto(&out.ExternalAddresses) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RpaasInstanceStatus. diff --git a/config/crd/bases/extensions.tsuru.io_rpaasflavors.yaml b/config/crd/bases/extensions.tsuru.io_rpaasflavors.yaml index 4ea9e3c3c..676d59a43 100644 --- a/config/crd/bases/extensions.tsuru.io_rpaasflavors.yaml +++ b/config/crd/bases/extensions.tsuru.io_rpaasflavors.yaml @@ -6633,6 +6633,12 @@ spec: Defaults to true. type: boolean type: object + shutdown: + default: false + description: Shutdown flag tells whether controller should scale + down Nginx instances. Any assosciated HorizontalPodAutoscaler + is remove/created when this flag is toggled. + type: boolean suspend: default: false description: Suspend flag tells whether controller should suspend diff --git a/config/crd/bases/extensions.tsuru.io_rpaasinstances.yaml b/config/crd/bases/extensions.tsuru.io_rpaasinstances.yaml index 967964832..22b87130f 100644 --- a/config/crd/bases/extensions.tsuru.io_rpaasinstances.yaml +++ b/config/crd/bases/extensions.tsuru.io_rpaasinstances.yaml @@ -6381,6 +6381,12 @@ spec: true. type: boolean type: object + shutdown: + default: false + description: Shutdown flag tells whether controller should scale down + Nginx instances. Any assosciated HorizontalPodAutoscaler is remove/created + when this flag is toggled. + type: boolean suspend: default: false description: Suspend flag tells whether controller should suspend diff --git a/controllers/controller.go b/controllers/controller.go index ee4e7e18c..39b5bd7b2 100644 --- a/controllers/controller.go +++ b/controllers/controller.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/tsuru/rpaas-operator/api/v1alpha1" @@ -583,7 +584,7 @@ func (r *RpaasInstanceReconciler) reconcileHPA(ctx context.Context, instance *v1 var observed autoscalingv2.HorizontalPodAutoscaler err = r.Client.Get(ctx, types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, &observed) if k8sErrors.IsNotFound(err) { - if !isAutoscaleEnabled(instance.Spec.Autoscale) { + if !isAutoscaleEnabled(&instance.Spec) { logger.V(4).Info("Skipping HorizontalPodAutoscaler reconciliation: both HPA resource and desired RpaasAutoscaleSpec not found") return cleanedKeda, nil } @@ -605,7 +606,7 @@ func (r *RpaasInstanceReconciler) reconcileHPA(ctx context.Context, instance *v1 logger = logger.WithValues("HorizontalPodAutoscaler", types.NamespacedName{Name: observed.Name, Namespace: observed.Namespace}) - if !isAutoscaleEnabled(instance.Spec.Autoscale) { + if !isAutoscaleEnabled(&instance.Spec) { logger.V(4).Info("Deleting HorizontalPodAutoscaler resource") if err = r.Client.Delete(ctx, &observed); err != nil { logger.Error(err, "Unable to delete the HorizontalPodAutoscaler resource") @@ -664,7 +665,7 @@ func (r *RpaasInstanceReconciler) reconcileKEDA(ctx context.Context, instance *v var observed kedav1alpha1.ScaledObject err = r.Client.Get(ctx, types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, &observed) if k8sErrors.IsNotFound(err) { - if !isAutoscaleEnabled(instance.Spec.Autoscale) { + if !isAutoscaleEnabled(&instance.Spec) { return false, nil // nothing to do } @@ -679,7 +680,7 @@ func (r *RpaasInstanceReconciler) reconcileKEDA(ctx context.Context, instance *v return false, err } - if !isAutoscaleEnabled(instance.Spec.Autoscale) { + if !isAutoscaleEnabled(&instance.Spec) { err = r.Client.Delete(ctx, &observed) if err != nil { return false, err @@ -699,12 +700,16 @@ func (r *RpaasInstanceReconciler) reconcileKEDA(ctx context.Context, instance *v return true, nil } -func isAutoscaleEnabled(a *v1alpha1.RpaasInstanceAutoscaleSpec) bool { +func isAutoscaleValid(a *v1alpha1.RpaasInstanceAutoscaleSpec) bool { return a != nil && (a.MinReplicas != nil && a.MaxReplicas > 0) && (a.TargetCPUUtilizationPercentage != nil || a.TargetMemoryUtilizationPercentage != nil || a.TargetRequestsPerSecond != nil || len(a.Schedules) > 0) } +func isAutoscaleEnabled(instance *v1alpha1.RpaasInstanceSpec) bool { + return !instance.Shutdown && isAutoscaleValid(instance.Autoscale) +} + func newKEDAScaledObject(instance *v1alpha1.RpaasInstance, nginx *nginxv1alpha1.Nginx) (*kedav1alpha1.ScaledObject, error) { var triggers []kedav1alpha1.ScaleTriggers if instance.Spec.Autoscale != nil && instance.Spec.Autoscale.TargetCPUUtilizationPercentage != nil { @@ -1186,7 +1191,11 @@ func newNginx(instanceMergedWithFlavors *v1alpha1.RpaasInstance, plan *v1alpha1. } replicas := instanceMergedWithFlavors.Spec.Replicas - if isAutoscaleEnabled(instanceMergedWithFlavors.Spec.Autoscale) { + if shutdown := instanceMergedWithFlavors.Spec.Shutdown; shutdown { + replicas = pointer.Int32(0) + } + + if isAutoscaleEnabled(&instanceMergedWithFlavors.Spec) { // NOTE: we should avoid changing the number of replicas as it's managed by HPA. replicas = nil } diff --git a/controllers/controller_test.go b/controllers/controller_test.go index bd2c9bcdb..769e50789 100644 --- a/controllers/controller_test.go +++ b/controllers/controller_test.go @@ -114,6 +114,18 @@ func Test_newNginx(t *testing.T) { }, }, + "with Shutdown enabled": { + instance: func(i *v1alpha1.RpaasInstance) *v1alpha1.RpaasInstance { + i.Spec.Replicas = func(n int32) *int32 { return &n }(8) + i.Spec.Shutdown = true + return i + }, + expected: func(n *nginxv1alpha1.Nginx) *nginxv1alpha1.Nginx { + n.Spec.Replicas = func(n int32) *int32 { return &n }(0) + return n + }, + }, + "with load balancer": { instance: func(i *v1alpha1.RpaasInstance) *v1alpha1.RpaasInstance { i.Spec.Service = &nginxv1alpha1.NginxService{ @@ -249,6 +261,88 @@ func Test_newNginx(t *testing.T) { } } +func Test_isAutoscaleValid(t *testing.T) { + tests := map[string]struct { + isValid bool + autoscale v1alpha1.RpaasInstanceAutoscaleSpec + }{ + "Invalid null autoscale": { + isValid: false, + autoscale: v1alpha1.RpaasInstanceAutoscaleSpec{}, + }, + + "Invalid minReplicas is greater than maxReplicas": { + isValid: false, + autoscale: v1alpha1.RpaasInstanceAutoscaleSpec{ + MinReplicas: func(n int32) *int32 { return &n }(5), + MaxReplicas: 1, + }, + }, + + "Valid autoscale": { + isValid: true, + autoscale: v1alpha1.RpaasInstanceAutoscaleSpec{ + MaxReplicas: 8, + MinReplicas: func(n int32) *int32 { return &n }(2), + TargetCPUUtilizationPercentage: func(n int32) *int32 { return &n }(90), + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := isAutoscaleValid(&tt.autoscale) + assert.Equal(t, tt.isValid, got) + }) + } +} + +func Test_isAutoscaleEnabled(t *testing.T) { + tests := map[string]struct { + isEnabled bool + instanceSpec v1alpha1.RpaasInstanceSpec + }{ + "Disabled autoscale by invalid spec": { + isEnabled: false, + instanceSpec: v1alpha1.RpaasInstanceSpec{ + Shutdown: false, + Autoscale: &v1alpha1.RpaasInstanceAutoscaleSpec{}, + }, + }, + + "Disabled autoscale by shutdown": { + isEnabled: false, + instanceSpec: v1alpha1.RpaasInstanceSpec{ + Shutdown: true, + Autoscale: &v1alpha1.RpaasInstanceAutoscaleSpec{ + MaxReplicas: 8, + MinReplicas: func(n int32) *int32 { return &n }(2), + TargetCPUUtilizationPercentage: func(n int32) *int32 { return &n }(90), + }, + }, + }, + + "Enabled autoscale": { + isEnabled: true, + instanceSpec: v1alpha1.RpaasInstanceSpec{ + Shutdown: false, + Autoscale: &v1alpha1.RpaasInstanceAutoscaleSpec{ + MaxReplicas: 4, + MinReplicas: func(n int32) *int32 { return &n }(2), + TargetCPUUtilizationPercentage: func(n int32) *int32 { return &n }(70), + }, + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := isAutoscaleEnabled(&tt.instanceSpec) + assert.Equal(t, tt.isEnabled, got) + }) + } +} + func Test_mergePlans(t *testing.T) { tests := []struct { base v1alpha1.RpaasPlanSpec @@ -1078,6 +1172,22 @@ func Test_reconcileHPA(t *testing.T) { expectedChanged: true, }, + "(native HPA controller) removing autoscale with shutdown flag": { + resources: []runtime.Object{ + baseExpectedHPA.DeepCopy(), + }, + instance: func(ri *v1alpha1.RpaasInstance) *v1alpha1.RpaasInstance { + ri.Spec.Shutdown = true + return ri + }, + customAssert: func(t *testing.T, r *RpaasInstanceReconciler) bool { + var hpa autoscalingv2.HorizontalPodAutoscaler + err := r.Client.Get(context.TODO(), types.NamespacedName{Name: "my-instance", Namespace: "default"}, &hpa) + return assert.True(t, k8sErrors.IsNotFound(err)) + }, + expectedChanged: true, + }, + "(native HPA controller) with RPS enabled": { instance: func(ri *v1alpha1.RpaasInstance) *v1alpha1.RpaasInstance { ri.Spec.Autoscale = &v1alpha1.RpaasInstanceAutoscaleSpec{ @@ -1319,6 +1429,22 @@ func Test_reconcileHPA(t *testing.T) { expectedChanged: true, }, + "(KEDA controller) removing autoscaling with shutdown flag": { + resources: []runtime.Object{ + baseExpectedScaledObject.DeepCopy(), + }, + instance: func(ri *v1alpha1.RpaasInstance) *v1alpha1.RpaasInstance { + ri.Spec.Shutdown = true + return ri + }, + customAssert: func(t *testing.T, r *RpaasInstanceReconciler) bool { + var so kedav1alpha1.ScaledObject + err := r.Client.Get(context.TODO(), types.NamespacedName{Name: baseExpectedScaledObject.Name, Namespace: baseExpectedScaledObject.Namespace}, &so) + return assert.True(t, k8sErrors.IsNotFound(err), "ScaledObject resource should not exist") + }, + expectedChanged: true, + }, + "(KEDA controller) KEDA controller enabled, but instance does not have RPS trigger": { instance: func(ri *v1alpha1.RpaasInstance) *v1alpha1.RpaasInstance { ri.Spec.Autoscale = &v1alpha1.RpaasInstanceAutoscaleSpec{