WIP: KEP-3920 finish feature flags KEP

[lavalamp]

• One-line PR description:

• Issue link:

• Other comments:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp

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:

• keps/sig-api-machinery/OWNERS [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tallclair]

Would this (eventually) obviate the need for storage version migrator?

[lavalamp]

Probably not, but this would provide a convenient place to trigger it, or wait for it to complete.

[johnbelamaric]

Great idea! A few thoughts.

[johnbelamaric]

It would help to give a short phrase hinting at what is to come, otherwise the stages don't make sense until a second reading. So, something like:

...with the current system, especially with respect to runtime discovery, enablement and disablement of features. This KEP solves...

[lavalamp]

The list of problems is long, I want people to actually read it. Would "read on for the list of problems and then the list of solutions" be better?

[johnbelamaric]

The problem is I have no idea what you are talking about and then you start talking about "stages" as if I should know why there should be stages, and the meaning of the contents of those stages.

I think people will still read it, you just need a hint as to where you are going - the actual problems, IMO.

[lavalamp]

I will make changes.

[lavalamp]

look again?

[johnbelamaric]

It would be a lot easier to understand if you discussed the roles that will be interacting with the Feature API - platform admin, cluster admin, feature developer, feature consumer (user), others? - and then explained the uses for those different roles.

As it reads now, I feel like I am missing context, at least on the first read.

[lavalamp]

I was trying to do that and introduce them one at a time, but I can uh give a list or something?

[johnbelamaric]

Yeah, something to help people build a mental model or frame it in context. Even if you define them one at a time, that could be OK, but you need to explain who they are when you introduce them, not leave it as implicit. I just felt dropped in the middle of a thought process as it is now.

[lavalamp]

I pushed some changes, is this better?

[johnbelamaric]

So the client is a binary, and using this to read its own command line for feature gate values? Or the client is the author of a feature that is included in the binary?

[lavalamp]

The former, I will clarify. The latter is a dev and devs write code which is a client (both today and in the future) and in the future may also write code for the server flow.

[johnbelamaric]

Yeah, sorry, I wasn't clear either, for the latter I meant: "the code embedded in the binary, checking if the feature is on in order to do it's thing or not".

[johnbelamaric]

Given that one binary supports many features, the parsing of the command line is code written only once by the binary author (or maybe automatic in a lib), whereas checking the feature gate is code every feature author will need to write. So, trying to understand which flow this is, and why we start with it. I would think we would start with how feature gates are consumed by feature authors in their feature code; that's the most common use.

[lavalamp]

The way it works today is that there's a FeatureGate implementation already written for people, the binary declares it statically and installs its flags in the command line parser, and feature authors call Enabled(f) for each feature f. The only thing in this story that will change is where the FeatureGate implementation lives. This is all part of the client flow, it seems like you're asking which half of this I'm talking about, but I'm talking about all of it.

[lavalamp]

Does the description of "clients" I added above fix this or is it still unclear?

[johnbelamaric]

I love that this can be used by third parties, that is really nice. What roles will have write access to the Feature API?

As I read this, it sounds like you are saying: anyone (with the right permissions) can add features in a namespace, and then this allows third parties to use this feature API in their own namespace to manage features of their component. Upstream k8s components will put their features in the kube-system namespace.

That's a reasonable approach. However, here's a crazy thought. Could we instead create a ClusterFeature and Feature, ala ClusterRole and Role, and allow features to be enabled and disabled on a per-namespace basis?

Whether it is an upstream feature or a third-party feature would just be an issue of the name of the Feature, and namespace would be irrelevant. On a per-namespace basis, a feature could be used to override the global state (if allowed). Something like:

ClusterFeature:

• on: on cluster wide, cannot be overridden per namespace
• default-on: on cluster wide, can be overridden per namespace
• off: off cluster wide, cannot be overridden per namespace
• default-off: off cluster wide, can be overridden per namespace

Feature of the same name can then be defined in a namespace as on/off, and of course status will reflect the composition of that and ClusterFeature.

If the ClusterFeature does not exist, then it is assumed to be "default-off". This allows third-parties to manage their namespaced features without a cluster or platform admin adding the ClusterFeature.

[johnbelamaric]

Obviously for some features, only cluster-wide makes sense. But this could be a nice way to safely try features that only have impact on individual resources or can otherwise be namespace scoped.

[lavalamp]

I do want to add canarying capabilities to this eventually, but not all features can be on/off on a namespace basis. I was going to try and avoid discussing how to make canarying work until we can agree on the rest of it :)

I was already planning on using namespaces to permission different classes of features differently, I don't think I should also use them for canarying in this way-- I have other plans that I'd like to keep vague until later :)

[johnbelamaric]

