-
Notifications
You must be signed in to change notification settings - Fork 1.7k
e2e-k8s.sh: don't manipulate SKIP when LABEL_FILTER is used #4015
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
All jobs using e2e-k8s.sh with PARALLEL=true automatically skipped serial tests even though technically they now could run in the job: https://github.com/kubernetes-sigs/kind/blame/d1eecc46e30cac9d35cd32dc52677ef75ec22e18/hack/ci/e2e-k8s.sh#L226-L234 kubernetes-sigs/kind#4015 is changing that mandatory skip for jobs using LABEL_FILTER because it may be desirable to include serial tests, depending on the job. It's also better to be explicit about it in each job's LABEL_FILTER to avoid confusion and potential mistakes (not running tests that were expected to run). To prepare for that change, jobs get updated based on the following principles: - If a presubmit runs infrequently and is only invoked to test certain aspects, then including serial tests is desirable to get full test coverage of that aspect. Example: pull-kubernetes-e2e-kind-beta-features - If a presubmit runs always, serial tests should be excluded. Example: pull-kubernetes-e2e-kind - Periodic jobs should always run all supported tests, including the serial ones. Example: ci-kubernetes-e2e-kind - Canary jobs match the behavior of the non-canary variant. Example: pull-kubernetes-e2e-kind-canary To make it more obvious where the upcoming e2e-k8s.sh will change test selection, "Includes serial tests for the sake of completeness." comments get added. Those are not true *right now*, but will be once the script is changed.
/hold Needs to be merged after kubernetes/test-infra#35594. |
hack/ci/e2e-k8s.sh
Outdated
# if we set PARALLEL=true, skip serial tests set --ginkgo-parallel | ||
if [ "${PARALLEL:-false}" = "true" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we set PARALLEL=true, skip serial tests set --ginkgo-parallel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated:
# If we have PARALLEL=true, set --ginkgo-parallel.
# Skip serial tests only if the job has not been converted to use LABEL_FILTER.
It's unexpected when a job runs a different set of tests than specified by LABEL_FILTER. Not only that, at least for periodic jobs it would be desirable to have only one variant of the job which runs all tests (serial and parallel), with parallel execution of the parallel tests. Maintaining two variants is more work and makes the testgrid dashboards larger. This change does not affect jobs that don't use LABEL_FILTER because the traditional behavior is preserved for those. It does affect the following jobs (based on "grep 'name: LABEL_FILTER'"): - pull-kind-conformance-parallel-dual-stack-ipv4-ipv6 - pull-kind-e2e-kubernetes - pull-kind-e2e-kubernetes-canary - ci-kubernetes-kube-network-policies-conformance-parallel - ci-kubernetes-kube-network-policies-conformance-parallel-ipv6 - pull-kube-network-policies-nftables - pull-kube-network-policies-nftables-ipv6 - pull-kubernetes-e2e-kind-dependencies (also for 1.34) - pull-kubernetes-e2e-kind-golang-tip (also for 1.34) - ci-kubernetes-e2e-kind-dependencies (also for 1.34) - ci-kubernetes-e2e-kind-golang-tip (also for 1.34) - ci-kubernetes-kind-e2e-json-logging (also in release branches) - ci-kubernetes-network-kind-alpha-beta-features - ci-kubernetes-e2e-kind (also for 1.34) - ci-kubernetes-e2e-kind-ipv6 - ci-kubernetes-e2e-kind-beta-features - ci-kubernetes-e2e-kind-alpha-beta-features - pull-kubernetes-e2e-kind - pull-kubernetes-e2e-kind-canary - pull-kubernetes-e2e-kind-ipv6 - pull-kubernetes-e2e-kind-ipv6-canary - pull-kubernetes-e2e-kind-beta-features - pull-kubernetes-e2e-kind-beta-enabled - pull-kubernetes-e2e-kind-beta-enabled-conformance - pull-kubernetes-e2e-kind-alpha-beta-features - pull-kubernetes-e2e-kind-alpha-beta-enabled - pull-kubernetes-e2e-kind-alpha-beta-enabled-conformance - pull-kubernetes-e2e-kind-alpha-beta-features-race - pull-kubernetes-e2e-kind-evented-pleg - pull-kubernetes-e2e-storage-kind-alpha-beta-features-slow kube-network-policies already ran serial conformance tests ('|| Conformance'), but not normal serial tests. Some other jobs using LABEL_FILTER already explicitly disabled serial tests and thus get the same behavior as before. The first two are inconsistent at the moment: pull-kind-e2e-kubernetes is probably meant to run more than conformance tests and correctly does so by not setting FOCUS. pull-kind-conformance-parallel-dual-stack-ipv4-ipv6 is meant to be limited to conformance, but does not filter by that in LABEL_FILTER and thus runs also non-conformance tests because the FOCUS default already got disabled earlier when using LABEL_FILTER. Other jobs may need an explicit "!Serial", depending on the intention of the job owner, before the e2e-k8s.sh can be changed.
c741b9f
to
46d774c
Compare
@pohly: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
pull-kind-conformance-parallel-dual-stack-ipv4-ipv6 timed out because kubernetes/test-infra#35594 hasn't been merged yet. That PR ensures that serial tests don't run in that job, avoiding the timeout. |
It's unexpected when a job runs a different set of tests than specified by LABEL_FILTER. Not only that, at least for periodic jobs it would be desirable to have only one variant of the job which runs all tests (serial and parallel), with parallel execution of the parallel tests. Maintaining two variants is more work and makes the testgrid dashboards larger.
This change does not affect jobs that don't use LABEL_FILTER because the traditional behavior is preserved for those.
It does affect the following jobs (based on "grep 'name: LABEL_FILTER'"):
kube-network-policies already ran serial conformance tests (
|| Conformance
), but not normal serial tests.Some other jobs using LABEL_FILTER already explicitly disabled serial tests and thus get the same behavior as before.
The first two are inconsistent at the moment: pull-kind-e2e-kubernetes is probably meant to run more than conformance tests and correctly does so by not setting FOCUS. pull-kind-conformance-parallel-dual-stack-ipv4-ipv6 is meant to be limited to conformance, but does not filter by that in LABEL_FILTER and thus runs also non-conformance tests because the FOCUS default already got disabled earlier when using LABEL_FILTER.
Other jobs may need an explicit
!Serial
, depending on the intention of the job owner, before the e2e-k8s.sh can be changed. kubernetes/test-infra#35594 contains those changes. It needs to be merged first.