add functionality to deploy cloud-sa secret and driver #705

hantaowang · 2019-07-29T22:24:40Z

This PR adds the ability to test PVs provisioned and managed by the CSI GCP PD Driver. Included is a change to cl2's run-e2e.sh that deploys a secret (if it exists) and a yaml that deploys the driver and all its associates rbacs and SAs (one of which uses this secret).

I will rebase this after #657 is merged so that the ability to deploy the driver yaml (if necessary) is build into the storage test config. It will be turned off by default and an additional override file in volume-types/persistentvolume will turn it on.

k8s-ci-robot · 2019-07-29T22:24:48Z

Hi @hantaowang. Thanks for your PR.

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

hantaowang · 2019-07-29T22:26:22Z

/hold
for rebase

hantaowang · 2019-07-30T08:20:36Z

Ive rebased it, but as it stands this PR 99% will not work. A couple to consider:

what will happen if i hard code default into the driver yaml and the CL2 tries to deploy to a test namespace? The driver and its secrets should be in namespace default.
if an error is raised or the test namespace overrides the hard coded one, then should i add functionality to manually specify a namespace in phase? This breaks the abstraction of phase a little because phasr allows multiple possible namespaces, hence a range.
if not, then how can i deploy the driver? Possibly via a script to be run pre test, but then this script will be an exact copy of the deploy script in the pd driver repo.

wojtek-t · 2019-07-30T08:17:20Z

clusterloader2/run-e2e.sh

@@ -22,5 +22,10 @@ CLUSTERLOADER_ROOT=$(dirname "${BASH_SOURCE}")
 export KUBECONFIG="${KUBECONFIG:-${HOME}/.kube/config}"
 export KUBEMARK_ROOT_KUBECONFIG="${KUBEMARK_ROOT_KUBECONFIG:-${HOME}/.kube/config}"

+# Deploy a secret that contains the e2e Google credentials
+if [[ -z "${E2E_GOOGLE_APPLICATION_CREDENTIALS}" ]]; then


Do we have access to this env-var here? [I hope we do, just asking if you verified it.]

Please add :- to avoid using unset var (both here and below)

The env var is set by prow afaik and is inherited for all prow jobs.

wojtek-t · 2019-07-30T08:18:05Z

clusterloader2/testing/experimental/storage/pod-startup/config.yaml

+    - Identifier: WaitForRunningDriverPods
+      Method: WaitForRunningPods
+      Params:
+        desiredPodCount: {{SumInt .Nodes 1}}


One driver per node plus a cluster wide driver.

worth commenting in the code here

wojtek-t · 2019-07-30T08:18:21Z

...er2/testing/experimental/storage/pod-startup/volume-types/persistentvolume/csi-override.yaml

@@ -0,0 +1,2 @@
+DEPLOY_CSI_DRIVER: true
+CSI_DRIVER_PATH: "volume-types/persistentvolume/gcp-csi-driver-stable.yaml"


nit: please add end-line here

wojtek-t · 2019-07-30T08:21:04Z

...ng/experimental/storage/pod-startup/volume-types/persistentvolume/gcp-csi-driver-stable.yaml

@@ -0,0 +1,343 @@
+apiVersion: v1


@msau42 @davidz627 - can you please review this file - I don't know enough about what exactly is needed and how to configure it to make it work.

Please see my latest comment. Tldr: it wont but raises an interesting problem.

this looks like a copy paste of the stable driver :) Please comment at top of file a xref to the original files.

This is the correct set of yamls to deploy a driver into a cluster (along with the secret defined in that other file)

wojtek-t · 2019-07-30T08:32:54Z

Ive rebased it, but as it stands this PR 99% will not work. A couple to consider:

