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

Avoid unnecessary API update and re-enqueue action #3490

Closed
Monokaix opened this issue May 24, 2024 · 10 comments
Closed

Avoid unnecessary API update and re-enqueue action #3490

Monokaix opened this issue May 24, 2024 · 10 comments
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@Monokaix
Copy link
Member

Monokaix commented May 24, 2024

What would you like to be added:

  • Currently, when update a resource, volcano haven't compare them before update, which leads to unnecessary update to API server.
  • Volcano controller use workqueue to process resources like queue,job, we should check whether a resource exists when processing it to avoid invalid processing and retrying.

Why is this needed:

Lower API server pressure
Avoid unnecessary enent process and enhance performance

@Monokaix Monokaix added the kind/feature Categorizes issue or PR as related to a new feature. label May 24, 2024
@Monokaix
Copy link
Member Author

Here are some codes need to be modified:

  1. Check whether newJob and original job are deep equal, update job only when it changed
    newJob, err := cc.vcClient.BatchV1alpha1().Jobs(job.Namespace).UpdateStatus(context.TODO(), job, metav1.UpdateOptions{})
  2. Skip retry and return nil if err is NotFoundErr
    return gc.vcClient.BatchV1alpha1().Jobs(fresh.Namespace).Delete(context.TODO(), fresh.Name, options)
  3. Check whether it's NotFoundErr

@Monokaix
Copy link
Member Author

/good-first-issue

@volcano-sh-bot
Copy link
Contributor

@Monokaix:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@volcano-sh-bot volcano-sh-bot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels May 24, 2024
@wangyysde
Copy link
Contributor

/assign
I want to try this.

@Monokaix
Copy link
Member Author

/assign I want to try this.

Thanks and welcome!

@wangyysde
Copy link
Contributor

@Monokaix


Here is skipped retry and returned nil if err is NotFoundErr, is right?

@Monokaix
Copy link
Member Author

Monokaix commented Jun 5, 2024

@Monokaix

Here is skipped retry and returned nil if err is NotFoundErr, is right?

It's right here.

@Monokaix Monokaix changed the title [Performance issue] Avoid unnecessary API update and re-enqueue action Avoid unnecessary API update and re-enqueue action Jun 12, 2024
@Monokaix
Copy link
Member Author

/close

@volcano-sh-bot
Copy link
Contributor

@Monokaix: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@lowang-bh
Copy link
Member

Here line307 also need to do comapre and then update:

if !syncTask {
if updateStatus != nil {
if updateStatus(&job.Status) {
job.Status.State.LastTransitionTime = metav1.Now()
jobCondition = newCondition(job.Status.State.Phase, &job.Status.State.LastTransitionTime)
job.Status.Conditions = append(job.Status.Conditions, jobCondition)
}
}
newJob, err := cc.vcClient.BatchV1alpha1().Jobs(job.Namespace).UpdateStatus(context.TODO(), job, metav1.UpdateOptions{})
if err != nil {
klog.Errorf("Failed to update status of Job %v/%v: %v",
job.Namespace, job.Name, err)
return err
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants