Repository: VictoriaMetrics/VictoriaMetrics
Pull request #2535 (2022-05-03 17:20:59)

{vmbackup, vmbackup/snapshot}: fixed problem with snapshot backup in another snapshot folder
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2503

codecov[bot]: # [Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) Report
> Merging [#2535](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) (085fb45) into [master](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/commit/1e26dd1f8265924376381d942282e872f003d6aa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) (1e26dd1) will **decrease** coverage by `0.00%`.
> The diff coverage is `50.00%`.

```diff
@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
- Coverage   59.49%   59.48%   -0.01%     
==========================================
  Files         252      252              
  Lines       42704    42710       +6     
==========================================
  Hits        25405    25405              
- Misses      15613    15618       +5     
- Partials     1686     1687       +1     
```


| [Impacted Files](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) | Coverage Δ | |
|---|---|---|
| [app/vmbackup/snapshot/snapshot.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-YXBwL3ZtYmFja3VwL3NuYXBzaG90L3NuYXBzaG90Lmdv) | `43.47% <50.00%> (+0.62%)` | :arrow_up: |
| [app/vmselect/promql/aggr\_incremental.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-YXBwL3Ztc2VsZWN0L3Byb21xbC9hZ2dyX2luY3JlbWVudGFsLmdv) | `82.83% <0.00%> (-4.82%)` | :arrow_down: |
| [lib/mergeset/table.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-bGliL21lcmdlc2V0L3RhYmxlLmdv) | `66.05% <0.00%> (+0.47%)` | :arrow_up: |
| [app/vmalert/remotewrite/remotewrite.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-YXBwL3ZtYWxlcnQvcmVtb3Rld3JpdGUvcmVtb3Rld3JpdGUuZ28=) | `64.82% <0.00%> (+5.51%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics). Last update [1e26dd1...085fb45](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/2535?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).


Patch:
diff --git a/app/vmbackup/main.go b/app/vmbackup/main.go
index c158a9eea..9c53f5fca 100644
--- a/app/vmbackup/main.go
+++ b/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.
diff --git a/app/vmbackup/snapshot/snapshot.go b/lib/snapshot/snapshot.go
similarity index 62%
rename from app/vmbackup/snapshot/snapshot.go
rename to lib/snapshot/snapshot.go
index 199d21114..ba19ea8e2 100644
--- a/app/vmbackup/snapshot/snapshot.go
+++ b/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
-// the snapshot name
+// Create creates a snapshot via the provided api endpoint and returns 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())
diff --git a/lib/storage/storage.go b/lib/storage/storage.go
index 1f48d0278..b7dc92af5 100644
--- a/lib/storage/storage.go
+++ b/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"
@@ -317,7 +318,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 {
@@ -372,8 +373,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"
@@ -389,7 +388,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)
@@ -400,8 +399,8 @@ func (s *Storage) ListSnapshots() ([]string, error) {
 
 // DeleteSnapshot deletes the given snapshot.
 func (s *Storage) DeleteSnapshot(snapshotName string) error {
-	if !snapshotNameRegexp.MatchString(snapshotName) {
-		return fmt.Errorf("invalid snapshotName %q", snapshotName)
+	if err := snapshot.Validate(snapshotName); err != nil {
+		return fmt.Errorf("invalid snapshotName %q: %w", snapshotName, err)
 	}
 	snapshotPath := s.path + "/snapshots/" + snapshotName
 
@@ -426,7 +425,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)
 		}
@@ -439,24 +438,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)
 }


Test patch:
diff --git a/app/vmbackup/snapshot/snapshot_test.go b/lib/snapshot/snapshot_test.go
similarity index 70%
rename from app/vmbackup/snapshot/snapshot_test.go
rename to lib/snapshot/snapshot_test.go
index 6d4266de6..42712e0f1 100644
--- a/app/vmbackup/snapshot/snapshot_test.go
+++ b/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)
+			}
+		})
+	}
+}