Add windows driver installation support #510

jingxu97 · 2020-05-19T18:29:38Z

This PR adds windows driver support. It adds a windows base dir to
install base yaml files. It also adds a windows alpha kustomization
file. To install driver for windows, first set env
GCE_PD_DRIVER_VERSION=windows/alpha and run
deploy/kubernetes/deploy-driver-win.sh script.

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?:

k8s-ci-robot · 2020-05-19T18:29:39Z

@jingxu97: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2020-05-19T18:29:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jingxu97
To complete the pull request process, please assign msau42
You can assign the PR to them by writing /assign @msau42 in a comment when ready.

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

deploy/kubernetes/base_windows/controller.yaml

jingxu97 · 2020-05-21T18:23:37Z

updated with the same base. This way is kind of tricky with using $patch: delete.
@msau42 PTAL

deploy/kubernetes/overlays/windows/alpha/gcepd.yaml

msau42 · 2020-05-22T00:51:17Z

deploy/kubernetes/overlays/windows/alpha/noderegistrar.yaml

+          args:
+            - --v=5
+            - --csi-address=unix://C:\\csi\\csi.sock
+            - --kubelet-registration-path=C:\\var\\lib\\kubelet\\plugins\\pd.csi.storage.gke.io\\csi.sock


The linux paths don't need to be removed?

this is for windows path, I think.

deploy/kubernetes/overlays/windows/alpha/gcepd.yaml

msau42 · 2020-05-26T23:12:32Z

deploy/kubernetes/base/node.yaml

@@ -15,21 +15,14 @@ spec:
      # Host network must be used for interaction with Workload Identity in GKE
      # since it replaces GCE Metadata Server with GKE Metadata Server. Remove
      # this requirement when issue is resolved and before any exposure of
-      # metrics ports.
-      hostNetwork: true
+      # metrics ports. But hostNetwork is not working for Windodws, might be an issue


typo: windows

Do we support workload identity on Windows? If so, then not being on host network may be problematic.

fixed. we might use a different way to workaround it. Currently we don't need to set it for windows.

deploy/kubernetes/base/node.yaml

deploy/kubernetes/deploy-driver.sh

deploy/kubernetes/overlays/linux/base/noderegistrar.yaml

deploy/kubernetes/overlays/linux/base/volumes.yaml

deploy/kubernetes/overlays/linux/dev/WARNING.md

deploy/kubernetes/overlays/windows/alpha/kustomization.yaml

deploy/kubernetes/overlays/linux/alpha/WARNING.md

jingxu97 · 2020-05-28T00:47:49Z

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

jingxu97 · 2020-05-28T00:55:09Z

comments are addressed. PTAL

jingxu97 · 2020-05-28T16:33:32Z

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

msau42 · 2020-05-28T18:03:00Z

deploy/kubernetes/base/node.yaml

-            - name: sys
-              mountPath: /sys
-      nodeSelector:
-        kubernetes.io/os: linux
      volumes:
        - name: registration-dir


Should these volumes be removed too since they are linux specific?

no, windows used it too, I think.

Windows seems to be overriding them? https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/510/files#diff-c6cce7581a0b791e1a6268807fdf0a3fR38

it will not override. just merging the two parts. But I think I need to remove lifecycle part similar to linux.

What is the purpose of keeping the volumes here, if this base spec doesn't have the corresponding volumeMounts? It may be easier to read if we keep both volumeMounts and volumes together in one file.

Now I look at the node.yaml again, the most part is about volumes and volumemounts, which are quite different from linux and windows. I think it is more clean just deploy node.yaml in overlay layer, and remove it from base layer.

deploy/kubernetes/overlays/windows/dev/drivertest.yaml

deploy/kubernetes/overlays/windows/dev/securityContext.yaml

deploy/kubernetes/overlays/windows/dev/tmp.yaml

verult · 2020-06-02T21:02:43Z

/lgtm

Consider adding a release note for the new OS env var in deployment script.

