Use CSI proxy v1 client library #738

mauriciopoppe · 2021-04-07T17:47:41Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds compatibility with the following versions of the csi-proxy client: disk v1, volume v1, filesystem v1, the plan to upgrade is:

deploy PD CSI with the csi-proxy v1 (and others) client, keep the beta csi-proxy server on the node, the v1 client would fallback to talking to the beta csi-proxy server
bump csi-proxy server to v1 on the node, to do this we need to drain the node and restart PD CSI which will then talk to v1 automatically

Summary of changes:

add new hostPath mounts to the windows nodes, these mounts are the v1 named pipes, these named pipes don't exist however the implementation is able to understand this fact and it fallbacks to the v1beta2/v1beta1 named pipes
safe-mounter_windows.go was cloned into a v1 and a v1beta version, the differences are the imports (I could also change this to multiple if statements but a clone makes the code easy to follow)
NewSafeMounter creates either a v1 mounter or a v1beta mounter, the Interface is casted to the required type on runtime

Logs of the new implementation:

Test steps:

create a kubernetes GCE cluster with 2 windows nodes
choose one of the windows nodes and deploy csi-proxy.exe, verify that the v1 named pipes are available

PS C:\Users\mauriciopoppe\go\src\github.com\kubernetes-csi\csi-proxy> [System.IO.Directory]::GetFiles("\\.\\pipe\\")
\\.\\pipe\\InitShutdown
\\.\\pipe\\lsass
\\.\\pipe\\ntsvcs
\\.\\pipe\\scerpc
\\.\\pipe\\Winsock2\CatalogChangeListener-364-0
\\.\\pipe\\epmapper
\\.\\pipe\\Winsock2\CatalogChangeListener-1f4-0
\\.\\pipe\\LSM_API_service
\\.\\pipe\\W32TIME_ALT
\\.\\pipe\\eventlog
\\.\\pipe\\Winsock2\CatalogChangeListener-138-0
\\.\\pipe\\wkssvc
\\.\\pipe\\atsvc
\\.\\pipe\\Winsock2\CatalogChangeListener-500-0
\\.\\pipe\\Winsock2\CatalogChangeListener-278-0
\\.\\pipe\\openssh-ssh-agent
\\.\\pipe\\TermSrv_API_service
\\.\\pipe\\Ctx_WinStation_API_service
\\.\\pipe\\SessEnvPublicRpc
\\.\\pipe\\Winsock2\CatalogChangeListener-a7c-0
\\.\\pipe\\srvsvc
\\.\\pipe\\Winsock2\CatalogChangeListener-264-0
\\.\\pipe\\PSHost.132685607009879421.2264.DefaultAppDomain.powershell
\\.\\pipe\\TSVCPIPE-6969a09d-6df7-43e9-8b4d-07ca51b17aeb
\\.\\pipe\\docker_engine
\\.\\pipe\\PSHost.132685607145678539.4928.DefaultAppDomain.powershell
\\.\\pipe\\PIPE_EVENTROOT\CIMV2SCM EVENT PROVIDER
\\.\\pipe\\dockershim
\\.\\pipe\\PSHost.132685608544452389.4896.DefaultAppDomain.powershell
\\.\\pipe\\csi-proxy-filesystem-v1alpha1
\\.\\pipe\\csi-proxy-filesystem-v1beta1
\\.\\pipe\\csi-proxy-filesystem-v1beta2
\\.\\pipe\\csi-proxy-filesystem-v1
\\.\\pipe\\csi-proxy-disk-v1alpha1
\\.\\pipe\\csi-proxy-disk-v1beta1
\\.\\pipe\\csi-proxy-disk-v1beta2
\\.\\pipe\\csi-proxy-disk-v1beta3
\\.\\pipe\\csi-proxy-disk-v1
\\.\\pipe\\csi-proxy-volume-v1alpha1
\\.\\pipe\\csi-proxy-volume-v1beta1
\\.\\pipe\\csi-proxy-volume-v1beta2
\\.\\pipe\\csi-proxy-volume-v1beta3
\\.\\pipe\\csi-proxy-volume-v1
\\.\\pipe\\csi-proxy-smb-v1alpha1
\\.\\pipe\\csi-proxy-smb-v1beta1
\\.\\pipe\\csi-proxy-smb-v1beta2
\\.\\pipe\\csi-proxy-smb-v1
\\.\\pipe\\csi-proxy-system-v1alpha1
\\.\\pipe\\csi-proxy-iscsi-v1alpha1
\\.\\pipe\\csi-proxy-iscsi-v1alpha2

compile PD CSI driver and deploy it the cluster, check the logs of PD CSI driver in both windows nodes

# windows node without v1 pipes enabled
I0619 08:14:54.770087    1504 main.go:73] Driver vendor version latest
I0619 08:14:54.843225    1504 safe-mounter_windows.go:35] failed to connect to csi-proxy v1 with error=open \\.\\pipe\\csi-proxy-filesystem-v1: The system cannot find the file specified., will try w
I0619 08:14:54.843225    1504 safe-mounter_windows.go:39] using CSIProxyMounterV1Beta, API Versions Disk: v1beta2, Filesystem: v1beta1, Volume: v1beta1

# windows node with v1 pipes enabled
I0619 08:14:54.500483    3260 main.go:73] Driver vendor version latest
I0619 08:14:54.503411    3260 safe-mounter_windows.go:28] using CSIProxyMounterV1, API Versions Disk: v1, Filesystem: v1, Volume: v1

Test on v1beta: Untaint the v1beta node and taint the v1 node and run the External Storage suite from k/k (compatibility with the existing csi-proxy server)

GINKGO_PARALLEL_NODES=4 ./hack/ginkgo-e2e.sh --ginkgo.focus="External.*Storage" --ginkgo.skip="\[Disruptive\]|\[Serial\]|\[LinuxOnly\]" --allowed-not-ready-nodes=10 --storage.testdriver="/usr/local/google/home/mauriciopoppe/go/src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/k8s-integration/config/test-config-windows.yaml" --node-os-distro=windows

Ran 36 of 6637 Specs in 2833.752 seconds
SUCCESS! -- 36 Passed | 0 Failed | 0 Pending | 6601 Skipped

Test on v1: Untaint the v1 node and taint v1beta node and run the External Storage suite from k/k

GINKGO_PARALLEL_NODES=4 ./hack/ginkgo-e2e.sh --ginkgo.focus="External.*Storage" --ginkgo.skip="\[Disruptive\]|\[Serial\]|\[LinuxOnly\]" --allowed-not-ready-nodes=10 --storage.testdriver="/usr/local/google/home/mauriciopoppe/go/src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/k8s-integration/config/test-config-windows.yaml" --node-os-distro=windows

Ran 36 of 6637 Specs in 2970.219 seconds
SUCCESS! -- 36 Passed | 0 Failed | 0 Pending | 6601 Skipped

(pending) Run the optional prow job e2e External Storage testsuite setting csi-proxy v1 in k/k Add optional presubmit prow job for GCE PD CSI Driver with csi-proxy v1 kubernetes/test-infra#22636

Next changes:

bump csi-proxy from v1-rc to v1

Does this PR introduce a user-facing change?:

Bumped csi-proxy client library to v1.0.0

/assign @jingxu97 @msau42

k8s-ci-robot · 2021-04-07T17:47:49Z

Hi @mauriciopoppe. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

go.mod

mauriciopoppe · 2021-04-07T17:48:43Z

manifest_osversion.sh

@@ -6,7 +6,6 @@
 # replace the following with annotation approach. https://github.com/docker/cli/pull/2578

 export DOCKER_CLI_EXPERIMENTAL=enabled
-_WINDOWS_VERSIONS="20H2 2004 1909 ltsc2019"


this line was unused in this program

k8s-ci-robot · 2021-04-07T23:53:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mauriciopoppe
To complete the pull request process, please ask for approval from jingxu97 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

mauriciopoppe · 2021-06-23T01:06:26Z

/hold

Even though this is working I have to make a small refactor taking all of the methods of both v1 and v1beta mounters to an interface to simplify the code

mauriciopoppe · 2021-06-24T18:24:05Z

/unhold

Made the refactor and run the e2e tests again and added the results in the description

mauriciopoppe · 2021-06-24T18:24:15Z

/ok-to-test

mauriciopoppe · 2021-06-24T18:26:52Z

/test pull-gcp-compute-persistent-disk-csi-driver-e2e-win2019

mattcary · 2021-06-24T21:15:00Z

It's the same integration test problem. We still haven't figured out what's going on. After you get an lgtm from Jing I can force-merge this.

jingxu97 · 2021-06-25T22:28:48Z

deploy/kubernetes/overlays/noauth-debug/kustomization.yaml

@@ -8,6 +8,7 @@ transformers:
 patchesStrategicMerge:
 - noauth.yaml
 - controller-overlay.yaml
+- node-overlay.yaml


why we need add this yaml too?

this is a dev only file, I used it to deploy the GCE PD CSI driver with AlwaysPull

jingxu97 · 2021-06-25T22:29:42Z

pkg/gce-pd-csi-driver/utils_windows.go

@@ -39,7 +39,7 @@ func formatAndMount(source, target, fstype string, options []string, m *mount.Sa
 // not exist. Currently kubelet creates the path beforehand, this is a workaround to
 // remove the path first.
 func preparePublishPath(path string, m *mount.SafeFormatAndMount) error {
-	proxy, ok := m.Interface.(*mounter.CSIProxyMounter)
+	proxy, ok := m.Interface.(mounter.CSIProxyMounter)


why pointer is removed?

Previously it was a struct, now it's an interface implemented by v1 and v1beta

jingxu97 · 2021-07-14T21:11:51Z

/lgtm

k8s-ci-robot · 2021-07-14T23:59:43Z

New changes are detected. LGTM label has been removed.

mauriciopoppe · 2021-07-15T00:00:44Z

@jingxu97 I've bumped the csi-proxy client library to v1, could you lgtm again please?

mattcary · 2021-07-15T00:02:52Z

I'll wait for the tests to run, confirm they pass except for the usual suspect, and force the merge.

k8s-ci-robot · 2021-07-15T01:08:17Z

@mauriciopoppe: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration	`2d0e8dd`	link	`/test pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

mattcary · 2021-07-15T19:59:57Z

/test pull-gcp-compute-persistent-disk-csi-driver-e2e

k8s-ci-robot assigned jingxu97 and msau42 Apr 7, 2021

k8s-ci-robot requested review from jingxu97 and msau42 April 7, 2021 17:47

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 7, 2021

mauriciopoppe commented Apr 7, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

mauriciopoppe commented Apr 7, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2021

mauriciopoppe force-pushed the csi-proxy-v1-client branch from 0e0646d to 666f2b9 Compare June 11, 2021 22:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2021

mauriciopoppe changed the title ~~Use CSI proxy v1~~ Use new CSI proxy v1beta3/v1beta2 client libraries Jun 11, 2021

mauriciopoppe changed the title ~~Use new CSI proxy v1beta3/v1beta2 client libraries~~ [WIP] Use new CSI proxy v1beta3/v1beta2 client libraries Jun 11, 2021

mauriciopoppe force-pushed the csi-proxy-v1-client branch from d623c9f to 6b2ce4c Compare June 11, 2021 23:38

mauriciopoppe changed the title ~~[WIP] Use new CSI proxy v1beta3/v1beta2 client libraries~~ [WIP] Use CSI proxy v1 client libraries Jun 17, 2021

mauriciopoppe mentioned this pull request Jun 21, 2021

Expose envvar CSI_PROXY_VERSION kubernetes/kubernetes#103060

Merged

mauriciopoppe force-pushed the csi-proxy-v1-client branch from 450e61b to 75565a3 Compare June 23, 2021 00:24

mauriciopoppe changed the title ~~[WIP] Use CSI proxy v1 client libraries~~ Use CSI proxy v1 client library Jun 23, 2021

mauriciopoppe marked this pull request as ready for review June 23, 2021 00:54

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2021

mauriciopoppe force-pushed the csi-proxy-v1-client branch from 75565a3 to f013e83 Compare June 23, 2021 03:37

use [email protected]

f668952

mauriciopoppe force-pushed the csi-proxy-v1-client branch from f013e83 to f668952 Compare June 23, 2021 03:40

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2021

jingxu97 reviewed Jun 25, 2021

View reviewed changes

mauriciopoppe mentioned this pull request Jul 6, 2021

panic: protobuf tag not enough fields in PathExistsResponse.state kubernetes-csi/csi-proxy#159

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2021

bump csi-proxy v1.rc to v1

2d0e8dd

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2021

mattcary merged commit f2ea711 into kubernetes-sigs:master Jul 16, 2021

This was referenced Jul 16, 2021

Prototype: multi-version CSI-proxy #716

Closed

Add optional presubmit prow job for GCE PD CSI Driver with csi-proxy v1 kubernetes/test-infra#22636

Closed

mauriciopoppe mentioned this pull request Aug 25, 2021

master: update release-tools #812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CSI proxy v1 client library #738

Use CSI proxy v1 client library #738

mauriciopoppe commented Apr 7, 2021 •

edited

Loading

k8s-ci-robot commented Apr 7, 2021

mauriciopoppe Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

mauriciopoppe commented Jun 23, 2021

mauriciopoppe commented Jun 24, 2021

mauriciopoppe commented Jun 24, 2021

mauriciopoppe commented Jun 24, 2021

mattcary commented Jun 24, 2021

jingxu97 Jun 25, 2021

mauriciopoppe Jun 25, 2021

jingxu97 Jun 25, 2021

mauriciopoppe Jun 25, 2021

jingxu97 commented Jul 14, 2021

k8s-ci-robot commented Jul 14, 2021

mauriciopoppe commented Jul 15, 2021

mattcary commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021 •

edited

Loading

mattcary commented Jul 15, 2021

Use CSI proxy v1 client library #738

Use CSI proxy v1 client library #738

Conversation

mauriciopoppe commented Apr 7, 2021 • edited Loading

Summary of changes:

Logs of the new implementation:

Next changes:

k8s-ci-robot commented Apr 7, 2021

mauriciopoppe Apr 7, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 7, 2021

mauriciopoppe commented Jun 23, 2021

mauriciopoppe commented Jun 24, 2021

mauriciopoppe commented Jun 24, 2021

mauriciopoppe commented Jun 24, 2021

mattcary commented Jun 24, 2021

jingxu97 Jun 25, 2021

Choose a reason for hiding this comment

mauriciopoppe Jun 25, 2021

Choose a reason for hiding this comment

jingxu97 Jun 25, 2021

Choose a reason for hiding this comment

mauriciopoppe Jun 25, 2021

Choose a reason for hiding this comment

jingxu97 commented Jul 14, 2021

k8s-ci-robot commented Jul 14, 2021

mauriciopoppe commented Jul 15, 2021

mattcary commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021 • edited Loading

mattcary commented Jul 15, 2021

mauriciopoppe commented Apr 7, 2021 •

edited

Loading

k8s-ci-robot commented Jul 15, 2021 •

edited

Loading