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.

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

Dynamically generate flags passed to etcd binary #16707

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

serathius
Copy link
Member

@serathius serathius commented Oct 7, 2023

Part of #16673 to randomize etcd configuration we need to expand coverage of flags that can be provided in e2e tests. Instead of manually transforming embed.Config struct fields, we can use flagset to automatically generate flags.

@serathius serathius force-pushed the dynamic-flags branch 4 times, most recently from 9bc7d93 to b4692a0 Compare October 10, 2023 13:33
@serathius serathius marked this pull request as ready for review October 10, 2023 13:58
@serathius
Copy link
Member Author

cc @ahrtr

Comment on lines 610 to 628
var value string
if value != "false" && value != "0" {
value = f.Value.String()
}
values[f.Name] = value
Copy link
Member

@ahrtr ahrtr Oct 10, 2023

Choose a reason for hiding this comment

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

Suggested change
var value string
if value != "false" && value != "0" {
value = f.Value.String()
}
values[f.Name] = value
value := f.Value.String()
if value != "" && value != "false" && value != "0" {
values[f.Name] = value
}

Copy link
Member Author

@serathius serathius Oct 10, 2023

Choose a reason for hiding this comment

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

I think the result is different, but I will double check if there impact on the list of flags generated.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to revert. The code change makes a difference for flags with default value true, that is set to false during the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I have time I will add a unit test for this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you understand my comment. I am talking about a simple golang syntax problem (although not a big problem): you are checking a variable value which is always ""

Copy link
Member Author

Choose a reason for hiding this comment

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

And I responded, that after I applied you change the test started to consistently fail and it needs further investigation.

Copy link
Member Author

Choose a reason for hiding this comment

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

With #16747 I have confirmed that your change breaks logic:

--- FAIL: TestEtcdServerProcessConfig (0.00s)
    --- FAIL: TestEtcdServerProcessConfig/StrictReconfigCheck (0.00s)
        cluster_test.go:214: diff:   []string{
                ... // 11 identical elements
                "/tmp/fake/member-0",
                "--initial-cluster-token=new",
            +   "--strict-reconfig-check=false",
              }
FAIL
FAIL    go.etcd.io/etcd/tests/v3/framework/e2e  0.051s
FAIL

Copy link
Member

Choose a reason for hiding this comment

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

Please forget my original suggested change. Instead, change

		var value string
		if value != "false" && value != "0" {
			value = f.Value.String()
		}

to

value := f.Value.String()

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed updated it. PTAL. Now with unit test it should be clear what was the problem.

ahrtr
ahrtr previously approved these changes Oct 10, 2023
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Nice simplification & refactoring! thx

@serathius
Copy link
Member Author

Flake

@serathius serathius force-pushed the dynamic-flags branch 2 times, most recently from 3aaef78 to 76a07cc Compare October 10, 2023 20:11
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Nice work.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

#16707 (comment)

action needed:

  • fix the simple golang programming problem (always checking a variable which is always "") as I mentioned.
  • resolve related test failure

@serathius
Copy link
Member Author

serathius commented Oct 11, 2023

Created the PR with tests #16707

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius
Copy link
Member Author

ping @ahrtr

@serathius serathius merged commit 6d68ab0 into etcd-io:main Oct 12, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants