[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

cc @kubernetes/cncf-conformance-wg

This is consistent with our statement that

a) role labels are advisory
b) masters aren't special
c) conformance doesn't require masters to be labeled a certain way
d) conformance tests should work no matter what the topology of the cluster is

/retest

/test pull-kubernetes-e2e-kops-aws

/hold
for feedback from sig-testing

is there anything I need to do to plumb this option through kubetest or ginkgo.sh?

/test pull-kubernetes-e2e-kind

is there anything I need to do to plumb this option through kubetest or ginkgo.sh?

kubetest has this flag:

  --test_args string                                   Space-separated list of arguments to pass to Ginkgo test runner.

so it can already pass the new --node-schedulable-selector.
for ginkgo-e2e.sh i don't think this is needed, but there might be jobs in test-infra that require it now.

cc @justinsb
https://github.com/kubernetes/kubernetes/pull/76654/files#diff-eb7b79470992813ea1905e96c298b47bL2752
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/76654/pull-kubernetes-e2e-kops-aws/1118264577910378496/
does this mean that after this PR the kops jobs will always timeout?

does this mean that after this PR the kops jobs will always timeout?

Only until we adjust the kops job invocation to inform the test framework that it shouldn't wait for all nodes to be schedulable based on the way kops sets up the cluster

the kind job SIGABRT-ed from ginkgo.
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/76654/pull-kubernetes-e2e-kind/1118292602886557696/build-log.txt

the backtrace does not include framework entries, but my guess here is a missing net/http timeout.

the kind(kubeadm) worker nodes are unlabeled, so if NodeSchedulableSelector is now mandatory it means we have to modify them with respect to the e2e testing.
@kubernetes/sig-cluster-lifecycle
cc @BenTheElder

/test pull-kubernetes-e2e-kind

/assign @timothysc
to have a look too.

the kind(kubeadm) nodes are unlabeled, so if NodeSchedulableSelector is now mandatory it means we have to modify them with respect to the e2e testing.

It is not mandatory. Environments with no special node labeling don't need to do anything. The existing e2e presubmits passed with no changes, for example. Only clusters that assign special labels to nodes they don't want to wait for need to inform the e2e of that.

they'd also been passing fine.. https://testgrid.k8s.io/conformance-kind#kind,%20master%20(dev)&width=20

that backtrace is not terribly informative so far :|

@liggitt: The following tests failed, say /retest to rerun them all:

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.

passing --node-schedulable-selector=\!node-role.kubernetes.io/master works.

but it seems to me the current logic is incorrect:
https://github.com/kubernetes/kubernetes/pull/76654/files#diff-eb7b79470992813ea1905e96c298b47bR2759

waiting for a node that is unschedulable for the duration of the job timeout (e.g. 30+minutes) doesn't feel right.

Apr 17 03:49:40.680: INFO: Waiting up to 30m0s for all nodes to be schedulable
Apr 17 03:49:40.704: INFO: Unschedulable nodes:
Apr 17 03:49:40.704: INFO: -> kind-control-plane Ready=true Network=true Taints=[{node-role.kubernetes.io/master  NoSchedule <nil>}]
Apr 17 03:49:40.704: INFO: ================================
Apr 17 03:50:10.717: INFO: Unschedulable nodes:
Apr 17 03:50:10.717: INFO: -> kind-control-plane Ready=true Network=true Taints=[{node-role.kubernetes.io/master  NoSchedule <nil>}]
Apr 17 03:50:10.717: INFO: ================================
....<loops>

the selector for all nodes is pretty easy (1=1

We discussed in sig-testing slack, TLDR tentative plan is to improve taint handling a bit in the wait-for-node-readiness logic.

We could plumb through another flag for kops/kind, but e.g. sonobuoy should work without that for clusters with less than surprising configuration.

Ideally we can ignore nodes with the node-role taint, or as @liggitt suggested only wait for certain known taints to be removed. When waiting for taints was originally added only certain taints were referenced ('not-ready and network-unavailable').

Another option would be removing the waiting for node readiness entirely and leaving that up to the tester / cluster operator.

cc @mborsz -- any other particular taints we should be waiting on?

Another option would be removing the waiting for node readiness entirely and leaving that up to the tester / cluster operator.

That would be my preference long-term. "Get your cluster ready, then run the test" makes so much more sense to me

The two mentioned in the PR that added the label checking were NetworkUnavailable and NotReady

waiting for a node that is unschedulable for the duration of the job timeout (e.g. 30+minutes) doesn't feel right.

that comes from the pre-existing --node-schedulable-timeout option and default of 30*time.Minute

Maybe a follow up issue but directly related to your questions about plumbing it through ginkgo.sh and such:

https://github.com/kubernetes/kubernetes/tree/master/cluster/images/conformance should be updated so that the flag can be set when using the conformance image. This way anyone pursuing CNCF certification via Sonobuoy can supply the values.

So TL;DR from my reading of this issue, is plumbing of a whitelist of taints is a reasonable behavior, but other taints should be respected and possibly dropped from the overall node count. If this is the case, then I think that's a good move forwards.

@liggitt: PR needs rebase.

Status check on this?

Closing in favor of #78500