Add test support for regional PD #621

mattcary · 2020-10-03T01:37:15Z

/kind cleanup

What this PR does / why we need it:
Add regional PD to tests.

Fixes #408

NONE

k8s-ci-robot · 2020-10-03T01:37:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary

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~~ [mattcary]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mattcary · 2020-10-03T01:37:37Z

/assign @verult

verult · 2020-10-06T18:17:24Z

test/k8s-integration/cluster.go

@@ -30,6 +30,10 @@ func gkeLocationArgs(gceZone, gceRegion string) (locationArg, locationVal string
 	return
 }

+func isRegionalGKECluster(gzeZone, gceRegion string) bool {


typo: "gzeZone"

verult · 2020-10-06T18:21:22Z

test/k8s-integration/main.go

@@ -270,6 +272,14 @@ func handle() error {
 		testDir = k8sDir
 	}

+	if *deploymentStrat == "gke" {
+		gkeRegional := isRegionalGKECluster(*gceZone, *gceRegion)
+		if *isRegionalCluster && !gkeRegional {


What if we enforce that only one of gceZone and gceRegion could be set? Then we know the cluster is regional if len(*gceRegion) > 0 and we don't need the --is-regional-cluster flag

We still need --is-regional-cluster for when you're using a pre-existing cluster---you don't set the region or zone in that case and just use your current kubeconfig.

Note that we already check that only one of these is set in main.go:112

Gotcha sounds good

nit: maybe make this field targeted to pre-existing clusters only, i.e. only make the flag settable if gceZone and gceRegion are unset, and update the flag description. Otherwise it could get confusing for someone new to understand when is-regional-cluster should be set just from looking at the flag description

Good idea will do.

(added in the bringupCluster check)

verult · 2020-10-06T18:26:01Z

test/k8s-integration/main.go

+				continue
+			}
+			if scFile == regionalPDStorageClass && !*isRegionalCluster {
+				continue // Regional PDs cannot be used in a zonal cluster.


Is there a way to log a warning?

yeah, especially considering the following comment about making the storage classes more precise when a regional cluster isn't being run.

verult · 2020-10-06T18:28:31Z

test/run-k8s-integration-ci.sh

-            --storageclass-files=sc-standard.yaml,sc-balanced.yaml,sc-ssd.yaml --snapshotclass-file=pd-volumesnapshotclass.yaml \
+            --do-driver-build=${do_driver_build} --teardown-driver=${teardown_driver} \
+            --boskos-resource-type=${boskos_resource_type} \
+            --storageclass-files=sc-standard.yaml,sc-balanced.yaml,sc-ssd.yaml,sc-regional.yaml \


nit: Set this flag in the [ -z "$gce_region" ] branch so it's more explicit that sc-regional.yaml is only expected to work in regional clusters

great point, thanks.

verult

lgtm, I could add the tag after the new commits come in

verult · 2020-10-07T00:29:50Z

test/k8s-integration/main.go

@@ -270,6 +272,14 @@ func handle() error {
 		testDir = k8sDir
 	}

+	if *deploymentStrat == "gke" {
+		gkeRegional := isRegionalGKECluster(*gceZone, *gceRegion)
+		if *isRegionalCluster && !gkeRegional {


Gotcha sounds good

nit: maybe make this field targeted to pre-existing clusters only, i.e. only make the flag settable if gceZone and gceRegion are unset, and update the flag description. Otherwise it could get confusing for someone new to understand when is-regional-cluster should be set just from looking at the flag description

mattcary · 2020-10-07T17:27:46Z

In the push above I changed the CI script to only use the regional SC on a regional cluster. I don't know if there's a good way to test that change?

verult · 2020-10-08T06:03:47Z

Yeah letting it run on CI might be the easiest, altho the other changes besides the script could be tested by updating one of the local scripts

mattcary · 2020-10-08T15:47:20Z

I ran what I could through local testing (--is-regional, etc).

verult · 2020-10-12T18:14:42Z

/lgtm

mattcary · 2020-10-12T19:42:55Z

secrets flake.

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

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 3, 2020

k8s-ci-robot requested review from jingxu97 and saikat-royc October 3, 2020 01:37

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2020

k8s-ci-robot assigned verult Oct 3, 2020

mattcary force-pushed the regional branch from 263657c to d7d8e96 Compare October 3, 2020 01:39

verult reviewed Oct 6, 2020

View reviewed changes

verult reviewed Oct 7, 2020

View reviewed changes

mattcary force-pushed the regional branch from d7d8e96 to 6bc917a Compare October 7, 2020 00:54

mattcary mentioned this pull request Oct 9, 2020

REQUEST: New membership for mattcary kubernetes/org#2251

Closed

6 tasks

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

mattcary force-pushed the regional branch from 6bc917a to da37e47 Compare October 12, 2020 16:59

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

mattcary force-pushed the regional branch from da37e47 to 3125f70 Compare October 12, 2020 18:07

Add test support for regional PD

9d1ce53

mattcary force-pushed the regional branch from 3125f70 to 9d1ce53 Compare October 12, 2020 18:09

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

k8s-ci-robot merged commit 12ea362 into kubernetes-sigs:master Oct 12, 2020

Add test support for regional PD #621

Add test support for regional PD #621

Uh oh!

Conversation

mattcary commented Oct 3, 2020

Uh oh!

k8s-ci-robot commented Oct 3, 2020

Uh oh!

mattcary commented Oct 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcary Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

verult left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcary commented Oct 7, 2020

Uh oh!

verult commented Oct 8, 2020

Uh oh!

mattcary commented Oct 8, 2020

Uh oh!

verult commented Oct 12, 2020

Uh oh!

mattcary commented Oct 12, 2020

Uh oh!

Uh oh!

mattcary Oct 6, 2020 •

edited

Loading