NodeSpecialFeature is basically a combination Feature + Environment tag as originally defined so 👍

I see a few problems with requiring these `[Alpha]/[Beta]/[Stable]/[Deprecated]' tags.

The first is conceptual: if a test depends on two different features, what tag should be used? All that apply, e.g. [FeatureGate:foo] [Beta] [FeatureGate:bar] [Stable]?

In the past, annotations where removed once a feature graduated to stable, which then also automatically enabled the test in the core jobs. Is the plan now to keep the [FeatureGate] annotation and instead adapt job definitions when a feature graduates to GA? I can see that it would be valuable to keep the [FeatureGate] annotation because it provides information that otherwise would get lost. For example, one might want to run all tests for a specific feature even after it has graduated. I am less sure that [Stable] is useful.

My biggest concern is that the [Alpha]/[Beta]/[Stable]/[Deprecated] annotations are redundant and that maintaining them is going to be error-prone.

Instead of adding them manually to the test description, can we do that with helper functions? Here's how that could look like:

Current solution:
ginkgo.It("foo bar [FeatureGate:foo] [Beta]", ...)

Revised approach:
framework.It("foo bar", framework.WithFeatureGate("foo"), ...)

framework.It then acts as wrapper around ginkgo.It. When one of the optional parameters is the result of framework.WithFeatureGate, it does several things:

• Checks that the feature is registered in a feature gate. That feature gate could be seeded with the Kubernetes features, but additional features could be added.
• Retrieves the stability level of that feature from the feature gate.
• Extends the test description with, in this example, [FeatureGate:foo] [Beta].
• Add ginkgo.Label("FeatureGate:foo") and ginkgo.Label("Beta") when calling ginkgo.It.

Another advantage is that typos in the feature name will be detected when registering the tests, i.e. when the suite starts. When embedded in the text we have to rely on code reviews.

Similar helper functions for [Environment] and [Feature] could also be added. Then it becomes possible to select tests with:

  --label-filter [expression] 
    If set, ginkgo will only run specs with labels that match the label-filter. 
    The passed-in expression can include boolean operations (!, &&, ||, ','),
    groupings via '()', and regular expressions '/regexp/'.  e.g. '(cat || dog)
    && !fruit'

The first is conceptual: if a test depends on two different features, what tag should be used? All that apply, e.g. [FeatureGate:foo] [Beta] [FeatureGate:bar] [Stable]?

we should not have features that depends on features that are not stable, right?

I can see that it would be valuable to keep the [FeatureGate] annotation because it provides information that otherwise would get lost

I see that more as part of the [Feature:] tag, feature-gates should be part of the system when they graduate, if the FeatureGate enables a new Feature we can use this tag

By requiring that all features, feature gates and environments must be registered in the framework, we can add command line flags that list them. Might be useful.

yeah, agree, we can do much better and we should aim for that, but at this point I think we should fix current state and have the right variables, and then iterate on those.
It seems this KEP allows us to define those variables with Feature, FeatureGate and Environment ... if we agree on those, we can start working later no how to "enforce" those and how to improve the automation , WDYT?

See https://github.com/kubernetes/kubernetes/pull/112894/files for a prototype implementation of this approach.

we should not have features that depends on features that are not stable, right?

Yes, but a test itself might depend on two independent features. It's unlikely, I just wanted to call it out.

I see that more as part of the [Feature:] tag, feature-gates should be part of the system when they graduate, if the FeatureGate enables a new Feature we can use this tag

So before a feature is GA, tests get annotated with [FeatureGate:foo] [Beta], then when they graduate, we replace that with [Feature:foo] [Stable]? That seems a bit inconsistent to me.

at this point I think we should fix current state and have the right variables, and then iterate on those

I'm worried about code churn and reviewer fatigue.If we do this in two steps, we have to touch all test definitions twice, first to clean up the text, then later to turn the text into function calls. Reviewing the first step will be harder because typos have to be caught by reviewers.

So before a feature is GA, tests get annotated with [FeatureGate:foo] [Beta], then when they graduate, we replace that with [Feature:foo] [Stable]? That seems a bit inconsistent to me.

I think that this is one of the problems we are having, FeatureGates and Feature are different things, the problem that I see is that sometimes FeatureGates are used to gate Features, but not all FeatureGates are Features

I'm worried about code churn and reviewer fatigue.If we do this in two steps, we have to touch all test definitions twice, first to clean up the text, then later to turn the text into function calls. Reviewing the first step will be harder because typos have to be caught by reviewers.

I think that is a fantastic idea to use the functions as you described and keep it in sync with the code ... but I don't want to impose it to the KEP author 😄

I can help with using those functions, both by updating the KEP and providing the implementation.

But let's clarify first how to handle graduated features. That has implications that are unclear, both with and without those helper functions.

Another thought: Feature and FeatureGate are easy to confuse. Kubernetes itself didn't get this right: the featuregate package has an implementation of one [Mutable]FeatureGate that stores the state of different Features, but everywhere else we call those Feature strings "feature gates".

Can we perhaps replace the term Feature? How about Functionality?

With Functionality and FeatureGate it is more obvious that Functionality is something else and "feature gate" is pretty well defined in Kubernetes (when ignoring the slight misnaming).

It's worth noting that renaming Feature has massive churn requirement on CI configs upstream and downstream. The default state of most jobs is to opt out of features, to only test stably supported portable things. With specific jobs running unstable or not necessarily available things.

yes, the back compat was mostly the reason to keep the Feature naming. Alternative may be Capability, but again, it will require a lot of renaming.

Makes sense. Perhaps add that rationale to the KEP?

"this confusion will not be addressed in this KEP"

If not now, when? If you're going to formalize this label, I think now is the appropriate time to choose a better name to avoid the confusion

The subtle differences as I see them:

• [NodeConformance] tests "enabled out of the box functionality [... that] may still require an additional environment set up"... can you give some examples here?
• [Beta] features are allowed since they're usually enabled by default (though beta APIs are usually disabled by default)

I get the need to classify and test all "default" Node level functionality, I would just prefer to avoid the word Conformance given these caveats. [NodeDefault], [NodeBase], even just [Node]

Yes, I just wanted to postpone solving this naming problem =)

plus changing name has some back compat issues I didn't want to explore too much

I'm not sure I understand what problem you are trying to solve here, so let's try a strawman.

What would be wrong with a test that used something like [Feature:DisabledExecProbeTimeout] and ran against a cluster that had the ExecProbeTimeout feature gate turned off (and would fail if the gate was on)?

To me this fits the "a test has non-default requirements to run" definition of [Feature:]

One issue with this is there are examples when Feature tag just stayed after feature gate was gone. Having explicit difference between not fully implemented and not running everywhere makes it easier to keep consistency in test definitions. Some treat Feature tag as "I implemented this feature and called it like this. Kubernetes has this feature so I keep the tag here".

I'm not sure I understand this problem either.

From my perspective we already do this today for [Conformance] tests by setting the feature gates AllAlpha and AllBeta to false:

• https://testgrid.k8s.io/sig-release-master-blocking#conformance-ga-only
• https://github.com/kubernetes/test-infra/blob/13e0036ab3dc00813758b2be3699056add81b478/config/jobs/kubernetes/sig-testing/conformance-e2e.yaml#L143-L184
• https://github.com/kubernetes-sigs/kind/blob/7c843a264b854942f9c18211d82d1bf79d4ca3ed/hack/ci/e2e-k8s.sh#L132

So I'm trying to understand how this is a labeling issue. What other subset of tests would you want to be able to run against a cluster setup this way?

agree with that, but we also have things like this https://github.com/kubernetes/test-infra/blob/4d7b0d15c5b7551ecc9d0a23ddf1a5a9c696de1f/config/jobs/kubernetes/sig-gcp/sig-gcp-gce-config.yaml#L209-L232

        - --test_args=--ginkgo.focus=\[Feature:(GRPCContainerProbe|InPlacePodVerticalScaling|ProbeTerminationGracePeriod|APIServerTracing|APISelfSubjectReview|StorageVersionAPI|PodPreset|StatefulSetMinReadySeconds|ProxyTerminatingEndpoints|NodeOutOfServiceVolumeDetach|RetroactiveDefaultStorageClass)\]|Networking --ginkgo.skip=\[Feature:(SCTPConnectivity|Volumes|Networking-Performance)\]|IPv6|csi-hostpath-v0 --minStartupPods=8

and that list has not been updated for a long time 🙃

but we also have things like this

yeah exactly replacing all that with --ginkgo.focus=\[Alpha\] makes complete sense to me

great, it seems this part has general agreement

I don't understand what degraded means in this context?

I interpret the example as "working as intended".. you removed the [Feature:] tag because the functionality is now default / enabled out of the box aka the test no longer "has non-default requirements to run or targets some non-core functionality". You can still include/exclude these tests by selecting the text in the Describe, e.g. Hostname of Pod

I'm missing context here... since it has "non-default requirements" why is it not marked as a [Feature:]?

because things are pre-installed on our test machines in OSS. And we were filtering them out in other environments based on name.

I don't understand what value [FeatureGate:] adds that makes it worth being distinct from [Feature:].

It seems like it's going add more boilerplate ([Feature:foo] tests become [Feature:foo][FeatureGate:foo] tests)

Meanwhile I'm having a tough time coming up with examples of why I would want to test [FeatureGate:foo] independent of [Feature:foo]

Separately, [Alpha], [Beta] and [Deprecated] seem like worthwhile additions

@spiffxp the problem i see with [Feature:] is that is misleading for developers, see current status:

      1 [Feature:CloudProvider][Disruptive]
      1 [Feature:ClusterSizeAutoscalingScaleUp]
      1 [Feature:CustomMetricsAutoscaling]
      1 [Feature:GPUDevicePlugin]
      1 [Feature:HPA]
      1 [Feature:NEG]
      1 [Feature:NodeOutOfServiceVolumeDetach]
      1 [Feature:PodGarbageCollector]
      1 [Feature:StatefulSet]
      1 [Feature:WindowsHostProcessContainers]
      2 [Feature:HPA]
      2 [Feature:NoSNAT]
      2 [Feature:Windows]
      3 [Feature:Flexvolumes]
      3 [Feature:vsphere]
      3 [Feature:Windows]
      4 [Feature:NetworkPolicy]
      6 [Feature:Windows]

KEP suggests to start implementing by adding [FeatureGate:Foo] and stage labels to tests. Then clean up other things. If we want to split addition and later clean up, I'm happy with this too

I'm not sold on [Environment] because it feels way too generic, compared the examples you're giving which are all about a given machine's configuration. Something like [NodeConfig:] instead?

I think I'm also trying to understand what sets of tests you would want to group together with these that you cannot today.

this tag is mostly to filter tests out. Like run everything that doesn't require non-standard environment. Instead of filtering out [Feature:Performance]

So if we introduced the [Alpha] and [Beta] labels, removing [Beta] from a feature would likely be the time to remove the [Feature:] tag entirely, no?

The problem is that we are not using the tag correctly, but if fFeature == FeatureGate I agree that after betas it has to be removed entirely

what @aojea said. Since we mixing up feature and featuregate, we had examples when tests keep being called Feature even though feature gate was not existing any longer and the functionality is almost a conformance. Explicit FeatureGate thing will ensure that we will remove the FeatureGate from that test and decide whether it's optional feature or conformance at this point