server: Handle cluster version equal downgrade version

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>

server/etcdserver/server.go

@@ -2171,7 +2171,10 @@ func (s *EtcdServer) monitorClusterVersions() {
 		if s.Leader() != s.MemberId() {
 			continue
 		}
-		monitor.UpdateClusterVersionIfNeeded()
+		err := monitor.UpdateClusterVersionIfNeeded()
+		if err != nil {
+			s.lg.Error("Failed to monitor cluster version", zap.Error(err))
+		}
 	}
 }
 

server/etcdserver/version/monitor.go

@@ -16,6 +16,7 @@ package version
 
 import (
 	"context"
+	"errors"
 
 	"github.com/coreos/go-semver/semver"
 	"go.etcd.io/etcd/api/v3/version"
@@ -50,45 +51,55 @@ func NewMonitor(lg *zap.Logger, storage Server) *Monitor {
 }
 
 // UpdateClusterVersionIfNeeded updates the cluster version.
-func (m *Monitor) UpdateClusterVersionIfNeeded() {
+func (m *Monitor) UpdateClusterVersionIfNeeded() error {
-	newClusterVersion := m.decideClusterVersion()
+	newClusterVersion, err := m.decideClusterVersion()
 	if newClusterVersion != nil {
 		newClusterVersion = &semver.Version{Major: newClusterVersion.Major, Minor: newClusterVersion.Minor}
 		m.s.UpdateClusterVersion(newClusterVersion.String())
 	}
+	return err
 }
 
 // decideClusterVersion decides whether to change cluster version and its next value.
 // New cluster version is based on the members versions server and whether cluster is downgrading.
 // Returns nil if cluster version should be left unchanged.
-func (m *Monitor) decideClusterVersion() *semver.Version {
+func (m *Monitor) decideClusterVersion() (*semver.Version, error) {
 	clusterVersion := m.s.GetClusterVersion()
 	minimalServerVersion := m.membersMinimalServerVersion()
 	if clusterVersion == nil {
 		if minimalServerVersion != nil {
-			return minimalServerVersion
+			return minimalServerVersion, nil
 		}
-		return semver.New(version.MinClusterVersion)
+		return semver.New(version.MinClusterVersion), nil
 	}
 	if minimalServerVersion == nil {
-		return nil
+		return nil, nil
 	}
 	downgrade := m.s.GetDowngradeInfo()
 	if downgrade != nil && downgrade.Enabled {
-		if IsValidVersionChange(clusterVersion, downgrade.GetTargetVersion()) && IsValidVersionChange(minimalServerVersion, downgrade.GetTargetVersion()) {
+		if downgrade.GetTargetVersion().Equal(*clusterVersion) {
-			return downgrade.GetTargetVersion()
+			return nil, nil
 		}
-		m.lg.Error("Cannot downgrade cluster version, version change is not valid",
+		if !isValidDowngrade(clusterVersion, downgrade.GetTargetVersion()) {
-			zap.String("downgrade-version", downgrade.TargetVersion),
+			m.lg.Error("Cannot downgrade from cluster-version to downgrade-target",
-			zap.String("cluster-version", clusterVersion.String()),
+				zap.String("downgrade-target", downgrade.TargetVersion),
-			zap.String("minimal-server-version", minimalServerVersion.String()),
+				zap.String("cluster-version", clusterVersion.String()),
-		)
+			)
-		return nil
+			return nil, errors.New("invalid downgrade target")
+		}
+		if !isValidDowngrade(minimalServerVersion, downgrade.GetTargetVersion()) {
+			m.lg.Error("Cannot downgrade from minimal-server-version to downgrade-target",
+				zap.String("downgrade-target", downgrade.TargetVersion),
+				zap.String("minimal-server-version", minimalServerVersion.String()),
+			)
+			return nil, errors.New("invalid downgrade target")
+		}
+		return downgrade.GetTargetVersion(), nil
 	}
 	if clusterVersion.LessThan(*minimalServerVersion) && IsValidVersionChange(clusterVersion, minimalServerVersion) {
-		return minimalServerVersion
+		return minimalServerVersion, nil
 	}
-	return nil
+	return nil, nil
 }
 
 // UpdateStorageVersionIfNeeded updates the storage version if it differs from cluster version.

server/etcdserver/version/monitor_test.go

@@ -2,6 +2,7 @@ package version
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -159,6 +160,7 @@ func TestUpdateClusterVersionIfNeeded(t *testing.T) {
 		memberVersions       map[string]*version.Versions
 		downgrade            *DowngradeInfo
 		expectClusterVersion *semver.Version
+		expectError          error
 	}{
 		{
 			name:                 "Default to 3.0 if there are no members",
@@ -167,24 +169,40 @@ func TestUpdateClusterVersionIfNeeded(t *testing.T) {
 		{
 			name: "Should pick lowest server version from members",
 			memberVersions: map[string]*version.Versions{
-				"a": {Cluster: "3.7.0", Server: "3.6.0"},
+				"a": {Server: "3.6.0"},
-				"b": {Cluster: "3.4.0", Server: "3.5.0"},
+				"b": {Server: "3.5.0"},
+			},
+			expectClusterVersion: &version.V3_5,
+		},
+		{
+			name: "Should support not full releases",
+			memberVersions: map[string]*version.Versions{
+				"b": {Server: "3.5.0-alpha.0"},
 			},
 			expectClusterVersion: &version.V3_5,
 		},
 		{
 			name: "Sets minimal version when member has broken version",
 			memberVersions: map[string]*version.Versions{
-				"a": {Cluster: "3.7.0", Server: "3.6.0"},
+				"a": {Server: "3.6.0"},
-				"b": {Cluster: "xxxx", Server: "yyyy"},
+				"b": {Server: "yyyy"},
 			},
 			expectClusterVersion: &version.V3_0,
 		},
 		{
-			name: "Should pick lowest server version from members (cv already set)",
+			name: "Should not downgrade cluster version without explicit downgrade request",
 			memberVersions: map[string]*version.Versions{
-				"a": {Cluster: "3.7.0", Server: "3.6.0"},
+				"a": {Server: "3.5.0"},
-				"b": {Cluster: "3.4.0", Server: "3.5.0"},
+				"b": {Server: "3.6.0"},
+			},
+			clusterVersion:       &version.V3_6,
+			expectClusterVersion: &version.V3_6,
+		},
+		{
+			name: "Should not upgrade cluster version if there is still member old member",
+			memberVersions: map[string]*version.Versions{
+				"a": {Server: "3.5.0"},
+				"b": {Server: "3.6.0"},
 			},
 			clusterVersion:       &version.V3_5,
 			expectClusterVersion: &version.V3_5,
@@ -192,8 +210,8 @@ func TestUpdateClusterVersionIfNeeded(t *testing.T) {
 		{
 			name: "Should upgrade cluster version if all members have upgraded (have higher server version)",
 			memberVersions: map[string]*version.Versions{
-				"a": {Cluster: "3.5.0", Server: "3.6.0"},
+				"a": {Server: "3.6.0"},
-				"b": {Cluster: "3.5.0", Server: "3.6.0"},
+				"b": {Server: "3.6.0"},
 			},
 			clusterVersion:       &version.V3_5,
 			expectClusterVersion: &version.V3_6,
@@ -201,32 +219,34 @@ func TestUpdateClusterVersionIfNeeded(t *testing.T) {
 		{
 			name: "Should downgrade cluster version if downgrade is set to allow older members to join",
 			memberVersions: map[string]*version.Versions{
-				"a": {Cluster: "3.6.0", Server: "3.6.0"},
+				"a": {Server: "3.6.0"},
-				"b": {Cluster: "3.6.0", Server: "3.6.0"},
+				"b": {Server: "3.6.0"},
 			},
 			clusterVersion:       &version.V3_6,
 			downgrade:            &DowngradeInfo{TargetVersion: "3.5.0", Enabled: true},
 			expectClusterVersion: &version.V3_5,
 		},
-		{
-			name: "Should maintain downgrade target version to allow older members to join",
-			memberVersions: map[string]*version.Versions{
-				"a": {Cluster: "3.5.0", Server: "3.6.0"},
-				"b": {Cluster: "3.5.0", Server: "3.6.0"},
-			},
-			clusterVersion:       &version.V3_5,
-			downgrade:            &DowngradeInfo{TargetVersion: "3.5.0", Enabled: true},
-			expectClusterVersion: &version.V3_5,
-		},
 		{
 			name: "Don't downgrade below supported range",
 			memberVersions: map[string]*version.Versions{
-				"a": {Cluster: "3.5.0", Server: "3.6.0"},
+				"a": {Server: "3.6.0"},
-				"b": {Cluster: "3.5.0", Server: "3.6.0"},
+				"b": {Server: "3.6.0"},
 			},
 			clusterVersion:       &version.V3_5,
 			downgrade:            &DowngradeInfo{TargetVersion: "3.4.0", Enabled: true},
 			expectClusterVersion: &version.V3_5,
+			expectError:          fmt.Errorf("invalid downgrade target"),
+		},
+		{
+			name: "Don't downgrade above cluster version",
+			memberVersions: map[string]*version.Versions{
+				"a": {Server: "3.5.0"},
+				"b": {Server: "3.5.0"},
+			},
+			clusterVersion:       &version.V3_5,
+			downgrade:            &DowngradeInfo{TargetVersion: "3.6.0", Enabled: true},
+			expectClusterVersion: &version.V3_5,
+			expectError:          fmt.Errorf("invalid downgrade target"),
 		},
 	}
 
@@ -239,11 +259,14 @@ func TestUpdateClusterVersionIfNeeded(t *testing.T) {
 			}
 			monitor := NewMonitor(zaptest.NewLogger(t), s)
 
-			// Run multiple times to ensure that results are stable
+			err := monitor.UpdateClusterVersionIfNeeded()
-			for i := 0; i < 3; i++ {
+			assert.Equal(t, tt.expectClusterVersion, s.clusterVersion)
-				monitor.UpdateClusterVersionIfNeeded()
+			assert.Equal(t, tt.expectError, err)
-				assert.Equal(t, tt.expectClusterVersion, s.clusterVersion)
+
-			}
+			// Ensure results are stable
+			newVersion, err := monitor.decideClusterVersion()
+			assert.Nil(t, newVersion)
+			assert.Equal(t, tt.expectError, err)
 		})
 	}
 }

server/etcdserver/version/version_test.go

@@ -147,7 +147,7 @@ func (c *clusterMock) StepMonitors() {
 	for _, m := range c.members {
 		fs = append(fs, m.monitor.UpdateStorageVersionIfNeeded)
 		if m.isLeader {
-			fs = append(fs, m.monitor.CancelDowngradeIfNeeded, m.monitor.UpdateClusterVersionIfNeeded)
+			fs = append(fs, m.monitor.CancelDowngradeIfNeeded, func() { m.monitor.UpdateClusterVersionIfNeeded() })
 		}
 	}
 	rand.Shuffle(len(fs), func(i, j int) {