refactor IsValidVersionChange.

Signed-off-by: Siyuan Zhang <sizhang@google.com>

server/etcdserver/api/capability.go

@@ -64,7 +64,7 @@ func UpdateCapability(lg *zap.Logger, v *semver.Version) {
 		return
 	}
 	enableMapMu.Lock()
-	if curVersion != nil && !serverversion.IsValidVersionChange(v, curVersion) {
+	if curVersion != nil && !serverversion.IsValidClusterVersionChange(v, curVersion) {
 		enableMapMu.Unlock()
 		return
 	}

server/etcdserver/version/downgrade.go

@@ -59,17 +59,17 @@ func allowedDowngradeVersion(ver *semver.Version) *semver.Version {
 	return &semver.Version{Major: ver.Major, Minor: ver.Minor - 1}
 }
 
-// IsValidVersionChange checks the two scenario when version is valid to change:
+// IsValidClusterVersionChange checks the two scenario when version is valid to change:
 // 1. Downgrade: cluster version is 1 minor version higher than local version,
 // cluster version should change.
 // 2. Cluster start: when not all members version are available, cluster version
 // is set to MinVersion(3.0), when all members are at higher version, cluster version
-// is lower than local version, cluster version should change
+// is lower than minimal server version, cluster version should change
-func IsValidVersionChange(cv *semver.Version, lv *semver.Version) bool {
+func IsValidClusterVersionChange(verFrom *semver.Version, verTo *semver.Version) bool {
-	cv = &semver.Version{Major: cv.Major, Minor: cv.Minor}
+	verFrom = &semver.Version{Major: verFrom.Major, Minor: verFrom.Minor}
-	lv = &semver.Version{Major: lv.Major, Minor: lv.Minor}
+	verTo = &semver.Version{Major: verTo.Major, Minor: verTo.Minor}
 
-	if isValidDowngrade(cv, lv) || (cv.Major == lv.Major && cv.LessThan(*lv)) {
+	if isValidDowngrade(verFrom, verTo) || (verFrom.Major == verTo.Major && verFrom.LessThan(*verTo)) {
 		return true
 	}
 	return false

server/etcdserver/version/downgrade_test.go

@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	"github.com/coreos/go-semver/semver"
+	"github.com/stretchr/testify/assert"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zaptest"
 
@@ -120,73 +121,68 @@ func TestIsValidDowngrade(t *testing.T) {
 }
 
 func TestIsVersionChangable(t *testing.T) {
-	v0 := semver.Must(semver.NewVersion("2.4.0"))
-	v1 := semver.Must(semver.NewVersion("3.4.0"))
-	v2 := semver.Must(semver.NewVersion("3.5.0"))
-	v3 := semver.Must(semver.NewVersion("3.5.1"))
-	v4 := semver.Must(semver.NewVersion("3.6.0"))
-
 	tests := []struct {
 		name           string
-		currentVersion *semver.Version
+		verFrom        string
-		localVersion   *semver.Version
+		verTo          string
 		expectedResult bool
 	}{
 		{
 			name:           "When local version is one minor lower than cluster version",
-			currentVersion: v2,
+			verFrom:        "3.5.0",
-			localVersion:   v1,
+			verTo:          "3.4.0",
 			expectedResult: true,
 		},
 		{
 			name:           "When local version is one minor and one patch lower than cluster version",
-			currentVersion: v3,
+			verFrom:        "3.5.1",
-			localVersion:   v1,
+			verTo:          "3.4.0",
 			expectedResult: true,
 		},
 		{
 			name:           "When local version is one minor higher than cluster version",
-			currentVersion: v1,
+			verFrom:        "3.4.0",
-			localVersion:   v2,
+			verTo:          "3.5.0",
 			expectedResult: true,
 		},
 		{
 			name:           "When local version is two minor higher than cluster version",
-			currentVersion: v1,
+			verFrom:        "3.4.0",
-			localVersion:   v4,
+			verTo:          "3.6.0",
 			expectedResult: true,
 		},
 		{
 			name:           "When local version is one major higher than cluster version",
-			currentVersion: v0,
+			verFrom:        "2.4.0",
-			localVersion:   v1,
+			verTo:          "3.4.0",
 			expectedResult: false,
 		},
 		{
 			name:           "When local version is equal to cluster version",
-			currentVersion: v1,
+			verFrom:        "3.4.0",
-			localVersion:   v1,
+			verTo:          "3.4.0",
 			expectedResult: false,
 		},
 		{
 			name:           "When local version is one patch higher than cluster version",
-			currentVersion: v2,
+			verFrom:        "3.5.0",
-			localVersion:   v3,
+			verTo:          "3.5.1",
 			expectedResult: false,
 		},
 		{
 			name:           "When local version is two minor lower than cluster version",
-			currentVersion: v4,
+			verFrom:        "3.6.0",
-			localVersion:   v1,
+			verTo:          "3.4.0",
 			expectedResult: false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if ret := IsValidVersionChange(tt.currentVersion, tt.localVersion); ret != tt.expectedResult {
+			verFrom := semver.Must(semver.NewVersion(tt.verFrom))
-				t.Errorf("Expected %v; Got %v", tt.expectedResult, ret)
+			verTo := semver.Must(semver.NewVersion(tt.verTo))
-			}
+			ret := IsValidClusterVersionChange(verFrom, verTo)
+			assert.Equal(t, tt.expectedResult, ret)
 		})
 	}
 }

server/etcdserver/version/monitor.go

@@ -97,7 +97,7 @@ func (m *Monitor) decideClusterVersion() (*semver.Version, error) {
 		}
 		return downgrade.GetTargetVersion(), nil
 	}
-	if clusterVersion.LessThan(*minimalServerVersion) && IsValidVersionChange(clusterVersion, minimalServerVersion) {
+	if clusterVersion.LessThan(*minimalServerVersion) && IsValidClusterVersionChange(clusterVersion, minimalServerVersion) {
 		return minimalServerVersion, nil
 	}
 	return nil, nil