etcdserver: separate function isValidDowngrade from mustDetectDowngra…

…de; remove new-added plog

etcdserver/api/membership/cluster.go

@@ -259,12 +259,13 @@ func (c *RaftCluster) Recover(onSet func(*zap.Logger, *semver.Version)) {
 
 	c.members, c.removed = membersFromStore(c.lg, c.v2store)
 	if c.be != nil {
-		c.version = clusterVersionFromBackend(c.lg, c.be)
+		// 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}
@@ -834,57 +835,52 @@ 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",
 						zap.String("target-cluster-version", d.TargetVersion.String()),
 						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.
@@ -957,7 +953,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
@@ -967,9 +963,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())
 		}
 	}
 }

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)
+			}
+		})
+	}
+}

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

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
 		}