Windows SMB Support #36

alexiondev · 2019-07-23T21:03:29Z

Refactored some Linux specific code to allow requests to be made when running on Windows to mount SMB shares.

The code has been manualy tested end-to-end and successfully mounts an SMB share on a Windows node. However, it doesn't run containerized yet (see this enhancement) and hasn't been tested against GCP FileStore because SMB support is not available.

k8s-ci-robot · 2019-07-23T21:03:31Z

Welcome @almos98!

It looks like this is your first PR to kubernetes-sigs/gcp-filestore-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gcp-filestore-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-07-23T21:03:44Z

Hi @almos98. Thanks for your PR.

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

pjh · 2019-07-23T21:07:18Z

/cc msau42 pjh

k8s-ci-robot · 2019-07-23T21:07:20Z

@pjh: GitHub didn't allow me to request PR reviews from the following users: pjh.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc msau42 pjh

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.

cmd/main.go

pkg/csi_driver/node.go

msau42 · 2019-07-23T22:43:31Z

/ok-to-test

msau42 · 2019-07-23T22:43:48Z

/assign @jingxu97 @yujuhong

cmd/main.go

pkg/csi_driver/node.go

Gopkg.toml

pkg/csi_driver/node.go

alexiondev

@msau42 I commented out a test for now because the behavior of FakeMount has changed and this test was failing after updating vendor.

msau42 · 2019-08-05T22:03:23Z

pkg/csi_driver/node_test.go

