featuregate: adds EtcdServer.FeatureEnabled interface. #18062

stackbaek · 2024-05-23T16:35:53Z

The interface can be used throughout the etcd server binary to check if the feature is enabled or not.

Related task: #18047

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

k8s-ci-robot · 2024-05-23T16:36:01Z

Hi @stackbaek. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

stackbaek · 2024-05-23T16:39:34Z

/assign @siyuanfoundation

pkg/featuregate/featuregate.go

stackbaek · 2024-05-24T05:52:22Z

per conversation with @siyuanfoundation - I imported k8s.io/component-base/featuregate instead of creating a new featuregate within etcd.

henrybear327 · 2024-05-24T07:28:32Z

@stackbaek Don't forget to signoff your commit with git rebase HEAD~ --signoff!

siyuanfoundation · 2024-05-24T16:11:16Z

LGTM, Thanks @stackbaek!

tjungblu · 2024-05-27T07:26:06Z

are we going to have issues with cyclic imports with k8s then?

	k8s.io/apimachinery v0.30.1 // indirect
	k8s.io/component-base v0.30.1 // indirect

siyuanfoundation · 2024-05-28T19:04:13Z

are we going to have issues with cyclic imports with k8s then?
	k8s.io/apimachinery v0.30.1 // indirect
	k8s.io/component-base v0.30.1 // indirect

neither apimachinery nor component-base depend on etcd. I don't see there is any cyclic dependency.

stackbaek · 2024-06-08T15:59:07Z

Updated commit to copy component-base FeatureGate into etcd.

henrybear327 · 2024-06-08T17:45:22Z

/ok-to-test

go.mod

server/internal/pkg/featuregate/feature_gate.go

codecov-commenter · 2024-06-14T06:44:51Z

Codecov Report

Attention: Patch coverage is 81.38298% with 35 lines in your changes missing coverage. Please review.

Project coverage is 68.92%. Comparing base (8a0054f) to head (6fa2ee7).
Report is 26 commits behind head on main.

❗ Current head 6fa2ee7 differs from pull request most recent head 14e15bc

Please upload reports for the commit 14e15bc to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
server/config/config.go	`79.76% <ø> (ø)`
server/storage/mvcc/key_index.go	`64.16% <ø> (ø)`
server/storage/wal/metrics.go	`100.00% <100.00%> (ø)`
server/etcdserver/server.go	`81.60% <0.00%> (-0.65%)`	⬇️
server/storage/mvcc/revision.go	`91.48% <80.00%> (-3.39%)`	⬇️
etcdutl/snapshot/v3_snapshot.go	`57.43% <64.00%> (+8.53%)`	⬆️
pkg/featuregate/feature_gate.go	`85.33% <85.33%> (ø)`

... and 21 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18062      +/-   ##
==========================================
- Coverage   68.99%   68.92%   -0.07%     
==========================================
  Files         416      417       +1     
  Lines       35127    35303     +176     
==========================================
+ Hits        24235    24333      +98     
- Misses       9503     9552      +49     
- Partials     1389     1418      +29

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a0054f...14e15bc. Read the comment docs.

siyuanfoundation · 2024-06-14T15:45:31Z

LGTM. Can you fix the verify error? Thank you!

The interface can be used throughout the etcd server binary to check if the feature is enabled or not. Note that this commit also copies necessary FeatureGate interface from k8s component-base. Signed-off-by: Baek <seungtackbaek@google.com>

We'll likely use most of the feature_gate package from component-base. Also this commit moves the pkg from server/internal/pkg to pkg/. Signed-off-by: Baek <seungtackbaek@google.com>

The removed packages are: * k8s.io/apimachinery/pkg/util/naming * k8s.io/klog/v2 Do note that removing naming package necessitates adding feature gate name argument to featuregate.New(). Signed-off-by: Baek <seungtackbaek@google.com>

stackbaek · 2024-06-16T04:25:07Z

/retest

pkg/featuregate/feature_gate.go

