Support GCEPD driver for Windows #483

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

Merged

k8s-ci-robot merged 1 commit into kubernetes-sigs:master from jingxu97:Feb/csiproxy

Apr 3, 2020

Contributor

jingxu97 commented Mar 17, 2020 •

edited

Loading

This PR adds the support for gcepd driver to work on Windows node. The
driver will call csi-proxy API to perform file, volume and disk
operations.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that 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:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add Windows support in GCE PD driver with the use of csi-proxy (https://github.com/kubernetes-csi/csi-proxy/).

k8s-ci-robot added do-not-merge/work-in-progress do-not-merge/release-note-label-needed cncf-cla: yes labels

k8s-ci-robot requested review from davidz627 and saad-ali

March 17, 2020 20:07

k8s-ci-robot added the size/XL label

jingxu97 force-pushed the Feb/csiproxy branch from 6655e6b to c222a47 Compare

March 21, 2020 01:27

jingxu97 changed the title ~~WIP: Support GCEPD driver for Windows~~ Support GCEPD driver for Windows

k8s-ci-robot removed the do-not-merge/work-in-progress label

jingxu97 force-pushed the Feb/csiproxy branch 2 times, most recently from f8d2676 to 9b72ff0 Compare

March 23, 2020 17:28

Contributor Author

jingxu97 commented Mar 23, 2020

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

Contributor Author

jingxu97 commented Mar 23, 2020

cc @ddebroy @kkmsft @msau42

Contributor

msau42 commented Mar 23, 2020

/retest

Contributor Author

jingxu97 commented Mar 23, 2020

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

1 similar comment

Contributor Author

jingxu97 commented Mar 24, 2020

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

jingxu97 force-pushed the Feb/csiproxy branch from 9b72ff0 to 7e38ddd Compare

March 24, 2020 18:45

Contributor Author

jingxu97 commented Mar 25, 2020

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

1 similar comment

Contributor Author

jingxu97 commented Mar 25, 2020

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

jingxu97 force-pushed the Feb/csiproxy branch 5 times, most recently from e7aa0f2 to c8c2d8b Compare

March 26, 2020 18:57

Contributor Author

jingxu97 commented Mar 26, 2020

/test pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration

Contributor Author

jingxu97 commented Mar 26, 2020

cc @saad-ali @msau42

Contributor

verult commented Mar 31, 2020

/cc

k8s-ci-robot requested a review from verult

March 31, 2020 01:48

verult reviewed

View reviewed changes

Contributor

verult left a comment

First pass, need to review the rest of safe-mounter_windows.go

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

+              limitations under the License.
+              */
+              package gceGCEDriver

Contributor

verult Mar 31, 2020

nit: definitely out of scope for this PR, but the convention is to avoid mixCaps: https://golang.org/doc/effective_go.html#package-names

Contributor Author

jingxu97 Apr 1, 2020

ack. we can do another PR for it.

pkg/gce-pd-csi-driver/utils_linux.go Outdated

+              	"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
+              )
+              func GetDevicePath(ns *GCENodeServer, volumeID, partition string) (string, error) {

Contributor

verult Mar 31, 2020

Should all functions here have the same casing for the first letter?

Contributor Author

jingxu97 Mar 31, 2020

fixed.

pkg/mount-manager/statter_windows.go Outdated

+              	return false, nil
+              }
+              //TODO: implement StatFS to get metrics

Contributor

verult Mar 31, 2020

nit: include username or issue #

Contributor Author

jingxu97 Mar 31, 2020

fixed

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

+              		return err
+              	}
+              	if exists {
+              		return proxy.Unmount(path)

Contributor

verult Mar 31, 2020

For Linux we reuse the path if it already exists. Why don't we unmount there?

Contributor Author

jingxu97 Mar 31, 2020 •

edited

Loading

for windows, before mounting (which is creating symlink), the dir should not exist. Only the parent dir should be created. Now in kubelet, it creates this dir beforehand, so this is a workaround to remove the dir first.
Added some comments for it.

Contributor

msau42 Apr 1, 2020

For some reason when I reviewed, I didn't see Cheng's comments and asked the same thing :-)

What if the volume is actually mounted? In that case, I don't think we should do an entire Unmount if it's already done.

Another confusing piece here is that the behavior we want is really "rmdir", but we're calling an api called "unmount".

Contributor Author

jingxu97 Apr 1, 2020

If volume is actually mounted, it should not call this function and return immediately
https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/483/files#diff-37c748634a23b1ed91abeaa8a6d21525R108

in windows, actually unmount and rmdir is the same thing. Mount is to create a symlink, and unmount is to delete this link.
I rename the functions on this and related part. Please let me know if there is any issue.

pkg/mount-manager/safe-mounter_windows.go

+              	}, nil
+              }
+              func (mounter *CSIProxyMounter) Mount(source string, target string, fstype string, options []string) error {

Contributor

verult Mar 31, 2020

Is this called anywhere?

Contributor Author

jingxu97 Mar 31, 2020

in node.go line 172

msau42 reviewed

View reviewed changes

go.mod Outdated

               	gopkg.in/gcfg.v1 v1.2.3
               	gopkg.in/warnings.v0 v0.1.2 // indirect
               	k8s.io/apimachinery v0.17.1
               	k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible
               	k8s.io/klog v1.0.0
+              	k8s.io/kubernetes v1.14.7

Contributor

msau42 Mar 31, 2020

what is this dependency for? the apimachinery dependency is on 1.17 so having this on 1.14 may cause strange problems.

Contributor Author

jingxu97 Apr 1, 2020

removed it. not sure how it was added.

pkg/gce-pd-csi-driver/node.go Outdated Show resolved Hide resolved

pkg/gce-pd-csi-driver/node.go Outdated

               		return &csi.NodeStageVolumeResponse{}, nil
               	}
