Enable gce pd driver via a flag rather than an env var #111481

mattcary · 2022-07-28T00:12:05Z

See kubernetes-sigs/kubetest2#202 for context.
/kind cleanup

/sig-storage

Add e2e test flag to specify which volume drivers should be installed. This deprecates the ENABLE_STORAGE_GCE_PD_DRIVER environment variable.

k8s-ci-robot · 2022-07-28T00:12:13Z

@mattcary: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

mattcary · 2022-07-28T00:13:57Z

/cc @msau42
/cc @dims

msau42

Can you also add a release note with the flag details and also saying that it removes the env var that was added in the other PR?

msau42 · 2022-07-28T00:21:53Z

test/e2e/framework/test_context.go

@@ -319,6 +322,8 @@ func RegisterCommonFlags(flags *flag.FlagSet) {

 	flags.StringVar(&TestContext.SnapshotControllerPodName, "snapshot-controller-pod-name", "", "The pod name to use for identifying the snapshot controller in the kube-system namespace.")
 	flags.IntVar(&TestContext.SnapshotControllerHTTPPort, "snapshot-controller-http-port", 0, "The port to use for snapshot controller HTTP communication.")
+
+	flags.BoolVar(&TestContext.EnableGcePdDriver, "enable-gcepd-driver", false, "Enable the gcepd volume driver. This should be used only on GCE clusters where the PD CSI driver is installed. See https://github.com/kubernetes/cloud-provider-gcp/tree/master/cluster/addons/pdcsi-driver for an example.")


can you make this a generic arg that takes a comma separated string of driver names? I'm thinking we may want to move all the platform-specific drivers to be off by default in the future.

msau42 · 2022-07-28T00:26:57Z

test/e2e/storage/in_tree_volumes.go

@@ -53,7 +53,7 @@ var testDrivers = []func() storageframework.TestDriver{

 // This executes testSuites for in-tree volumes.
 var _ = utils.SIGDescribe("In-tree Volumes", func() {
-	if enableGcePD := os.Getenv("ENABLE_STORAGE_GCE_PD_DRIVER"); enableGcePD == "yes" {
+	if TestContext.EnableGcePdDriver {


Maybe for now, support both env var and flag to give people time to fix their CI config.

Sure (although I'd be willing to bet that the only configs changed were the kubetest2 ones I did where this wasn't actually effective...)

I noticed openshift updated CI to use the envs.

msau42 · 2022-07-28T00:27:26Z

fyi @jsafrane @bertinatto @gnufied

mattcary · 2022-07-28T01:03:55Z

I implemented your changes. I'll work on testing this manually to make sure the plumbing actually works with kubetest2 (which is the ultimate goal here).

/hold

msau42 · 2022-07-28T01:18:37Z

test/e2e/framework/test_context.go

@@ -319,6 +343,8 @@ func RegisterCommonFlags(flags *flag.FlagSet) {

 	flags.StringVar(&TestContext.SnapshotControllerPodName, "snapshot-controller-pod-name", "", "The pod name to use for identifying the snapshot controller in the kube-system namespace.")
 	flags.IntVar(&TestContext.SnapshotControllerHTTPPort, "snapshot-controller-http-port", 0, "The port to use for snapshot controller HTTP communication.")
+
+	flags.Var(&stringArrayValue{&TestContext.EnabledVolumeDrivers}, "enabled-volume-drivers", "Comma-separated list of volume drivers which are disabled by default. An exmaple is gcepd; see test/e2e/storage/in_tree_volumes.go for full details.")


wording suggestion: "Comma-separated list of intree volume drivers to run tests against".

msau42 · 2022-07-28T01:20:48Z

test/e2e/storage/in_tree_volumes.go

@@ -53,7 +53,7 @@ var testDrivers = []func() storageframework.TestDriver{

 // This executes testSuites for in-tree volumes.
 var _ = utils.SIGDescribe("In-tree Volumes", func() {
-	if enableGcePD := os.Getenv("ENABLE_STORAGE_GCE_PD_DRIVER"); enableGcePD == "yes" {
+	if TestContext.EnableGcePdDriver {


I noticed openshift updated CI to use the envs.

msau42 · 2022-07-28T01:21:49Z

nit on release note wording. The env var is deprecated (instead of removed)

msau42 · 2022-07-28T01:22:02Z

/lgtm

/approve

k8s-ci-robot · 2022-07-28T01:22:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary, 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:

~~test/e2e/framework/OWNERS~~ [msau42]
~~test/e2e/storage/OWNERS~~ [msau42]

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

mattcary · 2022-07-28T16:14:38Z

/kind cleanup

mattcary · 2022-07-28T16:14:52Z

nit on release note wording. The env var is deprecated (instead of removed)

good catch, thx

msau42 · 2022-07-28T23:09:56Z

/lgtm

Change-Id: Ib7c99083ad334e5f6dd152e376a303de794e0bf1

mattcary · 2022-07-29T00:25:15Z

/unhold

Tested as best I could, seems to work. Although I'm not sure where the framework.Log messages go to,

mattcary · 2022-07-29T00:58:07Z

/retest

msau42 · 2022-07-29T16:17:52Z

/lgtm

k8s-ci-robot requested review from dims and msau42 July 28, 2022 00:13

mattcary mentioned this pull request Jul 28, 2022

Support GCE PD testing kubernetes-sigs/kubetest2#202

Closed

k8s-ci-robot added the area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework label Jul 28, 2022

k8s-ci-robot requested review from Jiawei0227 and oomichi July 28, 2022 00:16

k8s-ci-robot added area/test sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 28, 2022

mattcary changed the title ~~nable gce pd driver via a flag rather than an env var~~ Enable gce pd driver via a flag rather than an env var Jul 28, 2022

msau42 reviewed Jul 28, 2022

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 28, 2022

mattcary force-pushed the gcepd branch from dfd626b to 519191f Compare July 28, 2022 01:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 28, 2022

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 28, 2022

msau42 reviewed Jul 28, 2022

View reviewed changes

k8s-ci-robot assigned msau42 Jul 28, 2022

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

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

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 28, 2022

mattcary force-pushed the gcepd branch from 519191f to 81cd14b Compare July 28, 2022 16:17

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2022

mattcary force-pushed the gcepd branch from 81cd14b to 25d8749 Compare July 28, 2022 16:18

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

mattcary mentioned this pull request Jul 28, 2022

kubetest2-ginko tester overwrites local e2e.test kubernetes-sigs/kubetest2#203

Closed

enable gce pd driver via a flag rather than an env var

738b458

Change-Id: Ib7c99083ad334e5f6dd152e376a303de794e0bf1

mattcary force-pushed the gcepd branch from 25d8749 to 738b458 Compare July 29, 2022 00:24

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022

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

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

k8s-ci-robot merged commit 9e6d456 into kubernetes:master Jul 29, 2022

k8s-ci-robot added this to the v1.25 milestone Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable gce pd driver via a flag rather than an env var #111481

Enable gce pd driver via a flag rather than an env var #111481

mattcary commented Jul 28, 2022 •

edited

k8s-ci-robot commented Jul 28, 2022

mattcary commented Jul 28, 2022

msau42 left a comment

msau42 Jul 28, 2022

mattcary Jul 28, 2022

msau42 Jul 28, 2022

mattcary Jul 28, 2022

msau42 Jul 28, 2022

msau42 commented Jul 28, 2022

mattcary commented Jul 28, 2022

msau42 Jul 28, 2022

mattcary Jul 28, 2022

msau42 Jul 28, 2022

msau42 commented Jul 28, 2022

msau42 commented Jul 28, 2022

k8s-ci-robot commented Jul 28, 2022

mattcary commented Jul 28, 2022

mattcary commented Jul 28, 2022

msau42 commented Jul 28, 2022

mattcary commented Jul 29, 2022

mattcary commented Jul 29, 2022

msau42 commented Jul 29, 2022

Enable gce pd driver via a flag rather than an env var #111481

Enable gce pd driver via a flag rather than an env var #111481

Conversation

mattcary commented Jul 28, 2022 • edited

k8s-ci-robot commented Jul 28, 2022

mattcary commented Jul 28, 2022

msau42 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

msau42 commented Jul 28, 2022

mattcary commented Jul 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jul 28, 2022

msau42 commented Jul 28, 2022

k8s-ci-robot commented Jul 28, 2022

mattcary commented Jul 28, 2022

mattcary commented Jul 28, 2022

msau42 commented Jul 28, 2022

mattcary commented Jul 29, 2022

mattcary commented Jul 29, 2022

msau42 commented Jul 29, 2022

mattcary commented Jul 28, 2022 •

edited