Add "server-feature-gates" flag. #18279

siyuanfoundation · 2024-07-03T18:36:56Z

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

siyuanfoundation · 2024-07-03T19:25:00Z

ahrtr · 2024-07-08T12:36:13Z

server/features/etcd_features.go

+	// Every feature gate should add method here following this template:
+	//
+	// // owner: @username
+	// // kep: https://kep.k8s.io/NNN (or issue: https://github.com/etcd-io/etcd/issues/NNN)
+	// // alpha: v3.X
+	// MyFeature featuregate.Feature = "MyFeature"
+	//
+	// Feature gates should be listed in alphabetical, case-sensitive
+	// (upper before any lower case character) order. This reduces the risk
+	// of code conflicts because changes are more likely to be scattered
+	// across the file.
+
+	// DistributedTracing enables experimental distributed  tracing using OpenTelemetry Tracing.
+	// alpha: v3.5
+	// issue: https://github.com/etcd-io/etcd/issues/12460


How does Kubernetes manage this?

https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/

what do you mean by "manage this"? The docs are created manually and enforced as part of release process.

How can you make sure the comments, source code and user-facing doc are in sync? You don't have to do it in this PR, but please think about it.

The comments in the source code will have to be manually reviewed. For the user facing doc, I have a POC in k8s to auto generate the feature list with lifecycles with static analysis. We can add similar scripts to do that

server/embed/config.go

ahrtr · 2024-07-08T12:46:42Z

server/embed/config.go

