{vmbackup, vmbackup/snapshot}: fixed problem with snapshot backup in another snapshot folder (#2535)

* {vmbackup, vmbackup/snapshot}: validate snapshot name

* vmbackup/snapshot: added another checks

* backup/actions: added check that we ignore backup_complete.ignore file

* vmbackup: moved snapshot to lib directory

* lib/snapshot: added functions description

* lib/snapshot: fixed typo

* vmbackup: code cleanup

* wip

Co-authored-by: Aliaksandr Valialkin <valyala@victoriametrics.com>

app/vmbackup/main.go

@@ -7,7 +7,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmbackup/snapshot"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/backup/actions"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/backup/common"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/backup/fslocal"
@@ -17,6 +16,7 @@ import (
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/flagutil"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/httpserver"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/logger"
+	"github.com/VictoriaMetrics/VictoriaMetrics/lib/snapshot"
 )
 
 var (
@@ -71,6 +71,11 @@ func main() {
 				logger.Fatalf("cannot delete snapshot: %s", err)
 			}
 		}()
+	} else if len(*snapshotName) == 0 {
+		logger.Fatalf("`-snapshotName` or `-snapshot.createURL` must be provided")
+	}
+	if err := snapshot.Validate(*snapshotName); err != nil {
+		logger.Fatalf("invalid -snapshotName=%q: %s", *snapshotName, err)
 	}
 
 	go httpserver.Serve(*httpListenAddr, nil)
@@ -119,9 +124,6 @@ See the docs at https://docs.victoriametrics.com/vmbackup.html .
 }
 
 func newSrcFS() (*fslocal.FS, error) {
-	if len(*snapshotName) == 0 {
-		return nil, fmt.Errorf("`-snapshotName` or `-snapshot.createURL` must be provided")
-	}
 	snapshotPath := *storageDataPath + "/snapshots/" + *snapshotName
 
 	// Verify the snapshot exists.

lib/snapshot/snapshot.go

@@ -7,18 +7,23 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/url"
+	"regexp"
+	"strings"
+	"sync/atomic"
+	"time"
 
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/logger"
 )
 
+var snapshotNameRegexp = regexp.MustCompile(`^[0-9]{14}-[0-9A-Fa-f]+$`)
+
 type snapshot struct {
 	Status   string `json:"status"`
 	Snapshot string `json:"snapshot"`
 	Msg      string `json:"msg"`
 }
 
-// Create creates a snapshot and the provided api endpoint and returns
+// Create creates a snapshot via the provided api endpoint and returns the snapshot name
-// the snapshot name
 func Create(createSnapshotURL string) (string, error) {
 	logger.Infof("Creating snapshot")
 	u, err := url.Parse(createSnapshotURL)
@@ -53,7 +58,7 @@ func Create(createSnapshotURL string) (string, error) {
 	}
 }
 
-// Delete deletes a snapshot and the provided api endpoint returns any failure
+// Delete deletes a snapshot via the provided api endpoint
 func Delete(deleteSnapshotURL string, snapshotName string) error {
 	logger.Infof("Deleting snapshot %s", snapshotName)
 	formData := url.Values{
@@ -90,3 +95,37 @@ func Delete(deleteSnapshotURL string, snapshotName string) error {
 		return fmt.Errorf("Unkown status: %v", snap.Status)
 	}
 }
+
+// Validate validates the snapshotName
+func Validate(snapshotName string) error {
+	_, err := Time(snapshotName)
+	return err
+}
+
+// Time returns snapshot creation time from the given snapshotName
+func Time(snapshotName string) (time.Time, error) {
+	if !snapshotNameRegexp.MatchString(snapshotName) {
+		return time.Time{}, fmt.Errorf("unexpected snapshot name=%q; it must match %q regexp", snapshotName, snapshotNameRegexp.String())
+	}
+	n := strings.IndexByte(snapshotName, '-')
+	if n < 0 {
+		logger.Panicf("BUG: cannot find `-` in snapshotName=%q", snapshotName)
+	}
+	s := snapshotName[:n]
+	t, err := time.Parse("20060102150405", s)
+	if err != nil {
+		return time.Time{}, fmt.Errorf("unexpected timestamp=%q in snapshot name: %w; it must match YYYYMMDDhhmmss pattern", s, err)
+	}
+	return t, nil
+}
+
+// NewName returns new name for new snapshot
+func NewName() string {
+	return fmt.Sprintf("%s-%08X", time.Now().UTC().Format("20060102150405"), nextSnapshotIdx())
+}
+
+func nextSnapshotIdx() uint64 {
+	return atomic.AddUint64(&snapshotIdx, 1)
+}
+
+var snapshotIdx = uint64(time.Now().UnixNano())

lib/snapshot/snapshot_test.go

@@ -104,3 +104,54 @@ func TestDeleteSnapshotFailed(t *testing.T) {
 		t.Fatalf("Snapshot should have failed, got: %v", err)
 	}
 }
