Merge pull request #19352 from ajaysundark/migrate-experimental-snapshot-catch-up-entries

migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries

Author: ahrtr

server/embed/config.go

@@ -156,6 +156,7 @@ var (
 		"experimental-bootstrap-defrag-threshold-megabytes": "bootstrap-defrag-threshold-megabytes",
 		"experimental-max-learners":                         "max-learners",
 		"experimental-memory-mlock":                         "memory-mlock",
+		"experimental-snapshot-catchup-entries":             "snapshot-catchup-entries",
 		"experimental-compaction-sleep-interval":            "compaction-sleep-interval",
 		"experimental-downgrade-check-time":                 "downgrade-check-time",
 		"experimental-peer-skip-client-san-verification":    "peer-skip-client-san-verification",
@@ -199,12 +200,23 @@ type Config struct {
 	// TODO: remove it in 3.7.
 	SnapshotCount uint64 `json:"snapshot-count"`
 
-	// SnapshotCatchUpEntries is the number of entries for a slow follower
+	// ExperimentalSnapshotCatchUpEntries is the number of entries for a slow follower
 	// to catch-up after compacting the raft storage entries.
 	// We expect the follower has a millisecond level latency with the leader.
 	// The max throughput is around 10K. Keep a 5K entries is enough for helping
 	// follower to catch up.
-	SnapshotCatchUpEntries uint64 `json:"experimental-snapshot-catch-up-entries"`
+	// Deprecated in v3.6 and will be removed in v3.7.
+	// TODO: remove in v3.7.
+	// Note we made a mistake in https://github.com/etcd-io/etcd/pull/15033. The json tag
+	// `*-catch-up-*` isn't consistent with the command line flag `*-catchup-*`.
+	ExperimentalSnapshotCatchUpEntries uint64 `json:"experimental-snapshot-catch-up-entries"`
+
+	// SnapshotCatchUpEntries is the number of entires for a slow follower
+	// to catch-up after compacting the raft storage entries.
+	// We expect the follower has a millisecond level latency with the leader.
+	// The max throughput is around 10K. Keep a 5K entries is enough for helping
+	// follower to catch up.
+	SnapshotCatchUpEntries uint64 `json:"snapshot-catchup-entries"`
 
 	// MaxSnapFiles is deprecated in v3.6 and will be decommissioned in v3.7.
 	// TODO: remove it in 3.7.
@@ -631,8 +643,9 @@ func NewConfig() *Config {
 
 		Name: DefaultName,
 
-		SnapshotCount:          etcdserver.DefaultSnapshotCount,
-		SnapshotCatchUpEntries: etcdserver.DefaultSnapshotCatchUpEntries,
+		SnapshotCount:                      etcdserver.DefaultSnapshotCount,
+		ExperimentalSnapshotCatchUpEntries: etcdserver.DefaultSnapshotCatchUpEntries,
+		SnapshotCatchUpEntries:             etcdserver.DefaultSnapshotCatchUpEntries,
 
 		MaxTxnOps:            DefaultMaxTxnOps,
 		MaxRequestBytes:      DefaultMaxRequestBytes,
@@ -935,7 +948,8 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) {
 	// TODO: delete in v3.7
 	fs.IntVar(&cfg.ExperimentalMaxLearners, "experimental-max-learners", membership.DefaultMaxLearners, "Sets the maximum number of learners that can be available in the cluster membership. Deprecated in v3.6 and will be decommissioned in v3.7. Use --max-learners instead.")
 	fs.IntVar(&cfg.MaxLearners, "max-learners", membership.DefaultMaxLearners, "Sets the maximum number of learners that can be available in the cluster membership.")
-	fs.Uint64Var(&cfg.SnapshotCatchUpEntries, "experimental-snapshot-catchup-entries", cfg.SnapshotCatchUpEntries, "Number of entries for a slow follower to catch up after compacting the raft storage entries.")
+	fs.Uint64Var(&cfg.ExperimentalSnapshotCatchUpEntries, "experimental-snapshot-catchup-entries", cfg.ExperimentalSnapshotCatchUpEntries, "Number of entries for a slow follower to catch up after compacting the raft storage entries. Deprecated in v3.6 and will be decommissioned in v3.7. Use --snapshot-catchup-entries instead.")
+	fs.Uint64Var(&cfg.SnapshotCatchUpEntries, "snapshot-catchup-entries", cfg.SnapshotCatchUpEntries, "Number of entries for a slow follower to catch up after compacting the raft storage entries.")
 
 	// unsafe
 	fs.BoolVar(&cfg.UnsafeNoFsync, "unsafe-no-fsync", false, "Disables fsync, unsafe, will cause data loss.")
@@ -994,6 +1008,15 @@ func (cfg *configYAML) configFromFile(path string) error {
 		}
 	}
 
+	// attempt to fix a bug introduced in https://github.com/etcd-io/etcd/pull/15033
+	// both `experimental-snapshot-catch-up-entries` and `experimental-snapshot-catchup-entries` refer to the same field,
+	// 	map the YAML field "experimental-snapshot-catch-up-entries" to the flag "experimental-snapshot-catchup-entries".
+	if val, ok := cfgMap["experimental-snapshot-catch-up-entries"]; ok {
+		cfgMap["experimental-snapshot-catchup-entries"] = val
+		cfg.ExperimentalSnapshotCatchUpEntries = uint64(val.(float64))
+		cfg.FlagsExplicitlySet["experimental-snapshot-catchup-entries"] = true
+	}
+
 	getBoolFlagVal := func(flagName string) *bool {
 		flagVal, ok := cfgMap[flagName]
 		if !ok {

server/etcdmain/config.go

@@ -72,6 +72,7 @@ var (
 		"experimental-bootstrap-defrag-threshold-megabytes": "--experimental-bootstrap-defrag-threshold-megabytes is deprecated in v3.6 and will be decommissioned in v3.7. Use '--bootstrap-defrag-threshold-megabytes' instead.",
 		"experimental-max-learners":                         "--experimental-max-learners is deprecated in v3.6 and will be decommissioned in v3.7. Use '--max-learners' instead.",
 		"experimental-memory-mlock":                         "--experimental-memory-mlock is deprecated in v3.6 and will be decommissioned in v3.7. Use '--memory-mlock' instead.",
+		"experimental-snapshot-catchup-entries":             "--experimental-snapshot-catchup-entries is deprecated in v3.6 and will be decommissioned in v3.7. Use '--snapshot-catchup-entries' instead.",
 		"experimental-compaction-sleep-interval":            "--experimental-compaction-sleep-interval is deprecated in v3.6 and will be decommissioned in v3.7. Use 'compaction-sleep-interval' instead.",
 		"experimental-downgrade-check-time":                 "--experimental-downgrade-check-time is deprecated in v3.6 and will be decommissioned in v3.7. Use '--downgrade-check-time' instead.",
 		"experimental-enable-distributed-tracing":           "--experimental-enable-distributed-tracing is deprecated in 3.6 and will be decommissioned in 3.7. Use --enable-distributed-tracing instead.",
@@ -219,6 +220,10 @@ func (cfg *config) parse(arguments []string) error {
 		cfg.ec.MemoryMlock = cfg.ec.ExperimentalMemoryMlock
 	}
 
+	if cfg.ec.FlagsExplicitlySet["experimental-snapshot-catchup-entries"] {
+		cfg.ec.SnapshotCatchUpEntries = cfg.ec.ExperimentalSnapshotCatchUpEntries
+	}
+
 	if cfg.ec.FlagsExplicitlySet["experimental-compaction-sleep-interval"] {
 		cfg.ec.CompactionSleepInterval = cfg.ec.ExperimentalCompactionSleepInterval
 	}

server/etcdmain/config_test.go

@@ -33,6 +33,7 @@ import (
 	"go.etcd.io/etcd/pkg/v3/featuregate"
 	"go.etcd.io/etcd/pkg/v3/flags"
 	"go.etcd.io/etcd/server/v3/embed"
+	"go.etcd.io/etcd/server/v3/etcdserver"
 	"go.etcd.io/etcd/server/v3/features"
 )
 
@@ -67,7 +68,7 @@ func TestConfigFileMemberFields(t *testing.T) {
 		MaxWALFiles            uint   `json:"max-wals"`
 		Name                   string `json:"name"`
 		SnapshotCount          uint64 `json:"snapshot-count"`
-		SnapshotCatchUpEntries uint64 `json:"experimental-snapshot-catch-up-entries"`
+		SnapshotCatchUpEntries uint64 `json:"snapshot-catchup-entries"`
 		ListenPeerURLs         string `json:"listen-peer-urls"`
 		ListenClientURLs       string `json:"listen-client-urls"`
 		ListenClientHTTPURLs   string `json:"listen-client-http-urls"`
@@ -321,6 +322,73 @@ func TestConfigParsingMissedAdvertiseClientURLsFlag(t *testing.T) {
 	}
 }
 
+// TestExperimentalSnapshotCatchUpEntriesFlagMigration tests the migration from
+// --experimental-snapshot-catch-up-entries to --snapshot-catch-up-entries
+func TestExperimentalSnapshotCatchUpEntriesFlagMigration(t *testing.T) {
+	testCases := []struct {
+		name                               string
+		snapshotCatchUpEntries             uint64
+		experimentalSnapshotCatchUpEntries uint64
+		wantErr                            bool
+		wantConfig                         uint64
+	}{
+		{
+			name:       "default",
+			wantConfig: etcdserver.DefaultSnapshotCatchUpEntries,
+		},
+		{
+			name:                               "cannot set both experimental flag and non experimental flag",
+			experimentalSnapshotCatchUpEntries: 1000,
+			snapshotCatchUpEntries:             2000,
+			wantErr:                            true,
+		},
+		{
+			name:                               "can set experimental flag",
+			experimentalSnapshotCatchUpEntries: 1000,
+			wantConfig:                         1000,
+		},
+		{
+			name:                   "can set non-experimental flag",
+			snapshotCatchUpEntries: 2000,
+			wantConfig:             2000,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			cmdLineArgs := []string{}
+			yc := struct {
+				ExperimentalSnapshotCatchUpEntries uint64 `json:"experimental-snapshot-catch-up-entries,omitempty"`
+				SnapshotCatchUpEntries             uint64 `json:"snapshot-catchup-entries,omitempty"`
+			}{}
+
+			if tc.snapshotCatchUpEntries > 0 {
+				cmdLineArgs = append(cmdLineArgs, fmt.Sprintf("--snapshot-catchup-entries=%d", tc.snapshotCatchUpEntries))
+				yc.SnapshotCatchUpEntries = tc.snapshotCatchUpEntries
+			}
+
+			if tc.experimentalSnapshotCatchUpEntries > 0 {
+				cmdLineArgs = append(cmdLineArgs, fmt.Sprintf("--experimental-snapshot-catchup-entries=%d", tc.experimentalSnapshotCatchUpEntries))
+				yc.ExperimentalSnapshotCatchUpEntries = tc.experimentalSnapshotCatchUpEntries
+			}
+
+			cfgFromCmdLine, errFromCmdLine, cfgFromFile, errFromFile := generateCfgsFromFileAndCmdLine(t, yc, cmdLineArgs)
+
+			if tc.wantErr {
+				if errFromCmdLine == nil || errFromFile == nil {
+					t.Fatal("expect parse error")
+				}
+				return
+			}
+			if errFromCmdLine != nil || errFromFile != nil {
+				t.Fatal("error parsing config")
+			}
+
+			require.Equal(t, tc.wantConfig, cfgFromCmdLine.ec.SnapshotCatchUpEntries)
+			require.Equal(t, tc.wantConfig, cfgFromFile.ec.SnapshotCatchUpEntries)
+		})
+	}
+}
+
 func TestConfigIsNewCluster(t *testing.T) {
 	tests := []struct {
 		state  string
@@ -1274,6 +1342,7 @@ func TestConfigFileDeprecatedOptions(t *testing.T) {
 		ExperimentalWarningApplyDuration              time.Duration `json:"experimental-warning-apply-duration,omitempty"`
 		ExperimentalBootstrapDefragThresholdMegabytes uint          `json:"experimental-bootstrap-defrag-threshold-megabytes,omitempty"`
 		ExperimentalMaxLearners                       int           `json:"experimental-max-learners,omitempty"`
+		ExperimentalSnapshotCatchUpEntries            uint64        `json:"experimental-snapshot-catch-up-entries,omitempty"`
 		ExperimentalCompactionSleepInterval           time.Duration `json:"experimental-compaction-sleep-interval,omitempty"`
 		ExperimentalDowngradeCheckTime                time.Duration `json:"experimental-downgrade-check-time,omitempty"`
 	}
@@ -1300,6 +1369,7 @@ func TestConfigFileDeprecatedOptions(t *testing.T) {
 				ExperimentalWarningApplyDuration:              3 * time.Minute,
 				ExperimentalBootstrapDefragThresholdMegabytes: 100,
 				ExperimentalMaxLearners:                       1,
+				ExperimentalSnapshotCatchUpEntries:            1000,
 				ExperimentalCompactionSleepInterval:           30 * time.Second,
 				ExperimentalDowngradeCheckTime:                1 * time.Minute,
 			},
@@ -1312,6 +1382,7 @@ func TestConfigFileDeprecatedOptions(t *testing.T) {
 				"experimental-warning-apply-duration":               {},
 				"experimental-bootstrap-defrag-threshold-megabytes": {},
 				"experimental-max-learners":                         {},
+				"experimental-snapshot-catchup-entries":             {},
 				"experimental-compaction-sleep-interval":            {},
 				"experimental-downgrade-check-time":                 {},
 			},

server/etcdmain/help.go

@@ -342,6 +342,8 @@ Experimental feature:
   --experimental-memory-mlock
     Enable to enforce etcd pages (in particular bbolt) to stay in RAM. Deprecated in v3.6 and will be decommissioned in v3.7. Use '--memory-mlock' instead.
   --experimental-snapshot-catchup-entries
+    Number of entries for a slow follower to catch up after compacting the raft storage entries. Deprecated in v3.6 and will be decommissioned in v3.7. Use '--snapshot-catchup-entries' instead.
+  --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. It's deprecated, and will be decommissioned in v3.7. Use '--feature-gates=StopGRPCServiceOnDefrag=true' instead.

tests/framework/e2e/cluster.go

@@ -229,7 +229,9 @@ func WithSnapshotCount(count uint64) EPClusterOption {
 }
 
 func WithSnapshotCatchUpEntries(count uint64) EPClusterOption {
-	return func(c *EtcdProcessClusterConfig) { c.ServerConfig.SnapshotCatchUpEntries = count }
+	return func(c *EtcdProcessClusterConfig) {
+		c.ServerConfig.SnapshotCatchUpEntries = count
+	}
 }
 
 func WithClusterSize(size int) EPClusterOption {
@@ -455,6 +457,10 @@ func InitEtcdProcessCluster(t testing.TB, cfg *EtcdProcessClusterConfig) (*EtcdP
 		if cfg.Version == LastVersion && !CouldSetSnapshotCatchupEntries(BinPath.EtcdLastRelease) {
 			return nil, fmt.Errorf("cannot set SnapshotCatchUpEntries for last etcd version: %s", BinPath.EtcdLastRelease)
 		}
+		if cfg.Version != CurrentVersion && UsesExperimentalSnapshotCatchupEntriesFlag(BinPath.EtcdLastRelease) {
+			cfg.ServerConfig.ExperimentalSnapshotCatchUpEntries = cfg.ServerConfig.SnapshotCatchUpEntries
+			cfg.ServerConfig.SnapshotCatchUpEntries = etcdserver.DefaultSnapshotCatchUpEntries
+		}
 	}
 
 	etcdCfgs := cfg.EtcdAllServerProcessConfigs(t)
@@ -661,7 +667,7 @@ func (cfg *EtcdProcessClusterConfig) EtcdServerProcessConfig(tb testing.TB, i in
 		if defaultValue := defaultValues[flag]; value == "" || value == defaultValue {
 			continue
 		}
-		if flag == "experimental-snapshot-catchup-entries" && !CouldSetSnapshotCatchupEntries(execPath) {
+		if strings.HasSuffix(flag, "snapshot-catchup-entries") && !CouldSetSnapshotCatchupEntries(execPath) {
 			continue
 		}
 		args = append(args, fmt.Sprintf("--%s=%s", flag, value))

tests/framework/e2e/cluster_test.go

@@ -81,22 +81,22 @@ func TestEtcdServerProcessConfig(t *testing.T) {
 			name:   "CatchUpEntries",
 			config: NewConfig(WithSnapshotCatchUpEntries(100)),
 			expectArgsContain: []string{
-				"--experimental-snapshot-catchup-entries=100",
+				"--snapshot-catchup-entries=100",
 			},
 			mockBinaryVersion: &v3_5_14,
 		},
 		{
 			name:   "CatchUpEntriesNoVersion",
 			config: NewConfig(WithSnapshotCatchUpEntries(100), WithVersion(LastVersion)),
 			expectArgsNotContain: []string{
-				"--experimental-snapshot-catchup-entries=100",
+				"--snapshot-catchup-entries=100",
 			},
 		},
 		{
 			name:   "CatchUpEntriesOldVersion",
 			config: NewConfig(WithSnapshotCatchUpEntries(100), WithVersion(LastVersion)),
 			expectArgsNotContain: []string{
-				"--experimental-snapshot-catchup-entries=100",
+				"--snapshot-catchup-entries=100",
 			},
 			mockBinaryVersion: &v3_5_12,
 		},

tests/framework/e2e/etcd_process.go

@@ -531,3 +531,12 @@ func CouldSetSnapshotCatchupEntries(execPath string) bool {
 	v3_5_14 := semver.Version{Major: 3, Minor: 5, Patch: 14}
 	return v.Compare(v3_5_14) >= 0
 }
+
+func UsesExperimentalSnapshotCatchupEntriesFlag(execPath string) bool {
+	v, err := GetVersionFromBinary(execPath)
+	if err != nil {
+		return false
+	}
+	v3_6 := semver.Version{Major: 3, Minor: 6}
+	return v.LessThan(v3_6)
+}