@@ -177,11 +188,54 @@ func TestNodePublishVolume(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		// TODO: enable this test when FakeMount supports Windows behavior


It might be cleaner to actually have a separate test function just for the windows cases.

Is there something still preventing this test from being enabled? If so, let's open an issue for it and reference it here

On Windows, if TargetPath exists it should return an error but FakeMount doesn't do that.

Opened an issue on K/K: kubernetes/kubernetes#81609

pkg/csi_driver/node_test.go

alexiondev · 2019-08-06T17:16:43Z

@msau42 @yujuhong @jingxu97 I think this is ready for review now.
/cc adjackura

k8s-ci-robot · 2019-08-06T17:16:44Z

@almos98: GitHub didn't allow me to request PR reviews from the following users: adjackura.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@msau42 @yujuhong @jingxu97 I think this is ready for review now.
/cc adjackura

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.

pjh · 2019-08-06T18:31:45Z

cc @adjackura

pkg/csi_driver/node.go

pkg/csi_driver/node_test.go

pjh · 2019-08-06T19:42:31Z

LGTM!

@almos98 could you update the PR description to note that you've manually tested this end-to-end to confirm that it successfully mounts an SMB share on the Windows node, but 1) it doesn't run containerized yet (link to kubernetes/enhancements#1122), and 2) it hasn't been tested against GCP Filestore (since SMB support is still not available). Thanks!

msau42 · 2019-08-07T20:12:10Z

This is looking good. Can you also do the following:

squash your commits down into 2-3 commits. One for vendor updates, and 1 or 2 for the Windows implementation? If you haven't done this before, create a duplicate branch first so you still have all your commits
retitle the PR. this is more than a refactor
did you use any scripts to manually test the csi driver? If so, can you add them to test/experimental? Or at least document the steps somewhere

msau42 · 2019-08-07T20:14:16Z

Also is it possible to enhance https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/blob/master/test/run_unit.sh to also run the windows unit tests? Maybe @yujuhong or @pjh can help with how to do that.

msau42 · 2019-08-07T20:15:31Z

Also is it possible to enhance https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/blob/master/test/run_unit.sh to also run the windows unit tests? Maybe Yu-Ju Hong or Peter Hornyack can help with how to do that.

Sorry I realized the unit tests were written in a way that it doesn't actually require a windows env to run. So scratch that, I think it is fine the way it is.

alexiondev · 2019-08-08T20:56:47Z

Squashed commits, changed PR title and documented testing steps!

msau42

/approve

Will let the others do a final review

msau42 · 2019-08-12T19:58:14Z

Makefile

+windows-local:
+	mkdir -p bin
+	GOOS=windows GOARCH=amd64 go build -ldflags "-X main.vendorVersion=${VERSION}" -o bin/gcfs-csi-driver.exe ./cmd/
+
 push:
 	docker push $(IMAGE):$(VERSION)


Do we need to do anything special here for windows container images?

We will need to write a separate Dockerfile to build a Windows container. I haven't written one because the driver can't run from within a container on Windows. Can write one and include it if you'd like.

It may be good to add now so once CSI on windows is ready, we can easily leverage this

Added a Dockerfile for windows under test/experimental but the driver has to be compiled first. I added a target in the Makefile to do this.

pkg/csi_driver/node.go

msau42 · 2019-08-12T20:03:40Z

pkg/csi_driver/node_test.go

@@ -177,11 +188,54 @@ func TestNodePublishVolume(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		// TODO: enable this test when FakeMount supports Windows behavior


Is there something still preventing this test from being enabled? If so, let's open an issue for it and reference it here

k8s-ci-robot · 2019-08-12T20:13:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

pkg/csi_driver/gcfs_driver.go

msau42 · 2019-08-16T01:59:09Z

/retest

pjh · 2019-08-17T01:43:29Z

@davidz627 @msau42 can you provide your final feedback and approval so that we can get this merged? Thanks!

pjh · 2019-08-17T01:44:22Z

@almos98 consider squashing this back into two commits again.

Makefile

With these changes it's possible to make a request to mount a generic SMB share on Windows.

msau42 · 2019-08-19T21:43:36Z

/lgtm

Thanks!

Update vendor files

04704cf

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 23, 2019

k8s-ci-robot requested review from davidz627 and saad-ali July 23, 2019 21:03

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 23, 2019

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 23, 2019

k8s-ci-robot requested a review from msau42 July 23, 2019 21:07

msau42 reviewed Jul 23, 2019

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

pkg/csi_driver/node.go Outdated Show resolved Hide resolved

pkg/csi_driver/node.go Show resolved Hide resolved

pkg/csi_driver/node.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2019

k8s-ci-robot assigned jingxu97 and yujuhong Jul 23, 2019

davidz627 reviewed Jul 23, 2019

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

pkg/csi_driver/node.go Outdated Show resolved Hide resolved

yujuhong reviewed Jul 25, 2019

View reviewed changes

pkg/csi_driver/node.go Show resolved Hide resolved

pkg/csi_driver/node.go Outdated Show resolved Hide resolved

Gopkg.toml Outdated Show resolved Hide resolved

Gopkg.toml Show resolved Hide resolved

msau42 reviewed Jul 25, 2019

View reviewed changes

pkg/csi_driver/node.go Show resolved Hide resolved

pkg/csi_driver/node.go Outdated Show resolved Hide resolved

alexiondev commented Jul 30, 2019

View reviewed changes

msau42 reviewed Aug 5, 2019

View reviewed changes

pjh reviewed Aug 6, 2019

View reviewed changes

pkg/csi_driver/node.go Show resolved Hide resolved

pkg/csi_driver/node.go Outdated Show resolved Hide resolved

pkg/csi_driver/node_test.go Outdated Show resolved Hide resolved

alexiondev changed the title ~~Windows refactor~~ Windows SMB Support Aug 7, 2019

msau42 reviewed Aug 12, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2019

msau42 reviewed Aug 13, 2019

View reviewed changes

pkg/csi_driver/gcfs_driver.go Outdated Show resolved Hide resolved

msau42 reviewed Aug 19, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Support for Windows

14cf883

With these changes it's possible to make a request to mount a generic SMB share on Windows.

k8s-ci-robot assigned msau42 Aug 19, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2019

k8s-ci-robot merged commit f7d9d76 into kubernetes-sigs:master Aug 19, 2019

alexiondev deleted the windows_refactor branch August 19, 2019 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows SMB Support #36

Windows SMB Support #36

alexiondev commented Jul 23, 2019 •

edited

Loading

k8s-ci-robot commented Jul 23, 2019

k8s-ci-robot commented Jul 23, 2019

pjh commented Jul 23, 2019

k8s-ci-robot commented Jul 23, 2019

msau42 commented Jul 23, 2019

msau42 commented Jul 23, 2019

alexiondev left a comment •

edited

Loading

msau42 Aug 5, 2019

alexiondev Aug 6, 2019

msau42 Aug 12, 2019

alexiondev Aug 13, 2019

alexiondev Aug 19, 2019

alexiondev commented Aug 6, 2019

k8s-ci-robot commented Aug 6, 2019

pjh commented Aug 6, 2019

pjh commented Aug 6, 2019

msau42 commented Aug 7, 2019

msau42 commented Aug 7, 2019

msau42 commented Aug 7, 2019

alexiondev commented Aug 8, 2019

msau42 left a comment

msau42 Aug 12, 2019

alexiondev Aug 13, 2019

msau42 Aug 13, 2019

alexiondev Aug 16, 2019

msau42 Aug 12, 2019

k8s-ci-robot commented Aug 12, 2019

msau42 commented Aug 16, 2019

pjh commented Aug 17, 2019

pjh commented Aug 17, 2019

msau42 commented Aug 19, 2019

Windows SMB Support #36

Windows SMB Support #36

Conversation

alexiondev commented Jul 23, 2019 • edited Loading

k8s-ci-robot commented Jul 23, 2019

k8s-ci-robot commented Jul 23, 2019

pjh commented Jul 23, 2019

k8s-ci-robot commented Jul 23, 2019

msau42 commented Jul 23, 2019

msau42 commented Jul 23, 2019

alexiondev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexiondev commented Aug 6, 2019

k8s-ci-robot commented Aug 6, 2019

pjh commented Aug 6, 2019

pjh commented Aug 6, 2019

msau42 commented Aug 7, 2019

msau42 commented Aug 7, 2019

msau42 commented Aug 7, 2019

alexiondev commented Aug 8, 2019

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 12, 2019

msau42 commented Aug 16, 2019

pjh commented Aug 17, 2019

pjh commented Aug 17, 2019

msau42 commented Aug 19, 2019

alexiondev commented Jul 23, 2019 •

edited

Loading

alexiondev left a comment •

edited

Loading