Add downgrade cancellation e2e tests #19244
Conversation
henrybear327
force-pushed
the
e2e/downgrade_cancellation_e2e
branch
from
547c87e to
e1cbb11
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 23 files with indirect coverage changes @@ Coverage Diff @@
## main #19244 +/- ##
==========================================
+ Coverage 68.77% 68.80% +0.02%
==========================================
Files 420 420
Lines 35649 35654 +5
==========================================
+ Hits 24518 24531 +13
+ Misses 9705 9693 -12
- Partials 1426 1430 +4 Continue to review full report in Codecov by Sentry.
|
henrybear327
force-pushed
the
e2e/downgrade_cancellation_e2e
branch
from
e1cbb11 to
f1ff53e
Compare
henrybear327
force-pushed
the
e2e/downgrade_cancellation_e2e
branch
from
e2805ef to
d5d064b
Compare
Signed-off-by: Chun-Hung Tseng <[email protected]> Co-authored-by: Benjamin Wang <[email protected]>
henrybear327
force-pushed
the
e2e/downgrade_cancellation_e2e
branch
from
d5d064b to
4b25f0f
Compare
| t.Logf("Starting downgrade process to %q", lastVersionStr) | ||
| e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), currentVersion, lastClusterVersion) | ||
| err = e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), currentVersion, lastClusterVersion) |
There was a problem hiding this comment.
@siyuanfoundation is there a reason that the return value of DowngradeUpgradeMembers is ignored? :)
Signed-off-by: Chun-Hung Tseng <[email protected]>
henrybear327
force-pushed
the
e2e/downgrade_cancellation_e2e
branch
from
bb4669c to
b795e73
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, henrybear327 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 |
| if triggerCancellation == cancelRightBeforeEnable { | ||
| t.Logf("Cancelling downgrade before enabling") | ||
| e2e.DowngradeCancel(t, epc, generateIdenticalVersions(clusterSize, currentVersionStr)) | ||
| return // No need to perform downgrading, end the test here |
There was a problem hiding this comment.
I think the test should have some validation, like check if cluster version of all members. For cancellation before and right after enable we the cluster version should stay the same. Also would be good to confirm the state of downgrade.
There was a problem hiding this comment.
Maybe I am missing something.
The generateIdenticalVersions function generates the versions for the nodes that should be checked against already, no?
There was a problem hiding this comment.
Oh, I didn't noticed that e2e.DowngradeCancel is also doing validation. Any reason to combine it? I would recommend to either split it or make it clear from function name that you are validating.
Like
epc.Etcdutl().DowngradeCancel(context.TODO())
e2e.ValidateMemberVersions(epc, currentVersionStr)
| func DowngradeCancel(t *testing.T, epc *EtcdProcessCluster, versions []*version.Versions) { | ||
| t.Logf("etcdctl downgrade cancel") | ||
| c := epc.Etcdctl() | ||
| testutils.ExecuteWithTimeout(t, 20*time.Second, func() { |
There was a problem hiding this comment.
Why set timeout here? Don't remember, but shouldn't the context in DowngradeCancel work for timeout?
Part 1 of the e2e test - downgrade cancellation is called before any of the node is actually downgraded
A README fix was also associated with this PR, as downgrade command doesn't seem to take version as a parameter.
Reference: #17976
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.