Added hpa update predicate to reconcile a scaledobject only if hpa spec,label or annotation is changed by deefreak · Pull Request #5282 · kedacore/keda

deefreak · 2023-12-12T12:09:55Z

This change fixes #5281 .
We have verified that scaledobject reconcilation is triggered only when hpa spec/label changes or hpa is deleted.
I raise this PR to get early feedback and automated tests will be added soon.

github-actions · 2023-12-12T12:10:10Z

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Add an entry in our changelog in alphabetical order and link related issue
Update the documentation, if needed
Add unit & e2e tests for your changes
GitHub checks are passing
Is the DCO check failing? Here is how you can fix DCO issues

Learn more about:

Our contribution guide

zroubalik

We should probably add check for annotations here as well:

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

    
           func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error { 
        
           	hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr) 
        
           	if err != nil { 
        
           		logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject)) 
        
           		return err 
        
           	} 
        
           	// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different. 
        
           	if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) { 
        
           		logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec) 
        
           		if err = r.Client.Update(ctx, hpa); err != nil { 
        
           			foundHpa.Spec = hpa.Spec 
        
           			logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name) 
        
           			return err 
        
           		} 
        
           		logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name) 
        
           	} 
        
           	if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) { 
        
           		logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels) 
        
           		if err = r.Client.Update(ctx, hpa); err != nil { 
        
           			foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels 
        
           			logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name) 
        
           			return err 
        
           		} 
        
           		logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name) 
        
           	} 
        
           	return nil

deefreak · 2023-12-13T08:03:19Z

We should probably add check for annotations here as well:

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

I am thinking that as scaledobject controller is not adding any new annotation to the HPA and HPA controller might add some annotation to it, so it should be fine to remove predicate.AnnotationChangedPredicate{} and leave this piece of code as it is.

We are now on autoscaling/v2 where currently no annotation is being added by hpa controller but i saw in autoscaling/v1 that the current metrics were part of the annotations and they kept changing.
So, it would be better to not use this predicate as there may be changes in the future in HPA controller updating annotations.

…ec,label or ownerRefernce is changed Signed-off-by: deefreak <[email protected]>

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

…ec or label is changed Signed-off-by: deefreak <[email protected]>

JorTurFer · 2023-12-13T08:40:02Z

We should probably add check for annotations here as well:

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

I am thinking that as scaledobject controller is not adding any new annotation to the HPA and HPA controller might add some annotation to it, so it should be fine to remove predicate.AnnotationChangedPredicate{} and leave this piece of code as it is.

We are now on autoscaling/v2 where currently no annotation is being added by hpa controller but i saw in autoscaling/v1 that the current metrics were part of the annotations and they kept changing. So, it would be better to not use this predicate as there may be changes in the future in HPA controller updating annotations.

We are propagating the ScaledObject labels/annotations so I'd say that we have to keep the predicate:

keda/controllers/keda/hpa.go

Lines 134 to 139 in a2c21ec

    
           ObjectMeta: metav1.ObjectMeta{ 
        
           	Name:        getHPAName(scaledObject), 
        
           	Namespace:   scaledObject.Namespace, 
        
           	Labels:      labels, 
        
           	Annotations: scaledObject.Annotations, 
        
           },

deefreak · 2023-12-13T09:04:50Z

We should probably add check for annotations here as well:

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

I am thinking that as scaledobject controller is not adding any new annotation to the HPA and HPA controller might add some annotation to it, so it should be fine to remove predicate.AnnotationChangedPredicate{} and leave this piece of code as it is.
We are now on autoscaling/v2 where currently no annotation is being added by hpa controller but i saw in autoscaling/v1 that the current metrics were part of the annotations and they kept changing. So, it would be better to not use this predicate as there may be changes in the future in HPA controller updating annotations.

We are propagating the ScaledObject labels/annotations so I'd say that we have to keep the predicate:

keda/controllers/keda/hpa.go

Lines 134 to 139 in a2c21ec

ObjectMeta: metav1.ObjectMeta{

Name: getHPAName(scaledObject),

Namespace: scaledObject.Namespace,

Labels: labels,

Annotations: scaledObject.Annotations,

},

Sorry, my bad , missed this. Also, confirmed that currently hpa controller is not adding any annotations to the hpa object. So, it should be fine to add the annotation changed predicate . I will also then make changes to the

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

to add check for annotations as well.
I hope this is fine @JorTurFer

zroubalik · 2023-12-13T09:15:44Z

We should probably add check for annotations here as well:

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

I am thinking that as scaledobject controller is not adding any new annotation to the HPA and HPA controller might add some annotation to it, so it should be fine to remove predicate.AnnotationChangedPredicate{} and leave this piece of code as it is.
We are now on autoscaling/v2 where currently no annotation is being added by hpa controller but i saw in autoscaling/v1 that the current metrics were part of the annotations and they kept changing. So, it would be better to not use this predicate as there may be changes in the future in HPA controller updating annotations.

We are propagating the ScaledObject labels/annotations so I'd say that we have to keep the predicate:

keda/controllers/keda/hpa.go

Lines 134 to 139 in a2c21ec