@@ -108,6 +110,8 @@ const (
 	maxElectionMs = 50000
 	// backend freelist map type
 	freelistArrayType = "array"
+
+	ServerFeatureGateFlagName = "server-feature-gates"


Suggested change

ServerFeatureGateFlagName = "server-feature-gates"

Please see #18279 (comment)

pkg/featuregate/feature_gate.go

ivanvc · 2024-07-09T03:55:36Z

pkg/featuregate/feature_gate.go

-	AddFlag(fs *pflag.FlagSet)
+	AddFlag(fs *flag.FlagSet, flagName string)


Is there a particular reason why you replaced pflag with flag?

all the other flags in server/embed/config.go uses flag instead of pflag

That is good to know. Thanks for your answer. I would think that using pflag is better as it is POSIX-compliant, but I do see that across the project, we're using flag for flagsets. We still parse flags using Cobra, which makes them POSIX-compliant.

server/embed/config.go

server/features/etcd_features.go

codecov-commenter · 2024-07-12T22:35:12Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.

Project coverage is 68.90%. Comparing base (e7f5729) to head (bfad3ff).
Report is 324 commits behind head on main.

❗ Current head bfad3ff differs from pull request most recent head 7b35514

Please upload reports for the commit 7b35514 to get more accurate results.

Files with missing lines	Patch %	Lines
pkg/featuregate/feature_gate.go	60.00%	1 Missing and 1 partial ⚠️
server/features/etcd_features.go	60.00%	1 Missing and 1 partial ⚠️

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

Additional details and impacted files

Files with missing lines	Coverage Δ
server/embed/config.go	`79.02% <100.00%> (+0.07%)`	⬆️
server/embed/etcd.go	`75.86% <100.00%> (+0.04%)`	⬆️
pkg/featuregate/feature_gate.go	`84.41% <60.00%> (-0.92%)`	⬇️
server/features/etcd_features.go	`60.00% <60.00%> (ø)`

... and 21 files with indirect coverage changes

@@           Coverage Diff           @@
##             main   #18279   +/-   ##
=======================================
  Coverage   68.90%   68.90%           
=======================================
  Files         417      418    +1     
  Lines       35338    35350   +12     
=======================================
+ Hits        24348    24359   +11     
- Misses       9566     9573    +7     
+ Partials     1424     1418    -6

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 e7f5729...7b35514. Read the comment docs.

jmhbnz · 2024-07-16T10:55:24Z

/retest

jmhbnz

Thanks @siyuanfoundation - Overall this is looking good to me. Two minor items below.

server/etcdmain/help.go

server/etcdmain/config_test.go

jmhbnz

LGTM - Many thanks for the effort @siyuanfoundation

ivanvc

LGTM. Thanks for addresing my comments, @siyuanfoundation. Great job 😀

fuweid

LGTM

ahrtr · 2024-07-18T18:59:18Z

server/etcdmain/help.go

@@ -103,6 +105,8 @@ Member:
    Read timeout set on each rafthttp connection
  --raft-write-timeout '` + rafthttp.DefaultConnWriteTimeout.String() + `'
    Write timeout set on each rafthttp connection
+  --feature-gates ''
+    A set of key=value pairs that describe server level feature gates for alpha/experimental features. Options are:'` + strings.Join(features.NewDefaultServerFeatureGate("", nil).KnownFeatures(), ",") + `'


This is what I got. Please format the output, print each feature in a separate line.

--feature-gates '' A set of key=value pairs that describe server level feature gates for alpha/experimental features. Options are:'AllAlpha=true|false (ALPHA - default=false),AllBeta=true|false (BETA - default=false),DistributedTracing=true|false (ALPHA - default=false),StopGRPCServiceOnDefrag=true|false (ALPHA - default=false)'

Please let me know if you want to fix it in this PR or a separate PR.

server/etcdmain/help.go

Signed-off-by: Siyuan Zhang <[email protected]>

siyuanfoundation · 2024-07-18T21:27:04Z

/retest

ahrtr · 2024-07-19T05:25:47Z

server/features/etcd_features.go

+		DistributedTracing:      {Default: false, PreRelease: featuregate.Alpha},
+		StopGRPCServiceOnDefrag: {Default: false, PreRelease: featuregate.Alpha},


You just added these two features, but they are still controlled by the legacy flags, are you going to migrate them to feature gate in a followup PR?

Yes. They will be migrated in the next few followup PRs.

ahrtr

Overall looks good to me with a question.

Thanks @siyuanfoundation

cc @serathius

k8s-ci-robot · 2024-07-23T13:27:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, fuweid, ivanvc, 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:

~~OWNERS~~ [ahrtr,jmhbnz,serathius]

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 added the size/L label Jul 3, 2024

siyuanfoundation mentioned this pull request Jul 3, 2024

Add "feature-gates" flag and field in config file and add feature gate for StopGRPCServiceOnDefrag #18234

Closed

siyuanfoundation force-pushed the fg-1 branch from 3c19920 to cb9d69b Compare July 3, 2024 18:43

ahrtr self-requested a review July 3, 2024 18:48

k8s-ci-robot requested a review from serathius July 3, 2024 19:25

ahrtr reviewed Jul 8, 2024

View reviewed changes

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

ahrtr requested review from ivanvc, jmhbnz and tjungblu July 8, 2024 12:52

siyuanfoundation force-pushed the fg-1 branch 2 times, most recently from 8f81134 to d0bfa58 Compare July 8, 2024 18:08

siyuanfoundation requested a review from ahrtr July 8, 2024 20:55

ivanvc reviewed Jul 9, 2024

View reviewed changes

ahrtr reviewed Jul 9, 2024

View reviewed changes

server/embed/config.go Show resolved Hide resolved

ahrtr reviewed Jul 9, 2024

View reviewed changes

server/embed/config.go Show resolved Hide resolved

ivanvc reviewed Jul 11, 2024

View reviewed changes

server/features/etcd_features.go Outdated Show resolved Hide resolved

siyuanfoundation force-pushed the fg-1 branch from d0bfa58 to 3028ce0 Compare July 12, 2024 22:15

siyuanfoundation requested a review from ahrtr July 12, 2024 22:16

jmhbnz reviewed Jul 16, 2024

View reviewed changes

server/etcdmain/help.go Show resolved Hide resolved

server/etcdmain/config_test.go Show resolved Hide resolved

siyuanfoundation requested a review from jmhbnz July 16, 2024 23:37

jmhbnz approved these changes Jul 16, 2024

View reviewed changes

k8s-ci-robot added the approved label Jul 16, 2024

siyuanfoundation requested a review from jmhbnz July 16, 2024 23:43

ivanvc approved these changes Jul 17, 2024

View reviewed changes

fuweid approved these changes Jul 18, 2024

View reviewed changes

ahrtr reviewed Jul 18, 2024

View reviewed changes

server/etcdmain/help.go Outdated Show resolved Hide resolved

Add "server-feature-gates" flag.

7b35514

Signed-off-by: Siyuan Zhang <[email protected]>

siyuanfoundation force-pushed the fg-1 branch from 1065c0c to 7b35514 Compare July 18, 2024 20:20

siyuanfoundation requested a review from ahrtr July 18, 2024 22:40

ahrtr reviewed Jul 19, 2024

View reviewed changes

ahrtr approved these changes Jul 19, 2024

View reviewed changes

serathius approved these changes Jul 23, 2024

View reviewed changes

serathius merged commit 3b4e2f4 into etcd-io:main Jul 23, 2024
42 checks passed

siyuanfoundation mentioned this pull request Aug 2, 2024

Add server level feature gate #18023

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "server-feature-gates" flag. #18279

Add "server-feature-gates" flag. #18279

siyuanfoundation commented Jul 3, 2024

siyuanfoundation commented Jul 3, 2024

ahrtr Jul 8, 2024

siyuanfoundation Jul 8, 2024

ahrtr Jul 9, 2024

siyuanfoundation Jul 16, 2024

ahrtr Jul 8, 2024

siyuanfoundation Jul 16, 2024

ivanvc Jul 9, 2024

siyuanfoundation Jul 9, 2024

ivanvc Jul 11, 2024

codecov-commenter commented Jul 12, 2024 •

edited

Loading

jmhbnz commented Jul 16, 2024

jmhbnz left a comment

jmhbnz left a comment

ivanvc left a comment

fuweid left a comment

ahrtr Jul 18, 2024

siyuanfoundation Jul 18, 2024

siyuanfoundation commented Jul 18, 2024

ahrtr Jul 19, 2024

siyuanfoundation Jul 19, 2024

ahrtr left a comment

k8s-ci-robot commented Jul 23, 2024

		AddFlag(fs *pflag.FlagSet)
		AddFlag(fs *flag.FlagSet, flagName string)

		DistributedTracing: {Default: false, PreRelease: featuregate.Alpha},
		StopGRPCServiceOnDefrag: {Default: false, PreRelease: featuregate.Alpha},

Add "server-feature-gates" flag. #18279

Add "server-feature-gates" flag. #18279

Conversation

siyuanfoundation commented Jul 3, 2024

siyuanfoundation commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 12, 2024 • edited Loading

Codecov Report

jmhbnz commented Jul 16, 2024

jmhbnz left a comment

Choose a reason for hiding this comment

jmhbnz left a comment

Choose a reason for hiding this comment

ivanvc left a comment

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siyuanfoundation commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 23, 2024

codecov-commenter commented Jul 12, 2024 •

edited

Loading