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

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

Author: siyuanfoundation

etcd.conf.yml.sample

@@ -155,3 +155,6 @@ cipher-suites: [
 # Limit etcd to specific TLS protocol versions 
 tls-min-version: 'TLS1.2'
 tls-max-version: 'TLS1.3'
+
+# Comma-separated list of server feature gate enablement in the format of feature=true|false
+server-feature-gates: StopGRPCServiceOnDefrag=false,DistributedTracing=false

pkg/featuregate/feature_gate.go

@@ -16,6 +16,7 @@
 package featuregate
 
 import (
+	"flag"
 	"fmt"
 	"sort"
 	"strconv"
@@ -30,7 +31,7 @@ import (
 type Feature string
 
 const (
-	flagName = "feature-gates"
+	defaultFlagName = "feature-gates"
 
 	// allAlphaGate is a global toggle for alpha features. Per-feature key
 	// values override the default set by allAlphaGate. Examples:
@@ -98,7 +99,7 @@ type MutableFeatureGate interface {
 	FeatureGate
 
 	// AddFlag adds a flag for setting global feature gates to the specified FlagSet.
-	AddFlag(fs *pflag.FlagSet)
+	AddFlag(fs *flag.FlagSet, flagName string)
 	// Set parses and stores flag gates for known features
 	// from a string like feature1=true,feature2=false,...
 	Set(value string) error
@@ -121,6 +122,8 @@ 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
+	// SetLogger replaces the logger with the provided logger.
+	SetLogger(lg *zap.Logger)
 }
 
 // featureGate implements FeatureGate as well as pflag.Value for flag parsing.
@@ -165,6 +168,9 @@ func setUnsetBetaGates(known map[Feature]FeatureSpec, enabled map[Feature]bool,
 var _ pflag.Value = &featureGate{}
 
 func New(name string, lg *zap.Logger) *featureGate {
+	if lg == nil {
+		lg = zap.NewNop()
+	}
 	known := map[Feature]FeatureSpec{}
 	for k, v := range defaultFeatures {
 		known[k] = v
@@ -349,7 +355,10 @@ func (f *featureGate) Enabled(key Feature) bool {
 }
 
 // AddFlag adds a flag for setting global feature gates to the specified FlagSet.
-func (f *featureGate) AddFlag(fs *pflag.FlagSet) {
+func (f *featureGate) AddFlag(fs *flag.FlagSet, flagName string) {
+	if flagName == "" {
+		flagName = defaultFlagName
+	}
 	f.lock.Lock()
 	// TODO(mtaufen): Shouldn't we just close it on the first Set/SetFromMap instead?
 	// Not all components expose a feature gates flag using this AddFlag method, and
@@ -409,3 +418,10 @@ func (f *featureGate) DeepCopy() MutableFeatureGate {
 
 	return fg
 }
+
+// SetLogger replaces the logger with the provided logger.
+func (f *featureGate) SetLogger(lg *zap.Logger) {
+	f.lock.Lock()
+	defer f.lock.Unlock()
+	f.lg = lg
+}

pkg/featuregate/feature_gate_test.go

@@ -15,11 +15,11 @@
 package featuregate
 
 import (
+	"flag"
 	"fmt"
 	"strings"
 	"testing"
 
-	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 	"go.uber.org/zap/zaptest"
 )
@@ -203,15 +203,15 @@ func TestFeatureGateFlag(t *testing.T) {
 	}
 	for i, test := range tests {
 		t.Run(test.arg, func(t *testing.T) {
-			fs := pflag.NewFlagSet("testfeaturegateflag", pflag.ContinueOnError)
+			fs := flag.NewFlagSet("testfeaturegateflag", flag.ContinueOnError)
 			f := New("test", zaptest.NewLogger(t))
 			f.Add(map[Feature]FeatureSpec{
 				testAlphaGate: {Default: false, PreRelease: Alpha},
 				testBetaGate:  {Default: false, PreRelease: Beta},
 			})
-			f.AddFlag(fs)
+			f.AddFlag(fs, defaultFlagName)
 
-			err := fs.Parse([]string{fmt.Sprintf("--%s=%s", flagName, test.arg)})
+			err := fs.Parse([]string{fmt.Sprintf("--%s=%s", defaultFlagName, test.arg)})
 			if test.parseError != "" {
 				if !strings.Contains(err.Error(), test.parseError) {
 					t.Errorf("%d: Parse() Expected %v, Got %v", i, test.parseError, err)
@@ -603,8 +603,8 @@ func TestFeatureGateOverrideDefault(t *testing.T) {
 
 	t.Run("returns error if already added to flag set", func(t *testing.T) {
 		f := New("test", zaptest.NewLogger(t))
-		fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
-		f.AddFlag(fs)
+		fs := flag.NewFlagSet("test", flag.ContinueOnError)
+		f.AddFlag(fs, defaultFlagName)
 
 		if err := f.OverrideDefault("TestFeature", true); err == nil {
 			t.Error("expected a non-nil error to be returned")

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

@@ -42,6 +42,7 @@ import (
 	"go.etcd.io/etcd/client/pkg/v3/transport"
 	"go.etcd.io/etcd/client/pkg/v3/types"
 	clientv3 "go.etcd.io/etcd/client/v3"
+	"go.etcd.io/etcd/pkg/v3/featuregate"
 	"go.etcd.io/etcd/pkg/v3/flags"
 	"go.etcd.io/etcd/pkg/v3/netutil"
 	"go.etcd.io/etcd/server/v3/config"
@@ -50,6 +51,7 @@ import (
 	"go.etcd.io/etcd/server/v3/etcdserver/api/rafthttp"
 	"go.etcd.io/etcd/server/v3/etcdserver/api/v3compactor"
 	"go.etcd.io/etcd/server/v3/etcdserver/api/v3discovery"
+	"go.etcd.io/etcd/server/v3/features"
 )
 
 const (
@@ -108,6 +110,8 @@ const (
 	maxElectionMs = 50000
 	// backend freelist map type
 	freelistArrayType = "array"
+
+	ServerFeatureGateFlagName = "server-feature-gates"
 )
 
 var (
@@ -455,6 +459,9 @@ type Config struct {
 
 	// V2Deprecation describes phase of API & Storage V2 support
 	V2Deprecation config.V2DeprecationEnum `json:"v2-deprecation"`
+
+	// ServerFeatureGate is a server level feature gate
+	ServerFeatureGate featuregate.FeatureGate
 }
 
 // configYAML holds the config suitable for yaml parsing
@@ -476,6 +483,8 @@ type configJSON struct {
 
 	ClientSecurityJSON securityConfig `json:"client-transport-security"`
 	PeerSecurityJSON   securityConfig `json:"peer-transport-security"`
+
+	ServerFeatureGatesJSON string `json:"server-feature-gates"`
 }
 
 type securityConfig struct {
@@ -576,6 +585,7 @@ func NewConfig() *Config {
 		},
 
 		AutoCompactionMode: DefaultAutoCompactionMode,
+		ServerFeatureGate:  features.NewDefaultServerFeatureGate(DefaultName, nil),
 	}
 	cfg.InitialCluster = cfg.InitialClusterFromName(cfg.Name)
 	return cfg
@@ -762,6 +772,9 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) {
 	// unsafe
 	fs.BoolVar(&cfg.UnsafeNoFsync, "unsafe-no-fsync", false, "Disables fsync, unsafe, will cause data loss.")
 	fs.BoolVar(&cfg.ForceNewCluster, "force-new-cluster", false, "Force to create a new one member cluster.")
+
+	// featuregate
+	cfg.ServerFeatureGate.(featuregate.MutableFeatureGate).AddFlag(fs, ServerFeatureGateFlagName)
 }
 
 func ConfigFromFile(path string) (*Config, error) {
@@ -785,6 +798,26 @@ func (cfg *configYAML) configFromFile(path string) error {
 		return err
 	}
 
+	if cfg.configJSON.ServerFeatureGatesJSON != "" {
+		err = cfg.Config.ServerFeatureGate.(featuregate.MutableFeatureGate).Set(cfg.configJSON.ServerFeatureGatesJSON)
+		if err != nil {
+			return err
+		}
+	}
+	var cfgMap map[string]interface{}
+	err = yaml.Unmarshal(b, &cfgMap)
+	if err != nil {
+		return err
+	}
+	isExperimentalFlagSet := func(expFlag string) bool {
+		_, ok := cfgMap[expFlag]
+		return ok
+	}
+	err = cfg.SetFeatureGatesFromExperimentalFlags(cfg.ServerFeatureGate, isExperimentalFlagSet, 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 {
@@ -877,6 +910,37 @@ 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.
+// TODO: remove after all experimental flags are deprecated.
+func (cfg *Config) SetFeatureGatesFromExperimentalFlags(fg featuregate.FeatureGate, isExperimentalFlagSet func(string) bool, flagName, featureGatesVal string) error {
+	// verify that the feature gate and its experimental flag are not both set at the same time.
+	for expFlagName, featureName := range features.ExperimentalFlagToFeatureMap {
+		if isExperimentalFlagSet(expFlagName) && strings.Contains(featureGatesVal, string(featureName)) {
+			return fmt.Errorf("cannot specify both flags: --%s=(true|false) and --%s=%s=(true|false) at the same time, please just use --%s=%s=(true|false)",
+				expFlagName, flagName, featureName, flagName, featureName)
+		}
+	}
+
+	m := make(map[featuregate.Feature]bool)
+	defaultEc := NewConfig()
+	// if a ExperimentalXX config is different from the default value, that means the experimental flag is explicitly set.
+	// We need to pass that into the feature gate.
+	// This section should include all the experimental flag configs that are still in use.
+	if cfg.ExperimentalStopGRPCServiceOnDefrag != defaultEc.ExperimentalStopGRPCServiceOnDefrag {
+		m[features.StopGRPCServiceOnDefrag] = cfg.ExperimentalStopGRPCServiceOnDefrag
+	}
+	// filter out unknown features for fg
+	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)
@@ -907,6 +971,7 @@ func (cfg *Config) Validate() error {
 	if err := cfg.setupLogging(); err != nil {
 		return err
 	}
+	cfg.ServerFeatureGate.(featuregate.MutableFeatureGate).SetLogger(cfg.logger)
 	if err := checkBindURLs(cfg.ListenPeerUrls); err != nil {
 		return err
 	}

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 {
@@ -89,6 +92,110 @@ func TestConfigFileOtherFields(t *testing.T) {
 	assert.Equal(t, false, cfg.SocketOpts.ReuseAddress, "ReuseAddress does not match")
 }
 
+func TestConfigFileFeatureGates(t *testing.T) {
+	testCases := []struct {
+		name                                string
+		serverFeatureGatesJSON              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 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=false",
+			experimentalStopGRPCServiceOnDefrag: "true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.DistributedTracing:      false,
+				features.StopGRPCServiceOnDefrag: true,
+			},
+		},
+		{
+			name:                                "can set feature gate to true from experimental flag",
+			experimentalStopGRPCServiceOnDefrag: "true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+			},
+		},
+		{
+			name:                                "can set feature gate to false from experimental flag",
+			experimentalStopGRPCServiceOnDefrag: "false",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+			},
+		},
+		{
+			name:                   "can set feature gate to true from feature gate flag",
+			serverFeatureGatesJSON: "StopGRPCServiceOnDefrag=true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: true,
+			},
+		},
+		{
+			name:                   "can set feature gate to false from feature gate flag",
+			serverFeatureGatesJSON: "StopGRPCServiceOnDefrag=false",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			yc := struct {
+				ExperimentalStopGRPCServiceOnDefrag *bool  `json:"experimental-stop-grpc-service-on-defrag,omitempty"`
+				ServerFeatureGatesJSON              string `json:"server-feature-gates"`
+			}{
+				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)
+			}
+
+			tmpfile := mustCreateCfgFile(t, b)
+			defer os.Remove(tmpfile.Name())
+
+			cfg, err := ConfigFromFile(tmpfile.Name())
+			if tc.expectErr {
+				if err == nil {
+					if err == nil {
+						t.Fatal("expect parse error")
+					}
+				}
+				return
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+			for k, v := range tc.expectedFeatures {
+				if cfg.ServerFeatureGate.Enabled(k) != v {
+					t.Errorf("expected feature gate %s=%v, got %v", k, v, cfg.ServerFeatureGate.Enabled(k))
+				}
+			}
+		})
+	}
+}
+
 // TestUpdateDefaultClusterFromName ensures that etcd can start with 'etcd --name=abc'.
 func TestUpdateDefaultClusterFromName(t *testing.T) {
 	cfg := NewConfig()