migrate experimental-compact-hash-check-enabled to feature gate.

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

pkg/featuregate/feature_gate.go

@@ -91,6 +91,8 @@ type FeatureGate interface {
 	// set on the copy without mutating the original. This is useful for validating
 	// config against potential feature gate changes before committing those changes.
 	DeepCopy() MutableFeatureGate
+	// String returns a string containing all enabled feature gates, formatted as "key1=value1,key2=value2,...".
+	String() string
 }
 
 // MutableFeatureGate parses and stores flag gates for known features from

server/config/config.go

@@ -147,10 +147,9 @@ type ServerConfig struct {
 
 	// InitialCorruptCheck is true to check data corruption on boot
 	// before serving any peer/client traffic.
-	InitialCorruptCheck     bool
-	CorruptCheckTime        time.Duration
-	CompactHashCheckEnabled bool
-	CompactHashCheckTime    time.Duration
+	InitialCorruptCheck  bool
+	CorruptCheckTime     time.Duration
+	CompactHashCheckTime time.Duration
 
 	// PreVote is true to enable Raft Pre-Vote.
 	PreVote bool

server/embed/config.go

@@ -58,21 +58,21 @@ const (
 	ClusterStateFlagNew      = "new"
 	ClusterStateFlagExisting = "existing"
 
-	DefaultName                             = "default"
-	DefaultMaxSnapshots                     = 5
-	DefaultMaxWALs                          = 5
-	DefaultMaxTxnOps                        = uint(128)
-	DefaultWarningApplyDuration             = 100 * time.Millisecond
-	DefaultWarningUnaryRequestDuration      = 300 * time.Millisecond
-	DefaultMaxRequestBytes                  = 1.5 * 1024 * 1024
-	DefaultMaxConcurrentStreams             = math.MaxUint32
-	DefaultGRPCKeepAliveMinTime             = 5 * time.Second
-	DefaultGRPCKeepAliveInterval            = 2 * time.Hour
-	DefaultGRPCKeepAliveTimeout             = 20 * time.Second
-	DefaultDowngradeCheckTime               = 5 * time.Second
-	DefaultAutoCompactionMode               = "periodic"
-	DefaultAuthToken                        = "simple"
-	DefaultExperimentalCompactHashCheckTime = time.Minute
+	DefaultName                        = "default"
+	DefaultMaxSnapshots                = 5
+	DefaultMaxWALs                     = 5
+	DefaultMaxTxnOps                   = uint(128)
+	DefaultWarningApplyDuration        = 100 * time.Millisecond
+	DefaultWarningUnaryRequestDuration = 300 * time.Millisecond
+	DefaultMaxRequestBytes             = 1.5 * 1024 * 1024
+	DefaultMaxConcurrentStreams        = math.MaxUint32
+	DefaultGRPCKeepAliveMinTime        = 5 * time.Second
+	DefaultGRPCKeepAliveInterval       = 2 * time.Hour
+	DefaultGRPCKeepAliveTimeout        = 20 * time.Second
+	DefaultDowngradeCheckTime          = 5 * time.Second
+	DefaultAutoCompactionMode          = "periodic"
+	DefaultAuthToken                   = "simple"
+	DefaultCompactHashCheckTime        = time.Minute
 
 	DefaultDiscoveryDialTimeout      = 2 * time.Second
 	DefaultDiscoveryRequestTimeOut   = 5 * time.Second
@@ -128,6 +128,12 @@ var (
 
 	// indirection for testing
 	getCluster = srv.GetCluster
+
+	// in 3.6, we are migration all the --experimental flags to feature gate and flags without the prefix.
+	// This is the mapping from the non boolean `experimental-` to the new flags.
+	ExperimentalFlagMigrationMap = map[string]string{
+		"experimental-compact-hash-check-time": "compact-hash-check-time",
+	}
 )
 
 var (
@@ -356,10 +362,14 @@ type Config struct {
 	// AuthTokenTTL in seconds of the simple token
 	AuthTokenTTL uint `json:"auth-token-ttl"`
 
-	ExperimentalInitialCorruptCheck     bool          `json:"experimental-initial-corrupt-check"`
-	ExperimentalCorruptCheckTime        time.Duration `json:"experimental-corrupt-check-time"`
-	ExperimentalCompactHashCheckEnabled bool          `json:"experimental-compact-hash-check-enabled"`
-	ExperimentalCompactHashCheckTime    time.Duration `json:"experimental-compact-hash-check-time"`
+	// Deprecated in v3.6, and will be decommissioned in v3.7.
+	ExperimentalInitialCorruptCheck bool          `json:"experimental-initial-corrupt-check"`
+	ExperimentalCorruptCheckTime    time.Duration `json:"experimental-corrupt-check-time"`
+	// Deprecated in v3.6, and will be decommissioned in v3.7.
+	ExperimentalCompactHashCheckEnabled bool `json:"experimental-compact-hash-check-enabled"`
+	// Deprecated in v3.6, and will be decommissioned in v3.7.
+	ExperimentalCompactHashCheckTime time.Duration `json:"experimental-compact-hash-check-time"`
+	CompactHashCheckTime             time.Duration `json:"compact-hash-check-time"`
 
 	// ExperimentalEnableLeaseCheckpoint enables leader to send regular checkpoints to other members to prevent reset of remaining TTL on leader change.
 	ExperimentalEnableLeaseCheckpoint bool `json:"experimental-enable-lease-checkpoint"`
@@ -573,8 +583,7 @@ func NewConfig() *Config {
 		ExperimentalStopGRPCServiceOnDefrag:      false,
 		ExperimentalMaxLearners:                  membership.DefaultMaxLearners,
 
-		ExperimentalCompactHashCheckEnabled: false,
-		ExperimentalCompactHashCheckTime:    DefaultExperimentalCompactHashCheckTime,
+		CompactHashCheckTime: DefaultCompactHashCheckTime,
 
 		V2Deprecation: config.V2DeprDefault,
 
@@ -755,8 +764,9 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) {
 	// experimental
 	fs.BoolVar(&cfg.ExperimentalInitialCorruptCheck, "experimental-initial-corrupt-check", cfg.ExperimentalInitialCorruptCheck, "Enable to check data corruption before serving any client/peer traffic.")
 	fs.DurationVar(&cfg.ExperimentalCorruptCheckTime, "experimental-corrupt-check-time", cfg.ExperimentalCorruptCheckTime, "Duration of time between cluster corruption check passes.")
-	fs.BoolVar(&cfg.ExperimentalCompactHashCheckEnabled, "experimental-compact-hash-check-enabled", cfg.ExperimentalCompactHashCheckEnabled, "Enable leader to periodically check followers compaction hashes.")
-	fs.DurationVar(&cfg.ExperimentalCompactHashCheckTime, "experimental-compact-hash-check-time", cfg.ExperimentalCompactHashCheckTime, "Duration of time between leader checks followers compaction hashes.")
+	fs.BoolVar(&cfg.ExperimentalCompactHashCheckEnabled, "experimental-compact-hash-check-enabled", cfg.ExperimentalCompactHashCheckEnabled, "Enable leader to periodically check followers compaction hashes. Deprecated, and will be decommissioned in v3.7. Use '--feature-gates=CompactHashCheck=true' instead")
+	fs.DurationVar(&cfg.ExperimentalCompactHashCheckTime, "experimental-compact-hash-check-time", cfg.ExperimentalCompactHashCheckTime, "Duration of time between leader checks followers compaction hashes. Deprecated in v3.6, and will be decommissioned in v3.7. Use --compact-hash-check-time instead.")
+	fs.DurationVar(&cfg.CompactHashCheckTime, "compact-hash-check-time", cfg.CompactHashCheckTime, "Duration of time between leader checks followers compaction hashes.")
 
 	fs.BoolVar(&cfg.ExperimentalEnableLeaseCheckpoint, "experimental-enable-lease-checkpoint", false, "Enable leader to send regular checkpoints to other members to prevent reset of remaining TTL on leader change.")
 	// TODO: delete in v3.7
@@ -817,6 +827,14 @@ func (cfg *configYAML) configFromFile(path string) error {
 	if err != nil {
 		return err
 	}
+	// make sure there is no conflict in the flag settings in the ExperimentalFlagMigrationMap
+	for oldFlag, newFlag := range ExperimentalFlagMigrationMap {
+		_, okOld := cfgMap[oldFlag]
+		_, okNew := cfgMap[newFlag]
+		if okOld && okNew {
+			return fmt.Errorf("cannot set %s and %s at the same time in the config file, please use %s only", oldFlag, newFlag, newFlag)
+		}
+	}
 	getBoolFlagVal := func(flagName string) *bool {
 		flagVal, ok := cfgMap[flagName]
 		if !ok {
@@ -1084,8 +1102,14 @@ func (cfg *Config) Validate() error {
 		return fmt.Errorf("setting experimental-enable-lease-checkpoint-persist requires experimental-enable-lease-checkpoint")
 	}
 
-	if cfg.ExperimentalCompactHashCheckTime <= 0 {
-		return fmt.Errorf("--experimental-compact-hash-check-time must be >0 (set to %v)", cfg.ExperimentalCompactHashCheckTime)
+	if cfg.ExperimentalCompactHashCheckTime < 0 {
+		return fmt.Errorf("--experimental-compact-hash-check-time must be >=0 (set to %v)", cfg.ExperimentalCompactHashCheckTime)
+	}
+	if cfg.ExperimentalCompactHashCheckTime > 0 {
+		cfg.CompactHashCheckTime = cfg.ExperimentalCompactHashCheckTime
+	}
+	if cfg.CompactHashCheckTime <= 0 {
+		return fmt.Errorf("--compact-hash-check-time must be >0 (set to %v)", cfg.CompactHashCheckTime)
 	}
 
 	// If `--name` isn't configured, then multiple members may have the same "default" name.

server/embed/config_test.go

@@ -98,6 +98,7 @@ func TestConfigFileFeatureGates(t *testing.T) {
 		serverFeatureGatesJSON              string
 		experimentalStopGRPCServiceOnDefrag string
 		experimentalInitialCorruptCheck     string
+		experimentalCompactHashCheckEnabled string
 		expectErr                           bool
 		expectedFeatures                    map[featuregate.Feature]bool
 	}{
@@ -194,12 +195,46 @@ func TestConfigFileFeatureGates(t *testing.T) {
 				features.InitialCorruptCheck:     false,
 			},
 		},
+		{
+			name:                                "cannot set both experimental flag and feature gate flag for ExperimentalCompactHashCheckEnabled",
+			serverFeatureGatesJSON:              "CompactHashCheck=true",
+			experimentalCompactHashCheckEnabled: "false",
+			expectErr:                           true,
+		},
+		{
+			name:                                "can set feature gate experimentalCompactHashCheckEnabled to true from experimental flag",
+			experimentalCompactHashCheckEnabled: "true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+				features.DistributedTracing:      false,
+				features.CompactHashCheck:        true,
+			},
+		},
+		{
+			name:                                "can set feature gate experimentalCompactHashCheckEnabled to false from experimental flag",
+			experimentalCompactHashCheckEnabled: "false",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+				features.DistributedTracing:      false,
+				features.CompactHashCheck:        false,
+			},
+		},
+		{
+			name:                   "can set feature gate CompactHashCheck to true from feature gate flag",
+			serverFeatureGatesJSON: "CompactHashCheck=true",
+			expectedFeatures: map[featuregate.Feature]bool{
+				features.StopGRPCServiceOnDefrag: false,
+				features.DistributedTracing:      false,
+				features.CompactHashCheck:        true,
+			},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			yc := struct {
 				ExperimentalStopGRPCServiceOnDefrag *bool  `json:"experimental-stop-grpc-service-on-defrag,omitempty"`
 				ExperimentalInitialCorruptCheck     *bool  `json:"experimental-initial-corrupt-check,omitempty"`
+				ExperimentalCompactHashCheckEnabled *bool  `json:"experimental-compact-hash-check-enabled,omitempty"`
 				ServerFeatureGatesJSON              string `json:"feature-gates"`
 			}{
 				ServerFeatureGatesJSON: tc.serverFeatureGatesJSON,
@@ -221,6 +256,14 @@ func TestConfigFileFeatureGates(t *testing.T) {
 				yc.ExperimentalStopGRPCServiceOnDefrag = &experimentalStopGRPCServiceOnDefrag
 			}
 
+			if tc.experimentalCompactHashCheckEnabled != "" {
+				experimentalCompactHashCheckEnabled, err := strconv.ParseBool(tc.experimentalCompactHashCheckEnabled)
+				if err != nil {
+					t.Fatal(err)
+				}
+				yc.ExperimentalCompactHashCheckEnabled = &experimentalCompactHashCheckEnabled
+			}
+
 			b, err := yaml.Marshal(&yc)
 			if err != nil {
 				t.Fatal(err)
@@ -248,6 +291,83 @@ func TestConfigFileFeatureGates(t *testing.T) {
 	}
 }
 
+func TestCompactHashCheckTime(t *testing.T) {
+	testCases := []struct {
+		name                             string
+		compactHashCheckTime             string
+		experimentalCompactHashCheckTime string
+		expectErr                        bool
+		expectedCompactHashCheckTime     time.Duration
+	}{
+		{
+			name:                         "default",
+			expectedCompactHashCheckTime: time.Minute,
+		},
+		{
+			name:                             "cannot set both experimental flag and non experimental flag",
+			compactHashCheckTime:             "2m",
+			experimentalCompactHashCheckTime: "3m",
+			expectErr:                        true,
+		},
+		{
+			name:                             "can set experimental flag",
+			experimentalCompactHashCheckTime: "3m",
+			expectedCompactHashCheckTime:     3 * time.Minute,
+		},
+		{
+			name:                         "can set non experimental flag",
+			compactHashCheckTime:         "2m",
+			expectedCompactHashCheckTime: 2 * time.Minute,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			yc := struct {
+				ExperimentalCompactHashCheckTime time.Duration `json:"experimental-compact-hash-check-time,omitempty"`
+				CompactHashCheckTime             time.Duration `json:"compact-hash-check-time,omitempty"`
+			}{}
+
+			if tc.compactHashCheckTime != "" {
+				compactHashCheckTime, err := time.ParseDuration(tc.compactHashCheckTime)
+				if err != nil {
+					t.Fatal(err)
+				}
+				yc.CompactHashCheckTime = compactHashCheckTime
+			}
+
+			if tc.experimentalCompactHashCheckTime != "" {
+				experimentalCompactHashCheckTime, err := time.ParseDuration(tc.experimentalCompactHashCheckTime)
+				if err != nil {
+					t.Fatal(err)
+				}
+				yc.ExperimentalCompactHashCheckTime = experimentalCompactHashCheckTime
+			}
+
+			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 {
+					t.Fatal("expect parse error")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+			if cfg.CompactHashCheckTime != tc.expectedCompactHashCheckTime {
+				t.Errorf("expected CompactHashCheckTime=%v, got %v", tc.expectedCompactHashCheckTime, cfg.CompactHashCheckTime)
+			}
+		})
+	}
+}
+
 // TestUpdateDefaultClusterFromName ensures that etcd can start with 'etcd --name=abc'.
 func TestUpdateDefaultClusterFromName(t *testing.T) {
 	cfg := NewConfig()

server/embed/etcd.go

@@ -205,8 +205,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
 		CORS:                                     cfg.CORS,
 		HostWhitelist:                            cfg.HostWhitelist,
 		CorruptCheckTime:                         cfg.ExperimentalCorruptCheckTime,
-		CompactHashCheckEnabled:                  cfg.ExperimentalCompactHashCheckEnabled,
-		CompactHashCheckTime:                     cfg.ExperimentalCompactHashCheckTime,
+		CompactHashCheckTime:                     cfg.CompactHashCheckTime,
 		PreVote:                                  cfg.PreVote,
 		Logger:                                   cfg.logger,
 		ForceNewCluster:                          cfg.ForceNewCluster,
@@ -351,9 +350,9 @@ func print(lg *zap.Logger, ec Config, sc config.ServerConfig, memberInitialized
 		zap.Uint32("max-concurrent-streams", sc.MaxConcurrentStreams),
 
 		zap.Bool("pre-vote", sc.PreVote),
+		zap.String(ServerFeatureGateFlagName, sc.ServerFeatureGate.String()),
 		zap.Bool("initial-corrupt-check", sc.InitialCorruptCheck),
 		zap.String("corrupt-check-time-interval", sc.CorruptCheckTime.String()),
-		zap.Bool("compact-check-time-enabled", sc.CompactHashCheckEnabled),
 		zap.Duration("compact-check-time-interval", sc.CompactHashCheckTime),
 		zap.String("auto-compaction-mode", sc.AutoCompactionMode),
 		zap.Duration("auto-compaction-retention", sc.AutoCompactionRetention),

server/etcdmain/config.go

@@ -260,6 +260,13 @@ func (cfg *config) configFromCmdLine() error {
 		cfg.ec.InitialCluster = ""
 	}
 
+	// make sure there is no conflict in the flag settings in the ExperimentalFlagMigrationMap
+	for oldFlag, newFlag := range embed.ExperimentalFlagMigrationMap {
+		if flags.IsSet(cfg.cf.flagSet, oldFlag) && flags.IsSet(cfg.cf.flagSet, newFlag) {
+			return fmt.Errorf("cannot set --%s and --%s at the same time, please use --%s only", oldFlag, newFlag, newFlag)
+		}
+	}
+
 	getBoolFlagVal := func(flagName string) *bool {
 		boolVal, parseErr := flags.GetBoolFlagVal(cfg.cf.flagSet, flagName)
 		if parseErr != nil {