Ok, sounds good. I do think it's a step beyond "canarying", in the sense that namespaced feature enablement actually enables better multi-tenancy in the cluster (one less thing everyone has to agree on). I think what I describe above enables the same functionality the use of namespaces in the KEP does, plus more. But perhaps it conflicts with your thinking on the canarying, I look forward to seeing that.

[johnbelamaric]

One other possible use case for this is around discovering whether a particular workload can run on a given cluster. It would be great if we could inspect a set of manifests, and determine if the K8s cluster in question has the right features enabled to allow it to be successfully deployed. For example, does the cluster support Gateway API? RBAC? Metrics? Multus? Making that discoverable would be super useful in a lot of contexts.

This goes a bit beyond "features" though into "capabilities", so maybe it's a different but related API for a different KEP. But I think both have a lot of overlap so it's worth exploring if the same API could satisfy both.

[lavalamp]

discovering whether a particular workload can run on a given cluster

Yes, that's intended but I forgot to mention, will add.

[pacoxu]

/assign

[pacoxu]

Not sure if this is also a motivation case that we find a user with 3 apiservers but different configurations.

• PodTolerationRestriction: ignore duplicates when changing pod toleration  kubernetes#111712 is trying to fix a bug that the user created a daemonset when featuregate(an admission controller in that case) is disabled and failed to update it after enabling the feature.

This is similar to third state, off-with-dangling-references, and we may call it create-when-disabled-and-update-when-enabled or create-when-enabled-and-update-when-disabled.

[pacoxu]

Another case is kubelet feature gates that is not enabled but apiserver has enabled the FG. For such instances, should this KEP cover?

kubelet configuration is exposed to configz and this includes the --feature-gates part so this is stable.

[root@paco-centos-9 ~]# curl -k --key /etc/kubernetes/pki/apiserver-kubelet-client.key --cert /etc/kubernetes/pki/apiserver-kubelet-client.crt https://127.0.0.1:10250/configz | jq .kubeletconfig.featureGates

{
  "LocalStorageCapacityIsolationFSQuotaMonitoring": true,
  "NodeSwap": false
}

[lavalamp]

The case of apiservers with differing configurations is a great consideration, I will add text explaining what happens (basically, the leader determines which state the cluster is "trying" to get into, but the uses--the client flow--prevent it from actually getting into that state until all apiservers are put into the same condition).

[lavalamp]

Added, should cover both cases now.

[wojtek-t]

I think we would benefit a lot if you would actually provide the API structure here.

What I'm wondering about is how we represent the state (and how we generally track it) if the feature-gate is used by multiple components (let's say apiserver, scheduler and kubelets). Then technically the feature is on only when all of them are actually enabled.
Also - what we're doing with cases with 1 (or small percent) of kuebelets (or any other node components) are not changing their flag?

[pacoxu]

So we can know the status of the FGs by kubectl get features.

• 【draft】kubectl: add command show-feature-gates to get fg status from /metrics kubernetes#116887 is not needed then.
• Generate feature gates list from data website#25645: a proposal to add a history of feature-gate feed in JSON/table like the CVE feed. This may help to do some cleanup or check.

[lavalamp]

I was going to save the API type for the detailed design section, which I haven't written yet. But you can look at the prototype here. There's some stuff about it in the prototype that is wrong and needs to change, and there's other stuff that is right but counterintuitive, so reader beware, it will take some mental effort to tell the difference if you follow that link :)

[pohly]

Sometimes a feature depends on a specific new API group. For example, Dynamic Resource Allocation comes with resource.k8s.io/v1alpha2.

I don't see adding or removing API groups mentioned in the document (but I may have missed it). Can this be added?

Along the same line, removing fields that are feature-gated is mentioned. What would be useful is removal of entire objects that make no sense when a feature is off.

[lavalamp]

I don't see adding or removing API groups mentioned in the document (but I may have missed it). Can this be added?

and

Along the same line, removing fields that are feature-gated is mentioned. What would be useful is removal of entire objects that make no sense when a feature is off.

This is basically the same request, since groups and resources are added in the same startup mechanism. The way this can work is if the feature author writes the code to only add the resource and/or group if a feature flag is on.

I didn't include this as a story since it's possible (and already done?) today, but I can add that.

[pohly]

The way this can work is if the feature author writes the code to only add the resource and/or group if a feature flag is on.

But that is not how it is currently done, is it? At the moment, the admin needs to enable API groups independently from feature gates. kube-apiserver has two different parameters for this (--runtime-config and --feature-gates).

My first point wasn't about adding or removing the type definitions. It was about removing the objects from the etcd stored before removing the type. But I guess the part about "remove fields" also applies to that, it's just not mentioned.

[liggitt]

