Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix passing compaction-batch-limit to etcd v3.4 and v3.5 #19218

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+9 −16
Open

Fix passing compaction-batch-limit to etcd v3.4 and v3.5 #19218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions tests/e2e/corrupt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,22 +329,19 @@ func testCompactHashCheckDetectCorruptionInterrupt(t *testing.T, useFeatureGate
} else {
opts = append(opts, e2e.WithCompactHashCheckEnabled(true))
}
var compactionBatchLimit e2e.EPClusterOption
var flag string
if useExperimentalFlag {
compactionBatchLimit = e2e.WithExperimentalCompactionBatchLimit(1)
flag = "--experimental-compaction-batch-limit=1"
} else {
compactionBatchLimit = e2e.WithCompactionBatchLimit(1)
flag = "--compaction-batch-limit=1"
}

cfg := e2e.NewConfig(opts...)
epc, err := e2e.InitEtcdProcessCluster(t, cfg)
require.NoError(t, err)

// Assign a node a very slow compaction speed, so that its compaction can be interrupted.
err = epc.UpdateProcOptions(slowCompactionNodeIndex, t,
compactionBatchLimit,
e2e.WithCompactionSleepInterval(1*time.Hour),
)
err = epc.UpdateProcOptions(slowCompactionNodeIndex, t, flag, "--experimental-compaction-sleep-interval=1h")
require.NoError(t, err)

epc, err = e2e.StartEtcdProcessCluster(ctx, t, epc, cfg)
Expand Down
14 changes: 5 additions & 9 deletions tests/framework/e2e/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,6 @@ func WithCompactionBatchLimit(limit int) EPClusterOption {
return func(c *EtcdProcessClusterConfig) { c.ServerConfig.CompactionBatchLimit = limit }
}

func WithExperimentalCompactionBatchLimit(limit int) EPClusterOption {
return func(c *EtcdProcessClusterConfig) { c.ServerConfig.ExperimentalCompactionBatchLimit = limit }
}

func WithCompactionSleepInterval(time time.Duration) EPClusterOption {
return func(c *EtcdProcessClusterConfig) { c.ServerConfig.ExperimentalCompactionSleepInterval = time }
}
Expand Down Expand Up @@ -640,6 +636,9 @@ func (cfg *EtcdProcessClusterConfig) EtcdServerProcessConfig(tb testing.TB, i in
if flag == "experimental-snapshot-catchup-entries" && !CouldSetSnapshotCatchupEntries(execPath) {
continue
}
if flag == "compaction-batch-limit" {
flag = "experimental-compaction-batch-limit"
}
Comment on lines +639 to +641
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if flag == "compaction-batch-limit" {
flag = "experimental-compaction-batch-limit"
}
// To be compatible with 3.4 & 3.5, as they don't have flag `--compaction-batch-limit`.
if flag == "compaction-batch-limit" {
flag = "experimental-compaction-batch-limit"
}

But it's still a little tricky. Starting from 3.7, we will remove the experimental flags. So

  • 3.4 & 3.5 only have the experimental flags
  • 3.6 have both flags
  • 3.7 will only have the non-experimental flags.

So we may need to check the binary version and decide to use which flag. It's better to get them wrapped in a separate function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upgrade/downgrade robustness test can set this config, but will not update the flags before restart. We would need to rewrite the e2e framework to reload flags from config on every restart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to rewrite the e2e framework to reload flags from config on every restart.

It seems that we need to reconcile all the config items once we update the binary (i.e. from 3.6 to 3.5, or from 3.5 to 3.6)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the experimentalNonBoolFlagMigrationMap to map the flags in migration for now? It should work before we rewrite the framework functions.

args = append(args, fmt.Sprintf("--%s=%s", flag, value))
}
envVars := map[string]string{}
Expand Down Expand Up @@ -881,16 +880,13 @@ func (epc *EtcdProcessCluster) StartNewProcFromConfig(ctx context.Context, tb te

// UpdateProcOptions updates the options for a specific process. If no opt is set, then the config is identical
// to the cluster.
func (epc *EtcdProcessCluster) UpdateProcOptions(i int, tb testing.TB, opts ...EPClusterOption) error {
func (epc *EtcdProcessCluster) UpdateProcOptions(i int, tb testing.TB, newflags ...string) error {
if epc.Procs[i].IsRunning() {
return fmt.Errorf("process %d is still running, please close it before updating its options", i)
}
cfg := *epc.Cfg
for _, opt := range opts {
opt(&cfg)
}
Comment on lines -889 to -891
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure this won't cause any issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know, but the idea of UpdateProcOptions function is incorrect. We should not reload config like this.

serverCfg := cfg.EtcdServerProcessConfig(tb, i)

serverCfg.Args = append(serverCfg.Args, newflags...)
var initialCluster []string
for _, p := range epc.Procs {
initialCluster = append(initialCluster, fmt.Sprintf("%s=%s", p.Config().Name, p.Config().PeerURL.String()))
Expand Down
Loading