migrate experimental-stop-grpc-service-on-defrag flag to feature gate.

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

server/embed/config.go

@@ -888,6 +888,36 @@ func (cfg *configYAML) configFromFile(path string) error {
 	return cfg.Validate()
 }
 
+// SetFeatureGatesFromExperimentalFlags sets the feature gate values if the feature gate is not explicitly set
+// while their corresponding experimental flags are explicitly set for all the features in ExperimentalFlagToFeatureMap.
+// TODO: remove after all experimental flags are deprecated.
+func SetFeatureGatesFromExperimentalFlags(fg featuregate.FeatureGate, getExperimentalFlagVal func(string) *bool, featureGatesFlagName, featureGatesVal string) error {
+	m := make(map[featuregate.Feature]bool)
+	// verify that the feature gate and its experimental flag are not both set at the same time.
+	for expFlagName, featureName := range features.ExperimentalFlagToFeatureMap {
+		flagVal := getExperimentalFlagVal(expFlagName)
+		if flagVal == nil {
+			continue
+		}
+		if strings.Contains(featureGatesVal, string(featureName)) {
+			return fmt.Errorf("cannot specify both flags: --%s=%v and --%s=%s=%v at the same time, please just use --%s=%s=%v",
+				expFlagName, *flagVal, featureGatesFlagName, featureName, fg.Enabled(featureName), featureGatesFlagName, featureName, fg.Enabled(featureName))
+		}
+		m[featureName] = *flagVal
+	}
+
+	// filter out unknown features for fg, because we could use SetFeatureGatesFromExperimentalFlags both for
+	// server and cluster level feature gates.
+	allFeatures := fg.(featuregate.MutableFeatureGate).GetAll()
+	mFiltered := make(map[string]bool)
+	for k, v := range m {
+		if _, ok := allFeatures[k]; ok {
+			mFiltered[string(k)] = v
+		}
+	}
+	return fg.(featuregate.MutableFeatureGate).SetFromMap(mFiltered)
+}
+
 func updateCipherSuites(tls *transport.TLSInfo, ss []string) error {
 	if len(tls.CipherSuites) > 0 && len(ss) > 0 {
 		return fmt.Errorf("TLSInfo.CipherSuites is already specified (given %v)", ss)

server/embed/config_test.go

@@ -21,6 +21,7 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"strconv"
 	"testing"
 	"time"
 
@@ -31,6 +32,8 @@ import (
 	"go.etcd.io/etcd/client/pkg/v3/srv"
 	"go.etcd.io/etcd/client/pkg/v3/transport"
 	"go.etcd.io/etcd/client/pkg/v3/types"
+	"go.etcd.io/etcd/pkg/v3/featuregate"
+	"go.etcd.io/etcd/server/v3/features"
 )
 
 func notFoundErr(service, domain string) error {
@@ -669,3 +672,87 @@ func TestUndefinedAutoCompactionModeValidate(t *testing.T) {
 	err := cfg.Validate()
 	require.Error(t, err)
 }
+
+func TestSetFeatureGatesFromExperimentalFlags(t *testing.T) {
+	testCases := []struct {
+		name                                string
+		featureGatesFlag                    string
+		experimentalStopGRPCServiceOnDefrag string
+		expectErr                           bool
+		expectedFeatures                    map[featuregate.Feature]bool
+	}{
+		{
+			name: "default",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.DistributedTracing:      false,
+				features.StopGRPCServiceOnDefrag: false,
+			},
+		},
+		{
+			name:                                "cannot set experimental flag and feature gate to true at the same time",
+			featureGatesFlag:                    "StopGRPCServiceOnDefrag=true",
+			experimentalStopGRPCServiceOnDefrag: "true",
+			expectErr:                           true,
+		},
+		{
+			name:                                "cannot set experimental flag and feature gate to false at the same time",
+			featureGatesFlag:                    "StopGRPCServiceOnDefrag=false",
+			experimentalStopGRPCServiceOnDefrag: "false",
+			expectErr:                           true,
+		},
+		{
+			name:                                "cannot set experimental flag and feature gate to different values at the same time",
+			featureGatesFlag:                    "StopGRPCServiceOnDefrag=true",
+			experimentalStopGRPCServiceOnDefrag: "false",
+			expectErr:                           true,
+		},
+		{
+			name:                                "can set experimental flag",
+			featureGatesFlag:                    "DistributedTracing=true",
+			experimentalStopGRPCServiceOnDefrag: "true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.DistributedTracing:      true,
+				features.StopGRPCServiceOnDefrag: true,
+			},
+		},
+		{
+			name:             "can set feature gate",
+			featureGatesFlag: "DistributedTracing=true,StopGRPCServiceOnDefrag=true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.DistributedTracing:      true,
+				features.StopGRPCServiceOnDefrag: true,
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fg := features.NewDefaultServerFeatureGate("test", nil)
+			fg.(featuregate.MutableFeatureGate).Set(tc.featureGatesFlag)
+			getExperimentalFlagVal := func(flagName string) *bool {
+				if flagName != "experimental-stop-grpc-service-on-defrag" || tc.experimentalStopGRPCServiceOnDefrag == "" {
+					return nil
+				}
+				flagVal, err := strconv.ParseBool(tc.experimentalStopGRPCServiceOnDefrag)
+				if err != nil {
+					t.Fatal(err)
+				}
+				return &flagVal
+			}
+			err := SetFeatureGatesFromExperimentalFlags(fg, getExperimentalFlagVal, "feature-gates", tc.featureGatesFlag)
+			if tc.expectErr {
+				if err == nil {
+					t.Fatal("expect error")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+			for k, v := range tc.expectedFeatures {
+				if fg.Enabled(k) != v {
+					t.Errorf("expected feature gate %s=%v, got %v", k, v, fg.Enabled(k))
+				}
+			}
+		})
+	}
+}

server/embed/etcd.go

@@ -223,7 +223,6 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
 		WarningUnaryRequestDuration:              cfg.WarningUnaryRequestDuration,
 		ExperimentalMemoryMlock:                  cfg.ExperimentalMemoryMlock,
 		ExperimentalTxnModeWriteWithSharedBuffer: cfg.ExperimentalTxnModeWriteWithSharedBuffer,
-		ExperimentalStopGRPCServiceOnDefrag:      cfg.ExperimentalStopGRPCServiceOnDefrag,
 		ExperimentalBootstrapDefragThresholdMegabytes: cfg.ExperimentalBootstrapDefragThresholdMegabytes,
 		ExperimentalMaxLearners:                       cfg.ExperimentalMaxLearners,
 		V2Deprecation:                                 cfg.V2DeprecationEffective(),

server/etcdmain/config.go

@@ -22,12 +22,14 @@ import (
 	"fmt"
 	"os"
 	"runtime"
+	"strconv"
 	"time"
 
 	"go.uber.org/zap"
 
 	"go.etcd.io/etcd/api/v3/version"
 	"go.etcd.io/etcd/client/pkg/v3/logutil"
+	"go.etcd.io/etcd/pkg/v3/featuregate"
 	"go.etcd.io/etcd/pkg/v3/flags"
 	cconfig "go.etcd.io/etcd/server/v3/config"
 	"go.etcd.io/etcd/server/v3/embed"
@@ -76,8 +78,12 @@ type configFlags struct {
 }
 
 func newConfig() *config {
+	return newConfigFromEmbedConfig(embed.NewConfig())
+}
+
+func newConfigFromEmbedConfig(ec *embed.Config) *config {
 	cfg := &config{
-		ec:      *embed.NewConfig(),
+		ec:      *ec,
 		ignored: ignored,
 	}
 	cfg.cf = configFlags{
@@ -238,14 +244,44 @@ func (cfg *config) configFromCmdLine() error {
 		cfg.ec.InitialCluster = ""
 	}
 
+	// SetFeatureGatesFromExperimentalFlags validates that cmd line flags for experimental feature and their feature gates are not explicitly set simultaneously,
+	// and passes the values of cmd line flags for experimental feature to the server feature gate.
+	err = embed.SetFeatureGatesFromExperimentalFlags(cfg.ec.ServerFeatureGate, cfg.getBoolFlagVal, embed.ServerFeatureGateFlagName, cfg.cf.flagSet.Lookup(embed.ServerFeatureGateFlagName).Value.String())
+	if err != nil {
+		return err
+	}
+
 	return cfg.validate()
 }
 
+// getBoolFlagVal returns the value of the a given bool flag is explicitly set in the cmd line arguments,
+// and returns nil if it is not explicitly set.
+func (cfg *config) getBoolFlagVal(flagName string) *bool {
+	explicitlySet := false
+	cfg.cf.flagSet.Visit(func(f *flag.Flag) {
+		if f.Name == flagName {
+			explicitlySet = true
+		}
+	})
+	if !explicitlySet {
+		return nil
+	}
+	flagVal, parseErr := strconv.ParseBool(cfg.cf.flagSet.Lookup(flagName).Value.String())
+	if parseErr != nil {
+		panic(parseErr)
+	}
+	return &flagVal
+}
+
 func (cfg *config) configFromFile(path string) error {
 	eCfg, err := embed.ConfigFromFile(path)
 	if err != nil {
 		return err
 	}
+	// TODO: remove after adding feature-gates field in the config file.
+	if eCfg.ExperimentalStopGRPCServiceOnDefrag {
+		eCfg.ServerFeatureGate.(featuregate.MutableFeatureGate).Set("StopGRPCServiceOnDefrag=true")
+	}
 	cfg.ec = *eCfg
 
 	return nil

server/etcdmain/config_test.go

@@ -407,26 +407,88 @@ func TestParseFeatureGateFlags(t *testing.T) {
 		{
 			name: "default",
 			expectedFeatures: map[featuregate.Feature]bool{
-				features.DistributedTracing:      false,
 				features.StopGRPCServiceOnDefrag: false,
+				"TestAlpha":                      false,
+				"TestBeta":                       true,
 			},
 		},
 		{
-			name: "can set feature gate flag",
+			name: "cannot set both experimental flag and feature gate flag",
 			args: []string{
 				"--experimental-stop-grpc-service-on-defrag=false",
-				fmt.Sprintf("--%s=DistributedTracing=true,StopGRPCServiceOnDefrag=true", embed.ServerFeatureGateFlagName),
+				"--feature-gates=StopGRPCServiceOnDefrag=true",
+			},
+			expectErr: true,
+		},
+		{
+			name: "ok to set different experimental flag and feature gate flag",
+			args: []string{
+				"--experimental-stop-grpc-service-on-defrag=true",
+				"--feature-gates=TestAlpha=true",
 			},
 			expectedFeatures: map[featuregate.Feature]bool{
-				features.DistributedTracing:      true,
 				features.StopGRPCServiceOnDefrag: true,
+				"TestAlpha":                      true,
+				"TestBeta":                       true,
+			},
+		},
+		{
+			name: "can set feature gate to true from experimental flag",
+			args: []string{
+				"--experimental-stop-grpc-service-on-defrag=true",
+			},
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+				"TestAlpha":                      false,
+				"TestBeta":                       true,
+			},
+		},
+		{
+			name: "can set feature gate to false from experimental flag",
+			args: []string{
+				"--experimental-stop-grpc-service-on-defrag=false",
+			},
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+				"TestAlpha":                      false,
+				"TestBeta":                       true,
+			},
+		},
+		{
+			name: "can set feature gate to true from feature gate flag",
+			args: []string{
+				"--feature-gates=StopGRPCServiceOnDefrag=true",
+			},
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+				"TestAlpha":                      false,
+				"TestBeta":                       true,
+			},
+		},
+		{
+			name: "can set feature gate to false from feature gate flag",
+			args: []string{
+				"--feature-gates=StopGRPCServiceOnDefrag=false,TestBeta=false",
+			},
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+				"TestAlpha":                      false,
+				"TestBeta":                       false,
 			},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			cfg := newConfig()
+			fg := features.NewDefaultServerFeatureGate("test", nil)
+			ec := embed.NewConfig()
+			ec.ServerFeatureGate = fg
+			fg.(featuregate.MutableFeatureGate).Add(
+				map[featuregate.Feature]featuregate.FeatureSpec{
+					"TestAlpha": {Default: false, PreRelease: featuregate.Alpha},
+					"TestBeta":  {Default: true, PreRelease: featuregate.Beta},
+				})
+			cfg := newConfigFromEmbedConfig(ec)
 			err := cfg.parse(tc.args)
 			if tc.expectErr {
 				if err == nil {

server/etcdmain/help.go

@@ -312,7 +312,7 @@ Experimental feature:
   --experimental-snapshot-catchup-entries
     Number of entries for a slow follower to catch up after compacting the raft storage entries.
   --experimental-stop-grpc-service-on-defrag
-    Enable etcd gRPC service to stop serving client requests on defragmentation.
+    Enable etcd gRPC service to stop serving client requests on defragmentation. TO BE DEPRECATED in v3.7, use '--feature-gates=StopGRPCServiceOnDefrag=true' instead.
 
 Unsafe feature:
   --force-new-cluster 'false'

server/etcdserver/api/v3rpc/health.go

@@ -20,6 +20,7 @@ import (
 	healthpb "google.golang.org/grpc/health/grpc_health_v1"
 
 	"go.etcd.io/etcd/server/v3/etcdserver"
+	"go.etcd.io/etcd/server/v3/features"
 )
 
 const (
@@ -35,7 +36,7 @@ func newHealthNotifier(hs *health.Server, s *etcdserver.EtcdServer) notifier {
 	if hs == nil {
 		panic("unexpected nil gRPC health server")
 	}
-	hc := &healthNotifier{hs: hs, lg: s.Logger(), stopGRPCServiceOnDefrag: s.Cfg.ExperimentalStopGRPCServiceOnDefrag}
+	hc := &healthNotifier{hs: hs, lg: s.Logger(), stopGRPCServiceOnDefrag: s.FeatureEnabled(features.StopGRPCServiceOnDefrag)}
 	// set grpc health server as serving status blindly since
 	// the grpc server will serve iff s.ReadyNotify() is closed.
 	hc.startServe()

server/features/etcd_features.go

@@ -52,6 +52,12 @@ var (
 		DistributedTracing:      {Default: false, PreRelease: featuregate.Alpha},
 		StopGRPCServiceOnDefrag: {Default: false, PreRelease: featuregate.Alpha},
 	}
+	// ExperimentalFlagToFeatureMap is the map from the cmd line flags of experimental features
+	// to their corresponding feature gates.
+	// Deprecated: only add existing experimental features here. DO NOT use for new features.
+	ExperimentalFlagToFeatureMap = map[string]featuregate.Feature{
+		"experimental-stop-grpc-service-on-defrag": StopGRPCServiceOnDefrag,
+	}
 )
 
 func NewDefaultServerFeatureGate(name string, lg *zap.Logger) featuregate.FeatureGate {