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.

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

node: devicemgr: Graduate Kubelet DeviceManager to GA #112980

Merged
merged 4 commits into from Nov 2, 2022
Merged

node: devicemgr: Graduate Kubelet DeviceManager to GA #112980

merged 4 commits into from Nov 2, 2022

Conversation

swatisehgal
Copy link
Contributor

@swatisehgal swatisehgal commented Oct 11, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Captures changes to graduate Kubelet Device Manager to GA.

Which issue(s) this PR fixes:

Issue: kubernetes/enhancements#3573

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Graduate Kubelet Device Manager to GA.

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

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 11, 2022
@swatisehgal swatisehgal mentioned this pull request Oct 11, 2022
Closed
4 tasks
@swatisehgal
Copy link
Contributor Author

/test pull-kubernetes-unit

@ffromani
Copy link
Contributor

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 13, 2022
@ffromani
Copy link
Contributor

/priority important-soon
because sig-node would like to promote device manager GA in 1.26

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 13, 2022
@ffromani
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 13, 2022
@liggitt
Copy link
Member

liggitt commented Oct 13, 2022

/uncc
cc @kubernetes/sig-node-pr-reviews

@k8s-ci-robot k8s-ci-robot removed the request for review from liggitt October 13, 2022 15:08
@swatisehgal
Copy link
Contributor Author

/cc @fromanirh @klueska
for SIG node review

ffromani
ffromani reviewed Oct 24, 2022
View reviewed changes
Copy link
Contributor

@ffromani ffromani left a comment •
edited

Choose a reason for hiding this comment

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

LGTM (soft, non-binding) because: this change is correct; I'm not sure the rest of the code (and the tests) should be changed to consider the feature always on. Should this be changed when we lock the feature gate, or should we do this when we remove the feature gate? Or are both acceptable?

@swatisehgal
Copy link
Contributor Author

LGTM (soft, non-binding) because: this change is correct; I'm not sure the rest of the code (and the tests) should be changed to consider the feature always on. Should this be changed when we lock the feature gate, or should we do this when we remove the feature gate? Or are both acceptable?

Good point, I will look for references to the DevicePlugins feature gate and remove where it is no longer required.

@swatisehgal
node: devicemgr: Graduate Kubelet DeviceManager to GA
131704e 
Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
@klueska
Copy link
Contributor

klueska commented Oct 27, 2022

also @endocrimes for features

@swatisehgal
Copy link
Contributor Author

/test pull-kubernetes-e2e-capz-windows-containerd
/test pull-kubernetes-e2e-gce-alpha-features

@swatisehgal
Copy link
Contributor Author

/retest

@bart0sh
Copy link
Contributor

bart0sh commented Oct 31, 2022

/restest

@bart0sh
Copy link
Contributor

bart0sh commented Nov 2, 2022

@swatisehgal could it be that alpha-features test fails because device manager tries to call device plugin, but no plugin is available?

@swatisehgal
Copy link
Contributor Author

@swatisehgal could it be that alpha-features test fails because device manager tries to call device plugin, but no plugin is available?

I won't say I am 100% sure but it doesn't look like that is the case here. On inspection of the test args of pull-kubernetes-e2e-gce-alpha-features job here device manager e2e tests are not being executed as part of the job. Also the build logs show that the failures have nothing to do with device manager.

Both the jobs (pull-kubernetes-e2e-gce-alpha-features and pull-kubernetes-e2e-capz-windows-containerd ) appear to be very flaky on testgrid. Also, both are not showing up as required so I assume that they wouldn't block PR merging once we have all the needed approvals. Will retest again anyway.

/retest

@swatisehgal
node: devicemgr: GA graduation implies Feature Gate is ON by default
752fa09 
Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
@swatisehgal
node: devicemgr: Remove devicePluginEnabled field from container mgr
8b29ede 
With graduation of device plugins to GA in 1.26, the feature gate is
enabled by default so `devicePluginEnabled` field no longer needs to
be passed at the time of Container Manager creation.

In addition to that, we remove the `ManagerStub` as it is no longer
needed.

Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
@swatisehgal
node: devicemgr: Address warnings from golint
4074168 
Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2022
@dims
Copy link
Member

dims commented Nov 2, 2022

/retest
/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 Nov 2, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, klueska, swatisehgal

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2022
@swatisehgal
Copy link
Contributor Author

/test pull-kubernetes-integration

@swatisehgal
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind

@swatisehgal
Copy link
Contributor Author

/retest

1 similar comment
@swatisehgal
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 25dc4c4 into kubernetes:master Nov 2, 2022
SIG Node PR Triage automation moved this from Triage to Done Nov 2, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 2, 2022
@swatisehgal swatisehgal deleted the devicemanager-ga-graduation branch November 3, 2022 10:25
@swatisehgal swatisehgal mentioned this pull request Nov 3, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims dims left review comments

@klueska klueska klueska left review comments

@bart0sh bart0sh bart0sh left review comments

@ffromani ffromani ffromani left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

Assignees

@dims dims

@klueska klueska

@marseel marseel

@derekwaynecarr derekwaynecarr

@wojtek-t wojtek-t

@bart0sh bart0sh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/provider/gcp Issues or PRs related to gcp provider 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Done
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@swatisehgal @ffromani @liggitt @bart0sh @klueska @dims @k8s-ci-robot @marseel @derekwaynecarr @wojtek-t