-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-1948: Adding KEP for allowing deallocate in device plugin API call #1949
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
Conversation
|
Welcome @mewais! |
|
Hi @mewais. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
I'm surprised they didn't move the original design doc here! Maybe instead of having a Kep for this specific feature, convert the design doc to a kep in the first commit (or a separate PR) and add a section with your feature in the second commit. I think it makes more sense this way since we aren't versioning individual function calls but the whole API. What do you think? |
|
I have a paper deadline approaching so I will have to apologize as it looks like a bit of work that I may not have time for. That design document is quite old, there are things already implemented in the device plugin API but never described in that document, and things described in the document that are no longer in the implementation. Plus the images are outdated (at least the second) and will need some changes. If/When it is done, I'm happy to rebase this on top of it. On the other hand, if it is going to take time, is it fine if we just proceed with this and merge/rebase later?! |
| - "@mewais" | ||
| owning-sig: sig-node | ||
| participating-sigs: [] | ||
| status: implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be provisional or implementable - implemented is for KEPs that have been approved and whose work is completed.
|
@kikisdeliveryservice Fixed, thanks for the elaboration. |
|
/assign @dchen1107 |
|
@dchen1107 @derekwaynecarr @kikisdeliveryservice Any updates about this?! |
Hi @mewais Enhancements Lead here. I'd recommend bringing your KEP directly to sig-node and asking for feedback. Please see: https://github.com/kubernetes/community/tree/master/sig-node for meeting times and slack channels that might help. Best, |
I have actually started a discussion about this on sig-node's slack channel, the discussion ended by suggesting that I create this KEP. |
kikisdeliveryservice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This KEP is missing many sections from the template such as PRR, Design Details, graduation criteria & test criteria: https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template
Please review the template and update accordingly.
2997506 to
039176b
Compare
|
The KEP has been updated. I'll try to join some of the upcoming sig-node meetings to discuss. |
|
/ok-to-test |
fujitatomoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found a few typo
|
|
||
| This KEP proposes adding two extra API calls: | ||
| - `Deallocate`: (Optional). Which is the opposite of allocate, and is needed to inform device plugins that some devices are no longer being used. | ||
| - `PostStopContainer`: (Optional). Which allow the device plugins to do device cleanup, driver unloading, and any other actions that may be needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| - `PostStopContainer`: (Optional). Which allow the device plugins to do device cleanup, driver unloading, and any other actions that may be needed. | |
| - `PostStopContainer`: (Optional). Which allows the device plugins to do device cleanup, driver unloading, and any other actions that may be needed. |
|
|
||
| The device plugin API includes API calls for: | ||
| - `Allocate`: Which is used to instruct device plugins to allocate device(s) to requesting containers. | ||
| - `PreStartContainer`: (Optional). Which allow the device plugins to do device initialization, loading drivers, and any other initialization actions that may be needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - `PreStartContainer`: (Optional). Which allow the device plugins to do device initialization, loading drivers, and any other initialization actions that may be needed. | |
| - `PreStartContainer`: (Optional). Which allows the device plugins to do device initialization, loading drivers, and any other initialization actions that may be needed. |
|
|
||
| This KEP proposes adding two extra API calls, maintaining the same logical reasoning of the previous two. Those are: | ||
| - `Deallocate`: (Optional). Which is the opposite of allocate, and is needed to inform device plugins that some devices are no longer being used (this used to happen silently before) | ||
| - `PostStopContainer`: (Optional). Which allow the device plugins to do device cleanup, driver unloading, and any other cleanup actions that may be needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - `PostStopContainer`: (Optional). Which allow the device plugins to do device cleanup, driver unloading, and any other cleanup actions that may be needed. | |
| - `PostStopContainer`: (Optional). Which allows the device plugins to do device cleanup, driver unloading, and any other cleanup actions that may be needed. |
|
/cc @zvonkok |
|
@zvonkok: GitHub didn't allow me to request PR reviews from the following users: zvonkok. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
|
As discussed on sig-node Nov. 30th, see notes, I am taking ownership of this KEP/PR. FYI @Windrow14 regarding #3074 |
Got it, I closed that one. |
|
@zvonkok thanks for taking responsibility. if there anything we can do escalate the process, just let us know? we can share our use cases in SIG, if needed. thanks! |
|
@zvonkok Can you point us to the place(PR, issue, etc) where you're going to continue this work, please? |
|
@kubernetes/sig-node-pr-reviews please close this as this work has been moved to #3080 |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
/remove-lifecycle rotten |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
@kikisdeliveryservice @klueska probably we should close this? in favor of #3080 ? |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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. |
|
I think this can be closed in favor of #3080 |
#1948