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

[Umbrella] Skip Score plugins when coupled PreScore plugin returns Skip status #115745

Closed
9 tasks done
sanposhiho opened this issue Feb 14, 2023 · 22 comments
Closed
9 tasks done
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.

Comments

@sanposhiho
Copy link
Member

sanposhiho commented Feb 14, 2023

What would you like to be added?

It's much similar to #114399.
In #114827, we support Skip in PreScore/Score as well. (Thanks @kidddddddddddddddddddddd)
So, the plugin can return Skip in PreScore when it doesn't do anything in Score in that scheduling cycle.

For example, if the pod doesn't have any preferred inter-pod affinity, the inter-pod affinity plugin PreScore can return Skip, and the scheduler won't run the inter-pod affinity plugin Score.

Let's change our in-tree Score plugins to return Skip.

Here, I just listed all score plugins, not all of them may not need to return Skip in PreScore though. Assignee, please make sure your PreScore/Score plugin really needs to return Skip or not.

Please feel free to assign what you'd like to work on. And please comment in this issue before starting to work on so that we can avoid many people from working on the same thing!

Why is this needed?

The scheduler can know which plugins it really needs/doesn't need to run to get the scheduling result.
It'd contribute to the scheduling performance.

/sig scheduling
/assign

@sanposhiho sanposhiho added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 14, 2023
@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 14, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@sanposhiho
Copy link
Member Author

I'm thinking work on InterPodAffinity and NodeAffinity because I worked on the Skip implementation in PreFilter/Filter for them.

@sivchari
Copy link
Member

sivchari commented Feb 14, 2023

@sanposhiho
Please assign me to ImageLocality, PodTopologySpread and TaintToleration 👍
If there is no one else to work on it, I'll do it.

@AxeZhan
Copy link
Member

AxeZhan commented Feb 14, 2023

I'd like to work on VolumeBinding since I worked on it before.

@tangwz
Copy link
Contributor

tangwz commented Feb 14, 2023

@sanposhiho I'd like to work on Fit, BalancedAllocation.

@tangwz
Copy link
Contributor

tangwz commented Mar 12, 2023

@sanposhiho @alculquicondor For Fit and BalancedAllocation, I think If the plugin's args Resources not set, we can return a error in PreScore Plugin, but Skip does not need to implement.

@alculquicondor
Copy link
Member

Today, if args.resources is empty, scores return zero, but it doesn't produce an error. This doesn't make sense, but someone could have made a mistake. If we start returning an error, they would have to change their configuration to keep the scheduler running. I don't think we should be doing that.
In theory, if all nodes return the same score, we could return Skip, but since this is not a valid use case, I think we are adding complexity for no value. So I think we should leave it as is. At most, I would just produce a log line saying that not setting resources would not produce a meaningful score.

@sanposhiho
Copy link
Member Author

@sivchari Do you still plan to work on it? or want anyone to take over them?

@sivchari
Copy link
Member

@sanposhiho
Sorry, I'll work on it.

@alculquicondor
Copy link
Member

@sivchari was there any progress on the PodTopologySpread?

@sivchari
Copy link
Member

@alculquicondor
There is no progress on the PodTopologySpread 🙇

@utam0k
Copy link
Member

utam0k commented May 3, 2023

I talked to @sanposhiho elsewhere and was given a chance to work on PodAffnitiy.

@utam0k
Copy link
Member

utam0k commented May 3, 2023

@sanposhiho May I ask you to assign me to this issue to avoid missing 🙏 ?

@sanposhiho
Copy link
Member Author

/assign @utam0k

@utam0k
Copy link
Member

utam0k commented May 6, 2023

@sanposhiho Is there anything else I can help with that you would like to do? Of course, I can take another issue.

@sanposhiho
Copy link
Member Author

I think you can help some @sivchari has, they all have been pending for quite long time.
How about PodTopologySpread?

@utam0k
Copy link
Member

utam0k commented May 6, 2023

@sanposhiho I can take it. Please assign me 🙏

@sanposhiho
Copy link
Member Author

Can anyone be a volunteer for TaintToleration or ImageLocality?

@tangwz
Copy link
Contributor

tangwz commented May 23, 2023

@sanposhiho please assign me

@sanposhiho
Copy link
Member Author

/assign @tangwz

FYI, @sivchari I reassigned them to @tangwz. You can still help with the review on them.

@sanposhiho
Copy link
Member Author

The last one is merged, we can close it now finally 🎉
#117024

/close

@k8s-ci-robot
Copy link
Contributor

@sanposhiho: Closing this issue.

In response to this:

The last one is merged, we can close it now finally 🎉
#117024

/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants