Skip to content
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

KEP-4578: Server Feature Gate in etcd #4610

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

siyuanfoundation
Copy link
Contributor

  • One-line PR description: KEP to introduce feature gate in etcd
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 2, 2024
@k8s-ci-robot k8s-ci-robot requested a review from ahrtr May 2, 2024 00:39
@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label May 2, 2024
@k8s-ci-robot k8s-ci-robot requested a review from jmhbnz May 2, 2024 00:39
@k8s-ci-robot k8s-ci-robot added sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 2, 2024
@siyuanfoundation
Copy link
Contributor Author

/sig etcd

cc @serathius @ahrtr @logicalhan @jmhbnz @fuweid @stackbaek @henrybear327 @wenjiaswe

@siyuanfoundation siyuanfoundation force-pushed the etcd branch 8 times, most recently from 5e3a2db to 9e23e71 Compare May 3, 2024 16:08
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this started team. We have already had some discussions on this at KubeCon etc so overall intention makes sense to me.

Two minor questions below from my first read.

keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/kep.yaml Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
@siyuanfoundation siyuanfoundation force-pushed the etcd branch 6 times, most recently from f229b14 to e621249 Compare May 9, 2024 21:33
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
keps/sig-etcd/4578-feature-gate/README.md Outdated Show resolved Hide resolved
@siyuanfoundation siyuanfoundation force-pushed the etcd branch 3 times, most recently from d281e40 to 161b03d Compare June 6, 2024 00:15
* the new feature would need to be enabled by default to always apply the bug fix for new releases.
* it changes the API which is not desirable in patch version releases.

The proper way of handling these bug fixes should be:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, but we should get some feedback from K8s folks. cc @wojtek-t @logicalhan

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2024
@serathius
Copy link
Contributor

Some small nits, but overall proposal looks good to me.

Comment on lines +12 to +13
- "@jmhbnz"
- "@wenjiaswe"
- "@fuweid"
- "@logicalhan"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of reviewers, no need to have so many people, but if you want to keep it please make sure that you got review from everyone listed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I'll try to get their reviews.

@wenjiaswe
Copy link

/lgtm thanks!

@siyuanfoundation siyuanfoundation requested review from ahrtr and jmhbnz June 7, 2024 04:18
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
Great work team.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmhbnz, serathius, siyuanfoundation

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 808e248 into kubernetes:master Jun 8, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone Jun 8, 2024
Comment on lines +242 to +243
| Alpha | Same as [Kubernetes feature stages - Alpha feature](https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-stages): <ul><li>Disabled by default. </li><li>Might be buggy. Enabling the feature may expose bugs. </li><li>Support for feature may be dropped at any time without notice. </li><li>The API may change in incompatible ways in a later software release without notice. </li><li>Recommended for use only in short-lived testing clusters, due to increased risk of bugs and lack of long-term support.</li></ul> | Before moving a feature to Beta, it should have <ul><li> Full unit/integration/e2e/robustness test coverage.</li><li>Full performance benchmark/test if applicable.</li><li> No significant changes for at least 1 minor release.</li></ul> |
| Beta | Same as [Kubernetes feature stages - Beta feature](https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-stages) except etcd Beta feature does not allow incompatible schema change: <ul><li>Enabled by default. </li><li>The feature is well tested. Enabling the feature is considered safe.</li><li>Support for the overall feature will not be dropped, though details may change.</li><li>Recommended for only non-business-critical uses because of potential for discovering new hard-to-spot bugs through wider adoption.</li></ul> | Before moving a feature to GA, it should have <ul><li> Widespread usage.</li><li>No bug reported for at least 1 minor release.</li></ul> |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment:

  • For Alpha -> Beta, should add Criteria: No unresolved major bugs.
  • For Beta -> GA, update criteria: No major bug reported for at least 1 minor release

Note they are just minimal Graduation Criteria.

@ahrtr
Copy link
Member

ahrtr commented Jun 8, 2024

Overall looks good to me. Thanks @siyuanfoundation for driving the effort.

@fuweid
Copy link

fuweid commented Jun 10, 2024

LGTM. Thanks @siyuanfoundation !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.