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

[test] Add e2e downgrade automatic cancellation test #19399

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

henrybear327
Copy link
Contributor

@henrybear327 henrybear327 commented Feb 12, 2025

Verify that the downgrade can be cancelled automatically when the downgrade is completed (using no inflight downgrade job as the indicator)

Please see #19365 (comment)

Reference: #17976

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.


Close #17976

@henrybear327 henrybear327 self-assigned this Feb 12, 2025
@henrybear327 henrybear327 changed the title Add e2e downgrade automatic cancellation test [test] Add e2e downgrade automatic cancellation test Feb 12, 2025
@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch 2 times, most recently from 995bf3e to 60e8a40 Compare February 12, 2025 12:10
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.33%. Comparing base (29878cf) to head (a514ebe).
Report is 1 commits behind head on main.

Additional details and impacted files

see 89 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19399      +/-   ##
==========================================
+ Coverage   64.12%   69.33%   +5.20%     
==========================================
  Files         421      403      -18     
  Lines       35854    35293     -561     
==========================================
+ Hits        22993    24470    +1477     
+ Misses      11443     9387    -2056     
- Partials     1418     1436      +18     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e27246...a514ebe. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch from 60e8a40 to 11357be Compare February 12, 2025 13:07
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.

@ahrtr
Copy link
Member

ahrtr commented Feb 12, 2025

@henrybear327 can you please rebase this PR ? I just merged #19398

@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch from 11357be to c0f2170 Compare February 12, 2025 15:59
@henrybear327
Copy link
Contributor Author

@henrybear327 can you please rebase this PR ? I just merged #19398

Saw and done as you posted!

@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch from c0f2170 to 43e04f9 Compare February 12, 2025 16:14
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.

Please resolve the comments

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: henrybear327
Once this PR has been reviewed and has the lgtm label, please assign ahrtr for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr ahrtr mentioned this pull request Feb 13, 2025
15 tasks
@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch from 43e04f9 to a18de65 Compare February 13, 2025 20:35
@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch 2 times, most recently from 7baf245 to f39e84e Compare February 15, 2025 01:36
@henrybear327
Copy link
Contributor Author

Is this PR still relevant as we are doing #19439?

@ahrtr
Copy link
Member

ahrtr commented Feb 18, 2025

Is this PR still relevant as we are doing #19439?

Yes, I think so. The auto downgrade cancel should still work.

@ahrtr
Copy link
Member

ahrtr commented Feb 21, 2025

FYI. #19451 (comment)

Thanks for the effort anyway. @henrybear327

@ahrtr
Copy link
Member

ahrtr commented Feb 21, 2025

FYI. #19451 (comment)

3.5 has the auto downgrade cancellation as well, so this test case still makes sense. @fuweid @henrybear327

https://github.com/etcd-io/etcd/blob/release-3.5/server/etcdserver/server.go#L2724

@ahrtr
Copy link
Member

ahrtr commented Feb 21, 2025

FYI. #19451 (comment)

3.5 has the auto downgrade cancellation as well, so this test case still makes sense. @fuweid @henrybear327

https://github.com/etcd-io/etcd/blob/release-3.5/server/etcdserver/server.go#L2724

But 3.5 doesn't have the downgrade status query API. So this PR should be independent to #19451. So there is no need to wait for that PR. @fuweid @henrybear327 @siyuanfoundation

@henrybear327
Copy link
Contributor Author

FYI. #19451 (comment)

3.5 has the auto downgrade cancellation as well, so this test case still makes sense. @fuweid @henrybear327
https://github.com/etcd-io/etcd/blob/release-3.5/server/etcdserver/server.go#L2724

But 3.5 doesn't have the downgrade status query API. So this PR should be independent to #19451. So there is no need to wait for that PR. @fuweid @henrybear327 @siyuanfoundation

So ... is the conclusion to still merge this PR or no? Sorry that I am pretty lost during these exchanges.

@ahrtr
Copy link
Member

ahrtr commented Feb 22, 2025

So ... is the conclusion to still merge this PR or no?

YES. It's nice to merge this PR before next Tuesday. But it isn't a blocker for 3.6.0-rc.1.

@henrybear327
Copy link
Contributor Author

henrybear327 commented Feb 22, 2025

So ... is the conclusion to still merge this PR or no?

YES. It's nice to merge this PR before next Tuesday. But it isn't a blocker for 3.6.0-rc.1.

Got it! I have rebased it now, but I will likely complete addressing all comments and incorporating changes regarding cancellation status by Monday noon (as I am out on both days on the weekend).

Marking the PR as a draft now.

@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch from 65ae06d to d004d39 Compare March 6, 2025 19:44
@henrybear327 henrybear327 marked this pull request as ready for review March 6, 2025 20:31
@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch from d004d39 to a927742 Compare March 6, 2025 20:31
@henrybear327 henrybear327 requested a review from ahrtr March 6, 2025 20:32
@henrybear327
Copy link
Contributor Author

/retest

@henrybear327
Copy link
Contributor Author

Ready for a final pass!

@henrybear327
Copy link
Contributor Author

/retest

Verify that the downgrade can be cancelled
automatically when the downgrade is completed
(using `no inflight downgrade job`` as the indicator)

Please see: etcd-io#19365 (comment)
Reference: etcd-io#17976

Signed-off-by: Chun-Hung Tseng <[email protected]>
@henrybear327 henrybear327 force-pushed the e2e/downgrade_auto_cancel branch from 8785910 to a514ebe Compare March 21, 2025 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add downgrade cancellation e2e tests
4 participants