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

Author: Matthew Krajewski

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

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 "<base path>_v<etcd store revision>_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]
+}