Skip to content

Commit

Permalink
fix: HPA equality check should include annotations (kserve#3650)
Browse files Browse the repository at this point in the history 
* fix: HPA equality check should include annotations

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

* Only watch related autoscalerclass annotation

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

* simplify

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

* Add missing delete action

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

* fix logic

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
---------

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
  • Loading branch information
@terrytangyuan
terrytangyuan committed May 13, 2024
1 parent f7233ba commit 0a8ae16
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 21 deletions.
2 changes: 2 additions & 0 deletions pkg/constants/constants.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,8 @@ const (
CheckResultUpdate CheckResultType = 1
CheckResultExisted CheckResultType = 2
CheckResultUnknown CheckResultType = 3
CheckResultDelete CheckResultType = 4
CheckResultSkipped CheckResultType = 5
)

type DeploymentModeType string
Expand Down
4 changes: 1 addition & 3 deletions pkg/controller/v1beta1/inferenceservice/reconcilers/autoscaler/autoscaler_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
59 changes: 41 additions & 18 deletions pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down
71 changes: 71 additions & 0 deletions pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)},
}))
}

0 comments on commit 0a8ae16

Please sign in to comment.