Reintroduce CSI 0.3.x support in CSI Volume Plugin #71314

saad-ali · 2018-11-21T11:06:21Z

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
PR #71020 removed support for CSI 0.x while adding support for CSI 1.x. This PR reintroduces support for CSI 0.x, so that Kubernetes can continue to support older CSI drivers.

/sig storage

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #71228

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

nikopen · 2018-11-21T18:08:11Z

/milestone v1.13
/priority critical-urgent

msau42 · 2018-11-21T19:03:58Z

/assign

msau42 · 2018-11-21T19:29:52Z

I suggest also adding e2e tests using the old drivers in 1.13

saad-ali · 2018-11-21T21:13:29Z

/test pull-kubernetes-integration

vishh · 2018-11-21T21:43:05Z

pkg/kubelet/cm/devicemanager/manager.go

 	klog.V(2).Infof("Got Plugin %s at endpoint %s with versions %v", pluginName, endpoint, versions)

+	if foundInDeprecatedDir {
+		return fmt.Errorf("device plugin socket was found in a directory that is no longer supported")


Why fail with device plugins?

Per #70494 (comment) the plugin directory is changing, and the old one will be deprecated. That PR moved all consumers (CSI and device plugin to the new dir). This PR makes an exception for CSI 0.x but does not change anything else (meaning device plugin and CSI 1.x drivers are still required to use the new dir).

vishh · 2018-11-21T21:52:14Z

/approve

k8s-ci-robot · 2018-11-21T21:52:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali, vishh

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:

~~hack/OWNERS~~ [vishh]
~~pkg/kubelet/OWNERS~~ [vishh]
~~pkg/volume/csi/OWNERS~~ [saad-ali]
~~test/OWNERS~~ [saad-ali,vishh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

AishSundar · 2018-11-21T21:52:32Z

/test pull-kubernetes-integration

saad-ali · 2018-11-21T22:04:01Z

Hostpath CSI tests run as part of pull CI and are green.

I manually ran GCE PD CSI tests, and they are all green:

Ran 24 of 1919 Specs in 4412.483 seconds
SUCCESS! -- 24 Passed | 0 Failed | 0 Pending | 1895 Skipped PASS

Next I'm adding e2e tests using the old drivers in 1.13.

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

liggitt · 2018-11-21T21:44:46Z

pkg/kubelet/cm/devicemanager/manager.go

 	klog.V(2).Infof("Got Plugin %s at endpoint %s with versions %v", pluginName, endpoint, versions)

+	if foundInDeprecatedDir {


I'm unfamiliar with this code, but is this completely separate from the storage driver code, and just implements the same PluginHandler interface?

is the same plugin watcher used to feed events to both CSI and device manager PluginHandler impls? if so, doesn't the plugin dir change in 1.13 affect device plugins as well? what stage are device plugins at, and how many releases do we need to honor them in the v1.12 location?

this completely separate from the storage driver code, and just implements the same PluginHandler interface?

Correct.

is the same plugin watcher used to feed events to both CSI and device manager PluginHandler impls?

Yes.

if so, doesn't the plugin dir change in 1.13 affect device plugins as well?

Yes, it does. Per the discussions #70494 (comment) the owners of device plugin code @vishh @jiayingz are ok with the change.

what stage are device plugins at, and how many releases do we need to honor them in the v1.12 location?

@vishh @jiayingz can probably answer this better then I can.

Device plugins haven't yet adopted the switch to using plugin watcher yet and we believe the risk very very low to make the switch to the new directory format.
However, given that this PR is supporting old plugin watcher implementation anyways for CSI, it might make sense to support for device plugins too just for the sake of consistency.

Ok cool. I'll go ahead and remove the failure in this case then.

However, given that this PR is supporting old plugin watcher implementation anyways for CSI, it might make sense to support for device plugins too just for the sake of consistency.

I agree with that, thanks. Make sure the deprecation of the old dir is prominently announced for both device plugins and csi plugins in 1.13 release notes.

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

pkg/volume/csi/csi_client.go

thockin · 2018-11-21T22:25:22Z

Thanks for finding a way to DTRT

saad-ali · 2018-11-22T01:28:11Z

/test pull-kubernetes-e2e-gce

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

saad-ali · 2018-11-22T02:04:36Z

Add E2E test to verify CSI 0.x. The tests are passing.

Unless there is any other feedback. This PR is ready to go.

test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpathplugin.yaml

msau42 · 2018-11-22T02:06:30Z

csi volume plugin and e2es LGTM

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

Modify kubelet plugin watcher to support older CSI drivers that use an the old plugins directory for socket registration. Also modify CSI plugin registration to support multiple versions of CSI registering with the same name.

Remove csipb dependency from everywhere except the CSI client in preperation for supporting multiple CSI clients.

Modify the CSI volume plugin to handle CSI version 0.x as well as 1.x

liggitt · 2018-11-22T02:39:17Z

/lgtm

liggitt · 2018-11-22T02:42:22Z

set release note to NONE, will need to fold in updates in the 1.13 release notes manually, along with a revision of the release note in #71020

k8s-ci-robot requested review from davidz627 and pmorie November 21, 2018 11:08

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 21, 2018

saad-ali force-pushed the csi03Compat branch from e0621db to 9845a9e Compare November 21, 2018 12:37

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 21, 2018

saad-ali force-pushed the csi03Compat branch from 9845a9e to 87515c9 Compare November 21, 2018 17:37

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 21, 2018

k8s-ci-robot added this to the v1.13 milestone Nov 21, 2018

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 21, 2018

k8s-ci-robot assigned msau42 Nov 21, 2018

saad-ali mentioned this pull request Nov 21, 2018

CSI Upgrade Tests Failing for hostPath and gcePD #71228

Closed

vishh reviewed Nov 21, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2018

liggitt reviewed Nov 21, 2018

View reviewed changes

liggitt reviewed Nov 22, 2018

View reviewed changes

pkg/kubelet/util/pluginwatcher/plugin_watcher.go Outdated Show resolved Hide resolved

liggitt reviewed Nov 22, 2018

View reviewed changes

pkg/kubelet/util/pluginwatcher/plugin_watcher.go Outdated Show resolved Hide resolved

liggitt reviewed Nov 22, 2018

View reviewed changes

test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpathplugin.yaml Show resolved Hide resolved

saad-ali force-pushed the csi03Compat branch from a6161e4 to 9669b36 Compare November 22, 2018 02:17

liggitt reviewed Nov 22, 2018

View reviewed changes

pkg/kubelet/util/pluginwatcher/plugin_watcher.go Outdated Show resolved Hide resolved

saad-ali added 5 commits November 21, 2018 18:37

Modify kubelet watcher to support old versions

8f666d9

Modify kubelet plugin watcher to support older CSI drivers that use an the old plugins directory for socket registration. Also modify CSI plugin registration to support multiple versions of CSI registering with the same name.

Reduce csipb dependencies

2b44ec5

Remove csipb dependency from everywhere except the CSI client in preperation for supporting multiple CSI clients.

Add missing feature gate to CSI driver uninstall

d1b4485

Modify CSI to handle both 0.3 and 1.0

aa8244b

Modify the CSI volume plugin to handle CSI version 0.x as well as 1.x

Permit use of deprecated dir in device plugin.

a7c5582

saad-ali force-pushed the csi03Compat branch from 9669b36 to a7c5582 Compare November 22, 2018 02:37

k8s-ci-robot assigned liggitt Nov 22, 2018

saad-ali mentioned this pull request Nov 22, 2018

Deprecate old plugin watcher dir #71335

Closed

k8s-ci-robot merged commit 20e1ab6 into kubernetes:master Nov 22, 2018

This was referenced Nov 22, 2018

Add a test for "graceful" CSI upgrade between 1.12 to 1.13 #71282

Closed

CSI: registration process fails #71378

Closed

jberkus mentioned this pull request Nov 23, 2018

Flaking Test: subpath failures in new-master-upgrade-cluster-new-parallel, other jobs #71383

Closed

saad-ali mentioned this pull request Nov 26, 2018

Amend test to work with with version skew of 1.13+ by using CSI V1 compatible drivers and sidecars #71241

Closed

davidz627 mentioned this pull request Nov 26, 2018

Amend hostpath CSI Driver tests to run with version skew of 1.13+ by … #71244

Closed

msau42 mentioned this pull request Nov 28, 2018

Flake: sig-storage] [Serial] CSI Volumes CSI plugin test using CSI driver: gcePD [Serial] should provision storage #71519

Closed

This was referenced Dec 19, 2018

Ignore non socket files in the kubelet plugin watcher #70494

Merged

Pluginwatcher backport sanity patches on 1.12 #72221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce CSI 0.3.x support in CSI Volume Plugin #71314

Reintroduce CSI 0.3.x support in CSI Volume Plugin #71314

saad-ali commented Nov 21, 2018 •

edited by liggitt

Loading

nikopen commented Nov 21, 2018

msau42 commented Nov 21, 2018

msau42 commented Nov 21, 2018

saad-ali commented Nov 21, 2018

vishh Nov 21, 2018

saad-ali Nov 21, 2018 •

edited

Loading

vishh commented Nov 21, 2018

k8s-ci-robot commented Nov 21, 2018

AishSundar commented Nov 21, 2018

saad-ali commented Nov 21, 2018

liggitt Nov 21, 2018

liggitt Nov 21, 2018 •

edited

Loading

saad-ali Nov 21, 2018

vishh Nov 22, 2018

saad-ali Nov 22, 2018

liggitt Nov 22, 2018

thockin commented Nov 21, 2018

saad-ali commented Nov 22, 2018

saad-ali commented Nov 22, 2018

msau42 commented Nov 22, 2018

liggitt commented Nov 22, 2018

liggitt commented Nov 22, 2018

		klog.V(2).Infof("Got Plugin %s at endpoint %s with versions %v", pluginName, endpoint, versions)

		if foundInDeprecatedDir {

Reintroduce CSI 0.3.x support in CSI Volume Plugin #71314

Reintroduce CSI 0.3.x support in CSI Volume Plugin #71314

Conversation

saad-ali commented Nov 21, 2018 • edited by liggitt Loading

nikopen commented Nov 21, 2018

msau42 commented Nov 21, 2018

msau42 commented Nov 21, 2018

saad-ali commented Nov 21, 2018

vishh Nov 21, 2018

Choose a reason for hiding this comment

saad-ali Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

vishh commented Nov 21, 2018

k8s-ci-robot commented Nov 21, 2018

AishSundar commented Nov 21, 2018

saad-ali commented Nov 21, 2018

liggitt Nov 21, 2018

Choose a reason for hiding this comment

liggitt Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

saad-ali Nov 21, 2018

Choose a reason for hiding this comment

vishh Nov 22, 2018

Choose a reason for hiding this comment

saad-ali Nov 22, 2018

Choose a reason for hiding this comment

liggitt Nov 22, 2018

Choose a reason for hiding this comment

thockin commented Nov 21, 2018

saad-ali commented Nov 22, 2018

saad-ali commented Nov 22, 2018

msau42 commented Nov 22, 2018

liggitt commented Nov 22, 2018

liggitt commented Nov 22, 2018

saad-ali commented Nov 21, 2018 •

edited by liggitt

Loading

saad-ali Nov 21, 2018 •

edited

Loading

liggitt Nov 21, 2018 •

edited

Loading