nit: this number comes from an issue, not the initial PR #, to give a stable link destination

[lavalamp]

PR and issue numbers are from the same sequence, I don't think it matters?

[liggitt]

I'm pretty sure the KEP number is supposed to be the issue that tracks the feature through its lifetime and gets updated release to release. Whatever number this uses will be what https://kep.k8s.io/3920 links to, which you probably want to be the issue, not the first PR to touch the design

[lavalamp]

They will link to each other so I don't see why it matters, I never want to look at that issue, what I want to look at is the text of the KEP anyway

[liggitt]

but... this PR will be an increasingly stale snapshot of the initial draft of the text :)

the issue description can link right to the merged current state

[liggitt]

It's a little hard to tell if this is just surfacing information, or proposing modifying how we default feature enablement based on that information.

I'm very interested in improving how we deal with scenarios like this:

• a control plane with all nodes on 1.x+ enables features considered stable enough to default on in 1.x (up-to-date clusters get new features asap)
• a control plane with nodes older than 1.x avoids enabling features that require 1.x nodes (slow-moving clusters prioritize safety at the expense of feature velocity)
• a control plane with enabled features that require 1.x nodes handles an attempt to add a node older than 1.x by _____ (rejecting the node? disabling the feature? something else?)
• a control plane with nodes older than 1.x indicates it is not safe to upgrade the control plane beyond supported node/control-plane skew

[lavalamp]

So far this KEP describes surfacing information and the enablement/disablement flow. I need to add the version change flow also.

I think you are asking for some code to run to automatically change a feature state in a direction desired by devs if no preference has been expressed and it is safe to do so, looking at the rest of the cluster. I have not said anything about that in the KEP yet. I think that is possible and desirable but I need to write the version change flow down first.

I think the main requirement from this is that it should be possible to tell if a value is the default, was set on the command line, or was set dynamically (if permitted).

Also if there are multiple such default-changes-due-to-newly-found-safe-conditions, they should not all happen at the same time. But the issue is, what if it breaks the cluster? that will take some designing to get right.

[lavalamp]

Adding a follow-up goal.

[lavalamp]

The way this can work is if the feature author writes the code to only add the resource and/or group if a feature flag is on.

But that is not how it is currently done, is it? At the moment, the admin needs to enable API groups independently from feature gates. kube-apiserver has two different parameters for this (--runtime-config and --feature-gates).

Yeah for the feature flag to be a useful way to turn things on and off, the author would have to add the group by default, or the admin has to configure it and the binary needs to not crash when the group is requested but the flag is off. This is possibly different than what we do today. I don't think it will be hard to persuade people to do it differently once feature flags are actually useful. @deads2k at least partially disagrees with me.

My first point wasn't about adding or removing the type definitions. It was about removing the objects from the etcd stored before removing the type. But I guess the part about "remove fields" also applies to that, it's just not mentioned.

Yeah, removing objects when the collection goes away (or even changes alpha to beta) should be in here somewhere, I'll double check.

[k8s-ci-robot]

@lavalamp: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-enhancements-verify	cc13a7d	link	true	/test pull-enhancements-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[lavalamp]

I have added API details & test plan, PTAL

[wojtek-t]

This makes sense as long as we have a mechanism that cleans it up eventually (say 3 releases later or sth like that).
Is your plan to actually delete it eventually? What will be the mechanism for doing this?

[wojtek-t]

If it should match metadata.namespace, then what's the point of having it?

[wojtek-t]

What exactly this means if feature implementation is spread across multiple components (in particular control-plane and nodes) and these are not in the same version?

Also - why this is actually needed?

[wojtek-t]

What is the condition here? I don't see conditions in this API..

[wojtek-t]

I think I'm still not fully following how this field is supposed to be used.

As an example - if the feature is implemented in Kubelet, then how it will be represented here?
(1) a single "kubelet" entry? - then how do we distintgush e.g. nodes in different versions?
(2) per-kubelet entry - then it doesn't really scale to clusters with 5k nodes or sth...
(3) something else?

[And for "doesn't really scale" - I'm not even saying about the write load on this object [which you have some ideas, although I don't yet really follow it], but even the base object size that can become problematic and not fit our "max object size" limits...

[pacoxu]

BTW, https://openfeature.dev/ the openfeature is a CNCF project. Do we have a plan to use it in k/k?

[sftim]

What will it take to move this out of WIP?

[sftim]

Try:

# KEP-XXX: Improved feature gate handling

The proposal is to make an improvement; finishing the implementation is implied in every proposal we accept.

[sftim]

How will this work for feature gates that affect leader election? (might be an unresolved question)

[sftim]

Let's detail exactly how this would work and any important corner cases. Even for alpha.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.