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

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

pkg/flags/flag.go

@@ -19,6 +19,7 @@ import (
 	"flag"
 	"fmt"
 	"os"
+	"strconv"
 	"strings"
 
 	"github.com/spf13/pflag"
@@ -130,3 +131,16 @@ func IsSet(fs *flag.FlagSet, name string) bool {
 	})
 	return set
 }
+
+// GetBoolFlagVal returns the value of the a given bool flag if it is explicitly set
+// in the cmd line arguments, otherwise returns nil.
+func GetBoolFlagVal(fs *flag.FlagSet, flagName string) (*bool, error) {
+	if !IsSet(fs, flagName) {
+		return nil, nil
+	}
+	flagVal, parseErr := strconv.ParseBool(fs.Lookup(flagName).Value.String())
+	if parseErr != nil {
+		return nil, parseErr
+	}
+	return &flagVal, nil
+}

server/config/config.go

@@ -193,9 +193,6 @@ type ServerConfig struct {
 	// a shared buffer in its readonly check operations.
 	ExperimentalTxnModeWriteWithSharedBuffer bool `json:"experimental-txn-mode-write-with-shared-buffer"`
 
-	// ExperimentalStopGRPCServiceOnDefrag enables etcd gRPC service to stop serving client requests on defragmentation.
-	ExperimentalStopGRPCServiceOnDefrag bool `json:"experimental-stop-grpc-service-on-defrag"`
-
 	// ExperimentalBootstrapDefragThresholdMegabytes is the minimum number of megabytes needed to be freed for etcd server to
 	// consider running defrag during bootstrap. Needs to be set to non-zero value to take effect.
 	ExperimentalBootstrapDefragThresholdMegabytes uint `json:"experimental-bootstrap-defrag-threshold-megabytes"`

server/embed/config.go

@@ -805,6 +805,25 @@ func (cfg *configYAML) configFromFile(path string) error {
 		}
 	}
 
