migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries #19352
Conversation
|
Hi @ajaysundark. 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 24 files with indirect coverage changes @@ Coverage Diff @@
## main #19352 +/- ##
==========================================
- Coverage 68.90% 68.83% -0.08%
==========================================
Files 420 420
Lines 35723 35731 +8
==========================================
- Hits 24616 24596 -20
- Misses 9677 9712 +35
+ Partials 1430 1423 -7 Continue to review full report in Codecov by Sentry.
|
|
/ok-to-test |
| @@ -910,6 +922,15 @@ func (cfg *configYAML) configFromFile(path string) error { | |||
| cfg.FlagsExplicitlySet[flg] = true | |||
| } | |||
|
|
|||
| // attempt to fix a bug introduced in https://github.com/etcd-io/etcd/pull/15033 | |||
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ajaysundark, siyuanfoundation 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 |
|
Need to change in tests/framework/e2e/cluster.go func WithSnapshotCatchUpEntries(count uint64) EPClusterOption {
-- return func(c *EtcdProcessClusterConfig) { c.ServerConfig.SnapshotCatchUpEntries = count }
++ return func(c *EtcdProcessClusterConfig) { c.ServerConfig.ExperimentalSnapshotCatchUpEntries = count }
}to keep using the We do not want to use the non-experimental flag yet in those tests because of |
ajaysundark
force-pushed
the
migrate-experimental-snapshot-catch-up-entries
branch
from
da2d4d8 to
4605e40
Compare
ajaysundark
force-pushed
the
migrate-experimental-snapshot-catch-up-entries
branch
2 times, most recently
from
d39df53 to
09c560c
Compare
…p-entries Signed-off-by: Ajay Sundar Karuppasamy <[email protected]>
ajaysundark
force-pushed
the
migrate-experimental-snapshot-catch-up-entries
branch
from
d11c45d to
30f96c0
Compare
|
/retest |
|
This is blocked by #19354 as we have test framework dependency on the For example:
|
| @@ -910,6 +922,15 @@ func (cfg *configYAML) configFromFile(path string) error { | |||
| cfg.FlagsExplicitlySet[flg] = true | |||
| } | |||
|
|
|||
| // attempt to fix a bug introduced in https://github.com/etcd-io/etcd/pull/15033 | |||
| // Deprecated in v3.6 and will be removed in v3.7. | ||
| // TODO: remove in v3.7. | ||
| ExperimentalSnapshotCatchUpEntries uint64 `json:"experimental-snapshot-catch-up-entries"` |
There was a problem hiding this comment.
| // Deprecated in v3.6 and will be removed in v3.7. | |
| // TODO: remove in v3.7. | |
| ExperimentalSnapshotCatchUpEntries uint64 `json:"experimental-snapshot-catch-up-entries"` | |
| // Deprecated in v3.6 and will be removed in v3.7. | |
| // TODO: remove in v3.7. | |
| // Note we made a mistake in https://github.com/etcd-io/etcd/pull/15033. The json tag | |
| // `*-catch-up-*` isn't consistent with the command line flag `*-catchup-*`. | |
| ExperimentalSnapshotCatchUpEntries uint64 `json:"experimental-snapshot-catch-up-entries"` |
There was a problem hiding this comment.
Pls feel free to address this comment separately in release-3.6 (will be be cut later today my time) only if you want
|
/retest |
|
/test pull-etcd-e2e-amd64 |
|
/test pull-etcd-e2e-amd64 |
|
@ajaysundark: 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. |
| @@ -229,7 +229,7 @@ func WithSnapshotCount(count uint64) EPClusterOption { | |||
| } | |||
|
|
|||
| func WithSnapshotCatchUpEntries(count uint64) EPClusterOption { | |||
| return func(c *EtcdProcessClusterConfig) { c.ServerConfig.SnapshotCatchUpEntries = count } | |||
| return func(c *EtcdProcessClusterConfig) { c.ServerConfig.ExperimentalSnapshotCatchUpEntries = count } | |||
There was a problem hiding this comment.
The e2e test failure of TestRecoverSnapshotBackend is caused by this change. You need to update the line below to use the ExperimentalSnapshotCatchUpEntries
etcd/tests/robustness/failpoint/network.go
Line 147 in 3db3468
There was a problem hiding this comment.
Or it might be even better to set both SnapshotCatchUpEntries and ExperimentalSnapshotCatchUpEntries in this function WithSnapshotCatchUpEntries; then no need to update the line below,
etcd/tests/robustness/failpoint/network.go
Line 147 in 3db3468
There was a problem hiding this comment.
Thanks for your review, @ahrtr. I considered setting both the flags at test framework, but we explicitly disallow doing so (we validate for both flags being set), as that could allow misconfigurations in real world use-cases.
There was a problem hiding this comment.
I am aware of that setting both flags are disallowed, but what we are talking about here is just test code.
Also note that the robustness test cases test both main and release branches, while ExperimentalSnapshotCatchUpEntries doesn't exist in stable release branches, so setting both fields here might be best choice.
|
Note that I will cut branch |
This might not be correct. We can still merge this to main, then backport to 3.6. Eventually we will cleanup all experimental flags in main. |
|
PR needs rebase. 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. |
Migrate
experimental-snapshot-catchup-entriesflag tosnapshot-catchup-entriesSub task from #19141
Note:
experimental-snapshot-catch-up-entrieswhen cmd-line flagexperimental-snapshot-catchup-entrieswas used.experimental-snapshot-catch-up-entrieswill set flagexperimental-snapshot-catchup-entriescorrectly atFlagsExplicitlySet. This is required to mark / identify the field if used from 'config-file' as a deprecated flag usage.experimental-snapshot-catch-up-entriesYAML field usage is maintained the same field-name for backward compability.