henrybear327

Thanks for implementing the foundation for the server-level feature gate @stackbaek!

I will implement my 2 TODOs :)

stackbaek · 2024-06-17T05:00:39Z

/retest

stackbaek · 2024-06-17T06:04:00Z

/retest

stackbaek · 2024-06-17T07:00:37Z

/retest

siyuanfoundation · 2024-06-17T16:59:03Z

/cc @ahrtr

ahrtr

Overall looks good with a couple minor comments.

Thanks.

ahrtr · 2024-06-18T09:28:38Z

pkg/featuregate/feature_gate.go

+// FeatureGate indicates whether a given feature is enabled or not
+type FeatureGate interface {


Suggested change

// FeatureGate indicates whether a given feature is enabled or not

type FeatureGate interface {

// FeaturesGate is an interface for managing all features. It provides functionality

// to query all known features and determine whether a specific feature is enabled.

type FeaturesGate interface {

Thanks for the review! While I do see FeaturesGate captures what it actually does (gates all features), the name FeatureGate is taken from k8s component-base and it seems to be used more widely (~177k match in github) than FeaturesGate (~270). If you don't feel strongly for this name, I do want to keep the name as FeatureGate. WDYT?

the name FeatureGate is taken from k8s component-base and it seems to be used more widely (~177k match in github)

Yes, I know it's copied from k8s component-base. I guess the ~177K match is jus because everyone (including this PR) just copied the name.

Feel free to keep it as it's since the naming has already been widely used & accepted.

ahrtr · 2024-06-18T09:29:38Z

server/config/config.go

@@ -207,6 +208,9 @@ type ServerConfig struct {

 	// ExperimentalLocalAddress is the local IP address to use when communicating with a peer.
 	ExperimentalLocalAddress string `json:"experimental-local-address"`
+
+	// ServerFeatureGate is a server level feature gate
+	ServerFeatureGate featuregate.FeatureGate


Suggested change

ServerFeatureGate featuregate.FeatureGate

ServerFeaturesGate featuregate.FeaturesGate

k8s-ci-robot added the needs-ok-to-test label May 23, 2024

k8s-ci-robot assigned siyuanfoundation May 23, 2024

stackbaek marked this pull request as draft May 23, 2024 16:58

k8s-ci-robot added the do-not-merge/work-in-progress label May 23, 2024

henrybear327 suggested changes May 23, 2024

View reviewed changes

pkg/featuregate/featuregate.go Outdated Show resolved Hide resolved

pkg/featuregate/featuregate.go Outdated Show resolved Hide resolved

stackbaek force-pushed the main branch from c285f93 to 297e509 Compare May 24, 2024 05:50

stackbaek force-pushed the main branch from 297e509 to 685b03a Compare June 8, 2024 15:58

stackbaek marked this pull request as ready for review June 8, 2024 16:37

k8s-ci-robot removed the do-not-merge/work-in-progress label Jun 8, 2024

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jun 8, 2024

stackbaek force-pushed the main branch from 580debe to 9a50108 Compare June 9, 2024 05:38

stackbaek commented Jun 9, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

stackbaek force-pushed the main branch from 9a50108 to 596a14f Compare June 9, 2024 05:40

siyuanfoundation reviewed Jun 11, 2024

View reviewed changes

server/internal/pkg/featuregate/feature_gate.go Outdated Show resolved Hide resolved

k8s-ci-robot added the size/XXL label Jun 13, 2024

stackbaek force-pushed the main branch from 758a68a to 841d15b Compare June 14, 2024 13:33

siyuanfoundation approved these changes Jun 14, 2024

View reviewed changes

siyuanfoundation requested a review from henrybear327 June 14, 2024 15:45

stackbaek force-pushed the main branch from 841d15b to a0fc39a Compare June 15, 2024 05:34

keabsb added 3 commits June 15, 2024 05:34

Adds all feature_gate from component-base.

60e3f45

We'll likely use most of the feature_gate package from component-base. Also this commit moves the pkg from server/internal/pkg to pkg/. Signed-off-by: Baek <seungtackbaek@google.com>

stackbaek force-pushed the main branch from a0fc39a to 14e15bc Compare June 15, 2024 05:35

serathius approved these changes Jun 15, 2024

View reviewed changes

henrybear327 reviewed Jun 16, 2024

View reviewed changes

pkg/featuregate/feature_gate.go Show resolved Hide resolved

henrybear327 approved these changes Jun 16, 2024

View reviewed changes

stackbaek closed this Jun 17, 2024

stackbaek reopened this Jun 17, 2024

k8s-ci-robot requested a review from ahrtr June 17, 2024 16:59

ahrtr approved these changes Jun 18, 2024

View reviewed changes

ahrtr merged commit a043da5 into etcd-io:main Jun 19, 2024
83 checks passed

siyuanfoundation mentioned this pull request Jun 28, 2024

Add server level feature gate #18023

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

featuregate: adds EtcdServer.FeatureEnabled interface. #18062

featuregate: adds EtcdServer.FeatureEnabled interface. #18062

stackbaek commented May 23, 2024 •

edited

Loading

k8s-ci-robot commented May 23, 2024

stackbaek commented May 23, 2024

stackbaek commented May 24, 2024

henrybear327 commented May 24, 2024

siyuanfoundation commented May 24, 2024

tjungblu commented May 27, 2024

siyuanfoundation commented May 28, 2024

stackbaek commented Jun 8, 2024

henrybear327 commented Jun 8, 2024

codecov-commenter commented Jun 14, 2024 •

edited

Loading

siyuanfoundation commented Jun 14, 2024

stackbaek commented Jun 16, 2024

henrybear327 left a comment

stackbaek commented Jun 17, 2024

stackbaek commented Jun 17, 2024

stackbaek commented Jun 17, 2024

siyuanfoundation commented Jun 17, 2024

ahrtr left a comment

ahrtr Jun 18, 2024

stackbaek Jun 19, 2024

ahrtr Jun 19, 2024

ahrtr Jun 18, 2024

		// FeatureGate indicates whether a given feature is enabled or not
		type FeatureGate interface {

-// FeatureGate indicates whether a given feature is enabled or not
-type FeatureGate interface {
+// FeaturesGate is an interface for managing all features. It provides functionality
+// to query all known features and determine whether a specific feature is enabled.
+type FeaturesGate interface {

	ServerFeatureGate featuregate.FeatureGate
	ServerFeaturesGate featuregate.FeaturesGate

featuregate: adds EtcdServer.FeatureEnabled interface. #18062

featuregate: adds EtcdServer.FeatureEnabled interface. #18062

Conversation

stackbaek commented May 23, 2024 • edited Loading

k8s-ci-robot commented May 23, 2024

stackbaek commented May 23, 2024

stackbaek commented May 24, 2024

henrybear327 commented May 24, 2024

siyuanfoundation commented May 24, 2024

tjungblu commented May 27, 2024

siyuanfoundation commented May 28, 2024

stackbaek commented Jun 8, 2024

henrybear327 commented Jun 8, 2024

codecov-commenter commented Jun 14, 2024 • edited Loading

Codecov Report

siyuanfoundation commented Jun 14, 2024

stackbaek commented Jun 16, 2024

henrybear327 left a comment

Choose a reason for hiding this comment

stackbaek commented Jun 17, 2024

stackbaek commented Jun 17, 2024

stackbaek commented Jun 17, 2024

siyuanfoundation commented Jun 17, 2024

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr Jun 18, 2024

Choose a reason for hiding this comment

stackbaek Jun 19, 2024

Choose a reason for hiding this comment

ahrtr Jun 19, 2024

Choose a reason for hiding this comment

ahrtr Jun 18, 2024

Choose a reason for hiding this comment

stackbaek commented May 23, 2024 •

edited

Loading

codecov-commenter commented Jun 14, 2024 •

edited

Loading