-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add a doc on how we work with feature gates #5545
[WIP] Add a doc on how we work with feature gates #5545
Conversation
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
/approve cancel |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
## Other scenarios | ||
|
||
Sometimes we use`{Default: true, PreRelease: featuregate.Beta}` for keeping legacy behavior (see `LegacyNodeRoleBehavior`) on and when we basically want to drop the legacy behavior with new default behavior. So at GA we will end up with `{Default: false, PreRelease: featuregate.GA, LockToDefault: true}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment, there might be a typo or we might want to elaborate more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LockToDefault=true, means the default value is false and cannot be set to true here.
Only this case: kubernetes/kubernetes#97543 as designed.
## Deprecation | ||
|
||
* `PreRelease` is set to `featuregate.Deprecated` | ||
* `Default` is always set to `true` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this depend on the current state (alpha/beta)?
|
||
## Lifecycle | ||
|
||
Feature progress through `Alpha` -> `Beta` -> `GA`. Sometimes we end up deciding that a feature is not going to be supported and we end up marking them as `Deprecated`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth indicating that there is also a final state when the feature gates disappear and the feature became part of the product
It will be great to link a reference implementation or even better a package providing support for feature gates across the board (meanings re-usable also in Kubernetes-sigs projects) |
|
||
Feature progress through `Alpha` -> `Beta` -> `GA`. Sometimes we end up deciding that a feature is not going to be supported and we end up marking them as `Deprecated`. | ||
|
||
When we add a feature flag, we basically add if/else conditions to ensure that a feature is ONLY activated when either the default is on or if the deployer has switched it on explicitly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a link to https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions (point 3) since it explains the why behind using feature gates (to ensure no data loss on upgrades)?
|
||
This enables the feature to be on and available in the default installation of kubernetes. | ||
|
||
Sometimes (rarely) the `Default` is set to `false`. For example when this flag needs some explicit action or support outside of Kubernetes. This tells folks that while this feature is Beta, you still need to do some work to switch it on and use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we could add an example here. Maybe the CSIMigration feature gates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding more comments if it is locked to false. It is really weried for beginners.
|
||
Typically we add a comment `// remove in 1.23` to signal when we would entirely remove the feature gate. Remember when the feature gate is removed and the deployer has forgotten to drop the reference to the feature in the CLI flags (say the `kube-apiserver`), then they will see a hard failure. | ||
|
||
Also note that when we set `LockToDefault` to `true`, we remove all references (if/then conditions) to the feature gate from the codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a note that codebase
here refers to the kube_features.go
files?
edit: codebase here doesn't really refer to kube_features.go
but it'd be good to add a pointer to kube_features.go
elsewhere in this doc so folks know where to look for feature gates :)
@fabriziopandini did you mean being able to import the feature gates in If it's the latter, the implementation is defined in component-base, so it can be reused today - https://github.com/kubernetes/component-base/blob/master/featuregate/feature_gate.go |
Great! let's link this in the doc! |
Co-authored-by: Elana Hashman <ehashman@users.noreply.github.com>
Co-authored-by: Elana Hashman <ehashman@users.noreply.github.com>
Co-authored-by: Elana Hashman <ehashman@users.noreply.github.com>
closing in favor of #5630 thanks @kikisdeliveryservice |
Closing this for real. 😛 Thanks for picking this up, @kikisdeliveryservice! |
Signed-off-by: Davanum Srinivas davanum@gmail.com
Which issue(s) this PR fixes:
Fixes #5542