diff --git a/etcdserver/api/membership/cluster.go b/etcdserver/api/membership/cluster.go index 608f5fad0ed2..fa893f4380a2 100644 --- a/etcdserver/api/membership/cluster.go +++ b/etcdserver/api/membership/cluster.go @@ -258,10 +258,14 @@ func (c *RaftCluster) Recover(onSet func(*zap.Logger, *semver.Version)) { defer c.Unlock() c.members, c.removed = membersFromStore(c.lg, c.v2store) - // recover cluster version from backend - c.version = clusterVersionFromBackend(c.be) + if c.be != nil { + // recover cluster version from backend + c.version = clusterVersionFromBackend(c.be) + } else { + c.version = clusterVersionFromStore(c.lg, c.v2store) + } - c.downgradeInfo = downgradeFromBackend(c.be) + c.downgradeInfo = downgradeFromBackend(c.lg, c.be) var d *DowngradeInfo if c.downgradeInfo == nil { d = &DowngradeInfo{Enabled: false} @@ -811,26 +815,10 @@ func mustDetectDowngrade(lg *zap.Logger, cv *semver.Version, d *DowngradeInfo) { // only keep major.minor version for comparison against cluster version lv = &semver.Version{Major: lv.Major, Minor: lv.Minor} + // if the cluster enables downgrade, check local version against downgrade target version. 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) { - if lg != nil { - lg.Fatal( - "invalid downgrade; server version is not allowed to join when downgrade is enabled", - zap.String("current-server-version", version.Version), - zap.String("target-cluster-version", d.TargetVersion.String()), - ) - } else { - plog.Fatalf("invalid downgrade; server version is not allowed to join when downgrade is enabled "+ - "(current version: %s, target cluster version: %s).", version.Version, d.TargetVersion.String()) - } - return - } - } - - if cv != nil { - if d.Enabled && d.TargetVersion != nil { - if lv.Equal(*d.TargetVersion) || lv.Equal(*cv) { + if isValidDowngrade(d.TargetVersion, lv) { + if cv != nil { if lg != nil { lg.Info( "cluster is downgrading to target version", @@ -838,30 +826,41 @@ func mustDetectDowngrade(lg *zap.Logger, cv *semver.Version, d *DowngradeInfo) { zap.String("determined-cluster-version", version.Cluster(cv.String())), zap.String("current-server-version", version.Version), ) - } else { - plog.Infof("cluster is downgrading to target version: %s, "+ - "determined cluster version: %s, current-server-version: %s).", - d.TargetVersion.String(), version.Cluster(cv.String()), version.Version) } - return } + return } + if lg != nil { + lg.Fatal( + "invalid downgrade; server version is not allowed to join when downgrade is enabled", + zap.String("current-server-version", version.Version), + zap.String("target-cluster-version", d.TargetVersion.String()), + ) + } + } - if !d.Enabled && lv.LessThan(*cv) { - if lg != nil { - lg.Fatal( - "invalid downgrade; server version is lower than determined cluster version", - zap.String("current-server-version", version.Version), - zap.String("determined-cluster-version", version.Cluster(cv.String())), - ) - } else { - plog.Fatalf("invalid downgrade; server version is lower than determined cluster version (current version: %s, determined cluster version: %s).", - version.Version, version.Cluster(cv.String())) - } + // if the cluster disables downgrade, check local version against determined cluster version. + // the validation passes when local version is not less than cluster version + if cv != nil && lv.LessThan(*cv) { + if lg != nil { + lg.Fatal( + "invalid downgrade; server version is lower than determined cluster version", + zap.String("current-server-version", version.Version), + zap.String("determined-cluster-version", version.Cluster(cv.String())), + ) } } } +// isValidDowngrade checks whether joining local server is a valid downgrade when cluster enables downgrade. +// the validation passes when +// 1. local version is target version +// 2. local version is one minor version higher then target version +func isValidDowngrade(tv *semver.Version, lv *semver.Version) bool { + oneMinorHigher := &semver.Version{Major: tv.Major, Minor: tv.Minor + 1} + return lv.Equal(*tv) || lv.Equal(*oneMinorHigher) +} + // IsVersionChangable checks the two scenario when version is changable: // 1. Downgrade: cluster version is 1 minor version higher than local version, // cluster version should change. @@ -934,7 +933,7 @@ func (c *RaftCluster) SetDowngradeInfo(d *DowngradeInfo) { defer c.Unlock() if c.be != nil { - mustSaveDowngradeToBackend(c.be, d) + mustSaveDowngradeToBackend(c.lg, c.be, d) } c.downgradeInfo = d @@ -944,9 +943,6 @@ func (c *RaftCluster) SetDowngradeInfo(d *DowngradeInfo) { c.lg.Info("The server is ready to downgrade", zap.String("target-version", d.TargetVersion.String()), zap.String("server-version", version.Version)) - } else { - plog.Info("The server is ready to downgrade from current server version %v to target version %v", - version.Version, d.TargetVersion.String()) } } } diff --git a/etcdserver/api/membership/cluster_test.go b/etcdserver/api/membership/cluster_test.go index 92feb6ddd54e..a2cbeb15106b 100644 --- a/etcdserver/api/membership/cluster_test.go +++ b/etcdserver/api/membership/cluster_test.go @@ -1012,54 +1012,63 @@ func TestIsVersionChangable(t *testing.T) { v4 := semver.Must(semver.NewVersion("3.6.0")) tests := []struct { + name string currentVersion *semver.Version localVersion *semver.Version expectedResult bool }{ { + name: "Fail when local version is one major higher than cluster version", currentVersion: v0, localVersion: v1, expectedResult: false, }, { + name: "Fail when local version is equal to cluster version", currentVersion: v1, localVersion: v1, expectedResult: false, }, { + name: "Succeed when local version is one minor higher than cluster version", currentVersion: v1, localVersion: v2, expectedResult: true, }, { + name: "Succeed when local version is two minor higher than cluster version", currentVersion: v1, localVersion: v4, expectedResult: true, }, { + name: "Fail when local version is one patch higher than cluster version", currentVersion: v2, localVersion: v3, expectedResult: false, }, { + name: "Succeed when local version is one minor lower than cluster version", currentVersion: v2, localVersion: v1, expectedResult: true, }, { + name: "Succeed when local version is one minor and one patch lower than cluster version", currentVersion: v3, localVersion: v1, expectedResult: true, }, { + name: "Fail when local version is two minor lower than cluster version", currentVersion: v4, localVersion: v1, expectedResult: false, }, } - for i, tt := range tests { - t.Run(string(i), func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { if ret := IsVersionChangable(tt.currentVersion, tt.localVersion); ret != tt.expectedResult { t.Errorf("Expected %v; Got %v", tt.expectedResult, ret) } @@ -1069,29 +1078,33 @@ func TestIsVersionChangable(t *testing.T) { func TestGetDowngrade(t *testing.T) { tests := []struct { + name string cluster *RaftCluster expectedEnabled bool expectedTargetVersion *semver.Version }{ { + "When downgradeInfo is empty", &RaftCluster{}, false, nil, }, { + "When downgrade is disabled", &RaftCluster{downgradeInfo: &DowngradeInfo{Enabled: false}}, false, nil, }, { + "When downgrade is enabled", &RaftCluster{downgradeInfo: &DowngradeInfo{Enabled: true, TargetVersion: semver.Must(semver.NewVersion("3.4.0"))}}, true, semver.Must(semver.NewVersion("3.4.0")), }, } - for i, tt := range tests { - t.Run(string(i), func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { d := tt.cluster.DowngradeInfo() if d.Enabled != tt.expectedEnabled { t.Errorf("Expected %v; Got %v", tt.expectedEnabled, d.Enabled) @@ -1109,3 +1122,42 @@ func TestGetDowngrade(t *testing.T) { }) } } + +func TestIsValidDowngrade(t *testing.T) { + lv := semver.Must(semver.NewVersion(version.Version)) + lv = &semver.Version{Major: lv.Major, Minor: lv.Minor} + tv := &semver.Version{Major: lv.Major, Minor: lv.Minor - 1} + tests := []struct { + name string + targetVersion *semver.Version + localVersion *semver.Version + expectedResult bool + }{ + { + name: "When local version is equal to target version", + targetVersion: tv, + localVersion: lv, + expectedResult: true, + }, + { + name: "When local version is one minor version higher than target version", + targetVersion: tv, + localVersion: lv, + expectedResult: true, + }, + { + name: "When local version is lower than target version", + targetVersion: tv, + localVersion: &semver.Version{Major: tv.Major, Minor: tv.Minor - 1}, + expectedResult: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if ret := isValidDowngrade(tt.targetVersion, tt.localVersion); ret != tt.expectedResult { + t.Errorf("Expected %v; Got %v", tt.expectedResult, ret) + } + }) + } +} diff --git a/etcdserver/api/membership/store.go b/etcdserver/api/membership/store.go index 2d057043222b..0fbe6a85af6f 100644 --- a/etcdserver/api/membership/store.go +++ b/etcdserver/api/membership/store.go @@ -22,6 +22,7 @@ import ( "go.etcd.io/etcd/etcdserver/api/v2store" "go.etcd.io/etcd/mvcc/backend" "go.etcd.io/etcd/pkg/types" + "go.uber.org/zap" "github.com/coreos/go-semver/semver" ) @@ -75,20 +76,21 @@ func mustSaveClusterVersionToBackend(be backend.Backend, ver *semver.Version) { tx.UnsafePut(clusterBucketName, ckey, []byte(ver.String())) } -func mustSaveDowngradeToBackend(be backend.Backend, downgrade *DowngradeInfo) { +func mustSaveDowngradeToBackend(lg *zap.Logger, be backend.Backend, downgrade *DowngradeInfo) { dkey := backendDowngradeKey() dvalue, err := json.Marshal(downgrade) if err != nil { - plog.Panicf("marshal raftAttributes should never fail: %v", err) + if lg != nil { + lg.Panic("failed to marshal downgrade information", zap.Error(err)) + } } - tx := be.BatchTx() tx.Lock() defer tx.Unlock() tx.UnsafePut(clusterBucketName, dkey, dvalue) } -func downgradeFromBackend(be backend.Backend) *DowngradeInfo { +func downgradeFromBackend(lg *zap.Logger, be backend.Backend) *DowngradeInfo { dkey := backendDowngradeKey() if be != nil { tx := be.BatchTx() @@ -99,7 +101,9 @@ func downgradeFromBackend(be backend.Backend) *DowngradeInfo { if len(vs) != 0 { var d DowngradeInfo if err := json.Unmarshal(vs[0], &d); err != nil { - plog.Panicf("fail to unmarshal downgrade: %v", err) + if lg != nil { + lg.Panic("failed to unmarshal downgrade information", zap.Error(err)) + } } return &d } @@ -115,12 +119,9 @@ func clusterVersionFromBackend(be backend.Backend) *semver.Version { defer tx.Unlock() _, vs := tx.UnsafeRange(clusterBucketName, ckey, nil, 0) - if len(vs) != 0 { v := string(vs[0]) - if sv, err := semver.NewVersion(v); err == nil { - return sv - } + return semver.Must(semver.NewVersion(v)) } } return nil diff --git a/etcdserver/cluster_util.go b/etcdserver/cluster_util.go index 943645f5bc0b..215f101cd4e3 100644 --- a/etcdserver/cluster_util.go +++ b/etcdserver/cluster_util.go @@ -204,8 +204,6 @@ func decideDowngradeEnabled(lg *zap.Logger, cl *membership.RaftCluster, local ty if err != nil { if lg != nil { lg.Warn("failed to get downgrade enabled status", zap.String("remote-member-id", m.ID.String()), zap.Error(err)) - } else { - plog.Warningf("cannot get the downgrade enabled status of member %s (%v)", m.ID, err) } } else { // Since the "/downgrade/enabled" serves linearized data, @@ -238,8 +236,6 @@ func getDowngradeEnabled(lg *zap.Logger, m *membership.Member, rt http.RoundTrip zap.String("remote-member-id", m.ID.String()), zap.Error(err), ) - } else { - plog.Warningf("failed to reach the peerURL(%s) of member %s (%v)", u, m.ID, err) } continue } @@ -254,8 +250,6 @@ func getDowngradeEnabled(lg *zap.Logger, m *membership.Member, rt http.RoundTrip zap.String("remote-member-id", m.ID.String()), zap.Error(err), ) - } else { - plog.Warningf("failed to read out the response body from the peerURL(%s) of member %s (%v)", u, m.ID, err) } continue } @@ -268,8 +262,6 @@ func getDowngradeEnabled(lg *zap.Logger, m *membership.Member, rt http.RoundTrip zap.String("remote-member-id", m.ID.String()), zap.Error(err), ) - } else { - plog.Warningf("failed to unmarshal the response body got from the peerURL(%s) of member %s (%v)", u, m.ID, err) } continue } @@ -341,8 +333,6 @@ func isDowngradeFinished(lg *zap.Logger, targetVersion *semver.Version, vers map zap.String("remote-member-version", ver.Server), zap.Error(err), ) - } else { - plog.Errorf("cannot understand the version of member %s (%v)", mid, err) } return false } diff --git a/etcdserver/cluster_util_test.go b/etcdserver/cluster_util_test.go index 8a36a548b6e1..643b25276620 100644 --- a/etcdserver/cluster_util_test.go +++ b/etcdserver/cluster_util_test.go @@ -133,3 +133,42 @@ func TestIsCompatibleWithVers(t *testing.T) { } } } + +func TestDecideAllowedVersionRange(t *testing.T) { + minClusterV := semver.Must(semver.NewVersion(version.MinClusterVersion)) + localV := semver.Must(semver.NewVersion(version.Version)) + localV = &semver.Version{Major: localV.Major, Minor: localV.Minor} + + tests := []struct { + name string + downgradeEnabled bool + expectedMinV *semver.Version + expectedMaxV *semver.Version + }{ + { + "When cluster enables downgrade", + true, + localV, + &semver.Version{Major: localV.Major, Minor: localV.Minor + 1}, + }, + { + "When cluster disables downgrade", + false, + minClusterV, + localV, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + minV, maxV := decideAllowedVersionRange(tt.downgradeEnabled) + if !minV.Equal(*tt.expectedMinV) { + t.Errorf("Expected minV is %v; Got %v", tt.expectedMinV.String(), minV.String()) + } + + if !maxV.Equal(*tt.expectedMaxV) { + t.Errorf("Expected maxV is %v; Got %v", tt.expectedMaxV.String(), maxV.String()) + } + }) + } +} diff --git a/etcdserver/server.go b/etcdserver/server.go index a7ce579bc48d..c5f2fdfa69e7 100644 --- a/etcdserver/server.go +++ b/etcdserver/server.go @@ -930,8 +930,6 @@ func (h *downgradeEnabledHandler) ServeHTTP(w http.ResponseWriter, r *http.Reque if err != nil { if h.lg != nil { h.lg.Warn("failed to marshal downgrade.Enabled to json", zap.Error(err)) - } else { - plog.Warningf("failed to marshal downgrade.Enabled to json (%v)", err) } } w.Write(b) @@ -2652,14 +2650,10 @@ func (s *EtcdServer) monitorDowngrade() { if isDowngradeFinished(s.getLogger(), targetVersion, getVersions(s.getLogger(), s.cluster, s.id, s.peerRt)) { if lg != nil { lg.Info("the cluster has been downgraded", zap.String("cluster-version", targetVersion.String())) - } else { - plog.Infof("the cluster has been downgraded to version %v", targetVersion.String()) } if _, err := s.downgradeCancel(context.Background()); err != nil { if lg != nil { lg.Warn("failed to cancel downgrade", zap.Error(err)) - } else { - plog.Warningf("failed to cancel downgrade %v", err) } } continue diff --git a/tests/e2e/etcd_downgrade_test.go b/tests/e2e/etcd_downgrade_test.go index b3215c114d97..6114c77e760e 100644 --- a/tests/e2e/etcd_downgrade_test.go +++ b/tests/e2e/etcd_downgrade_test.go @@ -295,9 +295,7 @@ func TestDowngradeThenRestart(t *testing.T) { } // check cluster health after restart - if err := ctlV3EndpointHealth(cx); err != nil { - t.Fatalf("error check endpoints healthy (%v)", err) - } + testHealth(cx) // cluster version should be updated once there is a downgraded server clusterVersionTest(cx, `"etcdcluster":"`+tv.String()) } @@ -314,11 +312,7 @@ func TestDowngradeThenRestart(t *testing.T) { if err := epc.procs[i].Restart(); err != nil { t.Fatalf("error restarting etcd process (%v)(%d)", err, i) } - - // check cluster health after restart - if err := ctlV3EndpointHealth(cx); err != nil { - t.Fatalf("error check endpoints healthy (%v)", err) - } + testHealth(cx) testKVPairs(cx, kvs) } } @@ -384,9 +378,7 @@ func TestCrushRestartWhenDowngrade(t *testing.T) { } // check cluster health after restart - if err := ctlV3EndpointHealth(cx); err != nil { - t.Fatalf("error check endpoints healthy (%v)", err) - } + testHealth(cx) // cluster version should be updated once there is a downgraded server clusterVersionTest(cx, `"etcdcluster":"`+tv.String()) } @@ -573,6 +565,80 @@ func TestDowngradeFromSnapshot(t *testing.T) { testKVPairs(cx, kvs) } +func TestDowngradeAfterUpgrade(t *testing.T) { + defer testutil.AfterTest(t) + + cv := semver.Must(semver.NewVersion(version.Version)) + tv := semver.Version{Major: cv.Major, Minor: cv.Minor - 1} + tvBinary := fmt.Sprintf("/downgrade%d%d", tv.Major, tv.Minor) + mustFileExist(t, binDir+tvBinary) + + copiedCfg := configDowngrade + + epc := startCluster(t, &copiedCfg, tvBinary) + + defer func() { + if errC := epc.Close(); errC != nil { + t.Fatalf("error closing etcd processes (%v)", errC) + } + }() + + os.Setenv("ETCDCTL_API", "3") + defer os.Unsetenv("ETCDCTL_API") + cx := ctlCtx{ + t: t, + cfg: configNoTLS, + dialTimeout: 7 * time.Second, + quorum: true, + epc: epc, + } + clusterVersionTest(cx, `"etcdcluster":"`+version.Cluster(tv.String())) + var kvs []kv + for i := 0; i < 50; i++ { + kvs = append(kvs, kv{key: fmt.Sprintf("foo%d", i), val: "bar"}) + } + putKVPairs(cx, kvs) + + for i := range epc.procs { + if err := epc.procs[i].Stop(); err != nil { + t.Fatalf("#%d: error closing etcd process (%v)", i, err) + } + epc.procs[i].Config().execPath = binDir + "/etcd" + epc.procs[i].Config().keepDataDir = true + + if err := epc.procs[i].Restart(); err != nil { + t.Fatalf("error restarting etcd process (%v)", err) + } + testHealth(cx) + } + clusterVersionTest(cx, `"etcdcluster":"`+version.Cluster(cv.String())) + ctlDowngradeEnable(cx, tv.String()) + + for i := range epc.procs { + testDowngradeEnabled(cx, epc.procs[i].Config().acurl, "true") + } + + for i := range epc.procs { + if err := epc.procs[i].Stop(); err != nil { + t.Fatalf("#%d: error closing etcd process (%v)", i, err) + } + epc.procs[i].Config().execPath = binDir + tvBinary + epc.procs[i].Config().keepDataDir = true + + if err := epc.procs[i].Restart(); err != nil { + t.Fatalf("error restarting etcd process (%v)(%d)", err, i) + } + testHealth(cx) + // cluster version should be updated once there is a downgraded server + clusterVersionTest(cx, `"etcdcluster":"`+tv.String()) + } + + for i := range epc.procs { + testDowngradeEnabled(cx, epc.procs[i].Config().acurl, "false") + } + testKVPairs(cx, kvs) +} + func testDowngradeEnabled(cx ctlCtx, endpoint string, expected string) { cmdArgs := []string{"curl", "-L", endpoint + "/downgrade/enabled"} fmt.Println(cmdArgs)