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

Modified the Filter interface to pass in nodeinfo instead of node name. #82842

Merged
merged 1 commit into from
Sep 19, 2019

Conversation

ahg-g
Copy link
Member

@ahg-g ahg-g commented Sep 18, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
This is necessary to support preemption, which relies on passing modified nodeinfo objects to the filters to simulate evicting lower-priority pods.

Does this PR introduce a user-facing change?:

Modified the scheduling framework's Filter API.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 18, 2019
@ahg-g
Copy link
Member Author

ahg-g commented Sep 18, 2019

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 18, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 18, 2019
This is necessary to support preemption, which relies on passing modified nodeinfo objects to the filters to simulate evicting lower-priority pods.
@k82cn
Copy link
Member

k82cn commented Sep 19, 2019

LGTM overall :)

hm... do we keep backward comparability for framework interface when it's GA?

@ahg-g
Copy link
Member Author

ahg-g commented Sep 19, 2019

LGTM overall :)

hm... do we keep backward comparability for framework interface when it's GA?

This is a little different than API objects and json, k8s doesn't offer its code as a library, and so backward compatibility here is not something that k8s have strong guarantees for unless we place it in an external repo. In any case, I think we should be careful with changing the API when its "GA".

@ahg-g
Copy link
Member Author

ahg-g commented Sep 19, 2019

/assign @alculquicondor

Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit b0e5b9c into kubernetes:master Sep 19, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 19, 2019
@k82cn
Copy link
Member

k82cn commented Sep 20, 2019

In any case, I think we should be careful with changing the API when its "GA".

Maybe we should discuss this in community with our suggestion: IMO, the original idea of scheduler framework is to support customized scheduler for difference scenarios, as we can not put everything in default scheduler which will make complex; if "k8s doesn't offer its code as a library", will that make developer confuse about scheduler framework?

@ahg-g
Copy link
Member Author

ahg-g commented Sep 20, 2019

In any case, I think we should be careful with changing the API when its "GA".

Maybe we should discuss this in community with our suggestion:

For sure, I think once we have few predicates/priorities migrated, we can establish strong guarantees around the framework API, and perhaps suggest that our versioning/deprecation policy will be similar to object APIs.

IMO, the original idea of scheduler framework is to support customized scheduler for difference scenarios, as we can not put everything in default scheduler which will make complex;

I am strongly supportive of that. However, in my opinion, we can't practically offer any guarantees on the framework interface until some of our core predicates/priorities are migrated since we are still learning what it is missing.

if "k8s doesn't offer its code as a library", will that make developer confuse about scheduler framework?

That is k8s policy, we have two options to offer strong guarantees around the API:

  1. have its interfaces under a separate repo similar to what we are doing for component config.
  2. have our own versioning/deprecation policy, and we simply say that it is similar to object API.

but again, we can only do that after we migrate few predicates/priorities and have a first cut that is practical to use.

What do you think?

@ahg-g
Copy link
Member Author

ahg-g commented Sep 20, 2019

btw, the graduation criteria for the framework is aligned with the second option.

We purposely didn't graduate the framework to Beta yet, we wanted to wait until few default predicates/priorities are implemented first to have more confidence in the API.

@k82cn
Copy link
Member

k82cn commented Sep 25, 2019

Totally agree with you to migrate some predicates/priorities firstly; I think that's ok to get a guidance of API before scheduling framework GA :)

@ahg-g ahg-g deleted the ahg-framework-config branch January 10, 2020 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants