-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Fix passing compaction-batch-limit to etcd v3.4 and v3.5 #19218
base: main
Are you sure you want to change the base?
Conversation
@serathius: GitHub didn't allow me to request PR reviews from the following users: gangli113. Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: serathius The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 23 files with indirect coverage changes @@ Coverage Diff @@
## main #19218 +/- ##
==========================================
+ Coverage 68.76% 68.84% +0.08%
==========================================
Files 420 420
Lines 35638 35650 +12
==========================================
+ Hits 24506 24543 +37
+ Misses 9705 9688 -17
+ Partials 1427 1419 -8 Continue to review full report in Codecov by Sentry.
|
4d0a5fa
to
2ac891f
Compare
2ac891f
to
3824aed
Compare
3824aed
to
4db379d
Compare
4db379d
to
bb26c63
Compare
if flag == "compaction-batch-limit" { | ||
flag = "experimental-compaction-batch-limit" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
for _, opt := range opts { | ||
opt(&cfg) | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
bb26c63
to
f9e37d2
Compare
Signed-off-by: Marek Siarkowicz <[email protected]>
f9e37d2
to
b4a0981
Compare
/retest |
@serathius: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
PR #19196 broke robustness test periodics.
/cc @ahrtr @siyuanfoundation @gangli113