And a nit: maybe change "base" overlay name to something like "common". It could be confusing which "base" is referred to in other overlays.

msau42 · 2020-06-02T22:19:26Z

deploy/kubernetes/base/node.yaml

-            - name: sys
-              mountPath: /sys
-      nodeSelector:
-        kubernetes.io/os: linux
      volumes:
        - name: registration-dir


What is the purpose of keeping the volumes here, if this base spec doesn't have the corresponding volumeMounts? It may be easier to read if we keep both volumeMounts and volumes together in one file.

msau42 · 2020-06-02T22:21:27Z

deploy/kubernetes/delete-driver.sh

 source "${PKGDIR}/deploy/common.sh"

 ensure_kustomize

-${KUSTOMIZE_PATH} build ${PKGDIR}/deploy/kubernetes/overlays/${DEPLOY_VERSION} | ${KUBECTL} delete -v="${VERBOSITY}" --ignore-not-found -f -
+${KUSTOMIZE_PATH} build ${PKGDIR}/deploy/kubernetes/overlays/${OS}/${DEPLOY_VERSION} | ${KUBECTL} delete -v="${VERBOSITY}" --ignore-not-found -f -


Since the DaemonSet has an OS node selector, what do you think of deploying both Linux + Windows daemonsets in one overlay? Then this easily supports mixed OS nodes.

What do you mean by deploying in one overlay?

I mean actually have 2 DaemonSets in the same overlay (and not have os-specific overlays), one for linux and one for windows. Then when you run "deploy-driver.sh", you actually deploy both linux + windows daemonsets.

It means we need 2 DaemonSets in the base, and if we end up changing something, we'll have to remember to update both DaemonSets.

But this way, we can support clusters that have mixed windows + linux nodes.

I think I prefer os-specific overlay for clear structure. Not only daemonset is different, also have different other things such as pod security policy.
For cluster with mixed windows and linux, with current script, user can set os twice and deploy it twice. Or I can update script to accept multiple OSes.

I'm not sure if running the script twice will work. The script does things like delete/create the controller secret which should only be done once.

If you are planning to remove the node.yaml from the base anyway, and not have it share anything, then it may be simple to just have a node_linux.yaml and node_windows.yaml

I updated with changes similar to what you mentioned. The deployment env can take both os and version information, and also allow you to set multiple values. I tested the script and it can install drivers for a mixed cluster.
Some common objects might get applied/deleted twice in this case, but it is ok. Apply second time just give a message saying ... unchanged. For deletion, we have --ignore-not-found setting, so it won't give error messages.

k8s-ci-robot · 2020-06-02T22:48:14Z

New changes are detected. LGTM label has been removed.

jingxu97 · 2020-06-03T15:09:12Z

deploy/kubernetes/base/setup-cluster.yaml

+subjects:
+- kind: ServiceAccount
+  name: csi-gce-pd-controller-sa
+


I added this binding, because without it, when pod security policy admission controller is enabled, service account csi-gce-pd-controller-sa cannot create pod at all.

jingxu97 · 2020-06-03T17:03:49Z

I updated the PR. PTAL. Thanks!

msau42 · 2020-06-03T21:07:54Z

cc @mattcary

This PR adds windows driver support. It adds a windows base dir to install base yaml files. It also adds a windows alpha kustomization file. To install driver for windows, first set env NODE_OS=windows and GCE_PD_DRIVER_VERSION=alpha and run deploy/kubernetes/deploy-driver.sh script. This PR also reorgnize the dir structure for linux version. Now under overlay, we have a linux and a windows dir. Under each of them, we have alpha, stable, etc. Currently windows only has alpha version.

msau42 · 2020-06-04T00:45:08Z

deploy/kubernetes/base/controller.yaml

+      # See "special case". This will tolerate everything. Node component should
+      # be scheduled on all nodes.
+      tolerations:
+      - operator: Exists


This means that the controller may not get rescheduled if the Node becomes unhealthy. Is that what we want?