-              	err = ns.Mounter.FormatAndMount(devicePath, stagingTargetPath, fstype, options)
+              	//err = ns.Mounter.FormatAndMount(devicePath, stagingTargetPath, fstype, options)

Contributor

msau42 Mar 31, 2020

remove this

Contributor Author

jingxu97 Apr 1, 2020

fixed.

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

+              		return err
+              	}
+              	if exists {
+              		return proxy.Unmount(path)

Contributor

msau42 Mar 31, 2020

Why do we need to unmount if it's already mounted?

Contributor Author

jingxu97 Apr 1, 2020

I added a comment. actually it is to remove the dir, not a real unmount like linux. I think could change the name if it is more clear.

pkg/mount-manager/safe-mounter_windows.go Outdated

+              	// Call the LinkPath CSI proxy from the source path to the target path
+              	parentDir := filepath.Dir(target)
+              	if _, err := os.Stat(target); !os.IsNotExist(err) {
+              		os.RemoveAll(target)

Contributor

msau42 Mar 31, 2020

Generally we try to avoid calling RemoveAll because we had cases in the past where the volume was still mounted, and RemoveAll ended up deleting user data.

Contributor Author

jingxu97 Apr 1, 2020

fixed, it is removed because preparePublishPath did it

pkg/mount-manager/safe-mounter_windows.go Outdated

+              	return nil
+              }
+              func normalizeWindowsPath(path string) string {

Contributor

msau42 Mar 31, 2020

Can we add some unit tests for this?

Contributor Author

jingxu97 Apr 1, 2020

actually mount utility already has the exact function and has unit test for it. I replace it with mount utitliy's one.

go.mod Outdated

               	github.com/google/uuid v1.1.1
               	github.com/hashicorp/go-multierror v1.0.0 // indirect
+              	github.com/kubernetes-csi/csi-proxy/client v0.0.0-20200319061913-d6ab31300107

Contributor

msau42 Mar 31, 2020

We can actually use the real tag version, but we need add a special tag to the csi-proxy repo

Contributor Author

jingxu97 Apr 1, 2020

I will work on it.

jingxu97 force-pushed the Feb/csiproxy branch from c8c2d8b to cfd570d Compare

April 1, 2020 01:04

Contributor Author

jingxu97 commented Apr 1, 2020

I tried to addressed most the comments, working on adding unit test.

jingxu97 force-pushed the Feb/csiproxy branch 2 times, most recently from 24c7280 to fb989b6 Compare

April 1, 2020 20:22

Contributor Author

jingxu97 commented Apr 1, 2020

@verult @msau42 I think all comments were addressed. PTAL. Thanks!

Contributor

msau42 commented Apr 1, 2020

/lgtm
/approve
Can you add a release note?

k8s-ci-robot assigned msau42

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented Apr 1, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, 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:

~~OWNERS~~ [msau42]

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 added the approved label

jingxu97 force-pushed the Feb/csiproxy branch from fb989b6 to 62ad1aa Compare

April 2, 2020 00:16

k8s-ci-robot removed the lgtm label

verult reviewed

View reviewed changes

Contributor

verult left a comment •

edited

Loading

/lgtm

Minor nits

Could you also add your GitHub handle to the remaining TODOs (also a nit :) )?

pkg/mount-manager/safe-mounter_windows.go Outdated

+              	if err != nil {
+              		return err
+              	}
+              	// TODO: Check if the volume is formatted by calling IsVolumeFormatted CSI proxy call.

Contributor

verult Apr 2, 2020

Does this still apply?

Contributor Author

jingxu97 Apr 2, 2020

it is not needed. removed it.

pkg/mount-manager/safe-mounter_windows.go Outdated


		}

		// formatAndMount - accepts the source disk number, target path to mount, the fstype to format with and options to be used.

Contributor

verult Apr 2, 2020

Could you mention that it mounts to the host?

Contributor Author

jingxu97 Apr 3, 2020

fixed

k8s-ci-robot assigned verult

k8s-ci-robot added the lgtm label


          Support GCEPD driver for Windows

9ba1f1c

This PR adds the support for gcepd driver to work on Windows node. The
driver will call csi-proxy API to perform file, volume and disk
operations.

jingxu97 force-pushed the Feb/csiproxy branch from 62ad1aa to 9ba1f1c Compare

April 3, 2020 21:32

k8s-ci-robot added release-note and removed lgtm do-not-merge/release-note-label-needed labels

Contributor Author

jingxu97 commented Apr 3, 2020

@msau42 @verult I added release note, and update the PR. Could you please give /lgtm? Thanks!

Contributor

msau42 commented Apr 3, 2020

/lgtm

k8s-ci-robot added the lgtm label

k8s-ci-robot merged commit c83c131 into kubernetes-sigs:master

ddebroy mentioned this pull request

Install csi-proxy.exe to enable CSI plugins for Windows openshift/windows-machine-config-operator#140

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes lgtm release-note size/XL