From e1538c0aff9cb32a70ee4ccdfa5e9b0e50d8f881 Mon Sep 17 00:00:00 2001 From: Matthew Krajewski Date: Wed, 28 Aug 2019 09:51:26 -0400 Subject: [PATCH] etcd-backup-operator: fixed sorting of etcd backups to ensure deletion of only the oldest backups when rotating When rotating backups based on a maximum number allowed, newer backups were being deleted when the number of digits in an etcd store revision increased. E.g., when "v99" became "v100", the newer "v100" backup was being deleted, because the "1" in "100" was sorted to come before the first "9" in "99". This new sorting method relies on the timestamp in each backup path, rather than the revision number. The reason the timestamp is given precedence over the revision when sorting is because the revision could potentially go backwards when an older backup is restored. See my comment in pkg/backup/util/util.go's SortableBackupPaths type for more details. Fixes #2113 --- pkg/backup/backup_manager.go | 4 +- pkg/backup/util/util.go | 74 ++++++++ pkg/backup/util/util_test.go | 347 +++++++++++++++++++++++++++++++++++ 3 files changed, 424 insertions(+), 1 deletion(-) create mode 100644 pkg/backup/util/util_test.go diff --git a/pkg/backup/backup_manager.go b/pkg/backup/backup_manager.go index 2afadd36e..dc7ed6f1f 100644 --- a/pkg/backup/backup_manager.go +++ b/pkg/backup/backup_manager.go @@ -21,6 +21,7 @@ import ( "sort" "time" + "github.com/coreos/etcd-operator/pkg/backup/util" "github.com/coreos/etcd-operator/pkg/backup/writer" "github.com/coreos/etcd-operator/pkg/util/constants" @@ -73,6 +74,7 @@ func (bm *BackupManager) SaveSnap(ctx context.Context, s3Path string, isPeriodic } defer rc.Close() if isPeriodic { + // NOTE: make sure this path format stays in sync with util.SortableBackupPaths s3Path = fmt.Sprintf(s3Path+"_v%d_%s", rev, now.Format("2006-01-02-15:04:05")) } _, err = bm.bw.Write(ctx, s3Path, rc) @@ -89,7 +91,7 @@ func (bm *BackupManager) EnsureMaxBackup(ctx context.Context, basePath string, m if err != nil { return fmt.Errorf("failed to get exisiting snapshots: %v", err) } - sort.Sort(sort.Reverse(sort.StringSlice(savedSnapShots))) + sort.Sort(sort.Reverse(util.SortableBackupPaths(sort.StringSlice(savedSnapShots)))) for i, snapshotPath := range savedSnapShots { if i < maxCount { continue diff --git a/pkg/backup/util/util.go b/pkg/backup/util/util.go index 1b5e81259..447359619 100644 --- a/pkg/backup/util/util.go +++ b/pkg/backup/util/util.go @@ -16,6 +16,9 @@ package util import ( "fmt" + "regexp" + "sort" + "strconv" "strings" ) @@ -32,3 +35,74 @@ func ParseBucketAndKey(path string) (string, string, error) { } return toks[0], toks[1], nil } + +// SortableBackupPaths implements extends sort.StringSlice to allow sorting to work +// with paths used for backups, in the format "_v_YYYY-MM-DD-HH:mm:SS", +// where the etcd store revision (an integer) is what is being sorted on. +type SortableBackupPaths sort.StringSlice + +// regular expressions used in backup path order comparison +var backupTimestampRegex = regexp.MustCompile(`_\d+-\d+-\d+-\d+:\d+:\d+`) +var etcdStoreRevisionRegex = regexp.MustCompile(`_v\d+`) + +// Len is the number of elements in the collection. +func (s SortableBackupPaths) Len() int { + return len(s) +} + +// Less reports whether the element with index i should sort before the element with index j. +// Assumes that the two paths are part of a sequence of backups of the same etcd cluster, +// relying on comparison of the timestamps found in the two elements' paths, +// but not making assumptions about either path's base path. +// The last etcd store revision number found in each path is used to break ties. +// Timestamp comparison takes precedence in case an older revision of the etcd store is restored +// to the cluster, resulting in newer, more relevant backups that happen to have older revision numbers. +// If either base path happens to have a similarly formatted revision number, +// the last one in each path is compared. +// This method will work even if the format changes somewhat (e.g., the revision is placed after the timesamp). +// If a comparison can't be made based on path format, the one conforming to the format is considered to be newer, +// and if neither path conforms, false is returned. +func (s SortableBackupPaths) Less(i, j int) bool { + // compare timestamps first + itmatches := backupTimestampRegex.FindAll([]byte(s[i]), -1) + jtmatches := backupTimestampRegex.FindAll([]byte(s[j]), -1) + + if len(itmatches) < 1 { + return true + } + if len(jtmatches) < 1 { + return false + } + + itstr := string(itmatches[len(itmatches)-1]) + jtstr := string(jtmatches[len(jtmatches)-1]) + + timestampComparison := strings.Compare(string(jtstr), string(itstr)) + if timestampComparison != 0 { + return timestampComparison > 0 + } + + // fall through to revision comparison + irmatches := etcdStoreRevisionRegex.FindAll([]byte(s[i]), -1) + jrmatches := etcdStoreRevisionRegex.FindAll([]byte(s[j]), -1) + + if len(irmatches) < 1 { + return true + } + if len(jrmatches) < 1 { + return false + } + + irstr := string(irmatches[len(irmatches)-1]) + jrstr := string(jrmatches[len(jrmatches)-1]) + + ir, _ := strconv.ParseInt(irstr[2:len(irstr)], 10, 64) + jr, _ := strconv.ParseInt(jrstr[2:len(jrstr)], 10, 64) + + return ir < jr +} + +// Swap swaps the elements with indexes i and j. +func (s SortableBackupPaths) Swap(i, j int) { + s[i], s[j] = s[j], s[i] +} diff --git a/pkg/backup/util/util_test.go b/pkg/backup/util/util_test.go new file mode 100644 index 000000000..521fe0c62 --- /dev/null +++ b/pkg/backup/util/util_test.go @@ -0,0 +1,347 @@ +// Copyright 2017 The etcd-operator Authors +// +// 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 ( + "fmt" + "reflect" + "testing" +) + +// Tests that SortableBackupPaths.Len() simply delegates the call to the slice. +func TestSortableBackupPathsLen(t *testing.T) { + tests := []struct { + backupPaths []string + }{ + // empty list + { + backupPaths: []string{}, + }, + // single entry + { + backupPaths: []string{"path"}, + }, + // multiple entries + { + backupPaths: []string{"path1", "path2", "path3"}, + }, + } + + for i, tt := range tests { + expected := len(tt.backupPaths) + actual := SortableBackupPaths(tt.backupPaths).Len() + if actual != expected { + t.Errorf("#%d: SortableBackupPaths.Len failed. Expected %d, but got %d", i, expected, actual) + } + } +} + +// Tests that SortableBackupPaths.Less() correctly determines the age ordering of etcd backup paths. +func TestSortableBackupPathsLess(t *testing.T) { + // for all test cases, we expect the first element to be less/older than the second + tests := []struct { + backupPaths []string + lesserIndex int + greaterIndex int + expectPanic bool + }{ + // etcd in normal use - timestamp and revision increase + { + backupPaths: []string{ + "prefix_v10_2020-01-02-10:00:00", + "prefix_v20_2020-01-02-11:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // mixed with other paths that are not part of the comparison + { + backupPaths: []string{ + "foo", + "prefix_v10_2020-01-02-10:00:00", + "bar", + "prefix_v20_2020-01-02-11:00:00", + "baz", + }, + lesserIndex: 1, + greaterIndex: 3, + }, + // etcd idle - timestamp increases by a second, while revision remains the same + { + backupPaths: []string{ + "prefix_v1_2020-01-01-10:00:00", + "prefix_v1_2020-01-01-10:00:01", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // greater/newer is placed before lesser/older + { + backupPaths: []string{ + "prefix_v1_2020-01-01-10:00:01", + "prefix_v1_2020-01-01-10:00:00", + }, + lesserIndex: 1, + greaterIndex: 0, + }, + // index out of bounds - i + { + backupPaths: []string{ + "prefix_v1_2020-01-01-10:00:00", + "prefix_v1_2020-01-01-10:00:01", + }, + lesserIndex: 2, + greaterIndex: 1, + expectPanic: true, + }, + // index out of bounds - j + { + backupPaths: []string{ + "prefix_v1_2020-01-01-10:00:00", + "prefix_v1_2020-01-01-10:00:01", + }, + lesserIndex: 2, + greaterIndex: 1, + expectPanic: true, + }, + // timestamp minute takes precedence over second + { + backupPaths: []string{ + "prefix_v1_2020-01-01-10:00:59", + "prefix_v1_2020-01-01-10:01:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // timestamp hour takes precedence over minute + { + backupPaths: []string{ + "prefix_v1_2020-01-01-10:59:00", + "prefix_v1_2020-01-01-11:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // timestamp day takes precedence over hour + { + backupPaths: []string{ + "prefix_v1_2020-01-01-23:00:00", + "prefix_v1_2020-01-02-10:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // timestamp month takes precedence over day + { + backupPaths: []string{ + "prefix_v1_2020-01-30-10:00:00", + "prefix_v1_2020-02-01-10:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // timestamp year takes precedence over month + { + backupPaths: []string{ + "prefix_v1_2020-12-01-10:00:00", + "prefix_v1_2021-01-01-10:00:01", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // old backup restored - timestamp increases, but revision number reverts + { + backupPaths: []string{ + "prefix_v100_2020-01-03-10:00:00", + "prefix_v1_2020-01-03-11:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // ensure that only the last timestamp found is used + { + backupPaths: []string{ + "prefixWithUnexpectedTimestamp_2020-01-04-11:00:00-_v1000_2020-01-04-10:00:00", + "prefixWithUnexpectedTimestamp_2020-01-04-10:00:00-_v1000_2020-01-04-11:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // different prefixes of different lengths - still sort by timestamp + { + backupPaths: []string{ + "zzzLongLongLongPrefix_v1111_2020-01-05-10:00:00", + "aaaShorterPrefix_v1111_2020-01-05-11:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // the one without a timestamp in the expected format is considered older + { + backupPaths: []string{ + "prefix_v2222_2020-01-06-12:00", + "prefix_v2222_2020-01-06-10:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // timestamp tie - sort by revision number + { + backupPaths: []string{ + "prefix_v3333-2020-01-07-10:00:00", + "prefix_v4444_2020-01-07-10:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // timestamp tie - the last revision number found is used to break it + { + backupPaths: []string{ + "prefixWithUnexpectedRevisionNumber_v2222_v1111-2020-01-04-10:00:00", + "prefixWithUnexpectedRevisionNumber_v1111_v2222_2020-01-04-10:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // timestamp tie - the one without a revision number in the expected format is considered older + { + backupPaths: []string{ + "prefix_r6666_2020-01-06-10:00:00", + "prefix_v5555_2020-01-06-10:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + // a path with a completely wrong format is considered older + { + backupPaths: []string{ + "prefix", + "prefix_v1_2020-01-01-10:00:00", + }, + lesserIndex: 0, + greaterIndex: 1, + }, + } + for i, tt := range tests { + t.Run(fmt.Sprintf("SortableBackupPaths.Less test case #%d", i), func(t *testing.T) { + defer func() { + r := recover() + if !tt.expectPanic && r != nil { + t.Errorf("#%d: SortableBackupPaths.Less panicked unexpectedly: %v", i, r) + } + if tt.expectPanic && r == nil { + t.Errorf("#%d: SortableBackupPaths.Less didn't panic as expected", i) + } + }() + if !SortableBackupPaths(tt.backupPaths).Less(tt.lesserIndex, tt.greaterIndex) { + t.Errorf("#%d: SortableBackupPaths.Less failed. Expected %s to be lesser than %s", + i, tt.backupPaths[tt.lesserIndex], tt.backupPaths[tt.greaterIndex]) + } + // try again with the order swapped - returns false + if SortableBackupPaths(tt.backupPaths).Less(tt.greaterIndex, tt.lesserIndex) { + t.Errorf("#%d: SortableBackupPaths.Less failed. Expected %s to be lesser than %s", + i, tt.backupPaths[tt.lesserIndex], tt.backupPaths[tt.greaterIndex]) + } + }) + } +} + +// Tests SortableBackupPaths.Swap(). +func TestSortableBackupPathsSwap(t *testing.T) { + tests := []struct { + backupPaths []string + i int + j int + // only one of these needs to be set + expectResult []string + expectPanic bool + }{ + // multiple entries + { + backupPaths: []string{"path1", "path2", "path3"}, + i: 0, + j: 2, + expectResult: []string{"path3", "path2", "path1"}, + }, + // multiple entries, noop + { + backupPaths: []string{"path1", "path2", "path3"}, + i: 2, + j: 2, + expectResult: []string{"path1", "path2", "path3"}, + }, + // multiple entries, index out of bounds - i + { + backupPaths: []string{"path1", "path2", "path3"}, + i: 3, + j: 1, + expectPanic: true, + }, + // multiple entries, index out of bounds - j + { + backupPaths: []string{"path1", "path2", "path3"}, + i: 0, + j: 3, + expectPanic: true, + }, + // empty list, index out of bounds + { + backupPaths: []string{}, + i: 0, + j: 0, + expectPanic: true, + }, + // single entry, index out of bounds - i + { + backupPaths: []string{"path"}, + i: 1, + j: 0, + expectPanic: true, + }, + // single entry, index out of bounds - j + { + backupPaths: []string{"path"}, + i: 0, + j: 1, + expectPanic: true, + }, + // single entry, noop + { + backupPaths: []string{"path"}, + i: 0, + j: 0, + expectResult: []string{"path"}, + }, + } + + for i, tt := range tests { + t.Run(fmt.Sprintf("SortableBackupPaths.Swap test case #%d", i), func(t *testing.T) { + defer func() { + r := recover() + if !tt.expectPanic && r != nil { + t.Errorf("#%d: SortableBackupPaths.Swap panicked unexpectedly: %v", i, r) + } + if tt.expectPanic && r == nil { + t.Errorf("#%d: SortableBackupPaths.Swap didn't panic as expected", i) + } + }() + SortableBackupPaths(tt.backupPaths).Swap(tt.i, tt.j) + if !reflect.DeepEqual(tt.backupPaths, tt.expectResult) { + t.Errorf("#%d: SortableBackupPaths.Swap failed. Expected %v, but got %v", + i, tt.expectResult, tt.backupPaths) + } + }) + } +}