diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 98b8692979a..017192fceca 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -350,6 +350,8 @@ const ( CheckResultUpdate CheckResultType = 1 CheckResultExisted CheckResultType = 2 CheckResultUnknown CheckResultType = 3 + CheckResultDelete CheckResultType = 4 + CheckResultSkipped CheckResultType = 5 ) type DeploymentModeType string diff --git a/pkg/controller/v1beta1/inferenceservice/reconcilers/autoscaler/autoscaler_reconciler.go b/pkg/controller/v1beta1/inferenceservice/reconcilers/autoscaler/autoscaler_reconciler.go index 5c9bfb6a31c..971da788d32 100644 --- a/pkg/controller/v1beta1/inferenceservice/reconcilers/autoscaler/autoscaler_reconciler.go +++ b/pkg/controller/v1beta1/inferenceservice/reconcilers/autoscaler/autoscaler_reconciler.go @@ -85,10 +85,8 @@ func createAutoscaler(client client.Client, componentExt *v1beta1.ComponentExtensionSpec) (Autoscaler, error) { ac := getAutoscalerClass(componentMeta) switch ac { - case constants.AutoscalerClassHPA: + case constants.AutoscalerClassHPA, constants.AutoscalerClassExternal: return hpa.NewHPAReconciler(client, scheme, componentMeta, componentExt), nil - case constants.AutoscalerClassExternal: - return &NoOpAutoscaler{}, nil default: return nil, fmt.Errorf("unknown autoscaler class type: %v", ac) } diff --git a/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go b/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go index 76834471663..5cdf8738f10 100644 --- a/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go +++ b/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go @@ -131,7 +131,11 @@ func (r *HPAReconciler) checkHPAExist(client client.Client) (constants.CheckResu }, existingHPA) if err != nil { if apierr.IsNotFound(err) { - return constants.CheckResultCreate, nil, nil + if shouldCreateHPA(r.HPA) { + return constants.CheckResultCreate, nil, nil + } else { + return constants.CheckResultSkipped, nil, nil + } } return constants.CheckResultUnknown, nil, err } @@ -140,39 +144,58 @@ func (r *HPAReconciler) checkHPAExist(client client.Client) (constants.CheckResu if semanticHPAEquals(r.HPA, existingHPA) { return constants.CheckResultExisted, existingHPA, nil } + if shouldDeleteHPA(r.HPA) { + return constants.CheckResultDelete, existingHPA, nil + } return constants.CheckResultUpdate, existingHPA, nil } func semanticHPAEquals(desired, existing *autoscalingv2.HorizontalPodAutoscaler) bool { - return equality.Semantic.DeepEqual(desired.Spec, existing.Spec) + desiredAutoscalerClass, hasDesiredAutoscalerClass := desired.Annotations[constants.AutoscalerClass] + existingAutoscalerClass, hasExistingAutoscalerClass := existing.Annotations[constants.AutoscalerClass] + var autoscalerClassChanged bool + if hasDesiredAutoscalerClass && hasExistingAutoscalerClass { + autoscalerClassChanged = desiredAutoscalerClass != existingAutoscalerClass + } else if hasDesiredAutoscalerClass || hasExistingAutoscalerClass { + autoscalerClassChanged = true + } + return equality.Semantic.DeepEqual(desired.Spec, existing.Spec) && !autoscalerClassChanged +} + +func shouldDeleteHPA(desired *autoscalingv2.HorizontalPodAutoscaler) bool { + desiredAutoscalerClass, hasDesiredAutoscalerClass := desired.Annotations[constants.AutoscalerClass] + return hasDesiredAutoscalerClass && constants.AutoscalerClassType(desiredAutoscalerClass) == constants.AutoscalerClassExternal +} + +func shouldCreateHPA(desired *autoscalingv2.HorizontalPodAutoscaler) bool { + desiredAutoscalerClass, hasDesiredAutoscalerClass := desired.Annotations[constants.AutoscalerClass] + return !hasDesiredAutoscalerClass || (hasDesiredAutoscalerClass && constants.AutoscalerClassType(desiredAutoscalerClass) == constants.AutoscalerClassHPA) } // Reconcile ... func (r *HPAReconciler) Reconcile() (*autoscalingv2.HorizontalPodAutoscaler, error) { //reconcile Service checkResult, existingHPA, err := r.checkHPAExist(r.client) - log.Info("service reconcile", "checkResult", checkResult, "err", err) + log.Info("HorizontalPodAutoscaler reconcile", "checkResult", checkResult, "err", err) if err != nil { return nil, err } - if checkResult == constants.CheckResultCreate { - err = r.client.Create(context.TODO(), r.HPA) - if err != nil { - return nil, err - } else { - return r.HPA, nil - } - } else if checkResult == constants.CheckResultUpdate { //CheckResultUpdate - err = r.client.Update(context.TODO(), r.HPA) - if err != nil { - return nil, err - } else { - return r.HPA, nil - } - } else { + var opErr error + switch checkResult { + case constants.CheckResultCreate: + opErr = r.client.Create(context.TODO(), r.HPA) + case constants.CheckResultUpdate: + opErr = r.client.Update(context.TODO(), r.HPA) + case constants.CheckResultDelete: + opErr = r.client.Delete(context.TODO(), r.HPA) + default: return existingHPA, nil } + if opErr != nil { + return nil, opErr + } + return r.HPA, nil } func (r *HPAReconciler) SetControllerReferences(owner metav1.Object, scheme *runtime.Scheme) error { diff --git a/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler_test.go b/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler_test.go index 72ab454aeaa..c402c5a4bd1 100644 --- a/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler_test.go +++ b/pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler_test.go @@ -18,9 +18,12 @@ package hpa import ( "github.com/google/go-cmp/cmp" "github.com/kserve/kserve/pkg/apis/serving/v1beta1" + "github.com/kserve/kserve/pkg/constants" + "github.com/stretchr/testify/assert" autoscalingv2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/ptr" "testing" ) @@ -260,3 +263,71 @@ func TestCreateHPA(t *testing.T) { }) } } + +func TestSemanticHPAEquals(t *testing.T) { + assert.True(t, semanticHPAEquals( + &autoscalingv2.HorizontalPodAutoscaler{ + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{}, + }, + &autoscalingv2.HorizontalPodAutoscaler{ + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{}, + })) + + assert.False(t, semanticHPAEquals( + &autoscalingv2.HorizontalPodAutoscaler{ + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + }, + &autoscalingv2.HorizontalPodAutoscaler{ + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(4)}, + })) + + assert.False(t, semanticHPAEquals( + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.AutoscalerClass: "hpa"}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + }, + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.AutoscalerClass: "external"}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + })) + + assert.False(t, semanticHPAEquals( + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + }, + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.AutoscalerClass: "external"}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + })) + + assert.True(t, semanticHPAEquals( + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.AutoscalerClass: "hpa"}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + }, + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.AutoscalerClass: "hpa"}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + })) + + assert.True(t, semanticHPAEquals( + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + }, + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + })) + + assert.True(t, semanticHPAEquals( + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"unrelated": "true"}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + }, + &autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"unrelated": "false"}}, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{MinReplicas: ptr.Int32(3)}, + })) +}