-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Shift generic alpha and beta jobs to generic alpha and beta feature tags #34456
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: liggitt 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 |
@@ -474,7 +474,7 @@ presubmits: | |||
- --gcp-region=us-central1 | |||
- --provider=gce | |||
- --runtime-config=api/all=true | |||
- --test_args=--ginkgo.focus=\[Feature:(WatchList|InPlacePodVerticalScaling|APIServerTracing|SidecarContainers|StorageVersionAPI|PodPreset|ClusterTrustBundle|ClusterTrustBundleProjection|PodLifecycleSleepAction|RecoverVolumeExpansionFailure)\] --ginkgo.skip=\[Feature:(SCTPConnectivity|Volumes|Networking-Performance)\]|IPv6|csi-hostpath-v0|\[KubeUp\] --minStartupPods=8 | |||
- --test_args=--ginkgo.focus=\[Feature:(Alpha|WatchList|InPlacePodVerticalScaling|APIServerTracing|SidecarContainers|StorageVersionAPI|PodPreset|ClusterTrustBundle|ClusterTrustBundleProjection|PodLifecycleSleepAction|RecoverVolumeExpansionFailure)\] --ginkgo.skip=\[Feature:(Beta|SCTPConnectivity|Volumes|Networking-Performance)\]|IPv6|csi-hostpath-v0|\[KubeUp\] --minStartupPods=8 |
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.
Instead of tweaking focus regexps further I prefer to convert to -ginkgo.label-filter
. Let's end the pain.
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.
in this case, would that mean the following?
- --test_args=--ginkgo.focus=\[Feature:(Alpha|WatchList|InPlacePodVerticalScaling|APIServerTracing|SidecarContainers|StorageVersionAPI|PodPreset|ClusterTrustBundle|ClusterTrustBundleProjection|PodLifecycleSleepAction|RecoverVolumeExpansionFailure)\] --ginkgo.skip=\[Feature:(Beta|SCTPConnectivity|Volumes|Networking-Performance)\]|IPv6|csi-hostpath-v0|\[KubeUp\] --minStartupPods=8 | |
- --test_args=--ginkgo.label-filter=/Feature:(Alpha|WatchList|InPlacePodVerticalScaling|APIServerTracing|SidecarContainers|StorageVersionAPI|PodPreset|ClusterTrustBundle|ClusterTrustBundleProjection|PodLifecycleSleepAction|RecoverVolumeExpansionFailure)/ && !(/Feature:(Beta|SCTPConnectivity|Volumes|Networking-Performance)/ || IPv6 || csi-hostpath-v0 || KubeUp) --minStartupPods=8 |
Though I am not sure what "IPv6" and "csi-hostpath-v0" stand for, these don't appear to be labels in the k/k e2e test suite. I actually couldn't find mention of "csi-hostpath-v0" in the whole k/k codebase.
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.
No, that's not quite how Ginkgo --label-filter works. See #32911 and https://groups.google.com/a/kubernetes.io/g/dev/c/DTFEng143NY/m/FqdD3RADAQAJ for instructions.
A good first step would be to explain in plain English which tests should run in this job. Is it generic (= run everything that can run) or specific to certain features (as implied by the current focus expression which only runs tests selected by that expression)?
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.
That is actually quite a good mental exercise. I wonder what this and the below tests are supposed to do. From both the test name and the current test definition, I suspect the plain English explanation would be run tests for these specific features as long as they are alpha
.
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.
to unblock ClusterTrustBundle beta promotion, if something like kubernetes/kubernetes#130580 or kubernetes/kubernetes#130567 can get us coverage via the kind alpha / alpha-beta / beta jobs (which it seems like they both can), I'd actually be fine just dropping ClusterTrustBundle|ClusterTrustBundleProjection from ~all the other e2e job configurations for now
then those other jobs can switch to label filter at their leisure, and we're not entangled with ClusterTrustBundle work
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.
I wonder what this and the below tests are supposed to do.
That is the million dollar question. The answer might very well be "generic job which supports all feature-gated tests that have no special other requirements", in which case the label filter is as simple as the one in the kind jobs.
Most of the focus expression then becomes historic garbage that we should get rid of.
Who owns these jobs? The job owner should know what they are for and what they are willing to support with regards to triaging test failures.
then those other jobs can switch to label filter at their leisure, and we're not entangled with ClusterTrustBundle work
That sounds like a good plan.
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.
That is the million dollar question. The answer might very well be "generic job which supports all feature-gated tests that have no special other requirements", in which case the label filter is as simple as the one in the kind jobs.
IMHO that should be the end-state.
Most of the focus expression then becomes historic garbage that we should get rid of.
+1
Who owns these jobs? The job owner should know what they are for and what they are willing to support with regards to triaging test failures.
The GCE ones are tragedy of the commons.
The kind ones @aojea and I are actively maintaining.
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.
in which case the label filter is as simple as the one in the kind jobs.
IMHO that should be the end-state.
Cross-reference: kubernetes/kubernetes#130580 (comment) proposes a slightly different label filter for "cluster with default configuration".
The GCE ones are tragedy of the commons.
Should we make that official and move them under SIG Testing or Release? SIG release might be more appropriate, considering that they already get the alerts (testgrid-alert-email: [email protected]
).
/cc |
PR needs rebase. 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. |
Alternative to #34451
The synthetic generic Alpha and Beta "features" could be used for features which don't require any special configuration beyond alpha/beta API and AllAlpha/AllBeta gate enablement, e.g. kubernetes/kubernetes#130567
cc @stlaz @BenTheElder @pohly