BenTheElder
commented
1 year ago Just a point of clarity for others reading this discussion: ENABLE_STORAGE_GCE_PD_DRIVER is only an option for the e2e tests, not cluster configuration.
For the e2e tests, they shouldn't be consuming many environment variables, we usually implement options in the e2e tests as flags to that binary upstream in Kubernetes, and then kubetest2 does allow passing through additional ginkgo flags. The upstream e2e framework has a lot of flags, and few environment variables.
I'm not sure why we used an environment variable for this e2e test option, but we can special case handling it in the ginkgo e2e tester or else revisit that decision upstream in Kubernetes.
For cluster environment based configuration ...
The "configure e2e clusters with inscrutably layered environment variables shell-expansion-interpolated-into-config" pattern we used previously with kubetest etc. is going to bite us badly when we have to move off cluster-up.sh to some other tool at some point. Possibly as GCE is removed from tree, so I'm not sure we want to go back on that now.
cluster-up.sh is barely maintained (I am one of the very few volunteer approvers), otherwise pretty much only begrudgingly patched on the go when PRing Kubernetes because it is used in presubmit today in-tree with in-tree GCE so you have to fix it before your PR merges. if it gets removed along with the in tree provider (I know people are pushing for it), those scripts are a nightmare and ~totally unstaffed. I would guess we will move to kops or cluster-API and those cluster-up.sh environment variables are going to be very tedious to interpret / track down / understand and port to some other form of config, whereas the kubetest2-gce flags we have today should mostly be a lot more easily ported.
Thank you for working on fixing this!
mattcary
k8s-triage-robot
k8s-ci-robot
kubetest2 does not plumb env vars to its dependent processes as described here: https://github.com/kubernetes-sigs/kubetest2/blob/master/kubetest2-gce/deployer/common.go#L89.
From the comment that seems mostly an aesthetic decision, and a desire to understand the API between kubetest2 and the tests it runs rather than opening the door to promulgating a whole bunch of legacy crufty decisions.
This is currently causing problems due to https://github.com/kubernetes/kubernetes/pull/109541, which gates GCE PD testing on an environment variable due to CSI migration (which implements the storage side of the cloud provider extraction project).
The result has been that GCE PD testing is not possible or is very difficult, see https://github.com/kubernetes/test-infra/pull/26890.
Since enabling env var support in kubetest may be a long discussion, I'll start by adding a flag for GCE PD support. But this issue may be a good place for revisiting env var support in kubetest2, as that might also be needed if cluster creation needs more configuration (with cloud provider tests moving out of k/k, a lot of things that just worked by default may now need additional configuration).