Cluster Feature Gate in etcd

[siyuanfoundation]

• One-line PR description: Cluster Feature Gate in etcd

• Issue link: Cluster Feature Gate in etcd #4647

• Other comments:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: siyuanfoundation
Once this PR has been reviewed and has the lgtm label, please assign wenjiaswe for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/sig-etcd/OWNERS

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

[stackbaek]

Kubernetes means kube-apiserver right? (just to see if there's any other clients on k8s that reads from etcd)

[siyuanfoundation]

correct.

[stackbaek]

I think it's also worthwhile to highlight the benefit of backporting. Since proto3 fields are optional (and unknown fields are ignored), we want to provide rationale for backporting this change.

[siyuanfoundation]

actually because we are introducing a new raft message type, we have to backport. Changed the paragraph.

[stackbaek]

Just to clarify, we want to keep 2 implementations (if there's delta), not 2 previous minor versions (3 implementations), correct?

Also I wonder if we can make it easier for developers to make sure they keep the implementation. I am thinking some interface such as (very roughly):

type FeatureGateRunner struct {
  featuregate.Feature
  Handlers map[Version]func() error
}

func (r FeatureGateRunner) HandleAtVersion(v Version) error {
  return r.Handlers[v]()
}

[siyuanfoundation]

I don't think the additional interface is going to be beneficial. The main benefits of feature gate is it is easy to use, insert the following whereever I want to add new codes.

if featureGate.Enabled(feature) {
  // my fancy new codes
}

With the wrapper, it is much cumbersome to use and remove in the future.

I would expect change of feature direction to happen rarely. It is fine for it to be a little bit harder to keep multiple implementations.

[serathius]

Agree with @siyuanfoundation, let's not overdesign the interface until we need it.

[humblec]

Is there a possibility of converting or migrating a server level feature to cluster level feature in later releases?

[serathius]

Migration would mean we remove it from server level feature gate, which means a breaking change in cli.
So default answer should be no. We might still consider to have a process for migrating by deprecating a server feature and creating a similarly named cluster level feature, but we don't need to discuss it the KEP.

[humblec]

nit: a space in between ``feature-gates` flag...

[henrybear327]

Would there be a use case where a specific feature requires all the nodes to enable/disable it at the same time in order to function correctly? (thus we have to block inconsistent nodes from joining)

[siyuanfoundation]

There are cases like that. That's why we need the consensus algorithm to set the final values of feature gates. It should not block inconsistent nodes from joining, the leader will decide what value to use.

[serathius]

Don't understand this section. Can you clarify what backport you mean? Of the proposed mechanism or a feature gate.

[serathius]

Data compatibility should be handled by cluster version, not feature gates. Don't understand why we touch it in this KEP.

[serathius]

Please start from describing how features are enabled on command line. Does cluster feature gates share the same command line as server feature, I don't think they should. We should clarify this at the top.

[serathius]

Also we should clarify one important thing, server level feature represent stability of the feature and can still have a separate enablement. For cluster level features we focus on consistent enablement. Both share the same name "feature gate", but gating motivation is different.

[serathius]

What about using the grpc metadata to control non-API semantics?

[serathius]

Please discuss the process in a top down approach. I don't care about the proto details if I don't know how you plan to use it. Suggested order of topics:

• Properties that feature enablement needs
• How we plan to guarantee those properties
• Detail of the process.
• How proto will look.

[serathius]

Ups, I think the description of this user story is not fully relevant, sorry. What I described described should be solved using cluster version. 

Suggested change

	#### Story 1
	A developer is adding a new feature of a new write type. Because it requires a consensus write, the request should be rejected if the feature is not enabled or the request can be processed on all cluster members.
	Today, each change in etcd API, requires development spenning at least 2 minor versions. First a raft proto change needs to be added and wait a release before it can be used. Only a release later it can be actually used to prevent a incompatible proto being used during upgrade/downgrade process.
	With cluster level feature gate, a server would know if a feature is enabled on all members, and decide whether or not to accept the new write request.
	#### Story 1: Developing Features Impacting the Apply Loop
	A developer is adding a new feature that modifies the etcd apply loop. This change could impact how data is processed and replicated across the cluster. An example of such a feature is [persisted lease checkpoints](https://github.com/etcd-io/etcd/pull/13508). Currently, enabling such a feature requires careful coordination and potentially taking the entire cluster down to ensure all members apply the changes consistently. This lack of flexibility restricts development and can lead to operational disruptions.
	With cluster-wide feature enablement, we can enable or disable such features in a controlled manner, ensuring consistent behavior across all members during a rolling upgrade, while allowing users to enable/disable the feature as they wish. This empowers developers to iterate more quickly and safely, while also providing operators with greater flexibility to manage feature rollouts and upgrades without disrupting service.

[serathius]

Might be worth to node that the FeatureSupportChecker checks only member version at the start, and assumes that cluster will never stop supporting the feature. Which is gross oversimplification.

[serathius]

Would be nice to give some background why users would like to disable a feature. Let's say that after some qualification a feature was graduated from experimental and enabled by default in new release. However, it quickly turned out that the feature is bugged. Users would like to disable it ASAP to protect their production.

[serathius]

Would be good to note that it's impossible for etcd contributors to ensure that feature maintains correctness during the undefined state. Automatic testing would not be able to cover all possible edge cases in distributed systems, it's even too much for robustness tests.

[serathius]

I think this points is a little unclear. I think it's about fact that order of operations in distributed system can be hard to predict, as they can be arbitrarily re-ordered by leader, which can also be arbitrarily changed. In short, you cannot check if feature is enabled and then assume it will still be enabled when you send a following request.

[serathius]

I would state the problem differently, there are two alternative approaches:

• configure cluster feature gates globally (either on cluster initialization or via command line flags) and reject members reconnecting with different feature flags.
• configure cluster feature gates based on member proposed feature gates.

I stand for the second option as it allows us to maintain same way of cluster configuration. User just changes the etcd manifests/config file. The first option would cause user changing manifests to cause unexpected crashloop.

The only downside of the second option is that it requires member restart which might be to slow for some critical bugs, however I would be careful with allowing clients to etcd configuration from etcd endpoints. It's opens a security pandora box.

[serathius]

Not sure I understand the risk, it should be mitigated by not using the new proto if the ClusterVersion is low.

[serathius]

Can you cover a broader issue of how how the feature gates will work with downgrade/upgrade? What happens if cluster is being downgraded to version that doesn't support a feature gate? The solution should be the same, using Cluster Version.

Also do we even plan to remove feature gates? In Kubernetes we remove feature gates couple of releases after they go to GA and become default.

[serathius]

I would prefer to separate configuration of server and cluster features in different command line flag. The reason is user expectation. Users expect consistent behavior of flags, are they enabled immediately or they require to reconfigure the whole cluster. They shouldn't need to check the type of flag.

What do you think?

[serathius]

What about livecycle, for server level feature gate we expect they will go to GA and be enabled by default, but sometimes we would like to still keep the option to disable them for users. For example a if we were developing a feature like strict-reconfig-check using cluster feature gate. We still left an option for user to disable it.

I'm thinking that we might want to use different separation for feature development, instead of having "cluster feature gates" and "server feature gates" I would propose to use:

• "Feature gate" - tracks lifecycle of a feature across etcd releases (alpha,beta, GA, deprecated), defines it default enablement (true or false).
• "Cluster feature" - mechanism used by some features (can be gated with feature flag, but doesn't need to) to ensure it's consistent enablement across cluster.

[serathius]

As discussed before, we should avoid "feature_gates" as name as it implies that those are enabled features gates. They are not, that's the feature gates that member that member is proposing to enable. Recommended name "proposed_features"

[serathius]

That doesn't clarify how we can maintain cluster level flags during lifecycle of the cluster, member restarting with different flags. After member is elected is definitely not a good place to do that. Please clarify when exactly this is code is executed. I see two options:

• [preferred] On leaders apply loop when executing member set attribute.
• Periodic execution by leader like in case of cluster version

[serathius]

If cluster flags should be changed leader sends a proposal ClusterFeatureGateSetRequest through raft.

[serathius]

Suggested change

	Feature proposed_feature_gates = 3;
	repeated Feature proposed_feature_gates = 3;

?

[serathius]

Suggested change

	Feature feature_gates = 1;
	repeated Feature feature_gates = 1;

[serathius]

Don't agree with that, we should not depend on local defaults of leader. I would reserve the right to change to backport disabling the feature by default into a patch version. I imagine a situation that a feature is graduated to early to beta and made default in new minor version. If the feature is bugged we should be able to change default enablement in patch version. Might be a good user story to add.

[serathius]

I think we should consider reconciling cluster version and it's features in one proto to ensure they are consistent with each other.

[serathius]

I think we should decision about feature enablement from cluster version. If we implement Feature Implementation Change Risks with option for older cluster version it should be compatible.