Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RHOAIENG-6878][Cherry-pick][RHOAI-2.10] fix: HPA equality check should include annotations (#3650) #355

Merged
merged 1 commit into from
May 13, 2024
Merged

[RHOAIENG-6878][Cherry-pick][RHOAI-2.10] fix: HPA equality check should include annotations (#3650) #355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/constants/constants.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)},
}))
}
Loading