-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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 Cluster Downgrade #11362
Etcd Cluster Downgrade #11362
Conversation
Thanks @YoyinZyc for your work! @gyuho @xiang90 @jingyih @hexfusion Please help to take a look. Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #11362 +/- ##
==========================================
+ Coverage 64.29% 64.79% +0.49%
==========================================
Files 403 404 +1
Lines 38079 38751 +672
==========================================
+ Hits 24484 25109 +625
- Misses 11950 11988 +38
- Partials 1645 1654 +9
Continue to review full report at Codecov.
|
etcdserver/api/membership/cluster.go
Outdated
|
||
if d.Enabled && d.TargetVersion != nil { | ||
oneMinorHigher := &semver.Version{Major: d.TargetVersion.Major, Minor: d.TargetVersion.Minor + 1} | ||
if !lv.Equal(*d.TargetVersion) && !lv.Equal(*oneMinorHigher) { |
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.
Can we have a separate function with unit tests?
Something like isValidDowngrade(cur, target)
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.
Added isValidDowngrade(targetVersion, localVersion)
and unit tests
etcdserver/api/membership/cluster.go
Outdated
} | ||
} | ||
|
||
if !d.Enabled && lv.LessThan(*cv) { |
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.
Let's separate this part as well, to a separate function.
@gyuho |
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.
I did a first pass. Overall approach looks good. I think we need to explore if there is a cleaner implementation on the part where individual server quires for cluster downgrade status during startup. Thanks for implementing this feature!
Yeah |
6ce8fa9
to
2891693
Compare
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.
Reviewed the code path during server bootstrapping. Added few comments. PTAL:)
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.
Reviewed the server side implementation.
4aedcad
to
2caeb11
Compare
… restore from snapshot
…ng server and modify compatibility check when new member joins in an existing cluster; add http handler to get downgrade enabled status
…nterRPC and fieldsPrinter for downgrade
…luster. It is used when adding a new member to an downgrading cluster. The new member should match the downgrade target version.
…interface ServerPeerHTTP to wrap up ServerPeer.
…de; remove new-added plog
2caeb11
to
22ddb23
Compare
Rebased the migration of cluster attributes. |
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.
I did a quick pass on the new code changes since rebasing. Overall looks good. I'll spend more time to review later.
} | ||
resp.Version = cv.String() | ||
|
||
allowedTargetVersion := semver.Version{Major: cv.Major, Minor: cv.Minor - 1} |
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.
Does this work if current version is 4.0?
return nil, ErrIsNotDowngrading | ||
} | ||
|
||
raftRequest := membershippb.DowngradeInfoSetRequest{Enabled: false} |
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.
nit: If we set target version to 0 here, in apply stage we could simply reconstruct a new downgradeInfo from the request and overwrite the existing one.
@gyuho @jingyih @xiang90 @hexfusion Do you think this pr may be too big to review? I can divide it into smaller pr and collect more comments. |
@YoyinZyc Yes please. Maybe one for adding a new RPC in server side. And another one for |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Codecov Report
@@ Coverage Diff @@
## master #11362 +/- ##
==========================================
+ Coverage 64.62% 64.79% +0.16%
==========================================
Files 403 404 +1
Lines 38079 38751 +672
==========================================
+ Hits 24610 25109 +499
- Misses 11838 11988 +150
- Partials 1631 1654 +23
Continue to review full report at Codecov.
|
@YoyinZyc are you still working on this one, or is this split into smaller PRs? |
Downgrade will be implemented with other smaller PRs. It is ok to mark this one as stale. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions. |
In this branch, @wenjiaswe and I added downgrade etcdctl command to control cluster to downgrade to one minor version lower. There are three new apis.
downgrade validate <target_version>
: validate whether the cluster can downgrade to target versiondowngrade enable <target_version>
: enable cluster to downgrade to target versiondowngrade cancel
: cancel current downgrade jobThe implementation followed the design doc. Here is the demo video for basic downgrade workflow.
Related issue support seamless rollback 1 minor revision #7308 Improve etcd upgrade/downgrade policy and tests #9306
Future work