good point, will remove it. I added it because I see node has it. And my test setup has linux tainted.

msau42 · 2020-06-04T00:49:05Z

deploy/kubernetes/overlays/linux/base/enableHostNetwork.yaml

@@ -0,0 +1,8 @@
+kind: DaemonSet


this file can be removed now right?

right, fix it

msau42 · 2020-06-04T00:49:19Z

deploy/kubernetes/overlays/linux/base/noderegistrar.yaml

@@ -0,0 +1,13 @@
+kind: DaemonSet


this file can be removed now right?

msau42 · 2020-06-04T00:52:00Z

deploy/kubernetes/overlays/windows/base/noderegistrar.yaml

@@ -0,0 +1,17 @@
+kind: DaemonSet


this file can be removed now right?

msau42 · 2020-06-04T00:57:53Z

deploy/kubernetes/overlays/windows/base/kustomization.yaml

@@ -0,0 +1,36 @@
+apiVersion: kustomize.config.k8s.io/v1beta1


Are you going to need multiple overlays for windows too (ie staging, stable, dev)?

The challenge I see with this structure is that we now have 2 places that we need to keep in sync when we update overlays. For example, if we make a update to staging, then we need to update both windows and linux staging overlays.

So with one overlay, we have both windows and linux together. What if user need to deploy with mixed OS and versions, e.g, linux stable, and windows dev etc? Or just because some versions are only available in linux, not windows?

I think we'll still need to have different overlays for dev/alpha/staging/stable and we can control the versions used in each. For example, the dev overlay could use stable versions for linux, and unstable versions for windows. And in terms of development flow, we can add Windows first to the dev overlay, then once we're ready, add it to staging, and then when we're ready to release, add it to stable.

Maybe I miss something here, so you think user will not need a mixed version for windows and linux. If they want to deploy stable version, it will be stable version for both linux and windows? But what if windows don't have stable version yet, user must deploy dev version for both linux and windows, instead of one stable version and one dev version?

Also in the case user already deploy a Linux driver, and want to add a windows driver (might be a different version from Linux), with one overlay, it will try to deploy both linux and windows. Some setup might be messed up with different versions of deployment?

In the current layout, we have common things for both linux and windows in the base, and different things in overlay. So new changes, either common to linux and windows, which will go to base, or different from linux and windows, and we need to update them wherever applicable. So I don't see there is more work needed compared to one overlay which contains both linux and windows. Also there might be cases that some setup is only applicable for linux, and we might accidentally break windows setup if we don't pay attention to the windows part?

I have been thinking the structure again last night :), and realized this is more like a multi-stage process. So instead of base/overlay structure, I propose to use the following as multiple stages and use them as bases. I am working on this structure now and will update. Please let me know what you think about this structure.

cluster_controller_setup : includes cluster setup and controller yaml file

node_setup:
* Linux: node and other related yaml files for Linux
* Windows: node and other related yaml files for Windows

image_update
* alpha:
*. stable:
*. staging:
* dev:
...

I'm not quite sure how this would work out but am curious to see. One thing to note is that alpha/staging/stable is not necessarily just different versions. Different features may be added depending on their maturity.

what if windows don't have stable version yet, user must deploy dev version for both linux and windows, instead of one stable version and one dev version?

In terms of looking at the driver itself, I think we should consider both Linux + Windows parts of the driver to be part of one complete driver instead of 2 independent drivers. So considering that Windows is still alpha right now, I think it is fine if it uses the Linux alpha versions as well (which currently don't have any difference from stable). I don't think a user will want to have a mixed Linux + Windows cluster where the Linux part is stable and the Windows part is alpha. I would expect a user to have 2 separate clusters: a Linux-only cluster used for production, and an experimental Windows-only cluster.

Also in the case user already deploy a Linux driver, and want to add a windows driver (might be a different version from Linux), with one overlay, it will try to deploy both linux and windows. Some setup might be messed up with different versions of deployment?

I agree with one deployment and one place to define all the versions, it's easier to make sure the versions are in-sync with each other.

msau42 · 2020-06-04T01:02:05Z

deploy/kubernetes/overlays/windows/base/allowedHostPaths.yaml

+  name: csi-gce-pd-node-psp
+spec:
+  allowedHostPaths:
+  - pathPrefix: \var\lib\kubelet


I think this is going to replace the current PSP with these rules, instead of add, which is not desirable if we're going to be running both linux + windows.

good point, will try to make a merge here.

actually, I think it is better to have separate policy for linux and windows instead of merging

k8s-ci-robot · 2020-06-05T19:00:55Z

@jingxu97: PR needs rebase.

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.

jingxu97 · 2020-06-11T06:03:51Z

opened #520 to replace this one.

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 19, 2020

k8s-ci-robot requested review from saad-ali and verult May 19, 2020 18:29

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 19, 2020

msau42 reviewed May 19, 2020

View reviewed changes

deploy/kubernetes/base_windows/controller.yaml Outdated Show resolved Hide resolved

jingxu97 force-pushed the May/windows branch from 37b3b7b to 2c3a3b1 Compare May 21, 2020 18:15

msau42 reviewed May 22, 2020

View reviewed changes

jingxu97 force-pushed the May/windows branch from 2c3a3b1 to 0457f83 Compare May 26, 2020 18:25

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 26, 2020

jingxu97 force-pushed the May/windows branch from 0457f83 to 6fa2f52 Compare May 26, 2020 18:31

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 26, 2020

msau42 reviewed May 26, 2020

View reviewed changes

jingxu97 force-pushed the May/windows branch 4 times, most recently from 407013c to 2f223d3 Compare May 27, 2020 19:45

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2020

msau42 reviewed May 28, 2020

View reviewed changes

jingxu97 force-pushed the May/windows branch from 2f223d3 to 998782b Compare May 28, 2020 20:29

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 28, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 28, 2020

k8s-ci-robot assigned verult Jun 2, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2020

msau42 reviewed Jun 2, 2020

View reviewed changes

jingxu97 force-pushed the May/windows branch from 998782b to e738a11 Compare June 2, 2020 22:48

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 2, 2020

jingxu97 force-pushed the May/windows branch 2 times, most recently from 5099fda to a9123ee Compare June 3, 2020 02:27

jingxu97 commented Jun 3, 2020

View reviewed changes

jingxu97 force-pushed the May/windows branch from a9123ee to 226b530 Compare June 3, 2020 16:53

jingxu97 force-pushed the May/windows branch from 226b530 to 7b64a5d Compare June 3, 2020 17:24

jingxu97 force-pushed the May/windows branch 2 times, most recently from 9bb739c to 70e2c92 Compare June 4, 2020 00:29

jingxu97 force-pushed the May/windows branch from 70e2c92 to 6314bf5 Compare June 4, 2020 00:41

msau42 reviewed Jun 4, 2020

View reviewed changes

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

jingxu97 closed this Jun 11, 2020

Add windows driver installation support #510

Add windows driver installation support #510

Conversation

jingxu97 commented May 19, 2020

k8s-ci-robot commented May 19, 2020

k8s-ci-robot commented May 19, 2020

jingxu97 commented May 21, 2020

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

jingxu97 commented May 28, 2020

jingxu97 commented May 28, 2020

jingxu97 commented May 28, 2020

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

verult commented Jun 2, 2020

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

Choose a reason for hiding this comment

jingxu97 Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 2, 2020

Choose a reason for hiding this comment

jingxu97 commented Jun 3, 2020

msau42 commented Jun 3, 2020

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

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

jingxu97 Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

jingxu97 Jun 4, 2020 • 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

k8s-ci-robot commented Jun 5, 2020

jingxu97 commented Jun 11, 2020

jingxu97 Jun 4, 2020 •

edited

Loading

jingxu97 Jun 4, 2020 •

edited

Loading

jingxu97 Jun 4, 2020 •

edited

Loading