what will happen if i hard code default into the driver yaml and the CL2 tries to deploy to a test namespace? The driver and its secrets should be in namespace default.
if an error is raised or the test namespace overrides the hard coded one, then should i add functionality to manually specify a namespace in phase? This breaks the abstraction of phase a little because phasr allows multiple possible namespaces, hence a range.
if not, then how can i deploy the driver? Possibly via a script to be run pre test, but then this script will be an exact copy of the deploy script in the pd driver repo.

I missed this initially - those are good question.
I don't have very strong opinion here - I believe we should allow users to overwrite namespaces if they want (so not specifying namespace range would mean that we don't try to use automanaged ones).
OTOH, I would also be fine with running something as part run-e2e.sh before we trigger CL, but then we need some way to wait for waiting for them, so we may need to reimplement some stuff too...

@mm4tt - for thoughts too

hantaowang · 2019-07-30T08:38:55Z

OTOH, I would also be fine with running something as part run-e2e.sh before we trigger CL, but then we need some way to wait for waiting for them, so we may need to reimplement some stuff too...

I should point out in our tests we do this by just waiting 60 seconds after deploying the driver. But i dont know if that will work for larger scale clusters.

msau42 · 2019-07-30T15:03:48Z

Keep in mind that eventually the driver will be bundled in gke. Which can be optin during gcloud container create

msau42 · 2019-07-30T15:04:30Z

Regarding waiting for the driver to be up, we can watch csinode and wait for the driver to be registered

davidz627 · 2019-07-30T18:28:00Z

clusterloader2/run-e2e.sh

@@ -22,5 +22,10 @@ CLUSTERLOADER_ROOT=$(dirname "${BASH_SOURCE}")
 export KUBECONFIG="${KUBECONFIG:-${HOME}/.kube/config}"
 export KUBEMARK_ROOT_KUBECONFIG="${KUBEMARK_ROOT_KUBECONFIG:-${HOME}/.kube/config}"

+# Deploy a secret that contains the e2e Google credentials
+if [[ -z "${E2E_GOOGLE_APPLICATION_CREDENTIALS}" ]]; then


log/echo a warning if this couldn't be found. Looks like if the file doesn't exist we just give up

davidz627 · 2019-07-30T18:28:29Z

clusterloader2/testing/experimental/storage/pod-startup/config.yaml

+    - Identifier: WaitForRunningDriverPods
+      Method: WaitForRunningPods
+      Params:
+        desiredPodCount: {{SumInt .Nodes 1}}


worth commenting in the code here

davidz627 · 2019-07-30T18:30:27Z

...ng/experimental/storage/pod-startup/volume-types/persistentvolume/gcp-csi-driver-stable.yaml

@@ -0,0 +1,343 @@
+apiVersion: v1


this looks like a copy paste of the stable driver :) Please comment at top of file a xref to the original files.

This is the correct set of yamls to deploy a driver into a cluster (along with the secret defined in that other file)

hantaowang · 2019-08-01T08:49:49Z

Regarding waiting for the driver to be up, we can watch csinode and wait for the driver to be registered

