From 2b178fdd962f26c97b673171c10cb63fc6ccf413 Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Fri, 14 Oct 2022 14:41:04 +0200 Subject: [PATCH] server: Handle cluster version equal downgrade version Signed-off-by: Marek Siarkowicz --- server/etcdserver/server.go | 5 +- server/etcdserver/version/monitor.go | 43 +++++++----- server/etcdserver/version/monitor_test.go | 79 +++++++++++++++-------- server/etcdserver/version/version_test.go | 2 +- 4 files changed, 83 insertions(+), 46 deletions(-) diff --git a/server/etcdserver/server.go b/server/etcdserver/server.go index 5f039c524..2a5a4c4a5 100644 --- a/server/etcdserver/server.go +++ b/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)) + } } } diff --git a/server/etcdserver/version/monitor.go b/server/etcdserver/version/monitor.go index 2ccc897a0..8cfb916a7 100644 --- a/server/etcdserver/version/monitor.go +++ b/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() { - newClusterVersion := m.decideClusterVersion() +func (m *Monitor) UpdateClusterVersionIfNeeded() error { + 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()) { - return downgrade.GetTargetVersion() + if downgrade.GetTargetVersion().Equal(*clusterVersion) { + return nil, nil } - m.lg.Error("Cannot downgrade cluster version, version change is not valid", - zap.String("downgrade-version", downgrade.TargetVersion), - zap.String("cluster-version", clusterVersion.String()), - zap.String("minimal-server-version", minimalServerVersion.String()), - ) - return nil + if !isValidDowngrade(clusterVersion, downgrade.GetTargetVersion()) { + m.lg.Error("Cannot downgrade from cluster-version to downgrade-target", + zap.String("downgrade-target", downgrade.TargetVersion), + zap.String("cluster-version", clusterVersion.String()), + ) + 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. diff --git a/server/etcdserver/version/monitor_test.go b/server/etcdserver/version/monitor_test.go index 7d4e10884..43eafb79e 100644 --- a/server/etcdserver/version/monitor_test.go +++ b/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"}, - "b": {Cluster: "3.4.0", Server: "3.5.0"}, + "a": {Server: "3.6.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"}, - "b": {Cluster: "xxxx", Server: "yyyy"}, + "a": {Server: "3.6.0"}, + "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"}, - "b": {Cluster: "3.4.0", Server: "3.5.0"}, + "a": {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"}, - "b": {Cluster: "3.5.0", Server: "3.6.0"}, + "a": {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"}, - "b": {Cluster: "3.6.0", Server: "3.6.0"}, + "a": {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"}, - "b": {Cluster: "3.5.0", Server: "3.6.0"}, + "a": {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 - for i := 0; i < 3; i++ { - monitor.UpdateClusterVersionIfNeeded() - assert.Equal(t, tt.expectClusterVersion, s.clusterVersion) - } + err := monitor.UpdateClusterVersionIfNeeded() + assert.Equal(t, tt.expectClusterVersion, s.clusterVersion) + assert.Equal(t, tt.expectError, err) + + // Ensure results are stable + newVersion, err := monitor.decideClusterVersion() + assert.Nil(t, newVersion) + assert.Equal(t, tt.expectError, err) }) } } diff --git a/server/etcdserver/version/version_test.go b/server/etcdserver/version/version_test.go index 9863d525f..7b9290260 100644 --- a/server/etcdserver/version/version_test.go +++ b/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) {