RFC: e2e: enhance WithFeatureGate labels #124350
Conversation
k8s-ci-robot
added
release-note
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Apr 17 13:56:00 UTC 2024. |
k8s-ci-robot
added
cncf-cla: yes
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
k8s-ci-robot
added
the
needs-priority
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
area/e2e-test-framework
We want: - To keep test annotations simple, using both WithFeatureGate and WithFeature should only be necessary when a test really has requirements that go beyond "feature gate needs to be enabled". - To run tests which depend only on feature gates being enabled in the ci-kubernetes-e2e-kind-alpha-features resp. ci-kubernetes-e2e-kind-beta-features, because otherwise we may have a proliferation of many bespoke jobs which only run very few tests. This would make testing more expensive for Kubernetes. - To enable those tests only once in the ci-kubernetes-e2e-kind-alpha-features and ci-kubernetes-e2e-kind-beta-features definition instead of having to update those each time feature gates change. This can be achieved by adding `Feature:Alpha` resp. `Feature:Beta` as Ginkgo labels instead of just `Alpha` and `Beta`. Then jobs which are configured to skip tests with feature dependencies via --label-filter=!/Feature:.+/ will skip tests which are labeled with just WithFeatureGate. The ci-kubernetes jobs can select to include such tests with a special regexp that mimicks a negative lookahead (see k8s.io/community/contributors/devel/sig-testing/e2e-tests.md) Note that removing WithFeature depends on first updating job definitions to use --label-filter or to skip based on the inline `[Alpha]` or `[Beta]` text, otherwise tests that were previously skipped because of WithFeature might start to run in jobs which don't have the feature gate enabled.
pohly
force-pushed
the
e2e-feature-labels
branch
from
3009cd4
to
c57be6f
Compare
k8s-ci-robot
added
size/M
| // [Alpha] resp. [Beta] get added to the test name automatically depending | ||
| // on the current stability level of the feature. Feature:Alpha resp. | ||
| // Feature:Beta get added to the Ginkgo labels because this is a special | ||
| // requirement for how the cluster needs to be configured. |
There was a problem hiding this comment.
Beta is an edge case here: if the feature gate is on by default, any cluster might work out-of-the-box. I am saying "might" because:
- the cluster might explicitly turn off beta features
- the feature might depend on a beta API group - those are not on by default
I think it is safer to assume that "beta" does not mean "available by default" and thus add the Feature:Beta label.
There was a problem hiding this comment.
cc @carlory
See also kubernetes/test-infra#31665
| extra string | ||
| // extra is an optional feature name. It gets added as [<extraFeature>] | ||
| // to the test name and as Feature:<extraFeature> to the labels. | ||
| extraFeature string |
There was a problem hiding this comment.
you got me puzzled with this, so there is only one change in this PR that is
if slices.Contains(labels, "Feature:"+tag) {
// Okay, was also set as label.
continue
}
the rest is a rename of this field?
There was a problem hiding this comment.
The only user-visible change is indeed that the Alpha Ginkgo label becomes Feature:Alpha and Beta becomes Feature:Beta.
I'm not aware of anyone currently using the Ginkgo labels, so I think it's fairly safe to do this.
I am not renaming the inline [Alpha] and [Beta] tags. Those have been around for longer, so they might be used?
|
/lgtm /hold /assign @BenTheElder |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: a4eb4d858d64fa4de9805d769011a67d2b2a76b4
|
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We want:
To keep test annotations simple: using both WithFeatureGate and WithFeature should only be necessary when a test really has requirements that go beyond "feature gate needs to be enabled" (example: HonorPVReclaimPolicy in add e2e test for HonorPVReclaimPolicy #123151 only needs the feature gate).
To run tests which depend only on feature gates being enabled in the ci-kubernetes-e2e-kind-alpha-features resp. ci-kubernetes-e2e-kind-beta-features, because otherwise we may have a proliferation of many bespoke jobs which only run very few tests. This would make testing more expensive for Kubernetes.
To enable those tests only once in the ci-kubernetes-e2e-kind-alpha-features and ci-kubernetes-e2e-kind-beta-features definition instead of having to update those each time feature gates change.
Special notes for your reviewer:
This can be achieved by adding
Feature:Alpharesp.Feature:Betaas Ginkgo labels instead of justAlphaandBeta. Then jobs which are configured to skip tests with feature dependencies via--label-filter=!/^Feature:.+/will skip tests which are labeled with just WithFeatureGate. The ci-kubernetes jobs can select to include such tests with a special regexp that mimicks a negative lookahead (see kubernetes/community#7824).Note that removing WithFeature depends on first updating job definitions to use --label-filter or to skip based on the inline
[Alpha]or[Beta]text, otherwise tests that were previously skipped because of WithFeature might start to run in jobs which don't have the feature gate enabled.Does this PR introduce a user-facing change?