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.

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

When updating the HPA in the EHPA Controller, if there is a conflict, the updated content will be lost #815

Closed
mtdtdev opened this issue Jun 8, 2023 · 1 comment · Fixed by #816
Labels
kind/bug Something isn't working

Comments

@mtdtdev
Copy link
Contributor

mtdtdev commented Jun 8, 2023

Describe the bug
In the new feature (improvement hpa and substitute), if the update of hpa fails due to concurrency conflicts, the update content will be lost.
e977b5e

Specific code: https://github.com/gocrane/crane/blob/main/pkg/controller/ehpa/hpa.go#L165
The operator wants to update HPA A1, if the update conflicts, it will directly obtain the latest HPA object A2 from the cluster, and then use A2 to re-initiate the update. And this A2 is not the content A1 that is expected to be updated. At the same time, if A2 update is successful, the exception of update failure will also be lost, resulting in A1 not being updated, but operator thinks that HPA A1 has been updated

if needUpdate {
	hpaCopy := hpaExist.DeepCopy()
	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
		err := c.Update(ctx, hpaCopy)
		if err == nil {
			return nil
		}

		updated := &autoscalingv2.HorizontalPodAutoscaler{}
		errGet := c.Get(context.TODO(), types.NamespacedName{Namespace: hpaCopy.Namespace, Name: hpaCopy.Name}, updated)
		if errGet == nil {
			hpaCopy = updated
		}

		return err
	})

	if err != nil {
		c.Recorder.Event(ehpa, v1.EventTypeWarning, "FailedUpdateHPA", err.Error())
		klog.Errorf("Failed to update HorizontalPodAutoscaler %s error %v", klog.KObj(hpaExist), err)
		return nil, err
	}

	klog.Infof("Update HorizontalPodAutoscaler successful, HorizontalPodAutoscaler %s", klog.KObj(hpaExist))
}

Reproduce steps

Expected behavior

Screenshots

Environment (please complete the following information):

  • K8S Version: [e.g. 1.19]
  • Crane Version: [e.g. 0.1.0]
  • Browser [e.g. chrome, safari]
@mtdtdev mtdtdev added the kind/bug Something isn't working label Jun 8, 2023
@qmhu
Copy link
Member

qmhu commented Jun 8, 2023

I think it is a bug, would you like to fix it ? @mtdtdev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants