e2e: reject unknown providers #73402

pohly · 2019-01-28T13:41:01Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

This finishes the work started for 1.13: instead of merely warning
about an unknown value given to --profile, the test/e2e/e2e.test
binary will now print an error and refuse to run.

That was the intended behavior, we just couldn't do it earlier because it broke some users of the binary. At least kubeadm testing is now fixed (kubernetes/test-infra#10913).

Which issue(s) this PR fixes:

Fixes #70200

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

e2e.test now rejects unknown --provider values instead of merely warning about them. An empty provider name is not accepted anymore and was replaced by "skeleton" (= a provider with no special behavior).

/sig testing
/cc @neolit123 @timothysc

neolit123

thanks @pohly

/area test
/priority important-longterm
/lgtm

@kubernetes/sig-testing-pr-reviews
sent a ping in the #sig-testing chat as well.

neolit123 · 2019-01-28T14:21:20Z

pull-kubernetes-node-e2e this one is failing on all PRs ATM.

BenTheElder · 2019-01-28T15:31:54Z

Needs gofmting for verify.

pohly · 2019-01-28T15:37:16Z

/test ci-kubernetes-e2e-kubeadm-gce-1-13

Let's see whether the kubeadm job still works. I hope this runs it...

neolit123 · 2019-01-28T15:46:56Z

/test ci-kubernetes-e2e-kubeadm-gce-1-13
Let's see whether the kubeadm job still works. I hope this runs it...

that job moved to skeleton
one that i forgot to modify is:
https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-testing/bazel-build-test.yaml#L63

sending a PR in a bit.

neolit123 · 2019-01-28T15:48:20Z

this needs a minor fix:
https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/73402/pull-kubernetes-verify/120272

pohly · 2019-01-28T15:55:23Z

/test pull-kubernetes-node-e2e

neolit123 · 2019-01-28T16:07:16Z

pull-kubernetes-node-e2e

this one might fail for a while due to a GCE zone outage.

timothysc · 2019-01-28T16:49:02Z

test/e2e/framework/test_context.go

@@ -264,7 +266,7 @@ func RegisterClusterFlags() {
 	flag.StringVar(&TestContext.KubeVolumeDir, "volume-dir", "/var/lib/kubelet", "Path to the directory containing the kubelet volumes.")
 	flag.StringVar(&TestContext.CertDir, "cert-dir", "", "Path to the directory containing the certs. Default is empty, which doesn't use certs.")
 	flag.StringVar(&TestContext.RepoRoot, "repo-root", "../../", "Root directory of kubernetes repository, for finding test files.")
-	flag.StringVar(&TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, etc.)")
+	flag.StringVar(&TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, skeleton, etc.)")


Why not default to skeleton here?

Because "" traditionally had the meaning "fallback to 'local' with a specific warning". Selecting "skeleton" would change that.

I don't have any strong opinion either way, but as I don't know the background behind that behavior I don't want to be the one to change it.

so both local and skeleton are null providers here:
https://github.com/kubernetes/kubernetes/blob/b4384e9f828b97c380b93483bea279d2a71d1d00/test/e2e/framework/provider.go#L60-L66

if the previous fallback from "" as provider was local then today setting to skeleton as default should not have a regressive effect (famous last words).

local and skeleton in test-infra/kubetest on the other hand are different.

The question is whether anyone cares about having this log message when --provider is not set:
https://github.com/kubernetes/kubernetes/blob/b4384e9f828b97c380b93483bea279d2a71d1d00/test/e2e/framework/provider.go#L69

If we hard fail we should have a sane default imo.

i'm convinced that log-dump.sh is evil at this point.
needs + 💣

@pohly so "" already does not fallback to "local" if i'm not wrong, as per:
https://github.com/kubernetes/kubernetes/blob/b4384e9f828b97c380b93483bea279d2a71d1d00/test/e2e/framework/provider.go#L68-L71

it effectively falls back to a unnamed null-provider in 1.13 and master, so from what i'm seeing we already modified the old behavior.

@timothysc

If we hard fail we should have a sane default imo.

from my understanding we are not failing but instead we are only showing a warning if --provider is not set (equals "").

@neolit123 right, it currently basically falls back to "skeleton plus warning". Forget what I said about falling back to "local". I think that the current behavior is consistent with <1.13 but haven't checked.

I'm in favor of removing this odd log message and just making skeleton the explicit default. Any objections?

Done. See latest commit in this PR.

timothysc · 2019-01-28T16:49:49Z

test/e2e/framework/test_context.go

-		Failf("Failed to setup fallback skeleton provider config: %v", err)
+		if os.IsNotExist(errors.Cause(err)) {
+			// Provide a more helpful error message when the provider is unknown.
+			var providers []string


Please add a //TODO: when providers are extracted this code should be removed.

I've added a TODO and link to #70194

I've kept the wording a bit more open-ended because I can imagine scenarios where kubernetes/kubernetes itself has no provider-specific code, but still supports building custom testsuites where such code is included. That would allow different cloud providers to share common tests in a neutral location when those tests depend on cloud-provider specific code. Whether that is something that Kubernetes as an organization wants to support I don't know.

timothysc · 2019-01-28T16:51:02Z

test/e2e/framework/test_context.go

+		} else {
+			klog.Errorf("Failed to setup provider config for %q: %v", TestContext.Provider, err)
+		}
+		os.Exit(1)


I'm not really a fan, but if your long term goal is to extract provider code then I'll be ok with this if there are tracking/umbrealla issues that outline path forwards.

I personally don't have plans in that direction, but it is getting tracked here: #70194

This finishes the work started for 1.13: instead of merely warning about an unknown value given to --profile, the test/e2e/e2e.test binary will now print an error and refuse to run. Fixes: kubernetes#70200

The empty string was the default and then triggered a special warning. There's no good reason for that behavior, so now the special handling for "unset provider" is gone and "skeleton" is the non-empty default for the value.

pohly · 2019-01-29T20:22:34Z

/retest

neolit123 · 2019-01-29T20:23:35Z

/lgtm
thanks for the update @pohly

BenTheElder · 2019-01-30T05:52:01Z

/retest

pohly · 2019-01-30T10:34:44Z

/retest

pohly · 2019-01-31T07:17:56Z

/test pull-kubernetes-node-e2e

neolit123 · 2019-01-31T14:14:41Z

/skip

neolit123 · 2019-01-31T14:17:25Z

@pohly
it seems the node-e2e test is failing with:

I0131 07:24:17.735] [8] E0131 07:24:13.285727    1314 test_context.go:420] Unknown provider "". The following providers are known: local skeleton
I0131 07:24:17.735] 
I0131 07:24:17.735] Ginkgo ran 1 suite in 3.021294514s
I0131 07:24:17.735] Test Suite Failed

hm, i though "" should be defaulting to skeleton at this point.

pohly · 2019-01-31T14:39:05Z

@neolit123 thanks for investigating the error, I wasn't sure whether it was just flakiness or caused by the PR.

The --provider parameter now defaults to skeleton when not set. But if it is passed, a valid provider must be specified, so --provider= is currently not okay.

Should --provider= be allowed? Do we then want to proceed with an empty TestContext.Provider string or silently continue as if --provider=skeleton had been used?

neolit123 · 2019-01-31T14:40:53Z

Should --provider= be allowed? Do we then want to proceed with an empty TestContext.Provider string or silently continue as if --provider=skeleton had been used?

my initial reaction is that we should print a message that we are defaulting "" -> skeleton for both these cases:
A) --provider=
B) missing --provider flag

Not accepting --provider= (i.e. setting an empty provider name) broke some test jobs. As suggested in kubernetes#73402 (comment), now --provider= and not passing --provider at all both trigger a message and then continue as if --provider=skeleton had been used.

pohly · 2019-01-31T14:57:34Z

Pushed another commit with that change.

neolit123 · 2019-01-31T15:03:11Z

thanks you.
if CI passes and unless others have objections for the above, we can squash the commits and merge the PR.

k8s-ci-robot · 2019-01-31T15:38:07Z

@pohly: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`dde3445`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

pohly · 2019-01-31T15:46:14Z

/test pull-kubernetes-verify

neolit123 · 2019-01-31T16:34:57Z

/lgtm

pohly · 2019-01-31T17:03:14Z

