e2e-tests.md: update description of labels #7824
Conversation
k8s-ci-robot
added
do-not-merge/hold
|
[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/developer-guide
| Conceptually, these are non-default requirements as defined above under | ||
| `[Feature:.+]`, but for historic reasons and the sake of brevity they don't | ||
| have that prefix when embedded in test names. They *do* have that prefix in the | ||
| Ginkgo v2 label, so use e.g. `--filter-label=Feature:Alpha`. |
There was a problem hiding this comment.
My intent was to also add Feature:Privileged and Feature:Internet as Ginkgo labels. But when looking at the code I found that [Internet] isn't in the code base at all (should be removed above?) and [Privileged] is only used here:
test/e2e/network/conntrack.go: ginkgo.It("proxy implementation should not be vulnerable to the invalid conntrack state bug [Privileged]", func(ctx context.Context) {
test/e2e/network/kube_proxy.go: ginkgo.It("should set TCP CLOSE_WAIT timeout [Privileged]", func(ctx context.Context) {
I don't see any Prow job which uses it. Shall we remove it above and in the code?
There was a problem hiding this comment.
\[Deprecated\] is used in some jobs, but not in any current test. It probably shouldn't become a Feature:Deprecated, that doesn't match the intent. That means that I need to update this new paragraph a bit so that it is clear that it only refers to [Alpha] and [Beta].
There was a problem hiding this comment.
Feature:Internet means the cluster needs Internet access, is common to run CI on airgapped environment and this test broke those CIs, these CIs need to be able to filter.
Privileged for that test is because the pod needs special privileges and there are some distros that does not allow that
SecurityContext: &v1.SecurityContext{
Capabilities: &v1.Capabilities{
Add: []v1.Capability{"NET_RAW"},
but I can see there are other tests that require this
test/e2e/common/node/privileged.go: SecurityContext: &v1.SecurityContext{Privileged: &isPrivileged},
test/e2e/common/node/privileged.go: SecurityContext: &v1.SecurityContext{Privileged: ¬Privileged},
so this last one seems can be removed
There was a problem hiding this comment.
but if we prefix just with a different prefix we get this simpler, no? instead of reusing Feature we can use Graduation or Stability
Graduation:Alpha
Stability:Beta
There was a problem hiding this comment.
AFAIK we never used [Feature:Internet]; we have:
[Feature:Networking-IPv4]: requires access to external IPv4 addresses[Feature:Networking-IPv6]: requires access to external IPv6 addresses[Feature:Networking-DNS]: requires the ability to resolve (but not connect to) the IPs of external domain names
Though in #124422 I had to tweak a test to use curl -4 so that I could then mark it Networking-IPv4, so maybe it would be better to have just Internet and have it have mean "it's assumed that if pods have an IPv4 podIPs value then they have access to the IPv4 Internet and if they have an IPv6 podIPs value then they have access to the IPv6 Internet".
There was a problem hiding this comment.
oh, my mind assumed we already did change it, I remember it was discussed kubernetes/kubernetes#102963 (comment)
There was a problem hiding this comment.
So we agree to remove the stale documentation of [Internet] here, right?
We don't need to document all the [Feature:<something>] here, that's better handled via the source code which gets linked to.
There was a problem hiding this comment.
[Internet] removed and examples for Feature: definition updated based on real examples from kubernetes/kubernetes#124423 to make it clearer that the non-default requirement could also be about the behavior of the test. The [Performance] example was already, but without explaining it's meaning.
|
|
||
| # Run tests in parallel, skip any that must be run serially and keep the test namespace if test failed | ||
| GINKGO_PARALLEL=y kubetest --test --test_args="--ginkgo.skip=\[Serial\] --delete-namespace-on-failure=false" | ||
| GINKGO_PARALLEL=y kubetest --test --test_args='--ginkgo.label-filter=!Serial --delete-namespace-on-failure=false' |
There was a problem hiding this comment.
!Serial is special in an interactive bash when included in double quotes. Therefore I switched to single quotes here and (for the sake of consistency) also above.
| `Feature:Alpha`. It works because any word after `Feature:` other than exactly `Alpha` | ||
| will match one of the alternatives, but `Alpha` itself doesn't: | ||
|
|
||
| --filter-label=!/^Feature:([^A]|A[^l]|Al[^p]|Alp[^h]|Alph[^a]|Alpha.)/ |
There was a problem hiding this comment.
The alternative is to make Ginkgo use a regexp implementation which supports negative lookahead. But that then adds a new dependency to Kubernetes. I don't see a big need for this kind of special "exclude features except for these", so covering those few cases with this special regexp is the lesser evil?
There was a problem hiding this comment.
I would rather prefer to avoid complex expressions
There was a problem hiding this comment.
So do I, but I can't think of a simpler solution. Using Graduation:Alpha or Stability:Beta doesn't work (try writing a --label-filter for ci-kubernetes-e2e-kind-alpha-features based on that and you'll see why).
There was a problem hiding this comment.
we could provide our own command-line syntax and have e2e.test convert it into an ugly regex for ginkgo's benefit?
There was a problem hiding this comment.
This enters a grey area: it assumes that the -focus/skip/label-filter parameters that get passed to the ginkgo CLI command are just getting passed through to the e2e.test binary and all the parsing and logic happens there. This happens to work and we already rely on it in various places by passing -ginkgo.focus/skip only to the e2e.test binary, so I suppose we can assume that this works.
But what would such a command-line syntax look like? How does it interact with the other parameters? Is this really worth adding a special case for?
There was a problem hiding this comment.
... or we can work with @onsi on defining much richer semantics on test filtering ...
There was a problem hiding this comment.
Mirroring what I discussed with @aojea on Slack: coming up with something that avoids the negative lookahead workaround and/or regexp entirely is something that can be added later. For now we could proceed as proposed here.
/cc @BenTheElder
There was a problem hiding this comment.
But what would such a command-line syntax look like?
I was thinking mostly about the case of "I want to skip all features except for X, Y, and Z", which might be common enough to merit special syntax?
But anyway, yeah, this is "later" stuff
| `skip` argument. | ||
| `skip` argument. This is not using `[Feature:LinuxOnly]` because that | ||
| would have implied changing all CI jobs which skip tests with unknown | ||
| requirements. |
There was a problem hiding this comment.
[LinuxOnly] is kind of a dumpster fire (at least in the networking tests) and maybe should be revisited/refactored anyway...
There was a problem hiding this comment.
Agreed, but I'll leave that to others.
There was a problem hiding this comment.
And probably it is better to do it in smaller steps, i.e. just clarify Feature vs. FeatureGate now and start using Ginkgo labels.
This explains how to use --filter-label (possible since Kubernetes 1.29) and clarifies the usage of feature gate labels.
| --ginkgo.skip="": If set, ginkgo will only run specs that do not match this | ||
| regular expression. | ||
|
|
||
| --ginkgo.label-filter="": If set, select tests based on their labels as described under | ||
| "Spec Labels" in https://onsi.github.io/ginkgo/#filtering-specs. This can focus | ||
| on tests and exclude others in a single parameter without using regular expressions. |
There was a problem hiding this comment.
it still uses regular expressions no? what it solves is the need to use regex for both focus and skip IIUIRC
There was a problem hiding this comment.
"without having to use regular expressions"?
That's what I wanted to say ("can ... without using regular expressions"), not that it never uses regular expressions.
|
/lgtm /assign @danwinship @BenTheElder @SergeyKanzhelev |
k8s-ci-robot
added
the
lgtm
|
/hold The intent is that adding tests with only a
That is because the "traditional" |
This explains how to use --filter-label (possible since Kubernetes 1.29) and clarifies the usage of feature gate labels.
/hold
The intent is that adding tests with only a
FeatureGatelabel is sufficient. Before we allow such tests, we have to--label-filterto skip tests with special requirements including alpha/beta feature gates.That is because the "traditional"
SKIP=\[FeatureGate:.+\]will no longer match tests which have[FeatureGate:Foo] [Alpha], as explained in kubernetes/kubernetes#124350.