If we just deploy with an addition script in run-e2e.sh, we can actually just use kubectl wait I believe. (https://www.jeffgeerling.com/blog/2018/updating-kubernetes-deployment-and-waiting-it-roll-out-shell-script) This also makes the config a lot simplier because CSI and non CSI configs will now look the same where as previously I would need an {{ if $CSI }} condition.

wojtek-t · 2019-08-01T11:30:45Z

@hantaowang - did you verify that this actually works? I think that @mm4tt yesterday mentioned that having multiple objects in one file will not work with CL...

If we just deploy with an addition script in run-e2e.sh, we can actually just use kubectl wait I believe. (https://www.jeffgeerling.com/blog/2018/updating-kubernetes-deployment-and-waiting-it-roll-out-shell-script) This also makes the config a lot simplier because CSI and non CSI configs will now look the same where as previously I would need an {{ if $CSI }} condition.

I'm fine with that too. Technically, installing driver is more cluster-setup than test itself, so I would be fine with running something in run-e2e (if we can wait for it)

hantaowang · 2019-08-01T21:28:57Z

@hantaowang - did you verify that this actually works? I think that @mm4tt yesterday mentioned that having multiple objects in one file will not work with CL...

No I believe with how object bundles work right now, it won't work. This was just to mock what deploying with CL2 would look like. The default namespace issue also presents a problem, so thats another thing that won't work.

I'll try the run-e2e.sh method and make sure that works, and then ill push here and remove the hold.

hantaowang · 2019-08-01T22:51:06Z

I tested this method and it does work. The deployment is built into run-e2e.sh. There is no tear down, but that should be fine - we just need to not run csi and non csi tests on the same cluster.

To use it, add --deploy-csi-driver as the first flag of run-e2e.sh. Then override the USE_CSI config variable.

/assign @wojtek-t
/cc @davidz627 @jingxu97
cc @msau42 @verult
/hold cancel

davidz627 · 2019-08-01T22:54:32Z

/ok-to-test

davidz627 · 2019-08-02T00:33:10Z

clusterloader2/run-e2e.sh

@@ -22,5 +22,19 @@ CLUSTERLOADER_ROOT=$(dirname "${BASH_SOURCE}")
 export KUBECONFIG="${KUBECONFIG:-${HOME}/.kube/config}"
 export KUBEMARK_ROOT_KUBECONFIG="${KUBEMARK_ROOT_KUBECONFIG:-${HOME}/.kube/config}"

+# Deploy the GCP PD CSI Driver is required
+if [ $1 = "--deploy-csi-driver" ]; then


I don't know how back params work but I feel like theres a better way to do this, doesn't this just check the first arg for an exact string match? What if I use the "single-dash" syntax instead.

Worth investigating other methods, I've seen other things before that seem to work.

I not sure what other methods can work. i didnt want to check every argument although that is a possibility. Its a bit harder since that arg cant be passed to CL2, so it needs to be spliced out. Open to suggestions.

davidz627 · 2019-08-02T00:42:45Z

clusterloader2/run-e2e.sh

@@ -22,5 +22,19 @@ CLUSTERLOADER_ROOT=$(dirname "${BASH_SOURCE}")
 export KUBECONFIG="${KUBECONFIG:-${HOME}/.kube/config}"
 export KUBEMARK_ROOT_KUBECONFIG="${KUBEMARK_ROOT_KUBECONFIG:-${HOME}/.kube/config}"

+# Deploy the GCP PD CSI Driver is required
+if [ $1 = "--deploy-csi-driver" ]; then
+   echo "${E2E_GOOGLE_APPLICATION_CREDENTIALS}"


do we need to print the contents?

oh sorry, that was for testing

clusterloader2/run-e2e.sh

davidz627 · 2019-08-02T00:44:12Z

clusterloader2/testing/experimental/storage/pod-startup/config.yaml

@@ -13,6 +13,7 @@
  {{$VOLUMES_PER_POD := .VOLUMES_PER_POD}}
  {{$VOLUME_TEMPLATE_PATH := .VOLUME_TEMPLATE_PATH}}
  {{$PROVISION_VOLUME := DefaultParam .PROVISION_VOLUME false}}
+  {{$USE_CSI := DefaultParam .USE_CSI false}}


should this param be more specific like DEPLOY_PD_CSI_DRIVER or something?

I think in the future, we may want to support more drivers to test, so its vague here in the config on purpose.

USE_CSI sounds fine - (in test we're not deploying anything, just potentially using the functionality)

wojtek-t

overall looks fine - I added a couple comments

wojtek-t · 2019-08-02T06:51:04Z

clusterloader2/gcp-csi-driver-stable.yaml

@@ -0,0 +1,354 @@
+# This config generated from the GCP PD CSI Driver


I'm not a fan of putting this file in the clusterloader2/ main dir.
Can you move it to "clusterloader2/drivers" or "cl2/csi/" or sth like that.

[e.g. cl2/testing/prometheus contains manifests

wojtek-t · 2019-08-02T06:53:20Z

clusterloader2/run-e2e.sh

@@ -22,5 +22,18 @@ CLUSTERLOADER_ROOT=$(dirname "${BASH_SOURCE}")
 export KUBECONFIG="${KUBECONFIG:-${HOME}/.kube/config}"
 export KUBEMARK_ROOT_KUBECONFIG="${KUBEMARK_ROOT_KUBECONFIG:-${HOME}/.kube/config}"

+# Deploy the GCP PD CSI Driver is required
+if [ $1 = "--deploy-csi-driver" ]; then


I'm not a fan of this approach - other than supporting different flag, I prefer to be much more explicit.
Can we instead simply introduce a dedicated env var and use it.

e.g. "DEPLOY_GCI_DRIVER" (and just check if it is true).

is it ok to configure the test with env vars? I defaulted to flags because everything else was configured by flags but this seems like a more flexible solution.

wojtek-t · 2019-08-02T06:53:54Z

clusterloader2/run-e2e.sh

@@ -22,5 +22,18 @@ CLUSTERLOADER_ROOT=$(dirname "${BASH_SOURCE}")
 export KUBECONFIG="${KUBECONFIG:-${HOME}/.kube/config}"
 export KUBEMARK_ROOT_KUBECONFIG="${KUBEMARK_ROOT_KUBECONFIG:-${HOME}/.kube/config}"

+# Deploy the GCP PD CSI Driver is required
+if [ $1 = "--deploy-csi-driver" ]; then
+   if [[ -z "${E2E_GOOGLE_APPLICATION_CREDENTIALS}" ]]; then


:-

otherwise you're risking that this will fail with unbounded variable

wojtek-t · 2019-08-02T06:54:11Z

clusterloader2/run-e2e.sh

+      echo "Env var E2E_GOOGLE_APPLICATION_CREDENTIALS must be set to deploy driver"
+      exit 1
+   else
+      kubectl create secret generic cloud-sa --from-file="${E2E_GOOGLE_APPLICATION_CREDENTIALS}"


clusterloader2/run-e2e.sh

wojtek-t · 2019-08-02T06:55:16Z

clusterloader2/run-e2e.sh

+   fi
+   kubectl apply -f ${CLUSTERLOADER_ROOT}/gcp-csi-driver-stable.yaml
+   kubectl wait pods -l app=gcp-compute-persistent-disk-csi-driver --for condition=Ready --timeout=300s
+   shift


what does it do?

shift shifts the arguments by 1, essentially popping off the first arg. I will remove this for using env vars.

wojtek-t · 2019-08-02T06:56:07Z

clusterloader2/testing/experimental/storage/pod-startup/config.yaml

@@ -13,6 +13,7 @@
  {{$VOLUMES_PER_POD := .VOLUMES_PER_POD}}
  {{$VOLUME_TEMPLATE_PATH := .VOLUME_TEMPLATE_PATH}}
  {{$PROVISION_VOLUME := DefaultParam .PROVISION_VOLUME false}}
+  {{$USE_CSI := DefaultParam .USE_CSI false}}


USE_CSI sounds fine - (in test we're not deploying anything, just potentially using the functionality)

clusterloader2/testing/experimental/storage/pod-startup/volume-types/persistentvolume/pvc.yaml

wojtek-t · 2019-08-02T06:57:09Z

...erloader2/testing/experimental/storage/pod-startup/volume-types/persistentvolume/usecsi.yaml

@@ -0,0 +1 @@
+USE_CSI: true


nit: add end of line

wojtek-t

Just two additional minor comments

wojtek-t · 2019-08-02T08:44:19Z

clusterloader2/run-e2e.sh

@@ -22,5 +22,17 @@ CLUSTERLOADER_ROOT=$(dirname "${BASH_SOURCE}")
 export KUBECONFIG="${KUBECONFIG:-${HOME}/.kube/config}"
 export KUBEMARK_ROOT_KUBECONFIG="${KUBEMARK_ROOT_KUBECONFIG:-${HOME}/.kube/config}"

+# Deploy the GCP PD CSI Driver is required


wojtek-t · 2019-08-02T08:44:33Z

clusterloader2/run-e2e.sh

+   if [[ -z "${E2E_GOOGLE_APPLICATION_CREDENTIALS:-}" ]]; then
+      echo "Env var E2E_GOOGLE_APPLICATION_CREDENTIALS must be set to deploy driver"
+      exit 1
+   else


remove else - there is exit 1 above anyway

wojtek-t · 2019-08-02T08:57:44Z

/lgtm
/approve

k8s-ci-robot · 2019-08-02T08:57:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hantaowang, wojtek-t

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:

~~clusterloader2/OWNERS~~ [wojtek-t]

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 29, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2019

k8s-ci-robot requested review from krzysied and oxddr July 29, 2019 22:25

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

hantaowang force-pushed the deploy-csi-driver-cl2 branch from b422498 to 22457a4 Compare July 30, 2019 08:10

wojtek-t reviewed Jul 30, 2019

View reviewed changes

davidz627 reviewed Jul 30, 2019

View reviewed changes

hantaowang closed this Aug 1, 2019

hantaowang force-pushed the deploy-csi-driver-cl2 branch from 22457a4 to 1fb9a5c Compare August 1, 2019 21:39

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 1, 2019

hantaowang reopened this Aug 1, 2019

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 1, 2019

k8s-ci-robot assigned wojtek-t Aug 1, 2019

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

k8s-ci-robot requested review from davidz627 and jingxu97 August 1, 2019 22:51

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 Aug 1, 2019

davidz627 reviewed Aug 2, 2019

View reviewed changes

hantaowang force-pushed the deploy-csi-driver-cl2 branch from 5032354 to 3a89b69 Compare August 2, 2019 00:49

wojtek-t reviewed Aug 2, 2019

View reviewed changes

hantaowang force-pushed the deploy-csi-driver-cl2 branch 2 times, most recently from 454480e to 50d4d95 Compare August 2, 2019 08:30

wojtek-t reviewed Aug 2, 2019

View reviewed changes

add ability to deploy csi driver to run-e2e.sh script

771484f

hantaowang force-pushed the deploy-csi-driver-cl2 branch from 50d4d95 to 771484f Compare August 2, 2019 08:50

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

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

k8s-ci-robot merged commit 572490b into kubernetes:master Aug 2, 2019

		@@ -0,0 +1,2 @@
		DEPLOY_CSI_DRIVER: true
		CSI_DRIVER_PATH: "volume-types/persistentvolume/gcp-csi-driver-stable.yaml"

		@@ -0,0 +1,354 @@
		# This config generated from the GCP PD CSI Driver

add functionality to deploy cloud-sa secret and driver #705

add functionality to deploy cloud-sa secret and driver #705

Conversation

hantaowang commented Jul 29, 2019 • edited Loading

k8s-ci-robot commented Jul 29, 2019

hantaowang commented Jul 29, 2019

hantaowang commented Jul 30, 2019

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

wojtek-t commented Jul 30, 2019

hantaowang commented Jul 30, 2019

msau42 commented Jul 30, 2019

msau42 commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hantaowang commented Aug 1, 2019

wojtek-t commented Aug 1, 2019

hantaowang commented Aug 1, 2019

hantaowang commented Aug 1, 2019

davidz627 commented Aug 1, 2019

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

wojtek-t 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

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

wojtek-t 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

wojtek-t commented Aug 2, 2019

k8s-ci-robot commented Aug 2, 2019

hantaowang commented Jul 29, 2019 •

edited

Loading