@neolit123 do you still want me to squash? I think it is not necessary, each commit makes sense by itself, and I would prefer to merge it like it is now (given that we have clean test results, a lgtm, etc.).

neolit123 · 2019-01-31T18:06:21Z

I think it is not necessary, each commit makes sense by itself, and I would prefer to merge it like it is now

seems fine to me!
would leave the call on that to @timothysc

pohly · 2019-01-31T20:33:42Z

@timothysc please have another look and if you agree then approve the PR.

timothysc

/approve

k8s-ci-robot · 2019-02-04T16:58:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, timothysc

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/OWNERS~~ [timothysc]

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

neolit123 · 2019-02-05T20:55:27Z

looks like this PR still manged to break our signal even if it wasn't supposed to:
https://k8s-testgrid.appspot.com/sig-cluster-lifecycle-all#kubeadm-gce-master

will investigate why.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 28, 2019

k8s-ci-robot requested review from neolit123 and timothysc January 28, 2019 13:41

neolit123 reviewed Jan 28, 2019

View reviewed changes

k8s-ci-robot assigned neolit123 Jan 28, 2019

pohly force-pushed the e2e-vendor-parameter branch from 9d3ed49 to b4384e9 Compare January 28, 2019 15:35

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

neolit123 mentioned this pull request Jan 28, 2019

bazel-build-test.yaml: use 'skeleton' for kubeadm jobs kubernetes/test-infra#10995

Merged

timothysc suggested changes Jan 28, 2019

View reviewed changes

e2e: reject unknown providers

f3d79e1

This finishes the work started for 1.13: instead of merely warning about an unknown value given to --profile, the test/e2e/e2e.test binary will now print an error and refuse to run. Fixes: kubernetes#70200

pohly force-pushed the e2e-vendor-parameter branch from b4384e9 to f3d79e1 Compare January 28, 2019 18:57

e2e: change default for --provider

dde3445

The empty string was the default and then triggered a special warning. There's no good reason for that behavior, so now the special handling for "unset provider" is gone and "skeleton" is the non-empty default for the value.

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

BenTheElder mentioned this pull request Jan 31, 2019

[Question]How to execute kubetest kubernetes/test-infra#11038

Closed

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2019

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

timothysc approved these changes Feb 4, 2019

View reviewed changes

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

k8s-ci-robot merged commit 732cb10 into kubernetes:master Feb 4, 2019

This was referenced Feb 5, 2019

path-alias k/k into k8s.io/kubernetes kubernetes/test-infra#11129

Merged

kubeadm: use 'gce' when dumping logs kubernetes/test-infra#11131

Merged

e2e: reject unknown providers #73402

e2e: reject unknown providers #73402

Conversation

pohly commented Jan 28, 2019 • edited Loading

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Jan 28, 2019

BenTheElder commented Jan 28, 2019

pohly commented Jan 28, 2019

neolit123 commented Jan 28, 2019

neolit123 commented Jan 28, 2019

pohly commented Jan 28, 2019

neolit123 commented Jan 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Jan 29, 2019 • 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

pohly commented Jan 29, 2019

neolit123 commented Jan 29, 2019

BenTheElder commented Jan 30, 2019

pohly commented Jan 30, 2019

pohly commented Jan 31, 2019

neolit123 commented Jan 31, 2019

neolit123 commented Jan 31, 2019

pohly commented Jan 31, 2019

neolit123 commented Jan 31, 2019 • edited Loading

pohly commented Jan 31, 2019

neolit123 commented Jan 31, 2019

k8s-ci-robot commented Jan 31, 2019 • edited Loading

pohly commented Jan 31, 2019

neolit123 commented Jan 31, 2019

pohly commented Jan 31, 2019

neolit123 commented Jan 31, 2019

pohly commented Jan 31, 2019

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 4, 2019

neolit123 commented Feb 5, 2019 • edited Loading

pohly commented Jan 28, 2019 •

edited

Loading

neolit123 Jan 28, 2019 •

edited

Loading

neolit123 Jan 29, 2019 •

edited

Loading

neolit123 commented Jan 31, 2019 •

edited

Loading

k8s-ci-robot commented Jan 31, 2019 •

edited

Loading

neolit123 commented Feb 5, 2019 •

edited

Loading