Etcd Downgrade backend #11492

YoyinZyc · 2020-01-04T00:48:28Z

The original downgrade PR is too big to review. I will break it down into multiple small PRs.
This pr contains the server side of downgrade. The unit tests will be added in following PRs.

Based on the discussion here, the cluster only allows member with downgrade target version to join in during downgrading.
For more information about the design, please go to #11362
@gyuho @jingyih @wenjiaswe @hexfusion PTAL Thanks:)

gyuho · 2020-01-21T06:42:11Z

etcdserver/api/membership/cluster.go

@@ -778,6 +796,36 @@ func clusterVersionFromBackend(lg *zap.Logger, be backend.Backend) *semver.Versi
 	return semver.Must(semver.NewVersion(string(vals[0])))
 }

+func downgradeInfoFromBackend(lg *zap.Logger, be backend.Backend) *DowngradeInfo {
+	dkey := backendDowngradeKey()
+	if be != nil {


Can we panic on be == nil? So, we can reduce indentation?

Move nil check outside downgradeInfoFromBackend to reduce indentation.

gyuho · 2020-01-21T06:42:46Z

etcdserver/api/membership/cluster.go

+		}
+
+		if len(keys) != 1 {
+			if lg != nil {


Also, do we really need nil checks on logger?

I don't quite get it. Since we haven't deprecated plog and used zap log as default, lg is set to nil as default if flag --logger=zap is not enabled. I think we can remove all the nil check of log after we enable zap log as default?

I meant, if we set logger to https://godoc.org/go.uber.org/zap#NewNop once in the caller, all the downstream calls need not handle this nil checks?

gyuho · 2020-01-21T06:43:07Z

etcdserver/api/membership/cluster.go

+	if d != nil && d.Enabled && d.TargetVersion != nil {
+		if isValidDowngrade(d.TargetVersion, lv) {
+			if cv != nil {
+				if lg != nil {


Same. Can previous calls ensure logger is always non-nil?

gyuho · 2020-01-21T06:43:28Z

etcdserver/api/membership/cluster.go

 		}
 	}
 }

+// isValidDowngrade checks whether joining local server is a valid downgrade when cluster enables downgrade.
+// the validation passes when local version is one minor version higher then target version
+func isValidDowngrade(tv *semver.Version, lv *semver.Version) bool {


this needs unit tests?

Yes, it has unit test but not in this pr. I thought it will be a big pr if I include all the tests. Should I put them in or just add the unit tests?

gyuho · 2020-01-21T06:43:55Z

etcdserver/api/membership/store.go

+	dkey := backendDowngradeKey()
+	dvalue, err := json.Marshal(downgrade)
+	if err != nil {
+		if lg != nil {


Also, can previous calls ensure logger is non-nil always?

gyuho · 2020-01-21T06:44:06Z

etcdserver/api/membership/store.go

+	}
+	tx := be.BatchTx()
+	tx.Lock()
+	defer tx.Unlock()


unnecessary defer we can just place it under UnsafePut?

gyuho · 2020-01-21T06:45:21Z

etcdserver/cluster_util.go

@@ -177,6 +177,21 @@ func getVersions(lg *zap.Logger, cl *membership.RaftCluster, local types.ID, rt
 	return vers
 }

+// allowedVersionRange decides the available version range of the cluster that local server can join in;


Can these be unit tested?

gyuho · 2020-01-21T06:46:54Z

etcdserver/server.go

+
+		targetVersion := d.TargetVersion
+		if isDowngradeFinished(s.getLogger(), targetVersion, getVersions(s.getLogger(), s.cluster, s.id, s.peerRt)) {
+			if lg != nil {


Here as well, logger nil checking should be handled at the top level. Downstream packages should not worry about this checks.

jingyih · 2020-01-30T08:29:10Z

@gyuho, regarding the logger, today in master, we use lg != nil to indicate whether user choose to use zap or plog. Before deprecating plog, we cannot simply provide a no-op lg in the top level, right? I think we can keep the lg != nil check in this PR. Later when we deprecate plog, we will remove lg != nil everywhere.

gyuho · 2020-02-03T20:42:23Z

@jingyih Sounds good. Let's just keep it that way for now.

…side.

…wngradeInfo

…quests.

…nitor to server to monitor downgrade status.

YoyinZyc · 2020-02-20T00:04:05Z

Rebased the current master and removed the lg != nil check. Tests will be added in the following PR to keep a relative small size of this PR. PHAL.
The failed e2e test seems like a flaky test and it passed locally on my machine.

=== RUN   TestIssue6361
--- FAIL: TestIssue6361 (14.81s)
    ctl_v3_snapshot_test.go:262: read /dev/ptmx: input/output error (expected "val1", got ["{\"level\":\"warn\",\"ts\":\"2020-02-19T23:19:34.677Z\",\"caller\":\"clientv3/retry_interceptor.go:61\",\"msg\":\"retrying of unary invoker failed\",\"target\":\"endpoint://client-6cb9466f-d07c-40ff-bf75-24beea447b31/localhost:20030\",\"attempt\":0,\"error\":\"rpc error: code = DeadlineExceeded desc = context deadline exceeded\"}\r\n" "Error: context deadline exceeded\r\n"])

=== RUN   TestIssue6361
--- PASS: TestIssue6361 (11.27s)

YoyinZyc · 2020-03-09T22:08:19Z

@jpbetz Could you please also take a look? Thanks
cc @lavalamp

stale · 2020-06-07T22:50:06Z

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.

stale · 2020-09-05T23:32:41Z

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.

jingyih requested review from gyuho and jingyih January 15, 2020 08:48

gyuho reviewed Jan 21, 2020

View reviewed changes

YoyinZyc added 8 commits February 18, 2020 16:14

etcdserver: add downgrade proto api and update all related on client …

4d7e4a9

…side.

etcdserver: add downgrade api in maintenance server.

737a4f4

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

a549834

membership: add downgradeInfo to cluster; add getter and setter of do…

9afd2e6

…wngradeInfo

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

8c4efbc

…quests.

etcdserver: apply downgrade policy to server.

94367a3

etcdserver: add getDowngradeEnabled http handler; attach downgrade mo…

34cd8dd

…nitor to server to monitor downgrade status.

etcdserver: remove lg nil check.

0fc0a95

YoyinZyc force-pushed the downgrade-backend branch from 7ea1755 to 0fc0a95 Compare February 19, 2020 23:14

YoyinZyc requested a review from gyuho February 20, 2020 00:04

YoyinZyc mentioned this pull request Mar 23, 2020

Etcd cluster downgrade #11716

Open

13 tasks

stale bot added the stale label Jun 7, 2020

spzala removed the stale label Jun 7, 2020

stale bot added the stale label Sep 5, 2020

stale bot closed this Sep 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Etcd Downgrade backend #11492

Etcd Downgrade backend #11492

YoyinZyc commented Jan 4, 2020

gyuho Jan 21, 2020

YoyinZyc Feb 19, 2020

gyuho Jan 21, 2020

YoyinZyc Jan 22, 2020

gyuho Jan 28, 2020

gyuho Jan 21, 2020

gyuho Jan 21, 2020

YoyinZyc Jan 23, 2020

gyuho Jan 21, 2020

gyuho Jan 21, 2020 •

edited

Loading

YoyinZyc Feb 19, 2020

gyuho Jan 21, 2020

gyuho Jan 21, 2020

jingyih commented Jan 30, 2020

gyuho commented Feb 3, 2020

YoyinZyc commented Feb 20, 2020

YoyinZyc commented Mar 9, 2020

stale bot commented Jun 7, 2020

stale bot commented Sep 5, 2020

Etcd Downgrade backend #11492

Etcd Downgrade backend #11492

Conversation

YoyinZyc commented Jan 4, 2020

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 Jan 21, 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

jingyih commented Jan 30, 2020

gyuho commented Feb 3, 2020

YoyinZyc commented Feb 20, 2020

YoyinZyc commented Mar 9, 2020

stale bot commented Jun 7, 2020

stale bot commented Sep 5, 2020

gyuho Jan 21, 2020 •

edited

Loading