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

Add deviceManager in windows container manager #80917

Open
wants to merge 1 commit into
base: master
from

Conversation

@aarnaud
Copy link

aarnaud commented Aug 2, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:

Add devicemanager for Windows node to allow device plugin.

Does this PR introduce a user-facing change?:

Add devicemanager for Windows node to allow device plugin. 
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 2, 2019

Welcome @aarnaud!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 2, 2019

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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 understand the commands that are listed here.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 2, 2019

Hi @aarnaud. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot requested review from RenaudWasTaken and sjenning Aug 2, 2019
@aarnaud aarnaud force-pushed the aarnaud:windows-devicemanager branch from 01edb84 to 4ce09a7 Aug 2, 2019
@thomacos

This comment has been minimized.

Copy link

thomacos commented Sep 5, 2019

@derekwaynecarr @sjenning @RenaudWasTaken
We would be also very interested into that feature as soon as possible.

@aarnaud

This comment has been minimized.

Copy link
Author

aarnaud commented Sep 5, 2019

I hope this PR would be merge.

@thomacos Until merge you can find some build here : https://github.com/aarnaud/k8s-directx-device-plugin/releases

@thomacos

This comment has been minimized.

Copy link

thomacos commented Sep 5, 2019

I hope this PR would be merge.

@thomacos Until merge you can find some build here : https://github.com/aarnaud/k8s-directx-device-plugin/releases

That's great!
We just started into looking compiling kubelet.exe (and k8s-directx-device-plugin.exe) ourself, but this makes it way easier for us.
Thanks a lot!

Nevertheless, let's hope this PR finds its way into the official releases, too.

@aarnaud

This comment has been minimized.

Copy link
Author

aarnaud commented Sep 5, 2019

Also depends of your windows version you have to edit c:\k\start-kubelet.ps1

...
ipmo $helper
 
#Add this line below
Remove-Item c:\var\lib\kubelet\device-plugins\kubelet.sock -ErrorAction Ignore
 
if ($RegisterOnly.IsPresent)
{
    RegisterNode
    exit
}
...

There is a lite issue for Linux Socket on Windows c:\var\lib\kubelet\device-plugins\kubelet.sock

Tested on windows server 2019 with docker 19.03

@thomacos

This comment has been minimized.

Copy link

thomacos commented Sep 5, 2019

Works like charm!
Thanks again!

@aarnaud

This comment has been minimized.

Copy link
Author

aarnaud commented Oct 29, 2019

hi, this PR is ok for you? when can i expect to be merged

@matthyx

This comment has been minimized.

Copy link
Contributor

matthyx commented Nov 15, 2019

/cc
/ok-to-test

@k8s-ci-robot k8s-ci-robot requested a review from matthyx Nov 15, 2019
@aarnaud aarnaud force-pushed the aarnaud:windows-devicemanager branch from 4ce09a7 to 69ae7d7 Nov 15, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 15, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aarnaud
To complete the pull request process, please assign random-liu
You can assign the PR to them by writing /assign @random-liu in a comment when ready.

The full list of commands accepted by this bot can be found 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

@aarnaud aarnaud force-pushed the aarnaud:windows-devicemanager branch from 69ae7d7 to ecb9feb Nov 15, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Nov 15, 2019
@aarnaud aarnaud force-pushed the aarnaud:windows-devicemanager branch from ecb9feb to 6ad82d2 Nov 15, 2019
@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Nov 15, 2019
@aarnaud aarnaud force-pushed the aarnaud:windows-devicemanager branch from 6ad82d2 to 010a91d Nov 15, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Nov 15, 2019
@matthyx

This comment has been minimized.

Copy link
Contributor

matthyx commented Nov 15, 2019

/lgtm
/assign sjenning

@matthyx

This comment has been minimized.

Copy link
Contributor

matthyx commented Nov 15, 2019

Please run ./hack/update-bazel.sh and update your PR.

Signed-off-by: Anthony ARNAUD <github@anthony-arnaud.fr>
@aarnaud aarnaud force-pushed the aarnaud:windows-devicemanager branch from 010a91d to 2f6a92b Nov 15, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Nov 15, 2019
@matthyx

This comment has been minimized.

Copy link
Contributor

matthyx commented Nov 16, 2019

/test pull-kubernetes-integration
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 16, 2019
@aarnaud

This comment has been minimized.

Copy link
Author

aarnaud commented Nov 17, 2019

/assign @Random-Liu

@aarnaud

This comment has been minimized.

Copy link
Author

aarnaud commented Dec 8, 2019

hi @matthyx and @Random-Liu, Sorry to disturbing you.
When can expect this PR will be merged? k8s 1.17 will release soon, this feature can be present ?

thanks

@matthyx

This comment has been minimized.

Copy link
Contributor

matthyx commented Dec 9, 2019

Hello @aarnaud , unfortunately I am not an approver... we need to wait on @Random-Liu for that.
Regarding the 1.17, it's already too late to include this change as code freeze has already happened.
You can follow the release schedule here

@thomacos

This comment has been minimized.

Copy link

thomacos commented Dec 9, 2019

Hello @Random-Liu, hello @matthyx,

We are also urgently waiting for that feature.

The implementation of this PR is quite natural, well coded and risk free.
So I'm not sure what the reason for the holdup is.

Hello @aarnaud , unfortunately I am not an approver... we need to wait on @Random-Liu for that.
Regarding the 1.17, it's already too late to include this change as code freeze has already happened.
You can follow the release schedule here

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Dec 10, 2019

I'm going to add this to the 1.18 milestone so that it's properly tracked. cc @alejandrox1

/milestone v1.18

@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 10, 2019
@thomacos

This comment has been minimized.

Copy link

thomacos commented Dec 12, 2019

I'm going to add this to the 1.18 milestone so that it's properly tracked. cc @alejandrox1

/milestone v1.18

Sounds good, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.