From be61e2ced1925d6180f40a203113ad613656b2a4 Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Mon, 28 May 2018 16:25:25 +0200 Subject: [PATCH] sentinel: fix check for same reported vs spec sync standbys When electing a new master we are swapping the new master uid with the old master uid in the syncstandbys slice. This could end with an unordered slice in the spec that will make the sentinel fail the check that the reported standbys are the same of the spec one blocking any future syncstandby update. Since the reported order is not a problem just check that the syncstandbys are the same regardless of their order. We'll keep the sorting to avoid unneeded updates to synchronous_standby_names by the keeper. --- cmd/sentinel/cmd/sentinel.go | 13 +- cmd/sentinel/cmd/sentinel_test.go | 332 +++++++++++++++++++++++++++++- pkg/util/slice.go | 27 ++- pkg/util/slice_test.go | 69 +++++++ 4 files changed, 427 insertions(+), 14 deletions(-) create mode 100644 pkg/util/slice_test.go diff --git a/cmd/sentinel/cmd/sentinel.go b/cmd/sentinel/cmd/sentinel.go index f5471a9e5..0809c1bcd 100644 --- a/cmd/sentinel/cmd/sentinel.go +++ b/cmd/sentinel/cmd/sentinel.go @@ -304,8 +304,6 @@ func (s *Sentinel) updateKeepersStatus(cd *cluster.ClusterData, keepersInfo clus db.Status.TimelinesHistory = dbs.TimelinesHistory db.Status.PGParameters = cluster.PGParameters(dbs.PGParameters) - // Sort synchronousStandbys so we can compare the slice regardless of its order - sort.Sort(sort.StringSlice(dbs.SynchronousStandbys)) db.Status.SynchronousStandbys = dbs.SynchronousStandbys db.Status.OlderWalFile = dbs.OlderWalFile @@ -1074,6 +1072,11 @@ func (s *Sentinel) updateCluster(cd *cluster.ClusterData, pis cluster.ProxiesInf if len(newMasterDB.Spec.SynchronousStandbys) == 0 { newMasterDB.Spec.ExternalSynchronousStandbys = []string{fakeStandbyName} } + + // Just sort to always have them in the same order and avoid + // unneeded updates to synchronous_standby_names by the keeper. + sort.Sort(sort.StringSlice(newMasterDB.Spec.SynchronousStandbys)) + sort.Sort(sort.StringSlice(newMasterDB.Spec.ExternalSynchronousStandbys)) } else { newMasterDB.Spec.SynchronousReplication = false newMasterDB.Spec.SynchronousStandbys = nil @@ -1189,7 +1192,7 @@ func (s *Sentinel) updateCluster(cd *cluster.ClusterData, pis cluster.ProxiesInf // this way, when we have to choose a new master we are sure // that there're no intermediate changes between the // reported standbys and the required ones. - if !util.CompareStringSlice(masterDB.Status.SynchronousStandbys, masterDB.Spec.SynchronousStandbys) { + if !util.CompareStringSliceNoOrder(masterDB.Status.SynchronousStandbys, masterDB.Spec.SynchronousStandbys) { log.Infof("won't update masterDB required synchronous standby since the latest master reported synchronous standbys are different from the db spec ones", "reported", curMasterDB.Status.SynchronousStandbys, "spec", curMasterDB.Spec.SynchronousStandbys) } else { addFakeStandby := false @@ -1338,10 +1341,10 @@ func (s *Sentinel) updateCluster(cd *cluster.ClusterData, pis cluster.ProxiesInf masterDB.Spec.ExternalSynchronousStandbys = append(masterDB.Spec.ExternalSynchronousStandbys, fakeStandbyName) } - // Sort synchronousStandbys so we can compare the slice regardless of its order + // Just sort to always have them in the same order and avoid + // unneeded updates to synchronous_standby_names by the keeper. sort.Sort(sort.StringSlice(masterDB.Spec.SynchronousStandbys)) sort.Sort(sort.StringSlice(masterDB.Spec.ExternalSynchronousStandbys)) - } } else { masterDB.Spec.SynchronousReplication = false diff --git a/cmd/sentinel/cmd/sentinel_test.go b/cmd/sentinel/cmd/sentinel_test.go index 4dcc44f88..4456c6df4 100644 --- a/cmd/sentinel/cmd/sentinel_test.go +++ b/cmd/sentinel/cmd/sentinel_test.go @@ -3211,6 +3211,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -3265,7 +3273,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 1, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -3328,6 +3336,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -3382,7 +3398,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 1, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -3451,6 +3467,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -3505,7 +3529,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 1, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -3568,6 +3592,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -3622,7 +3654,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 1, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -3689,6 +3721,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -3743,7 +3783,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 1, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -3806,6 +3846,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -3860,7 +3908,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 1, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -3929,6 +3977,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -3983,7 +4039,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 1, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -4046,6 +4102,14 @@ func TestUpdateCluster(t *testing.T) { LastHealthyTime: now, }, }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, }, DBs: cluster.DBs{ "db1": &cluster.DB{ @@ -4100,7 +4164,7 @@ func TestUpdateCluster(t *testing.T) { Generation: 2, ChangeTime: time.Time{}, Spec: &cluster.DBSpec{ - KeeperUID: "keeper2", + KeeperUID: "keeper3", RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, MaxStandbys: cluster.DefaultMaxStandbys, AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, @@ -4126,6 +4190,258 @@ func TestUpdateCluster(t *testing.T) { }, }, }, + // #23 One master and two standbys. Synchronous replication already + // enabled with MinSynchronousStandbys and MaxSynchronousStandbys to 2. + // master (db1) and db2 failed, db3 elected as master. + // This test checks that the db3 synchronousStandbys are correctly sorted + { + cd: &cluster.ClusterData{ + Cluster: &cluster.Cluster{ + UID: "cluster1", + Generation: 1, + Spec: &cluster.ClusterSpec{ + ConvergenceTimeout: &cluster.Duration{Duration: cluster.DefaultConvergenceTimeout}, + InitTimeout: &cluster.Duration{Duration: cluster.DefaultInitTimeout}, + SyncTimeout: &cluster.Duration{Duration: cluster.DefaultSyncTimeout}, + MaxStandbysPerSender: cluster.Uint16P(cluster.DefaultMaxStandbysPerSender), + SynchronousReplication: cluster.BoolP(true), + }, + Status: cluster.ClusterStatus{ + CurrentGeneration: 1, + Phase: cluster.ClusterPhaseNormal, + Master: "db1", + }, + }, + Keepers: cluster.Keepers{ + "keeper1": &cluster.Keeper{ + UID: "keeper1", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, + "keeper2": &cluster.Keeper{ + UID: "keeper2", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, + }, + DBs: cluster.DBs{ + "db1": &cluster.DB{ + UID: "db1", + Generation: 1, + ChangeTime: time.Time{}, + Spec: &cluster.DBSpec{ + KeeperUID: "keeper1", + RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, + MaxStandbys: cluster.DefaultMaxStandbys, + AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, + InitMode: cluster.DBInitModeNone, + SynchronousReplication: true, + Role: common.RoleMaster, + Followers: []string{"db2", "db3"}, + SynchronousStandbys: []string{"db2", "db3"}, + ExternalSynchronousStandbys: []string{}, + }, + Status: cluster.DBStatus{ + Healthy: false, + CurrentGeneration: 1, + SynchronousStandbys: []string{"db2", "db3"}, + }, + }, + "db2": &cluster.DB{ + UID: "db2", + Generation: 1, + ChangeTime: time.Time{}, + Spec: &cluster.DBSpec{ + KeeperUID: "keeper2", + RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, + MaxStandbys: cluster.DefaultMaxStandbys, + AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, + InitMode: cluster.DBInitModeNone, + SynchronousReplication: false, + Role: common.RoleStandby, + Followers: []string{}, + FollowConfig: &cluster.FollowConfig{ + Type: cluster.FollowTypeInternal, + DBUID: "db1", + }, + SynchronousStandbys: nil, + ExternalSynchronousStandbys: nil, + }, + Status: cluster.DBStatus{ + Healthy: false, + CurrentGeneration: 1, + }, + }, + "db3": &cluster.DB{ + UID: "db3", + Generation: 1, + ChangeTime: time.Time{}, + Spec: &cluster.DBSpec{ + KeeperUID: "keeper3", + RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, + MaxStandbys: cluster.DefaultMaxStandbys, + AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, + InitMode: cluster.DBInitModeNone, + SynchronousReplication: false, + Role: common.RoleStandby, + Followers: []string{}, + FollowConfig: &cluster.FollowConfig{ + Type: cluster.FollowTypeInternal, + DBUID: "db1", + }, + SynchronousStandbys: nil, + ExternalSynchronousStandbys: nil, + }, + Status: cluster.DBStatus{ + Healthy: true, + CurrentGeneration: 1, + }, + }, + }, + Proxy: &cluster.Proxy{ + Generation: 1, + Spec: cluster.ProxySpec{ + MasterDBUID: "db1", + EnabledProxies: []string{}, + }, + }, + }, + outcd: &cluster.ClusterData{ + Cluster: &cluster.Cluster{ + UID: "cluster1", + Generation: 1, + Spec: &cluster.ClusterSpec{ + ConvergenceTimeout: &cluster.Duration{Duration: cluster.DefaultConvergenceTimeout}, + InitTimeout: &cluster.Duration{Duration: cluster.DefaultInitTimeout}, + SyncTimeout: &cluster.Duration{Duration: cluster.DefaultSyncTimeout}, + MaxStandbysPerSender: cluster.Uint16P(cluster.DefaultMaxStandbysPerSender), + SynchronousReplication: cluster.BoolP(true), + }, + Status: cluster.ClusterStatus{ + CurrentGeneration: 1, + Phase: cluster.ClusterPhaseNormal, + Master: "db3", + }, + }, + Keepers: cluster.Keepers{ + "keeper1": &cluster.Keeper{ + UID: "keeper1", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, + "keeper2": &cluster.Keeper{ + UID: "keeper2", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, + "keeper3": &cluster.Keeper{ + UID: "keeper3", + Spec: &cluster.KeeperSpec{}, + Status: cluster.KeeperStatus{ + Healthy: true, + LastHealthyTime: now, + }, + }, + }, + DBs: cluster.DBs{ + "db1": &cluster.DB{ + UID: "db1", + Generation: 2, + ChangeTime: time.Time{}, + Spec: &cluster.DBSpec{ + KeeperUID: "keeper1", + RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, + MaxStandbys: cluster.DefaultMaxStandbys, + AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, + InitMode: cluster.DBInitModeNone, + SynchronousReplication: true, + Role: common.RoleMaster, + Followers: []string{}, + SynchronousStandbys: []string{"db2", "db3"}, + ExternalSynchronousStandbys: []string{}, + }, + Status: cluster.DBStatus{ + Healthy: false, + CurrentGeneration: 1, + SynchronousStandbys: []string{"db2", "db3"}, + }, + }, + "db2": &cluster.DB{ + UID: "db2", + Generation: 1, + ChangeTime: time.Time{}, + Spec: &cluster.DBSpec{ + KeeperUID: "keeper2", + RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, + MaxStandbys: cluster.DefaultMaxStandbys, + AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, + InitMode: cluster.DBInitModeNone, + SynchronousReplication: false, + Role: common.RoleStandby, + Followers: []string{}, + FollowConfig: &cluster.FollowConfig{ + Type: cluster.FollowTypeInternal, + DBUID: "db1", + }, + SynchronousStandbys: nil, + ExternalSynchronousStandbys: nil, + }, + Status: cluster.DBStatus{ + Healthy: false, + CurrentGeneration: 1, + }, + }, + "db3": &cluster.DB{ + UID: "db3", + Generation: 2, + ChangeTime: time.Time{}, + Spec: &cluster.DBSpec{ + KeeperUID: "keeper3", + RequestTimeout: cluster.Duration{Duration: cluster.DefaultRequestTimeout}, + MaxStandbys: cluster.DefaultMaxStandbys, + AdditionalWalSenders: cluster.DefaultAdditionalWalSenders, + InitMode: cluster.DBInitModeNone, + SynchronousReplication: true, + Role: common.RoleMaster, + Followers: []string{}, + SynchronousStandbys: []string{"db1", "db2"}, + ExternalSynchronousStandbys: []string{}, + }, + Status: cluster.DBStatus{ + Healthy: true, + CurrentGeneration: 1, + }, + }, + }, + Proxy: &cluster.Proxy{ + Generation: 2, + Spec: cluster.ProxySpec{ + MasterDBUID: "", + EnabledProxies: []string{}, + }, + }, + }, + }, } for i, tt := range tests { diff --git a/pkg/util/slice.go b/pkg/util/slice.go index fa48c031f..05fc20d4f 100644 --- a/pkg/util/slice.go +++ b/pkg/util/slice.go @@ -1,4 +1,4 @@ -// Copyright 2015 Sorint.lab +// Copyright 2018 Sorint.lab // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -14,6 +14,8 @@ package util +import "sort" + func StringInSlice(s []string, e string) bool { for _, v := range s { if v == e { @@ -37,6 +39,29 @@ func CompareStringSlice(a []string, b []string) bool { return true } +// CompareStringSliceNoOrder compares two slices of strings regardless of their order, a nil slice is considered an empty one +func CompareStringSliceNoOrder(a []string, b []string) bool { + if len(a) != len(b) { + return false + } + + // This isn't the faster way but it's cleaner and enough for us + + // Take a copy of the original slice + a = append([]string(nil), a...) + b = append([]string(nil), b...) + + sort.Sort(sort.StringSlice(a)) + sort.Sort(sort.StringSlice(b)) + + for i, v := range a { + if v != b[i] { + return false + } + } + return true +} + // CommonElements return the common elements in two slices of strings func CommonElements(a []string, b []string) []string { common := []string{} diff --git a/pkg/util/slice_test.go b/pkg/util/slice_test.go new file mode 100644 index 000000000..0d1915e99 --- /dev/null +++ b/pkg/util/slice_test.go @@ -0,0 +1,69 @@ +// Copyright 2018 Sorint.lab +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied +// See the License for the specific language governing permissions and +// limitations under the License. + +package util + +import "testing" + +func TestCompareStringSlice(t *testing.T) { + tests := []struct { + a []string + b []string + ok bool + }{ + {[]string{}, []string{}, true}, + {[]string{"", ""}, []string{""}, false}, + {[]string{"", ""}, []string{"", ""}, true}, + {[]string{"a", "b"}, []string{"a", "b"}, true}, + {[]string{"a", "b"}, []string{"b", "a"}, false}, + {[]string{"a", "b", "c"}, []string{"a", "b"}, false}, + {[]string{"a", "b", "c"}, []string{"a", "b", "c"}, true}, + {[]string{"a", "b", "c"}, []string{"b", "c", "a"}, false}, + {[]string{"a", "b", "c", "a"}, []string{"a", "c", "b", "b"}, false}, + {[]string{"a", "b", "c", "a"}, []string{"a", "c", "b", "b"}, false}, + } + + for i, tt := range tests { + ok := CompareStringSlice(tt.a, tt.b) + if ok != tt.ok { + t.Errorf("%d: got %t but wanted: %t a: %v, b: %v", i, ok, tt.ok, tt.a, tt.b) + } + } +} + +func TestCompareStringSliceNoOrder(t *testing.T) { + tests := []struct { + a []string + b []string + ok bool + }{ + {[]string{}, []string{}, true}, + {[]string{"", ""}, []string{""}, false}, + {[]string{"", ""}, []string{"", ""}, true}, + {[]string{"a", "b"}, []string{"a", "b"}, true}, + {[]string{"a", "b"}, []string{"b", "a"}, true}, + {[]string{"a", "b", "c"}, []string{"a", "b"}, false}, + {[]string{"a", "b", "c"}, []string{"a", "b", "c"}, true}, + {[]string{"a", "b", "c"}, []string{"b", "c", "a"}, true}, + {[]string{"a", "b", "c", "a"}, []string{"a", "c", "b", "b"}, false}, + {[]string{"a", "b", "c", "a"}, []string{"a", "c", "b", "b"}, false}, + } + + for i, tt := range tests { + ok := CompareStringSliceNoOrder(tt.a, tt.b) + if ok != tt.ok { + t.Errorf("%d: got %t but wanted: %t a: %v, b: %v", i, ok, tt.ok, tt.a, tt.b) + } + } +}