sig-testing: uses LABEL_FILTER instead of FOCUS #32867
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
size/S
| @@ -454,8 +454,8 @@ presubmits: | |||
| value: '{"EventedPLEG":true}' | |||
| - name: RUNTIME_CONFIG | |||
| value: '{"api/alpha":"true", "api/ga":"true"}' | |||
| - name: FOCUS | |||
There was a problem hiding this comment.
@pacoxu should the job pull-kubernetes-e2e-kind-evented-pleg be moved into sig-node?
| - name: FOCUS | ||
| value: "." | ||
| - name: LABEL_FILTER | ||
| value: "!/Feature:.+/" | ||
| # TODO(bentheelder): reduce the skip list further | ||
| - name: SKIP | ||
| value: \[Slow\]|\[Disruptive\]|\[Flaky\]|\[Feature:.+\]|\[sig-storage\]|PodSecurityPolicy|LoadBalancer|load.balancer|In-tree.Volumes.\[Driver:.nfs\]|PersistentVolumes.NFS|Simple.pod.should.support.exec.through.an.HTTP.proxy|subPath.should.support.existing |
There was a problem hiding this comment.
This SKIP is historic. It looks like some tests were specifically added probably because they didn't work in this environment, others (like this blanket exclusion of sig-storage) perhaps because they don't add value and ran too long.
As @BenTheElder said above in the TODO, this list should get reduced. I'm just not sure how to do that safely. Aggressively prune, monitor the result, and fix it up afterwards if something becomes worse (test failures, significantly longer runtime)?
Unrelated to that, when using LABEL_FILTER let's drop the SKIP. That makes the jobs simpler.
There was a problem hiding this comment.
let's do incremental steps, if we do multiple changes at the same time and something fails we'll not be able to trace it back easily
There was a problem hiding this comment.
Then at least remove |\[Feature:.+\] from SKIP because that is what "!/Feature:.+/" replaces. Otherwise it's not different from before.
Actually, let's not use the regexp. Instead: Feature: isEmpty.
There was a problem hiding this comment.
@pohly @aojea I open a PR to make the canary job identical to the normal job. #32907
Let’s reduce the SKIP list later.
I want to make ci pass in kubernetes/kubernetes#125452
There was a problem hiding this comment.
I'm okay with keeping the SKIP different. Perhaps add a comment about it to the YAML?
There was a problem hiding this comment.
Also cross-reference the jobs and explain that one should update the canary first when making potentially intrusive changes. This is tribal knowledge - we should start documenting this.
There was a problem hiding this comment.
@pohly added. Please review it again. thanks
|
/cc @pohly |
|
/approve defer @pohly for LGTM |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
size/M
k8s-ci-robot
added
size/S
k8s-ci-robot
added
size/M
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, carlory, pohly 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:
Approvers can indicate their approval by writing |
|
@carlory: Updated the
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. |
| # TODO(bentheelder): reduce the skip list further | ||
| - name: SKIP | ||
| value: \[Slow\]|\[Disruptive\]|\[Flaky\]|\[Feature:.+\]|PodSecurityPolicy|LoadBalancer|load.balancer|Simple.pod.should.support.exec.through.an.HTTP.proxy|subPath.should.support.existing|NFS|nfs|inline.execution.and.attach|should.be.rejected.when.no.endpoints.exist | ||
| value: \[Slow\]|\[Disruptive\]|\[Flaky\]||PodSecurityPolicy|LoadBalancer|load.balancer|Simple.pod.should.support.exec.through.an.HTTP.proxy|subPath.should.support.existing|NFS|nfs|inline.execution.and.attach|should.be.rejected.when.no.endpoints.exist |
There was a problem hiding this comment.
did we ever test this with a canary job first? we appear to be running ZERO test cases now kubernetes/kubernetes#126401
There was a problem hiding this comment.
Yes, we did. Let's discuss on Slack.
|
embarrasing, sorry, we should have validated after merge this was working as expecting |
xref: #32648