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.

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

Implement Windows NodePublish/Unpublish #823

Merged
merged 3 commits into from
Jun 22, 2021

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Apr 7, 2021

Is this a bug fix or adding new feature? /feature

What is this PR about? / Why do we need it? In-tree EBS driver supports windows, so too must CSI in case an in-tree EBS volume gets migrated to CSI

I will add build/push job to CI in a separate PR. Since windows images are big, don't want to prematurely upload stuff to registry.

NOTE: this requires a not-yet-released version of csi-proxy with changes from

TODO:

  • basic/minimal e2e test with migration off
  • doc/example

What testing is done?
Windows mount/unmount works, data persists.

I will let CI test that my refactoring has not broken Linux.

# build and push image
 docker buildx build --platform windows -f Dockerfile.windows .

Manual test:

# Create EKS cluster with Windows nodes.

# Checkout this PR.

# Install the driver including the windows daemonset
helm upgrade --install aws-ebs-csi-driver --namespace kube-system ./charts/aws-ebs-csi-driver

# Replace the windows daemonset image with the windows image. It's a TODO to make the image multiarch... # public.ecr.aws/b5w6x5z2/aws-ebs-csi-driver:windows
k edit daemonset ebs-csi-node-windows -n kube-system

kubectl apply -f examples/kubernetes/windows/specs/

k exec -it windows-server-iis-7c5fc8f6c5-t5mk9 -- powershell

PS C:\> New-Item -Path data -Name "testfile1.txt" -ItemType "file" -Value "This 
is a text string."


    Directory: C:\data


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
-a----         4/7/2021  12:31 AM             22 testfile1.txt

k delete po windows-server-iis-7c5fc8f6c5-t5mk9

k exec -it windows-server-iis-7c5fc8f6c5-j44qv -- powershell

PS C:\> ls data 


    Directory: C:\data 


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
-a----         4/7/2021  12:31 AM             22 testfile1.txt

Basic/minimal e2e test:

$ ginkgo -nodes=1 -v --focus="External.Storage.*default.fs.*should.store.data" ./tests/e2e-kubernetes/ -- -kubeconfig=$KUBECONFIG -gce-zone=us-west-2a -node-os-distro=windows
...

• [SLOW TEST:144.768 seconds]
External Storage [Driver: ebs.csi.aws.com]
/home/ANT.AMAZON.COM/mattwon/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/vendor/k8s.io/kubernetes/test/e2e/storage/external/external.go:169
  [Testpattern: Dynamic PV (default fs)] volumes
  /home/ANT.AMAZON.COM/mattwon/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/vendor/k8s.io/kubernetes/test/e2e/storage/testsuites/base.go:129
    should store data
    /home/ANT.AMAZON.COM/mattwon/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/vendor/k8s.io/kubernetes/test/e2e/storage/testsuites/volumes.go:151
------------------------------
...
Ran 1 of 481 Specs in 144.773 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 480 Skipped
PASS

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 7, 2021
@coveralls
Copy link

coveralls commented Apr 7, 2021

Coverage Status

Coverage increased (+0.06%) to 79.312% when pulling 1631174 on wongma7:windowsforreal into 95ab71a on kubernetes-sigs:master.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 7, 2021
@wongma7

This comment has been minimized.

@wongma7

This comment has been minimized.

@wongma7

This comment has been minimized.

@wongma7

This comment has been minimized.

@wongma7

This comment has been minimized.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wongma7

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 Apr 8, 2021
@wongma7

This comment has been minimized.

@wongma7

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Apr 14, 2021

turns out to be issue in csi-proxy, fix here: kubernetes-csi/csi-proxy#126

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 16, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2021
@wongma7 wongma7 force-pushed the windowsforreal branch 2 times, most recently from 5d2bcf4 to b27a7b9 Compare April 16, 2021 22:02
@AndyXiangLi AndyXiangLi mentioned this pull request Apr 19, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2021
@wongma7 wongma7 changed the title WIP: Implement Windows NodePublish/Unpublish Implement Windows NodePublish/Unpublish Apr 26, 2021
@k8s-ci-robot 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 Apr 26, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Apr 26, 2021

@AndyXiangLi PTAL

We have to settle for my manual testing because there is no EKS windows AMI with csi-proxy.exe (with the necessary patches) installed meaning it's IMO too difficult to write an automated test. (somethingn eeds to apply the patches, build the binary, install the service into the node ami, all as prerequisite for the test, as described in my example README) However we will need to have CI exercising automatic tests before kubernetes 1.23-ish? to make sure the GA driver + windows + GA migration all works together.

Copy link
Contributor

@AndyXiangLi AndyXiangLi left a comment

Choose a reason for hiding this comment

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

I agree that we should have CI ready to cover windows use case by 1.23.
To set the expectation right, should we update the readme and mention resizing is not supported on windows?

@wongma7
Copy link
Contributor Author

wongma7 commented Apr 27, 2021

I added one more sentence to the windows example README: "Only basic read/write (mount/unmount and attach/detach) functionality has been tested, other features like resize don't work yet."

I don't want to mention windows support in top-level README yet because it's not possible without applying patches to csi-proxy.exe, as mentioned in pre-requisites section of windows example README

@wongma7 wongma7 mentioned this pull request Apr 28, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Apr 28, 2021

/hold

@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 Apr 28, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented May 6, 2021

/hold cancel

@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 May 6, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 1, 2021

kubernetes-csi/csi-proxy#144

/hold

need to retest again after kubernetes-csi/csi-proxy#144 gets fixed

@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 Jun 1, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 3, 2021

/hold cancel

all the required changes to csi-proxy have merged and I tested the PR with them

@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 Jun 3, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 3, 2021

/test pull-aws-ebs-csi-driver-external-test

examples/kubernetes/windows/README.md Show resolved Hide resolved
return mountutils.PathExists(path)
}

//TODO: use common util from vendor kubernetes/mount-util

Choose a reason for hiding this comment

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

What is the difference in terms of function between what you have and the common util?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's copied, but IIRC it existed in kubernetes/kubernetes not kubernetes/mount-util at the time so xiangli and I agreed to copy and leave as TODO , instead of depending on kubernetes/kubernetes which is generally bad idea, see

"
TODO: Upstream is planning on move the common resize code to kubernetes/mount-util, #99223, we should use the resize function from there instead of maintaining our own once the changes are available"
#753

pkg/driver/mount_linux.go Show resolved Hide resolved
@nckturner
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit dded803 into kubernetes-sigs:master Jun 22, 2021
@wongma7 wongma7 mentioned this pull request Jul 6, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants