[Etcd downgrade] Implement downgrade validate, enable and cancel #11801

YoyinZyc · 2020-04-21T06:58:02Z

3rd step of #11716
This PR added the implementation of downgrade validate, enable and cancel in v3_server.go and apply.go.
Server will receive downgrade requests in v3_server.go and then send out internal raft request DowngradeInfoSetRequest. Finally, the downgrade info update will be applied.

The required tests are all passed.

YoyinZyc · 2020-04-21T06:59:28Z

@gyuho @jingyih PHAL. Thanks!

codecov-io · 2020-04-21T07:40:13Z

Codecov Report

Merging #11801 into master will decrease coverage by 0.40%.
The diff coverage is 11.59%.

@@            Coverage Diff             @@
##           master   #11801      +/-   ##
==========================================
- Coverage   66.23%   65.82%   -0.41%     
==========================================
  Files         403      403              
  Lines       36950    37018      +68     
==========================================
- Hits        24473    24368     -105     
- Misses      10991    11160     +169     
- Partials     1486     1490       +4

Impacted Files	Coverage Δ
etcdserver/api/v3rpc/rpctypes/error.go	`90.47% <ø> (ø)`
etcdserver/api/v3rpc/util.go	`67.74% <ø> (ø)`
etcdserver/apply.go	`79.18% <0.00%> (-0.85%)`	⬇️
etcdserver/errors.go	`0.00% <ø> (ø)`
etcdserver/v3_server.go	`67.49% <0.00%> (-7.19%)`	⬇️
etcdserver/util.go	`98.91% <100.00%> (+0.10%)`	⬆️
client/client.go	`42.81% <0.00%> (-35.63%)`	⬇️
pkg/testutil/recorder.go	`77.77% <0.00%> (-3.71%)`	⬇️
clientv3/retry_interceptor.go	`68.62% <0.00%> (-0.99%)`	⬇️
auth/store.go	`52.94% <0.00%> (-0.71%)`	⬇️
... and 12 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 8d52be5...cc31878. Read the comment docs.

jingyih

Thanks Yuchen! I added few comments.

jingyih · 2020-04-29T18:50:07Z

etcdserver/errors.go

+	ErrInvalidDowngradeTargetVersion = errors.New("etcdserver: invalid target version")
+	ErrIsDowngrading                 = errors.New("etcdserver: cluster has an ongoing downgrade job")
+	ErrIsNotDowngrading              = errors.New("etcdserver: cluster is not downgrading")
+	ErrUnknownDowngradeAction        = errors.New("etcdserver: unknown downgrade action")


Is this basically ErrUnknownMethod?

It should be. I changed to use ErrUnknownMethod.

jingyih · 2020-04-29T18:57:03Z

etcdserver/util.go

@@ -171,3 +172,17 @@ func warnOfExpensiveGenericRequest(lg *zap.Logger, now time.Time, reqStringer fm
 func isNil(msg proto.Message) bool {
 	return msg == nil || reflect.ValueOf(msg).IsNil()
 }
+
+func changeToClusterVersion(v string) (*semver.Version, error) {


convertToClusterVersion?

jingyih · 2020-04-29T18:59:12Z

etcdserver/v3_server.go

+		return nil, err
+	}
+
+	// do linearized read to avoid using stale downgrade information


nit (on code comment): this is not actually doing linearizable read. It gets leaders commit index and wait for local store to finish applying that index.

It is clearer. Changed.

jingyih · 2020-04-29T19:00:31Z

etcdserver/v3_server.go

+	}
+	resp.Version = cv.String()
+
+	allowedTargetVersion := semver.Version{Major: cv.Major, Minor: cv.Minor - 1}


In the future if cluster version is 4.0, does this work?

If the cluster version is 4.0, I think the downgrade target should be 3.x which is the latest v3 minor version according to downgrade policy. Since we cannot predict what the 3.x could be, I don't handle this case. I encapsulate this line into a function called allowedDowngradeVersion and add a todo there. It deserve an update before we bump to v4.0. WDYT?

gyuho · 2020-04-30T07:59:25Z

etcdserver/v3_server.go