+	// parses the yaml bytes to raw map first, then getBoolFlagVal can get the top level bool flag value.
+	var cfgMap map[string]interface{}
+	err = yaml.Unmarshal(b, &cfgMap)
+	if err != nil {
+		return err
+	}
+	getBoolFlagVal := func(flagName string) *bool {
+		flagVal, ok := cfgMap[flagName]
+		if !ok {
+			return nil
+		}
+		boolVal := flagVal.(bool)
+		return &boolVal
+	}
+	err = SetFeatureGatesFromExperimentalFlags(cfg.ServerFeatureGate, getBoolFlagVal, ServerFeatureGateFlagName, cfg.configJSON.ServerFeatureGatesJSON)
+	if err != nil {
+		return err
+	}
+
 	if cfg.configJSON.ListenPeerURLs != "" {
 		u, err := types.NewURLs(strings.Split(cfg.configJSON.ListenPeerURLs, ","))
 		if err != nil {
@@ -897,6 +916,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"
 
@@ -93,10 +94,11 @@ func TestConfigFileOtherFields(t *testing.T) {
 
 func TestConfigFileFeatureGates(t *testing.T) {
 	testCases := []struct {
-		name                   string
-		serverFeatureGatesJSON string
-		expectErr              bool
-		expectedFeatures       map[featuregate.Feature]bool
+		name                                string
+		serverFeatureGatesJSON              string
+		experimentalStopGRPCServiceOnDefrag string
+		expectErr                           bool
+		expectedFeatures                    map[featuregate.Feature]bool
 	}{
 		{
 			name: "default",
@@ -106,25 +108,51 @@ func TestConfigFileFeatureGates(t *testing.T) {
 			},
 		},
 		{
-			name:                   "set StopGRPCServiceOnDefrag to true",
-			serverFeatureGatesJSON: "StopGRPCServiceOnDefrag=true",
+			name:                                "cannot set both experimental flag and feature gate flag",
+			serverFeatureGatesJSON:              "StopGRPCServiceOnDefrag=true",
+			experimentalStopGRPCServiceOnDefrag: "false",
+			expectErr:                           true,
+		},
+		{
+			name:                                "ok to set different experimental flag and feature gate flag",
+			serverFeatureGatesJSON:              "DistributedTracing=true",
+			experimentalStopGRPCServiceOnDefrag: "true",
 			expectedFeatures: map[featuregate.Feature]bool{
-				features.DistributedTracing:      false,
+				features.DistributedTracing:      true,
 				features.StopGRPCServiceOnDefrag: true,
 			},
 		},
 		{
-			name:                   "set both features to true",
-			serverFeatureGatesJSON: "DistributedTracing=true,StopGRPCServiceOnDefrag=true",
+			name:                                "can set feature gate to true from experimental flag",
+			experimentalStopGRPCServiceOnDefrag: "true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+				features.DistributedTracing:      false,
+			},
+		},
+		{
+			name:                                "can set feature gate to false from experimental flag",
+			experimentalStopGRPCServiceOnDefrag: "false",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+				features.DistributedTracing:      false,
+			},
+		},
+		{
+			name:                   "can set feature gate to true from feature gate flag",
+			serverFeatureGatesJSON: "StopGRPCServiceOnDefrag=true",
 			expectedFeatures: map[featuregate.Feature]bool{
-				features.DistributedTracing:      true,
 				features.StopGRPCServiceOnDefrag: true,
+				features.DistributedTracing:      false,
 			},
 		},
 		{
-			name:                   "error setting unrecognized feature",
-			serverFeatureGatesJSON: "DistributedTracing=true,StopGRPCServiceOnDefragExp=true",
-			expectErr:              true,
+			name:                   "can set feature gate to false from feature gate flag",
+			serverFeatureGatesJSON: "StopGRPCServiceOnDefrag=false",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+				features.DistributedTracing:      false,
+			},
 		},
 	}
 	for _, tc := range testCases {
@@ -136,6 +164,13 @@ func TestConfigFileFeatureGates(t *testing.T) {
 				ServerFeatureGatesJSON: tc.serverFeatureGatesJSON,
 			}
 
+			if tc.experimentalStopGRPCServiceOnDefrag != "" {
+				experimentalStopGRPCServiceOnDefrag, err := strconv.ParseBool(tc.experimentalStopGRPCServiceOnDefrag)
+				if err != nil {
+					t.Fatal(err)
+				}
+				yc.ExperimentalStopGRPCServiceOnDefrag = &experimentalStopGRPCServiceOnDefrag
+			}
 			b, err := yaml.Marshal(&yc)
 			if err != nil {
 				t.Fatal(err)
@@ -743,3 +778,99 @@ 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.StopGRPCServiceOnDefrag: false,
+				"TestAlpha":                      false,
+				"TestBeta":                       true,
+			},
+		},
+		{
+			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 and other feature gates",
+			featureGatesFlag:                    "TestAlpha=true,TestBeta=false",
+			experimentalStopGRPCServiceOnDefrag: "true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+				"TestAlpha":                      true,
+				"TestBeta":                       false,
+			},
+		},
+		{
+			name:             "can set feature gate",
+			featureGatesFlag: "TestAlpha=true,StopGRPCServiceOnDefrag=true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+				"TestAlpha":                      true,
+				"TestBeta":                       true,
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fg := features.NewDefaultServerFeatureGate("test", nil)
+			err := fg.(featuregate.MutableFeatureGate).Add(
+				map[featuregate.Feature]featuregate.FeatureSpec{
+					"TestAlpha": {Default: false, PreRelease: featuregate.Alpha},
+					"TestBeta":  {Default: true, PreRelease: featuregate.Beta},
+				})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			fg.(featuregate.MutableFeatureGate).Set(tc.featureGatesFlag)
+			getExperimentalFlagVal := func(flagName string) *bool {
+				if flagName != "experimental-stop-grpc-service-on-defrag" || tc.experimentalStopGRPCServiceOnDefrag == "" {
+					return nil
+				}
+				flagVal, parseErr := strconv.ParseBool(tc.experimentalStopGRPCServiceOnDefrag)
+				if parseErr != nil {
+					t.Fatal(parseErr)
+				}
+				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

@@ -238,6 +238,21 @@ func (cfg *config) configFromCmdLine() error {
 		cfg.ec.InitialCluster = ""
 	}
 
+	getBoolFlagVal := func(flagName string) *bool {
+		boolVal, parseErr := flags.GetBoolFlagVal(cfg.cf.flagSet, flagName)
+		if parseErr != nil {
+			panic(parseErr)
+		}
+		return boolVal
+	}
+
+	// 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, getBoolFlagVal, embed.ServerFeatureGateFlagName, cfg.cf.flagSet.Lookup(embed.ServerFeatureGateFlagName).Value.String())
+	if err != nil {
+		return err
+	}
+
 	return cfg.validate()
 }
 

server/etcdmain/config_test.go

@@ -407,19 +407,47 @@ func TestParseFeatureGateFlags(t *testing.T) {
 		{
 			name: "default",
 			expectedFeatures: map[featuregate.Feature]bool{
-				features.DistributedTracing:      false,
 				features.StopGRPCServiceOnDefrag: false,
+				features.DistributedTracing:      false,
 			},
 		},
 		{
-			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=DistributedTracing=true",
 			},
 			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
 				features.DistributedTracing:      true,
+			},
+		},
+		{
+			name: "can set feature gate from experimental flag",
+			args: []string{
+				"--experimental-stop-grpc-service-on-defrag=true",
+			},
+			expectedFeatures: map[featuregate.Feature]bool{
 				features.StopGRPCServiceOnDefrag: true,
+				features.DistributedTracing:      false,
+			},
+		},
+		{
+			name: "can set feature gate from feature gate flag",
+			args: []string{
+				"--feature-gates=StopGRPCServiceOnDefrag=true,DistributedTracing=true",
+			},
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+				features.DistributedTracing:      true,
 			},
 		},
 	}

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. It's deprecated, and will be decommissioned in v3.7. Use '--feature-gates=StopGRPCServiceOnDefrag=true' instead.
 
 Unsafe feature:
   --force-new-cluster 'false'