Skip to content

robustness-test: Migrate experimental-compaction-batch-limit flag #19506

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

robustness-test: Migrate experimental-compaction-batch-limit flag #19506

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
26 changes: 22 additions & 4 deletions tests/framework/e2e/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"time"

"github.com/coreos/go-semver/semver"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"

Expand Down Expand Up @@ -645,8 +646,11 @@ func (cfg *EtcdProcessClusterConfig) EtcdServerProcessConfig(tb testing.TB, i in
}
}

defaultValues := values(*embed.NewConfig())
overrideValues := values(cfg.ServerConfig)
version, err := GetVersionFromBinary(execPath)
require.NoError(tb, err)

defaultValues := values(version, *embed.NewConfig())
overrideValues := values(version, cfg.ServerConfig)
for flag, value := range overrideValues {
if defaultValue := defaultValues[flag]; value == "" || value == defaultValue {
continue
Expand Down Expand Up @@ -727,16 +731,30 @@ func (epc *EtcdProcessCluster) MinServerVersion() (*semver.Version, error) {
return minVersion, nil
}

func values(cfg embed.Config) map[string]string {
// canonicalFlagName takes any flag name and returns it's canonical name
// handling the experimental flags in different versions
func canonicalFlagName(version *semver.Version, name string) string {
v3_6 := semver.Version{Major: 3, Minor: 6}
if name == "compaction-batch-limit" {
if version.Compare(v3_6) < 0 {
name = "experimental-compaction-batch-limit"
}
}
return name
}

func values(version *semver.Version, cfg embed.Config) map[string]string {
fs := flag.NewFlagSet("etcd", flag.ContinueOnError)

cfg.AddFlags(fs)
values := map[string]string{}
fs.VisitAll(func(f *flag.Flag) {
name := canonicalFlagName(version, f.Name)
Copy link
Member

Choose a reason for hiding this comment

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

This approach will not work because rendering flags happens before we run the cluster and we never re-render them. There are tests that upgrade/downgrade binary while using same args. The proper solution would be to re-render the flags every time we start binary.

For that to work we we would want to rewrite EtcdServerProcessConfig to not include Args field, but Args method, however this is pretty complicated rewrite.

Copy link
Author

@kavirajk kavirajk Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @serathius for catching that :)

I'm assuming you are talking this code about upgrade/downgrade the etcd process with newPath/newVersion by using same args. correct?

I like your idea of making EtcdServerProcessConfig's Args more dynamic by making it as method instead (keeping the filed args as private). So during actual spawning of the process, it will call ep.cfg.Args() and that will return canonical flags based on version derived from it's ep.cfg.ExecPath. Do that sounds good to you? wdyt?

however this is pretty complicated rewrite.

Your concern here is I believe is mostly extra work needed to update it in every places. Or am I missing anything?

One more clarification. Any efficient way to run e2e-tests in local? Every time I run make test-e2e it takes for ever even to start (and I think we need approval to trigger from CI)

Copy link
Author

Choose a reason for hiding this comment

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

Thinking bit more about this.

On the other hand, this is only needed for specific cases in Robustness tests. Making it part of original EtcdServerProcessConfig seems bit irrelevant. Plus Args() api then need to know the version to return correct Args and in order to extract it from binary path introduce a failure case, that this api has to handle. All feel bit un-necessary to put it in EtcdServerProcessConfig.

Because by general definition of this config, once the it is created, it's Args are fixed for a single binary. It's our use case of downgrade and upgrade in robustness tests makes this problematic. So we can have a helper(version, args) args and use it just in robustness tests before spawning the server. Just like the way we do with newExecPath in those tests.

I don't know. Just thinking out loud. curious to know what you think :)

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, this is only needed for specific cases in Robustness tests.

Don't think this is specific to robustness tests, but the upgrade/downgrade tests which is one of the scenarios of robustness tests. There are other dedicated upgrade/downgrade e2e tests running.

So we can have a helper(version, args) args and use it just in robustness tests before spawning the server.

I was thinking about similar approach as a midpoint. Calling this helper in etcdServer.Start to fix the flags just before they are used. I think it's an acceptable midstep, but I would be worried about leaving such hacks for long.

value := f.Value.String()
if value == "false" || value == "0" {
value = ""
}
values[f.Name] = value
values[name] = value
})
return values
}
Expand Down
29 changes: 29 additions & 0 deletions tests/framework/e2e/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ import (
func TestEtcdServerProcessConfig(t *testing.T) {
v3_5_12 := semver.Version{Major: 3, Minor: 5, Patch: 12}
v3_5_14 := semver.Version{Major: 3, Minor: 5, Patch: 14}

v3_4 := semver.Version{Major: 3, Minor: 4}
v3_5 := semver.Version{Major: 3, Minor: 5}
v3_6 := semver.Version{Major: 3, Minor: 6}

tcs := []struct {
name string
config *EtcdProcessClusterConfig
Expand Down Expand Up @@ -107,6 +112,30 @@ func TestEtcdServerProcessConfig(t *testing.T) {
"--listen-client-http-urls=http://localhost:4",
},
},
{
name: "CompactionBatchLimit_v3.6",
config: NewConfig(WithCompactionBatchLimit(10)),
expectArgsContain: []string{
"--compaction-batch-limit",
},
mockBinaryVersion: &v3_6,
},
{
name: "CompactionBatchLimit_v3.5",
config: NewConfig(WithCompactionBatchLimit(10)),
expectArgsContain: []string{
"--experimental-compaction-batch-limit",
},
mockBinaryVersion: &v3_5,
},
{
name: "CompactionBatchLimit_v3.4",
config: NewConfig(WithCompactionBatchLimit(10)),
expectArgsContain: []string{
"--experimental-compaction-batch-limit",
},
mockBinaryVersion: &v3_4,
},
{
name: "ForceNewCluster",
config: NewConfig(WithForceNewCluster(true)),
Expand Down