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

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

pkg/featuregate/feature_gate.go

@@ -122,6 +122,9 @@ type MutableFeatureGate interface {
 	// overriding its default to true for a limited number of components without simultaneously
 	// changing its default for all consuming components.
 	OverrideDefault(name Feature, override bool) error
+	// ForceAddForTest adds features to the featureGate, and it works even after AddFlag is called.
+	// It should only be used in tests.
+	ForceAddForTest(features map[Feature]FeatureSpec) error
 }
 
 // featureGate implements FeatureGate as well as pflag.Value for flag parsing.
@@ -267,6 +270,13 @@ func (f *featureGate) Type() string {
 	return "mapStringBool"
 }
 
+func (f *featureGate) ForceAddForTest(features map[Feature]FeatureSpec) error {
+	f.lock.Lock()
+	f.closed = false
+	f.lock.Unlock()
+	return f.Add(features)
+}
+
 // Add adds features to the featureGate.
 func (f *featureGate) Add(features map[Feature]FeatureSpec) error {
 	f.lock.Lock()

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 is explicitly set in the cmd line arguments,
+// and returns nil if it is not explicitly set.
+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

@@ -785,6 +785,21 @@ func ConfigFromFile(path string) (*Config, error) {
 	return &cfg.Config, nil
 }
 
+// getBoolFlagValFromFile parses the yaml bytes to raw map, and get the top level bool flag value.
+func getBoolFlagValFromFile(yamlBytes []byte, flagName string) (*bool, error) {
+	var cfgMap map[string]interface{}
+	err := yaml.Unmarshal(yamlBytes, &cfgMap)
+	if err != nil {
+		return nil, err
+	}
+	flagVal, ok := cfgMap[flagName]
+	if !ok {
+		return nil, nil
+	}
+	boolVal := flagVal.(bool)
+	return &boolVal, nil
+}
+
 func (cfg *configYAML) configFromFile(path string) error {
 	b, err := os.ReadFile(path)
 	if err != nil {
@@ -805,6 +820,18 @@ func (cfg *configYAML) configFromFile(path string) error {
 		}
 	}
 
+	getBoolFlagVal := func(flagName string) *bool {
+		boolVal, parseErr := getBoolFlagValFromFile(b, flagName)
+		if parseErr != nil {
+			panic(parseErr)
+		}
+		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 +924,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.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 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.StopGRPCServiceOnDefrag: true,
+				features.DistributedTracing:      false,
+			},
+		},
+		{
+			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,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

@@ -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()
 }