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

Remove plugin watching of deprecated directory and CSI v0 support in accordance with deprecation policy #84533

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Remove plugin watching of deprecated directory and CSI v0 support in accordance with deprecation policy #84533

merged 1 commit into from
Nov 12, 2019

Conversation

davidz627
Copy link
Contributor

@davidz627 davidz627 commented Oct 29, 2019 •
edited
Loading

The original deprecation warning said removal in 1.15; however, this may have been a mistake since this is behavior not a Beta API and thus deprecation policy is actually 1 year which puts it at Kubernetes v1.17.

/kind cleanup
/kind documentation

/assign @taragu @saad-ali

BREAKING CHANGE: Remove plugin watching of deprecated directory {kubelet_root_dir}/plugins and CSI V0 support in accordance with deprecation announcement in https://v1-13.docs.kubernetes.io/docs/setup/release/notes/

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 29, 2019
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. 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. 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 Oct 29, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 29, 2019
@davidz627
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 29, 2019
@davidz627
Copy link
Contributor Author

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 29, 2019
@davidz627 davidz627 force-pushed the fix/deprecatedPath branch 2 times, most recently from 5efd709 to 5f7802c Compare October 30, 2019 00:09
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2019
@davidz627 davidz627 changed the title Add deprecation warning for using deprecated dir in plugin watcher Remove plugin watching of deprecated directory {kubelet_root_dir}/plugins in accordance with deprecation announcement in https://v1-13.docs.kubernetes.io/docs/setup/release/notes/ Oct 30, 2019
@davidz627 davidz627 changed the title Remove plugin watching of deprecated directory {kubelet_root_dir}/plugins in accordance with deprecation announcement in https://v1-13.docs.kubernetes.io/docs/setup/release/notes/ Remove plugin watching of deprecated directory Oct 30, 2019
@liggitt
Copy link
Member

liggitt commented Nov 7, 2019

one comment on the way this validates driver versions (I don't think it's wrong as written, but the error could be clearer). lgtm otherwise

/approve
/hold for addressing the version comment if desired

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, liggitt, msau42

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 7, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2019
@davidz627
Copy link
Contributor Author

@liggitt @msau42 comments addressed - simplified that validation logic a bit and it feels better now (still not perfect but I wanted to minimize disruptions)

msau42
msau42 reviewed Nov 11, 2019
View reviewed changes
pkg/volume/csi/csi_plugin.go
if isV0Version(version) {
return true
}
if highestSupportedVersion.Major() != 1 {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a driver supports 2.x and 1.x? In the future, if we have a 2.x, the driver Daemonset could theoretically run with a driver that supports both 1.x and 2.x.

Copy link
Contributor Author

@davidz627 davidz627 Nov 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think we would have to go in and update the validation code. I think the theory is that v2.x will have breaking changes and we wouldn't want to just "quietly accept" that version in older versions of k8s. If we do decide that older versions of k8s support v2.x+ in the future we can always cherry-pick a relaxation in validation.

Copy link
Member

Choose a reason for hiding this comment

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

if the driver supports 1.x and 2.x, current versions of kube should allow it and choose to use 1.x, right? does this code do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh just understood this - yeah fixing. If v2.0 AND v1.0 are supported we should return the highest v1.0 version. Fixing now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, fixed. In the code above it will skip any >v1 versions as a highestSupportedVersion candidate - so if a driver supports v2.0 and v1.5 the highestSupportedVersion will be v1.5

@davidz627
Copy link
Contributor Author

/retest

@msau42
Copy link
Member

msau42 commented Nov 11, 2019

/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 11, 2019
@davidz627
Copy link
Contributor Author

/hold cancel
version comment addressed. Thanks for the reviews!

@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 Nov 11, 2019
@k8s-ci-robot k8s-ci-robot merged commit 897ce30 into kubernetes:master Nov 12, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 12, 2019
@davidz627 davidz627 deleted the fix/deprecatedPath branch November 12, 2019 22:53
@msau42 msau42 mentioned this pull request Dec 4, 2019
Merged
@jsafrane jsafrane mentioned this pull request Dec 10, 2019
Merged
@ravilr ravilr mentioned this pull request Jan 21, 2020
Closed
@haoshuwei haoshuwei mentioned this pull request Feb 11, 2020
Closed
mlmhl added a commit to tkestack/csi-operator that referenced this pull request Feb 21, 2020
@mlmhl
update node socket address due to kubernetes/kubernetes#84533
77de4ba
mlmhl added a commit to mlmhl/csi-operator that referenced this pull request Feb 21, 2020
@mlmhl
update node socket address due to kubernetes/kubernetes#84533
dd5c18f
mlmhl added a commit to tkestack/csi-operator that referenced this pull request Feb 21, 2020
@mlmhl
update node socket address due to kubernetes/kubernetes#84533
41f98c3
@sstoner sstoner mentioned this pull request Apr 15, 2020
Closed
yujunz pushed a commit to juicedata/juicefs-csi-driver that referenced this pull request Apr 26, 2020
@rogerz
Fix harmless warnings for k8s < 1.17
dd9ffd1 
> Note that before Kubernetes v1.17, if the csi socket is in the /var/lib/kubelet/plugins/ path, kubelet may log a lot of harmless errors regarding grpc GetInfo call not implemented (fix in kubernetes/kubernetes#84533). The /var/lib/kubelet/csi-plugins/ path is preferred in Kubernetes versions prior to v1.17.

Reference: https://github.com/kubernetes-csi/node-driver-registrar#usage
Close: #54
@Madhu-1 Madhu-1 mentioned this pull request Jul 6, 2020
Closed
@ehashman ehashman mentioned this pull request Oct 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@msau42 msau42 msau42 left review comments

@feiskyer feiskyer Awaiting requested review from feiskyer

@yifan-gu yifan-gu Awaiting requested review from yifan-gu

@jiayingz jiayingz Awaiting requested review from jiayingz

Assignees

@liggitt liggitt

@Random-Liu Random-Liu

@taragu taragu

@saad-ali saad-ali

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. 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/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. 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.17
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@davidz627 @msau42 @liggitt @k8s-ci-robot @Random-Liu @taragu @saad-ali