+	// validate downgrade capability before starting downgrade
+	v := r.Version
+	if resp, err := s.downgradeValidate(ctx, v); err != nil {
+		return resp, err


Can we log when downgrade request is denied?

Added warning level log.

gyuho · 2020-05-12T17:59:45Z

@YoyinZyc Sorry for delay. Will take a look.

gyuho · 2020-05-12T18:29:38Z

etcdserver/errors.go

+	ErrKeyNotFound                   = errors.New("etcdserver: key not found")
+	ErrCorrupt                       = errors.New("etcdserver: corrupt cluster")
+	ErrBadLeaderTransferee           = errors.New("etcdserver: bad leader transferee")
+	ErrClusterVersionUnavailable     = errors.New("etcdserver: cluster version is unavailable")


Can we update all new error message with "downgrade"; "etcdserver: cluster version is unavailable" is not clear, which cluster version it refers to. It can be something like "etcdserver: target downgrade cluster version is unavailable"

Or "etcdserver: cluster version not found during downgrade

gyuho · 2020-05-12T18:32:21Z

etcdserver/errors.go

+	ErrWrongVersionFormat            = errors.New("etcdserver: wrong version format")
+	ErrInvalidDowngradeTargetVersion = errors.New("etcdserver: invalid target version")
+	ErrIsDowngrading                 = errors.New("etcdserver: cluster has an ongoing downgrade job")
+	ErrIsNotDowngrading              = errors.New("etcdserver: cluster is not downgrading")


Should we name this ErrNoInflightDowngrade?

gyuho · 2020-05-12T18:34:34Z

etcdserver/errors.go

+	ErrClusterVersionUnavailable     = errors.New("etcdserver: cluster version is unavailable")
+	ErrWrongVersionFormat            = errors.New("etcdserver: wrong version format")
+	ErrInvalidDowngradeTargetVersion = errors.New("etcdserver: invalid target version")
+	ErrIsDowngrading                 = errors.New("etcdserver: cluster has an ongoing downgrade job")


Should we name this ErrDowngradeInProgress?

gyuho · 2020-05-12T18:36:02Z

etcdserver/v3_server.go

+	downgradeInfo := s.cluster.DowngradeInfo()
+	if downgradeInfo.Enabled {
+		// Todo: return the downgrade status along with the error msg
+		return resp, ErrIsDowngrading


return nil, ...?

gyuho · 2020-05-12T18:37:23Z

etcdserver/v3_server.go

+	allowedTargetVersion := allowedDowngradeVersion(cv)
+	if !targetVersion.Equal(*allowedTargetVersion) {
+		err = ErrInvalidDowngradeTargetVersion
+		return nil, err


We can just return nil, ErrInvalidDowngradeTargetVersion?

gyuho

Overall looks great.

I've requested some minor changes in error messages.

Thanks!

…quests

YoyinZyc · 2020-05-16T03:27:26Z

Overall looks great.

I've requested some minor changes in error messages.

Thanks!

Fixed all your comments. PHAL again. Thanks!

gyuho

lgtm thx!

YoyinZyc mentioned this pull request Apr 21, 2020

Etcd cluster downgrade #11716

Open

13 tasks

jingyih reviewed Apr 29, 2020

View reviewed changes

gyuho reviewed Apr 30, 2020

View reviewed changes

YoyinZyc force-pushed the downgrade-server branch from cc31878 to 83f404f Compare April 30, 2020 20:57

YoyinZyc requested review from gyuho and jingyih May 4, 2020 16:44

gyuho self-assigned this May 12, 2020

gyuho reviewed May 12, 2020

View reviewed changes

gyuho suggested changes May 12, 2020

View reviewed changes

YoyinZyc added 2 commits May 15, 2020 11:51

etcdserver: define error string and link with grpc error code.

37e598a

etcdserver: handle and apply downgrade validate, enable and cancel re…

d230e6b

…quests

YoyinZyc force-pushed the downgrade-server branch from 83f404f to d230e6b Compare May 15, 2020 21:46

YoyinZyc requested a review from gyuho May 18, 2020 05:01

gyuho approved these changes May 18, 2020

View reviewed changes

gyuho merged commit 5e2815e into etcd-io:master May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Etcd downgrade] Implement downgrade validate, enable and cancel #11801

[Etcd downgrade] Implement downgrade validate, enable and cancel #11801

YoyinZyc commented Apr 21, 2020

YoyinZyc commented Apr 21, 2020

codecov-io commented Apr 21, 2020

jingyih left a comment

jingyih Apr 29, 2020

YoyinZyc Apr 30, 2020

jingyih Apr 29, 2020

YoyinZyc Apr 30, 2020

jingyih Apr 29, 2020

YoyinZyc Apr 30, 2020

jingyih Apr 29, 2020

YoyinZyc Apr 30, 2020

gyuho Apr 30, 2020

YoyinZyc Apr 30, 2020

gyuho commented May 12, 2020

gyuho May 12, 2020

gyuho May 12, 2020

gyuho May 12, 2020 •

edited

Loading

gyuho May 12, 2020

gyuho May 12, 2020

gyuho May 12, 2020

gyuho left a comment

YoyinZyc commented May 16, 2020

gyuho left a comment

[Etcd downgrade] Implement downgrade validate, enable and cancel #11801

[Etcd downgrade] Implement downgrade validate, enable and cancel #11801

Conversation

YoyinZyc commented Apr 21, 2020

YoyinZyc commented Apr 21, 2020

codecov-io commented Apr 21, 2020

Codecov Report

jingyih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho left a comment

Choose a reason for hiding this comment

YoyinZyc commented May 16, 2020

gyuho left a comment

Choose a reason for hiding this comment

gyuho May 12, 2020 •

edited

Loading