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

[Etcd downgrade] Add http handler to enable downgrade info communication between each member #12099

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

YoyinZyc
Copy link
Contributor

@YoyinZyc YoyinZyc commented Jul 1, 2020

5th step of #11716

This http handler has similar functionality with other handlers like version handler. It is used to communicate with peers in the cluster about the downgrade enabled status and decide the downgrade enabled status of the cluster.

If all members in the cluster have downgraded to target version, the server will send internal downgrade cancel request to finish downgrade.

@gyuho @jingyih PTAL.Thanks! :)

@gyuho gyuho self-assigned this Jul 21, 2020
etcdserver/cluster_util_test.go Outdated Show resolved Hide resolved
etcdserver/cluster_util.go Outdated Show resolved Hide resolved
etcdserver/server.go Outdated Show resolved Hide resolved
etcdserver/server.go Show resolved Hide resolved
etcdserver/server.go Outdated Show resolved Hide resolved
etcdserver/cluster_util.go Show resolved Hide resolved
etcdserver/cluster_util.go Outdated Show resolved Hide resolved
etcdserver/api/etcdhttp/peer.go Outdated Show resolved Hide resolved
etcdserver/server.go Outdated Show resolved Hide resolved
etcdserver/cluster_util.go Show resolved Hide resolved
@YoyinZyc YoyinZyc force-pushed the downgrade-httphandler branch from 236a54d to bda541e Compare July 29, 2020 19:27
v := semver.Must(semver.NewVersion(targetVersion))
if isMatchedVersions(s.getLogger(), v, getVersions(s.getLogger(), s.cluster, s.id, s.peerRt)) {
lg.Info("the cluster has been downgraded", zap.String("cluster-version", targetVersion))
if _, err := s.downgradeCancel(context.Background()); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to block forever with background context? If that's the case, can we comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use context with timeout.

@YoyinZyc YoyinZyc force-pushed the downgrade-httphandler branch from bda541e to c69041d Compare August 5, 2020 23:34
@codecov-commenter
Copy link

Codecov Report

Merging #12099 into master will decrease coverage by 0.15%.
The diff coverage is 74.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12099      +/-   ##
==========================================
- Coverage   64.08%   63.93%   -0.16%     
==========================================
  Files         403      403              
  Lines       37384    37509     +125     
==========================================
+ Hits        23959    23981      +22     
- Misses      11902    12003     +101     
- Partials     1523     1525       +2     
Impacted Files Coverage Δ
etcdserver/config.go 79.51% <ø> (ø)
etcdserver/corrupt.go 41.11% <ø> (ø)
etcdserver/server.go 74.85% <55.10%> (-0.88%) ⬇️
etcdserver/cluster_util.go 70.90% <85.71%> (+4.52%) ⬆️
embed/config.go 54.40% <100.00%> (+0.13%) ⬆️
embed/etcd.go 75.33% <100.00%> (+0.78%) ⬆️
etcdmain/config.go 84.21% <100.00%> (+0.06%) ⬆️
etcdserver/api/etcdhttp/peer.go 88.00% <100.00%> (+0.32%) ⬆️
auth/store.go 67.51% <0.00%> (-12.43%) ⬇️
raft/tracker/inflights.go 91.83% <0.00%> (-4.09%) ⬇️
... and 15 more

Continue to review full report at Codecov.

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

@YoyinZyc YoyinZyc requested a review from gyuho August 13, 2020 21:12
@YoyinZyc YoyinZyc force-pushed the downgrade-httphandler branch from c69041d to f8798c8 Compare August 17, 2020 21:13
@gyuho
Copy link
Contributor

gyuho commented Aug 21, 2020

@YoyinZyc
Copy link
Contributor Author

@YoyinZyc Can we check test failures https://semaphoreci.com/etcd-io/etcd/branches/pull-request-12099/builds/4?

I think the failed test TestIssue6361 is somehow flaky because it passed when I ran it locally.

@YoyinZyc
Copy link
Contributor Author

YoyinZyc commented Sep 8, 2020

Friendly ping @gyuho

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YoyinZyc Sorry for delay.

Just minor nits and rebase please.

Then, we should be able to merge.

Thanks!

etcdserver/server.go Outdated Show resolved Hide resolved
etcdserver/server.go Outdated Show resolved Hide resolved
etcdserver/cluster_util.go Outdated Show resolved Hide resolved
etcdserver/cluster_util.go Outdated Show resolved Hide resolved
@YoyinZyc YoyinZyc mentioned this pull request Sep 28, 2020
13 tasks
@YoyinZyc YoyinZyc force-pushed the downgrade-httphandler branch from f8798c8 to df6dbbb Compare September 29, 2020 00:13
@YoyinZyc YoyinZyc requested a review from gyuho September 29, 2020 00:41
@gyuho
Copy link
Contributor

gyuho commented Oct 5, 2020

@YoyinZyc Can we resolve conflicts and look into test failures?

Copy link
Contributor

@gyuho gyuho 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 git conflicts and look into test failures. Otherwise, LGTM.

…nitor to server to monitor downgrade status.
@YoyinZyc YoyinZyc force-pushed the downgrade-httphandler branch from df6dbbb to 3e8ffc7 Compare October 12, 2020 17:51
@YoyinZyc
Copy link
Contributor Author

@gyuho I increased the dialTimeout period for TestIssue6361 then it passed. As for the Travis failure, it passed on locally on my side.
image

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #12099 into master will increase coverage by 0.11%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12099      +/-   ##
==========================================
+ Coverage   63.32%   63.44%   +0.11%     
==========================================
  Files         448      448              
  Lines       62342    62466     +124     
==========================================
+ Hits        39479    39632     +153     
+ Misses      18303    18268      -35     
- Partials     4560     4566       +6     
Impacted Files Coverage Δ
etcdserver/config.go 79.51% <ø> (ø)
etcdserver/corrupt.go 46.29% <ø> (ø)
etcdserver/server.go 79.05% <68.75%> (-0.15%) ⬇️
etcdserver/cluster_util.go 71.90% <85.71%> (+4.22%) ⬆️
embed/config.go 57.57% <100.00%> (+0.12%) ⬆️
embed/etcd.go 77.45% <100.00%> (+0.10%) ⬆️
etcdmain/config.go 84.27% <100.00%> (+0.06%) ⬆️
etcdserver/api/etcdhttp/peer.go 88.00% <100.00%> (+0.32%) ⬆️
pkg/fileutil/purge.go 69.56% <0.00%> (-6.53%) ⬇️
etcdserver/api/v3rpc/lease.go 77.21% <0.00%> (-5.07%) ⬇️
... and 23 more

Continue to review full report at Codecov.

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

@gyuho gyuho merged commit bc3a77d into etcd-io:master Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants