SIG testing: use only label filter for alpha/beta jobs #35575
Conversation
The implicit default for SKIP is '\[Serial\]'. That's surprising ("what you see
is *not* what you get") and unnecessary for these jobs. If we have serial tests,
then we can and should run them to get full coverage.
There is a risk that too many serial tests slow down the overall job. If that
is the case, then skipping serial tests should be added back inside the label
filter.
FOCUS doesn't have a default in these jobs, but elsewhere it defaults to
'[Conformance]'. It's safer to explicitly set it - perhaps someone does a
copy-and-paste of these settings.
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
[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 |
k8s-ci-robot
added
area/config
| - name: RUNTIME_CONFIG | ||
| value: '{"api/beta":"true", "api/ga":"true"}' | ||
| - name: SKIP | ||
| value: "" |
There was a problem hiding this comment.
/hold
This does not actually work 😠
https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/134250/pull-kubernetes-e2e-kind-alpha-beta-enabled/1971169750193016832 used it and still got '--ginkgo.skip=[Serial]' added.
Whatever does that is missing a "variable already set" check.
There was a problem hiding this comment.
IIRC it's the script in kind, but we also have the overly complex ginkgo-e2e.sh in k/k, I still want to do kubernetes/kubernetes#131058 but we had more pending discussion it seemed
There was a problem hiding this comment.
There was a problem hiding this comment.
/close
Replaced by #35594.
k8s-ci-robot
added
the
do-not-merge/hold
|
one of the goals of kind is to run fast, that means you get ~1100 tests in ~ 23 mins, if you run them serial it is ~ 2 hours for ~ 400 tests, plus you don;t get coverage on the impact of features on other features or test in other tests, Serial is a very happy scenario. So, Serial should not even exist, other thing is that a few test require them for reasons, but we should get coverage on parallel with high parallism first and later serial, for completeness |
|
I fully agree, we should have very few tests which need serial, ideally none. But if we have them for reasons that were justified, then these jobs should run them, and the choice whether they run should be left to the job definition, not some script somewhere in the bowls of our test machinery.
EDIT: I was wrong (eyes a bit tired): there is |
|
So the source of skipping serial is this: I suspect that comes from the times when Ginkgo didn't support running serial tests sequentially in a parallel runs. If true, then this was a workaround for a technical limitation, not a conscious choice because of runtime concerns. Whether a job is parallel or serial is not a good indicator for "must run quickly" vs. "may take longer". |
|
Let me try to formulate some basic principles. I'll use separate comments, so you can upvote/downvote them individually. cc @BenTheElder |
|
|
|
|
|
|
|
@pohly: Closed this PR. In response to this:
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. |
The implicit default for SKIP is '[Serial]'. That's surprising ("what you see is not what you get") and unnecessary for these jobs. If we have serial tests, then we can and should run them to get full coverage.
There is a risk that too many serial tests slow down the overall job. If that is the case, then skipping serial tests should be added back inside the label filter. Before https://github.com/kubernetes/test-infra/pull/35570/files (merged today), serial tests were enabled.
FOCUS doesn't have a default in these jobs, but elsewhere it defaults to '[Conformance]'. It's safer to explicitly set it - perhaps someone does a copy-and-paste of these settings.
This was noticed after reviewing the result of the jobs without the previous legacy SKIP:
/assign @aojea