robustness-test: Migrate experimental-compaction-batch-limit flag
#19506
Conversation
Fixes: etcd-io#19354 Robustness run different versions of binaries. This flag is experiemental in v3.4 and v3.5. But non-experiemental in v3.6+. This PR adds ability to run different binaries with it's canonical flag name. In robustness tests we pass these config via `embed.ServerConfig` struct and get converted into CLI flags before spawning the etcd server binary. This PR maps into right canonical flag based on versions. So even the experimental field is removed from the struct (in v3.7), the flags will be correct mapped. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kavirajk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @kavirajk. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| cfg.AddFlags(fs) | ||
| values := map[string]string{} | ||
| fs.VisitAll(func(f *flag.Flag) { | ||
| name := canonicalFlagName(version, f.Name) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 :)
Related: #19354
Robustness tests run different versions of binaries. This flag is experimental in v3.4 and v3.5. But non-experimental in v3.6+. This PR adds ability to run different binaries with it's canonical flag name.
In robustness tests we pass these config via
embed.ServerConfigstruct and get converted into CLI flags before spawning the etcd server binary. This PR maps into right canonical flag based on versions. So even when the experimental field is removed from the struct (in v3.7), the flags will be correctly mapped.Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.