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)
+ }
+ })
+ }
+}