ObjectMeta: metav1.ObjectMeta{

Name: getHPAName(scaledObject),

Namespace: scaledObject.Namespace,

Labels: labels,

Annotations: scaledObject.Annotations,

},

Sorry, my bad , missed this. Also, confirmed that currently hpa controller is not adding any annotations to the hpa object. So, it should be fine to add the annotation changed predicate . I will also then make changes to the

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

to add check for annotations as well. I hope this is fine @JorTurFer

yes please

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

deefreak · 2023-12-13T09:59:29Z

@zroubalik I have done all the commented changes. Have added unit tests to test the HPASpecChangedPredicate as well.

zroubalik

Looking good, I wonder whether we can test that updates to status don't trigger a reconcile loop?

zroubalik · 2023-12-13T13:25:51Z

/run-e2e
Update: You can check the progress here

deefreak · 2023-12-13T13:51:10Z

Looking good, I wonder whether we can test that updates to status don't trigger a reconcile loop?

As there will be no change in scaledobjects, it will be really hard to test through a unit test. I mean there will be nothing to compare for before and after reconcile. Although i have compared it on a cluster with more than 100+ scaledobjects and there were no reconcilations for hpa status updates(.status.currentMetrics) after applying this fix.

zroubalik · 2023-12-13T13:59:06Z

Looking good, I wonder whether we can test that updates to status don't trigger a reconcile loop?

As there will be no change in scaledobjects, it will be really hard to test through a unit test. I mean there will be nothing to compare for before and after reconcile. Although i have compared it on a cluster with more than 100+ scaledobjects and there were no reconcilations for hpa status updates(.status.currentMetrics) after applying this fix.

yeah, agree

deefreak · 2023-12-13T15:13:59Z

We should probably add check for annotations here as well:

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

I am thinking that as scaledobject controller is not adding any new annotation to the HPA and HPA controller might add some annotation to it, so it should be fine to remove predicate.AnnotationChangedPredicate{} and leave this piece of code as it is.
We are now on autoscaling/v2 where currently no annotation is being added by hpa controller but i saw in autoscaling/v1 that the current metrics were part of the annotations and they kept changing. So, it would be better to not use this predicate as there may be changes in the future in HPA controller updating annotations.

We are propagating the ScaledObject labels/annotations so I'd say that we have to keep the predicate:

keda/controllers/keda/hpa.go

Lines 134 to 139 in a2c21ec

ObjectMeta: metav1.ObjectMeta{

Name: getHPAName(scaledObject),

Namespace: scaledObject.Namespace,

Labels: labels,

Annotations: scaledObject.Annotations,

},

Sorry, my bad , missed this. Also, confirmed that currently hpa controller is not adding any annotations to the hpa object. So, it should be fine to add the annotation changed predicate . I will also then make changes to the

keda/controllers/keda/hpa.go

Lines 154 to 183 in a2c21ec

func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error {

hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)

if err != nil {

logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))

return err

}

// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.

if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {

logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.Spec = hpa.Spec

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {

logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)

if err = r.Client.Update(ctx, hpa); err != nil {

foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels

logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

return err

}

logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)

}

return nil

to add check for annotations as well. I hope this is fine @JorTurFer

yes please

I added the check for annotations as similar to the labels.
Wrote a test for reconciliation of scaledobject when annotation is changed, noticed that equality.Semantic.DeepDerative(a1,a2 interface{}) returns true if a1 == nil .

when I was testing, the hpa created through scaledobject has no annotations, thus annotation was nil and therefore adding an annotation to the hpa was not reconciling again.

So, we should add this condition, verified that this is working
if (hpa.ObjectMeta.Annotations == nil && foundHpa.ObjectMeta.Annotations != nil) ||
!equality.Semantic.DeepDerivative(hpa.ObjectMeta.Annotations, foundHpa.ObjectMeta.Annotations)

@zroubalik @JorTurFer what do you think?

zroubalik · 2023-12-13T15:37:26Z

Good catch, please do that!

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

deefreak · 2023-12-14T08:05:32Z

Good catch, please do that!

Done and added tests for label and annotation changes as well.

…tus-updates

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

…ect-reconciles-by-HPA-status-updates

…s-by-HPA-status-updates' into redundant-scaledobject-reconciles-by-HPA-status-updates

zroubalik · 2023-12-14T10:27:53Z

/run-e2e
Update: You can check the progress here

deefreak · 2023-12-15T05:00:07Z

@zroubalik I see this run-e2e is failing . Any idea how to fix this?

…tus-updates

JorTurFer · 2023-12-15T10:42:05Z

/run-e2e
Update: You can check the progress here

JorTurFer · 2023-12-15T10:42:25Z

@zroubalik I see this run-e2e is failing . Any idea how to fix this?

I think that it was an unrelated issue

deefreak · 2023-12-15T13:35:04Z

Given checks have passed, can it be approved now?