+
+func Test_Validate(t *testing.T) {
+	tests := []struct {
+		name         string
+		snapshotName string
+		want         bool
+	}{
+		{
+			name:         "empty snapshot name",
+			snapshotName: "",
+			want:         false,
+		},
+		{
+			name:         "short snapshot name",
+			snapshotName: "",
+			want:         false,
+		},
+		{
+			name:         "short first part of the snapshot name",
+			snapshotName: "2022050312163-16EB56ADB4110CF2",
+			want:         false,
+		},
+		{
+			name:         "short second part of the snapshot name",
+			snapshotName: "20220503121638-16EB56ADB4110CF",
+			want:         true,
+		},
+		{
+			name:         "correct snapshot name",
+			snapshotName: "20220503121638-16EB56ADB4110CF2",
+			want:         true,
+		},
+		{
+			name:         "invalid time part snapshot name",
+			snapshotName: "00000000000000-16EB56ADB4110CF2",
+			want:         false,
+		},
+		{
+			name:         "not enough parts of the snapshot name",
+			snapshotName: "2022050312163816EB56ADB4110CF2",
+			want:         false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := Validate(tt.snapshotName); (err == nil) != tt.want {
+				t.Errorf("checkSnapshotName() = %v, want %v", err, tt.want)
+			}
+		})
+	}
+}

lib/storage/storage.go

@@ -24,6 +24,7 @@ import (
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/fs"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/logger"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/memory"
+	"github.com/VictoriaMetrics/VictoriaMetrics/lib/snapshot"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/storagepacelimiter"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/timerpool"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/uint64set"
@@ -332,7 +333,7 @@ func (s *Storage) CreateSnapshot() (string, error) {
 	s.snapshotLock.Lock()
 	defer s.snapshotLock.Unlock()
 
-	snapshotName := fmt.Sprintf("%s-%08X", time.Now().UTC().Format("20060102150405"), nextSnapshotIdx())
+	snapshotName := snapshot.NewName()
 	srcDir := s.path
 	dstDir := fmt.Sprintf("%s/snapshots/%s", srcDir, snapshotName)
 	if err := fs.MkdirAllFailIfExist(dstDir); err != nil {
@@ -387,8 +388,6 @@ func (s *Storage) CreateSnapshot() (string, error) {
 	return snapshotName, nil
 }
 
-var snapshotNameRegexp = regexp.MustCompile("^[0-9]{14}-[0-9A-Fa-f]+$")
-
 // ListSnapshots returns sorted list of existing snapshots for s.
 func (s *Storage) ListSnapshots() ([]string, error) {
 	snapshotsPath := s.path + "/snapshots"
@@ -404,7 +403,7 @@ func (s *Storage) ListSnapshots() ([]string, error) {
 	}
 	snapshotNames := make([]string, 0, len(fnames))
 	for _, fname := range fnames {
-		if !snapshotNameRegexp.MatchString(fname) {
+		if err := snapshot.Validate(fname); err != nil {
 			continue
 		}
 		snapshotNames = append(snapshotNames, fname)
@@ -415,8 +414,8 @@ func (s *Storage) ListSnapshots() ([]string, error) {
 
 // DeleteSnapshot deletes the given snapshot.
 func (s *Storage) DeleteSnapshot(snapshotName string) error {
-	if !snapshotNameRegexp.MatchString(snapshotName) {
+	if err := snapshot.Validate(snapshotName); err != nil {
-		return fmt.Errorf("invalid snapshotName %q", snapshotName)
+		return fmt.Errorf("invalid snapshotName %q: %w", snapshotName, err)
 	}
 	snapshotPath := s.path + "/snapshots/" + snapshotName
 
@@ -441,7 +440,7 @@ func (s *Storage) DeleteStaleSnapshots(maxAge time.Duration) error {
 	}
 	expireDeadline := time.Now().UTC().Add(-maxAge)
 	for _, snapshotName := range list {
-		t, err := snapshotTime(snapshotName)
+		t, err := snapshot.Time(snapshotName)
 		if err != nil {
 			return fmt.Errorf("cannot parse snapshot date from %q: %w", snapshotName, err)
 		}
@@ -454,24 +453,6 @@ func (s *Storage) DeleteStaleSnapshots(maxAge time.Duration) error {
 	return nil
 }
 
-func snapshotTime(snapshotName string) (time.Time, error) {
-	if !snapshotNameRegexp.MatchString(snapshotName) {
-		return time.Time{}, fmt.Errorf("unexpected snapshotName must be in the format `YYYYMMDDhhmmss-idx`; got %q", snapshotName)
-	}
-	n := strings.IndexByte(snapshotName, '-')
-	if n < 0 {
-		return time.Time{}, fmt.Errorf("cannot find `-` in snapshotName=%q", snapshotName)
-	}
-	s := snapshotName[:n]
-	return time.Parse("20060102150405", s)
-}
-
-var snapshotIdx = uint64(time.Now().UnixNano())
-
-func nextSnapshotIdx() uint64 {
-	return atomic.AddUint64(&snapshotIdx, 1)
-}
-
 func (s *Storage) idb() *indexDB {
 	return s.idbCurr.Load().(*indexDB)
 }