Add e2e CI tests for Windows #529

jingxu97 · 2020-06-17T22:39:07Z

This PR adds e2e CI tests for Windows.

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

NONE

jingxu97 · 2020-06-22T19:09:54Z

/retest

jingxu97 · 2020-06-23T00:35:08Z

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

jingxu97 · 2020-06-23T19:11:59Z

/retest

msau42 · 2020-06-26T00:05:12Z

test/k8s-integration/main.go

-		}
-		defer func() {
-			err = setEnvProject(string(oldProject))
+		if *platform != "windows" {


Do we intend to have a prow CI job to run windows? Then we should use boskos to get a project.

yes. this is prow config file. https://github.com/kubernetes/test-infra/pull/17980/files

/workspace/scenarios/kubernetes_e2e.py already set up the project and kubernetes for windows I think. So instead of getting boskos for a new project, just use the current project. This is how other Windows test works.

Can you add a comment here explaining this?

msau42 · 2020-06-26T00:07:48Z

test/k8s-integration/main.go

@@ -352,6 +355,9 @@ func generateGCETestSkip(clusterVersion string) string {
 	if v.LessThan(apimachineryversion.MustParseSemantic("1.16.0")) {
 		skipString = skipString + "|volumeMode\\sshould\\snot\\smount\\s/\\smap\\sunused\\svolumes\\sin\\sa\\spod"
 	}
+	if platform == "windows" {
+		skipString = skipString + "|\\[Slow\\]|\\[LinuxOnly\\]|\\[Feature:.+\\][Ephemeral]"


What slow, and feature tests don't work on windows? Also ephemeral can probably be removed, there shouldn't be any external storage tests tagged with ephemeral

en, I forgot why I added slow. I will remove it.

msau42 · 2020-06-26T00:08:22Z

test/k8s-integration/main.go

@@ -352,6 +355,9 @@ func generateGCETestSkip(clusterVersion string) string {
 	if v.LessThan(apimachineryversion.MustParseSemantic("1.16.0")) {
 		skipString = skipString + "|volumeMode\\sshould\\snot\\smount\\s/\\smap\\sunused\\svolumes\\sin\\sa\\spod"
 	}
+	if platform == "windows" {


Can you put this before or after the version checks instead of in the middle?

msau42 · 2020-06-26T00:10:16Z

test/run-windows-script.sh

@@ -0,0 +1,48 @@
+#!/bin/bash


What is this script used for?

at first, I was trying to use a simple script to deploy and run tests for windows cluster. But later I release it missed driver build part which is available in the run-integration-test go program. So this script is not really used. I can remove it or adjust it and make it available just for users

If you want to add it in, maybe put it in an "experimental" directory and add a comment at the top explaining what the script is used for.

I will remove it for now, and add it later if needed.

msau42 · 2020-06-26T00:14:24Z

Dockerfile.Windows

+WORKDIR /code
+ADD . /code/
+
+RUN cd /code/ && GOARCH=$(echo $TARGETPLATFORM | cut -f2 -d '/') GCE_PD_CSI_STAGING_VERSION=dev make gce-pd-driver-windows


Weren't there issues with building in a container?

right, but here the RUN command is being run on a Linux image
FROM --platform=$BUILDPLATFORM golang:1.13.3 AS builder. So this RUN can work here

msau42 · 2020-06-26T00:17:18Z

test/k8s-integration/config/test-driver-windows.yaml

@@ -0,0 +1,20 @@
+StorageClass:
+  FromName: false
+  FromFile: sc-windows.yaml


storageclass, snapshotclass, and capabilities should be templated as they are generated by the test runner, like https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/test/k8s-integration/config/test-config-template.in

updated using template.

Not seeing the update?

will remove it.

msau42 · 2020-06-26T00:21:26Z

deploy/kubernetes/base/controller/cluster_setup.yaml

@@ -1,3 +1,10 @@
+kind: Namespace


I'm curious why none of our tests were failing without this?

It was created in the driver deployment script. It is configurable. But looking at the kustomization code. it was hardcoded to gce-pd-csi-driver as namespace. Then it seems like we should not make it configurable?

The installation script also creates a secret in the namespace using the JSON key provided by the user, which doesn't have a retry loop, so if the namespace hasn't been created yet the deployment would be broken.

msau42 · 2020-06-26T00:23:37Z

Dockerfile.Windows

+WORKDIR /code
+ADD . /code/
+
+RUN cd /code/ && GOARCH=$(echo $TARGETPLATFORM | cut -f2 -d '/') GCE_PD_CSI_STAGING_VERSION=dev make gce-pd-driver-windows


Should GCE_PD_CSI_STAGING_VERSION be hardcoded?

msau42 · 2020-06-26T00:24:36Z

/assign @mattcary

k8s-ci-robot · 2020-06-26T00:24:37Z

@msau42: GitHub didn't allow me to assign the following users: mattcary.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mattcary

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-30T17:51:46Z

Updated PR. @msau42 @verult PTAL

jingxu97 · 2020-06-30T19:37:40Z

/retest

msau42 · 2020-07-01T03:13:28Z

test/k8s-integration/config/test-driver-windows.yaml

@@ -0,0 +1,20 @@
+StorageClass:
+  FromName: false
+  FromFile: sc-windows.yaml


Not seeing the update?

msau42 · 2020-07-01T03:14:26Z

test/k8s-integration/driver.go

+			fmt.Sprintf("GCE_PD_CSI_STAGING_VERSION=%s", stagingVersion),
+			fmt.Sprintf("GCE_PD_CSI_STAGING_IMAGE=%s", stagingImage))
+	} else {
+		cmd = exec.Command("make", "-C", pkgDir, "build-and-push-windows-container-1909",


We don't need to test ltsc2019?

I was thinking to test this first and will add for ltsc2019 after finish testing.

msau42 · 2020-07-01T03:15:37Z

test/k8s-integration/main.go

-		}
-		defer func() {
-			err = setEnvProject(string(oldProject))
+		if *platform != "windows" {


Can you add a comment here explaining this?

msau42 · 2020-07-01T03:23:05Z

test/k8s-integration/main.go

 		oldProject, err := exec.Command("gcloud", "config", "get-value", "project").CombinedOutput()
+		*stagingImage = fmt.Sprintf("gcr.io/%s/gcp-persistent-disk-csi-driver", strings.TrimSpace(string(oldProject)))


staging image should only be overridden if doDriverBuild = true. Can we actually pull L173 out of the linux block so we set it in one place for both windows and linux?

msau42 · 2020-07-01T03:25:56Z

test/k8s-integration/main.go

 	if v.LessThan(apimachineryversion.MustParseSemantic("1.17.0")) {
 		skipString = skipString + "|VolumeSnapshotDataSource"
 	}
+	if platform == "windows" {
+		skipString = skipString + "|\\[LinuxOnly\\]|\\[Feature:.+\\][Ephemeral]"


Do we need to skip Feature and Ephemeral? Skipping Feature means snapshot tests won't run on windows. Ephemeral I think is not run as part of the external storage test suite anyway.

remove Feature and Ephemeral. I think the reason windows tests in presubmit remove Feature is to reduce running time. But periodic tests will test it.

msau42 · 2020-07-01T03:30:37Z

test/run-windows-k8s-integration.sh

+            --platform=windows --local-k8s-dir=${LOCAL_K8S_DIR} --bringup-cluster=false --teardown-cluster=false --teardown-driver=true\
+            --run-in-prow=true --deploy-overlay-name=${overlay_name} --service-account-file=${E2E_GOOGLE_APPLICATION_CREDENTIALS} \
+            --do-driver-build=${do_driver_build} --gce-zone=${gce_zone}\
+            --storageclass-file=sc-windows.yaml --snapshotclass-file=pd-volumesnapshotclass.yaml --test-focus="External.Storage.*ntfs" \


Do we need focus on ntfs? Configuring the fstype in the driver config should run the ntfs tests

msau42 · 2020-07-01T03:35:17Z

test/k8s-integration/main.go

@@ -33,6 +34,7 @@ var (
 	teardownCluster   = flag.Bool("teardown-cluster", true, "teardown the cluster after the e2e test")
 	teardownDriver    = flag.Bool("teardown-driver", true, "teardown the driver after the e2e test")
 	bringupCluster    = flag.Bool("bringup-cluster", true, "build kubernetes and bringup a cluster")
+	platform          = flag.String("platform", "linux", "platform that the tests will be run, either linux or windows")


Can you add some ensureFlag logic below that bringup-cluster and teardown-cluster are required to be false for windows?

msau42 · 2020-07-01T03:37:25Z

test/run-windows-script.sh

@@ -0,0 +1,48 @@
+#!/bin/bash


If you want to add it in, maybe put it in an "experimental" directory and add a comment at the top explaining what the script is used for.

msau42 · 2020-07-01T03:38:03Z

test/run-windows-k8s-integration.sh

+            --run-in-prow=true --deploy-overlay-name=${overlay_name} --service-account-file=${E2E_GOOGLE_APPLICATION_CREDENTIALS} \
+            --do-driver-build=${do_driver_build} --gce-zone=${gce_zone}\
+            --storageclass-file=sc-windows.yaml --snapshotclass-file=pd-volumesnapshotclass.yaml --test-focus="External.Storage.*ntfs" \
+            --deployment-strategy=${deployment_strategy} --num-nodes=4 \


Is there a reason why we need 4 nodes?

--num-nodes was a required parameter. Remove this if bringupCluster is false

msau42 · 2020-07-01T03:38:59Z

test/run-windows-k8s-integration.sh

+#readonly gce_region=${GCE_CLUSTER_REGION:-}
+readonly image_type=${IMAGE_TYPE:-cos}
+
+export GCE_PD_VERBOSITY=9


I think we can remove the verbosity. Before we were debugging some test config errors and bumped it up to debug, but forgot to bump it back down.

jingxu97 · 2020-07-01T23:43:04Z

/retest

jingxu97 · 2020-07-01T23:43:18Z

/retest

jingxu97 · 2020-07-02T01:59:00Z

/retest

jingxu97 · 2020-07-02T16:23:17Z

@msau42 comments were addressed. PTAL.

msau42 · 2020-07-06T22:42:46Z

test/k8s-integration/main.go

+	}
+
+	if *platform == "windows" && *inProw {
+		ensureFlag(bringupCluster, false, "bringupCluster is set to false if it is for prow test in windows cluster")


Actually bringupCluster is not supported for any windows testing right? (even outside of prow)

right now yes. Later we could add an option to bring up windows cluster.
Remove the inPros check.

msau42 · 2020-07-06T22:46:52Z

test/run-windows-k8s-integration.sh

+            --platform=windows --local-k8s-dir=${LOCAL_K8S_DIR} --bringup-cluster=false --teardown-cluster=false --teardown-driver=${teardown_driver}\
+            --run-in-prow=true --deploy-overlay-name=${overlay_name} --service-account-file=${E2E_GOOGLE_APPLICATION_CREDENTIALS} \
+            --do-driver-build=${do_driver_build} --gce-zone=${gce_zone}\
+            --storageclass-file=sc-windows.yaml --snapshotclass-file=pd-volumesnapshotclass.yaml --test-focus="External.Storage" \


Missing sc-windows.yaml

msau42 · 2020-07-06T22:51:55Z

test/run-windows-k8s-integration.sh

+readonly GCE_PD_DO_DRIVER_BUILD=false
+readonly PKGDIR=${GOPATH}/src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver
+readonly LOCAL_K8S_DIR=${GOPATH}/src/k8s.io/kubernetes
+readonly overlay_name="${GCE_PD_OVERLAY_NAME:-alpha}"


Right now the alpha overlay is pointing to an image in your own gcr gcr.io/jing-k8s-dev/gce-pd-windows-2019. Is that publicly accessible?

This PR adds e2e CI tests for Windows.

msau42 · 2020-07-07T00:19:17Z

/lgtm
/approve

k8s-ci-robot · 2020-07-07T00:19:28Z

[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

jingxu97 · 2020-07-07T15:12:48Z

/retest

k8s-ci-robot requested review from msau42 and saad-ali June 17, 2020 22:39

jingxu97 force-pushed the June/windowstest branch from 7a847f7 to ce1b374 Compare June 17, 2020 22:41

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2020

jingxu97 force-pushed the June/windowstest branch 3 times, most recently from 06e6d4e to b9689dd Compare June 22, 2020 18:31

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

jingxu97 force-pushed the June/windowstest branch from b9689dd to ec1ccd6 Compare June 23, 2020 02:10

jingxu97 force-pushed the June/windowstest branch from ec1ccd6 to be33e2e Compare June 23, 2020 20:32

msau42 reviewed Jun 26, 2020

View reviewed changes

jingxu97 force-pushed the June/windowstest branch 3 times, most recently from 9401be4 to 569ad3c Compare June 30, 2020 17:45

jingxu97 force-pushed the June/windowstest branch from 569ad3c to 9b332c7 Compare June 30, 2020 23:12

msau42 reviewed Jul 1, 2020

View reviewed changes

jingxu97 force-pushed the June/windowstest branch from 9b332c7 to 1c2dbc5 Compare July 1, 2020 22:58

jingxu97 force-pushed the June/windowstest branch from 1c2dbc5 to 3aa390a Compare July 2, 2020 00:41

jingxu97 force-pushed the June/windowstest branch from 3aa390a to 87b3b36 Compare July 2, 2020 02:19

msau42 reviewed Jul 6, 2020

View reviewed changes

Add e2e CI tests for Windows

33654c9

This PR adds e2e CI tests for Windows.

jingxu97 force-pushed the June/windowstest branch from 87b3b36 to 33654c9 Compare July 7, 2020 00:03

k8s-ci-robot assigned msau42 Jul 7, 2020

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2020

k8s-ci-robot merged commit 1c51cca into kubernetes-sigs:master Jul 7, 2020

		oldProject, err := exec.Command("gcloud", "config", "get-value", "project").CombinedOutput()
		*stagingImage = fmt.Sprintf("gcr.io/%s/gcp-persistent-disk-csi-driver", strings.TrimSpace(string(oldProject)))

Add e2e CI tests for Windows #529

Add e2e CI tests for Windows #529

Conversation

jingxu97 commented Jun 17, 2020 • edited Loading

jingxu97 commented Jun 22, 2020

jingxu97 commented Jun 23, 2020

jingxu97 commented Jun 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 Jun 27, 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

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

msau42 commented Jun 26, 2020

k8s-ci-robot commented Jun 26, 2020

jingxu97 commented Jun 30, 2020

jingxu97 commented Jun 30, 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

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 Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

jingxu97 commented Jul 1, 2020

jingxu97 commented Jul 1, 2020

jingxu97 commented Jul 2, 2020

jingxu97 commented Jul 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

msau42 commented Jul 7, 2020

k8s-ci-robot commented Jul 7, 2020

jingxu97 commented Jul 7, 2020

jingxu97 commented Jun 17, 2020 •

edited

Loading

jingxu97 Jun 27, 2020 •

edited

Loading

jingxu97 Jul 1, 2020 •

edited

Loading