…ec,label or annotation is changed (kedacore#5282) Signed-off-by: deefreak <[email protected]> Signed-off-by: anton.lysina <[email protected]>

deefreak requested a review from a team as a code owner December 12, 2023 12:09

zroubalik reviewed Dec 12, 2023

View reviewed changes

Comment thread controllers/keda/scaledobject_controller.go Outdated

zroubalik reviewed Dec 12, 2023

View reviewed changes

Comment thread controllers/keda/util/predicate.go Outdated

Comment thread controllers/keda/util/predicate.go Outdated

deefreak added 4 commits December 13, 2023 13:57

added hpa update predicate to reconcile a scaledobject only if hpa sp…

f3725d9

…ec,label or ownerRefernce is changed Signed-off-by: deefreak <[email protected]>

added hpa update predicate to reconcile a scaledobject only if hpa sp…

13cadee

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

added hpa update predicate to reconcile a scaledobject only if hpa sp…

edad66a

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

added hpa update predicate to reconcile a scaledobject only if hpa sp…

00193f8

…ec or label is changed Signed-off-by: deefreak <[email protected]>

deefreak force-pushed the redundant-scaledobject-reconciles-by-HPA-status-updates branch from c1011c6 to 00193f8 Compare December 13, 2023 08:28

added hpa update predicate to reconcile a scaledobject only if hpa sp…

e6cef7e

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

zroubalik reviewed Dec 13, 2023

View reviewed changes

Comment thread controllers/keda/scaledobject_controller_test.go

added hpa update predicate to reconcile a scaledobject only if hpa sp…

048cbbc

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

deefreak and others added 4 commits December 14, 2023 13:47

Merge branch 'main' into redundant-scaledobject-reconciles-by-HPA-sta…

814a755

…tus-updates

added hpa update predicate to reconcile a scaledobject only if hpa sp…

74bb882

…ec,label or annotation is changed Signed-off-by: deefreak <[email protected]>

Merge remote-tracking branch 'upstream/main' into redundant-scaledobj…

ef69e91

…ect-reconciles-by-HPA-status-updates

Merge remote-tracking branch 'origin/redundant-scaledobject-reconcile…

f619424

…s-by-HPA-status-updates' into redundant-scaledobject-reconciles-by-HPA-status-updates

Merge branch 'main' into redundant-scaledobject-reconciles-by-HPA-sta…

0c3e2ce

…tus-updates

zroubalik approved these changes Dec 15, 2023

View reviewed changes

zroubalik merged commit 99a1fe5 into kedacore:main Dec 15, 2023

zroubalik mentioned this pull request Dec 15, 2023

Fix issue where paused annotation being set to false still leads to scaled objects/jobs being paused #5257

Merged

4 tasks

deefreak mentioned this pull request Dec 28, 2024

Frequent Reconciliation Caused by Minor Changes in HPA Status from Prometheus Scalers #6442

Closed

	func (r ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger logr.Logger, scaledObject kedav1alpha1.ScaledObject, foundHpa autoscalingv2.HorizontalPodAutoscaler, gvkr kedav1alpha1.GroupVersionKindResource) error {
	hpa, err := r.newHPAForScaledObject(ctx, logger, scaledObject, gvkr)
	if err != nil {
	logger.Error(err, "Failed to create new HPA resource", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", getHPAName(scaledObject))
	return err
	}

	// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.
	if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) \|\| !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {
	logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)
	if err = r.Client.Update(ctx, hpa); err != nil {
	foundHpa.Spec = hpa.Spec
	logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)
	return err
	}

	logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)
	}

	if !equality.Semantic.DeepDerivative(hpa.ObjectMeta.Labels, foundHpa.ObjectMeta.Labels) {
	logger.V(1).Info("Found difference in the HPA labels accordint to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Labels, "newHPA", hpa.ObjectMeta.Labels)
	if err = r.Client.Update(ctx, hpa); err != nil {
	foundHpa.ObjectMeta.Labels = hpa.ObjectMeta.Labels
	logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)
	return err
	}
	logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)
	}

	return nil

Uh oh!

Conversation

deefreak commented Dec 12, 2023

Uh oh!

github-actions Bot commented Dec 12, 2023

Uh oh!

zroubalik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deefreak commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JorTurFer commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deefreak commented Dec 13, 2023

Uh oh!

zroubalik commented Dec 13, 2023

Uh oh!

deefreak commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zroubalik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zroubalik commented Dec 13, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deefreak commented Dec 13, 2023

Uh oh!

zroubalik commented Dec 13, 2023

Uh oh!

deefreak commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zroubalik commented Dec 13, 2023

Uh oh!

deefreak commented Dec 14, 2023

Uh oh!

zroubalik commented Dec 14, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deefreak commented Dec 15, 2023

Uh oh!

JorTurFer commented Dec 15, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JorTurFer commented Dec 15, 2023

Uh oh!

deefreak commented Dec 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deefreak commented Dec 13, 2023 •

edited

Loading

JorTurFer commented Dec 13, 2023 •

edited

Loading

deefreak commented Dec 13, 2023 •

edited

Loading

zroubalik commented Dec 13, 2023 •

edited by github-actions Bot

Loading

deefreak commented Dec 13, 2023 •

edited

Loading

zroubalik commented Dec 14, 2023 •

edited by github-actions Bot

Loading

JorTurFer commented Dec 15, 2023 •

edited by github-actions Bot

Loading