Skip to content

Add GetPreferredAllocation() call to the v1beta1 device plugin API #92665

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.

Sign up for GitHub

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

Jump to bottom
Merged
k8s-ci-robot merged 11 commits into from
Jul 4, 2020
Merged

Add GetPreferredAllocation() call to the v1beta1 device plugin API #92665

k8s-ci-robot merged 11 commits into from
Jul 4, 2020

Conversation

@klueska
Copy link
Contributor

@klueska klueska commented Jun 30, 2020 •
edited
Loading

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
This PR adds support for a new, optional call in the v1beta1 device plugin API called GetPreferredAllocation(). Because it is an optional extension (and not a change) to the API, it does not require a version bump.

Details of this new API call can be found in the following KEP update: kubernetes/enhancements#1121

Please review commit-by-commit.

Which issue(s) this PR fixes:

Fixes #83483

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Added a new `GetPreferredAllocation()` call to the `v1beta1` device plugin API.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/pull/1121

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 30, 2020
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 30, 2020
@klueska klueska mentioned this pull request Jun 30, 2020
Closed
11 tasks
@klueska
Copy link
Contributor Author

klueska commented Jun 30, 2020 •
edited
Loading

/assign @derekwaynecarr
To approve the API changes

@klueska klueska force-pushed the upstream-add-get-preferred-allocation-api branch 2 times, most recently from 1eba5c5 to a7f9e6a Compare June 30, 2020 20:38
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 30, 2020
@klueska
Copy link
Contributor Author

klueska commented Jun 30, 2020 •
edited
Loading

/assign @mkumatag
/assign @listx
For approval of the small change to test/images/ in support of this new API.

@klueska klueska force-pushed the upstream-add-get-preferred-allocation-api branch 2 times, most recently from e0a6605 to 76a6f4b Compare July 1, 2020 11:07
nolancon
nolancon reviewed Jul 1, 2020
View reviewed changes
Copy link

@nolancon nolancon left a comment

Choose a reason for hiding this comment

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

Just one nit and a couple of questions for my own understanding.

vpickard
vpickard reviewed Jul 1, 2020
View reviewed changes
Copy link
Contributor

@vpickard vpickard left a comment

Choose a reason for hiding this comment

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

Looks good, few minor comment. Thanks!

@klueska klueska force-pushed the upstream-add-get-preferred-allocation-api branch 3 times, most recently from f0ed490 to 2a4ce8d Compare July 2, 2020 11:38
nolancon
nolancon reviewed Jul 2, 2020
View reviewed changes
Copy link

@nolancon nolancon left a comment

Choose a reason for hiding this comment

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

One question. Changes look good otherwise.

@klueska klueska force-pushed the upstream-add-get-preferred-allocation-api branch from e4f663b to 6b7bb9b Compare July 2, 2020 15:16
@derekwaynecarr
Copy link
Member

derekwaynecarr commented Jul 2, 2020

nice job, tests look good too.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2020
@klueska klueska force-pushed the upstream-add-get-preferred-allocation-api branch from 6b7bb9b to 98559a0 Compare July 2, 2020 22:04
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2020
klueska added 2 commits July 2, 2020 22:07
@klueska
Add callGetPreferredAllocationIfAvailable() function in devicemanager
bb56a09 
This function mimics what is already done for the conditional call to
PreStartContainer() via the callPreStartContainerIfNeeded() function.
@klueska
Updates logic in devicesToAllocate() to call GetPreferredAllocation()
a780ccf
@klueska klueska force-pushed the upstream-add-get-preferred-allocation-api branch from 98559a0 to 2cbb158 Compare July 2, 2020 22:07
mkumatag
mkumatag suggested changes Jul 3, 2020
View reviewed changes
Copy link
Member

@mkumatag mkumatag left a comment

Choose a reason for hiding this comment

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

Please bump the version here: https://github.com/kubernetes/kubernetes/blob/master/test/images/sample-device-plugin/VERSION

@klueska klueska force-pushed the upstream-add-get-preferred-allocation-api branch from 2cbb158 to 5bd0db0 Compare July 3, 2020 13:02
@klueska
Copy link
Contributor Author

klueska commented Jul 3, 2020

Please bump the version here: https://github.com/kubernetes/kubernetes/blob/master/test/images/sample-device-plugin/VERSION

Thanks for the review. VERSION has been updated here: cbd405d#diff-efcabe360f36f7a76db00afca0404498

@mkumatag
Copy link
Member

mkumatag commented Jul 3, 2020

/lgtm
/approve
/hold

Only for k/k/test/images content, adding hold for the remaining

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 3, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 3, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, klueska, mkumatag

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

The pull request process is described here

Details 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2020
@klueska
Copy link
Contributor Author

klueska commented Jul 3, 2020 •
edited
Loading

/hold cancel

@derekwaynecarr already approved / lgtmd the other changes

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2020
@k8s-ci-robot k8s-ci-robot merged commit efb56da into kubernetes:master Jul 4, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 4, 2020
@klueska klueska mentioned this pull request Jul 4, 2020
Merged
@github-actions github-actions bot mentioned this pull request Jul 8, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rohitagarwal003 rohitagarwal003 Awaiting requested review from rohitagarwal003

@RenaudWasTaken RenaudWasTaken Awaiting requested review from RenaudWasTaken

3 more reviewers

@vpickard vpickard vpickard left review comments

@nolancon nolancon nolancon left review comments

@mkumatag mkumatag mkumatag requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@listx listx

@derekwaynecarr derekwaynecarr

@mkumatag mkumatag

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

v1.19

Development

Successfully merging this pull request may close these issues.

TopologyManager: Add support for device-specific topology constraints

7 participants

@klueska @derekwaynecarr @mkumatag @k8s-ci-robot @vpickard @nolancon @listx