From dab519de7606539c7520eccf0cf2d259a26844e7 Mon Sep 17 00:00:00 2001 From: Seshachalam Yerasala Venkata Date: Wed, 31 Jul 2024 20:04:56 +0530 Subject: [PATCH 01/15] Implement handling of immutable snapshots in etcd backup restore - Added `RetentionExpiry` field to Snapshot struct to store retention expiry time. - Added `IsDeletable` method to `Snapshot` struct to determine if a snapshot is deletable based on `RetentionExpiry`. - Introduced `ExcludeSnapshotMetadataKey` constant for marking snapshots to be ignored. - Modified garbage collection logic to skip immutable snapshots. - Updated GCS snapstore list function to exclude objects with `x-etcd-snapshot-exclude` metadata. --- pkg/snapshot/snapshotter/garbagecollector.go | 10 ++++++++- pkg/snapstore/gcs_snapstore.go | 19 +++++++++++----- pkg/types/snapstore.go | 23 ++++++++++++++++---- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/pkg/snapshot/snapshotter/garbagecollector.go b/pkg/snapshot/snapshotter/garbagecollector.go index 8b3aced34..f0e075a78 100644 --- a/pkg/snapshot/snapshotter/garbagecollector.go +++ b/pkg/snapshot/snapshotter/garbagecollector.go @@ -140,6 +140,10 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { } if deleteSnap { + if nextSnap.IsImmutable() { + ssr.logger.Infof("GC: Skipping : %s, since it is immutable", nextSnap.SnapName) + continue + } ssr.logger.Infof("GC: Deleting old full snapshot: %s %v", nextSnap.CreatedOn.UTC(), deleteSnap) if err := ssr.store.Delete(*nextSnap); err != nil { ssr.logger.Warnf("GC: Failed to delete snapshot %s: %v", path.Join(nextSnap.SnapDir, nextSnap.SnapName), err) @@ -246,9 +250,13 @@ func (ssr *Snapshotter) GarbageCollectDeltaSnapshots(snapStream brtypes.SnapList cutoffTime := time.Now().UTC().Add(-ssr.config.DeltaSnapshotRetentionPeriod.Duration) for i := len(snapStream) - 1; i >= 0; i-- { if (*snapStream[i]).Kind == brtypes.SnapshotKindDelta && snapStream[i].CreatedOn.Before(cutoffTime) { + snapPath := path.Join(snapStream[i].SnapDir, snapStream[i].SnapName) ssr.logger.Infof("GC: Deleting old delta snapshot: %s", snapPath) - + if snapStream[i].IsImmutable() { + ssr.logger.Infof("GC: Skipping : %s, since it is immutable", snapPath) + continue + } if err := ssr.store.Delete(*snapStream[i]); err != nil { ssr.logger.Warnf("GC: Failed to delete snapshot %s: %v", snapPath, err) metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: err.Error()}).Inc() diff --git a/pkg/snapstore/gcs_snapstore.go b/pkg/snapstore/gcs_snapstore.go index eaf26ffe2..955a95b32 100644 --- a/pkg/snapstore/gcs_snapstore.go +++ b/pkg/snapstore/gcs_snapstore.go @@ -283,14 +283,15 @@ func (s *GCSSnapStore) componentUploader(wg *sync.WaitGroup, stopCh <-chan struc } } -// List will return sorted list with all snapshot files on store. -func (s *GCSSnapStore) List() (brtypes.SnapList, error) { +// List will return a sorted list with all snapshot files on store, excluding those marked with x-etcd-snapshot-exclude. +func (s *GCSSnapStore) List(includeTagged bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(s.prefix, "/") - // Last element of the tokens is backup version - // Consider the parent of the backup version level (Required for Backward Compatibility) + // Consider the parent of the last element for backward compatibility. prefix := path.Join(strings.Join(prefixTokens[:len(prefixTokens)-1], "/")) - it := s.client.Bucket(s.bucket).Objects(context.TODO(), &storage.Query{Prefix: prefix}) + it := s.client.Bucket(s.bucket).Objects(context.TODO(), &storage.Query{ + Prefix: prefix, + }) var attrs []*storage.ObjectAttrs for { @@ -301,6 +302,12 @@ func (s *GCSSnapStore) List() (brtypes.SnapList, error) { if err != nil { return nil, err } + + // Check if the snapshot should be ignored. + if attr.Metadata[brtypes.ExcludeSnapshotMetadataKey] == "true" { + logrus.Infof("Ignoring snapshot due to exclude metadata: %s", attr.Name) + continue + } attrs = append(attrs, attr) } @@ -309,10 +316,10 @@ func (s *GCSSnapStore) List() (brtypes.SnapList, error) { if strings.Contains(v.Name, backupVersionV1) || strings.Contains(v.Name, backupVersionV2) { snap, err := ParseSnapshot(v.Name) if err != nil { - // Warning logrus.Warnf("Invalid snapshot %s found, ignoring it: %v", v.Name, err) continue } + snap.RetentionExpiry = v.RetentionExpirationTime snapList = append(snapList, snap) } } diff --git a/pkg/types/snapstore.go b/pkg/types/snapstore.go index 93de02397..a470c92fe 100644 --- a/pkg/types/snapstore.go +++ b/pkg/types/snapstore.go @@ -56,6 +56,8 @@ const ( // MinChunkSize is set to 5Mib since it is lower chunk size limit for AWS. MinChunkSize int64 = 5 * (1 << 20) //5 MiB + + ExcludeSnapshotMetadataKey = "x-etcd-snapshot-exclude" ) // SnapStore is the interface to be implemented for different @@ -73,18 +75,31 @@ type SnapStore interface { Delete(Snapshot) error } -// Snapshot structure represents the metadata of snapshot.s +// Snapshot structure represents the metadata of snapshot. type Snapshot struct { - Kind string `json:"kind"` //incr:incremental,full:full + Kind string `json:"kind"` // incr:incremental, full:full StartRevision int64 `json:"startRevision"` - LastRevision int64 `json:"lastRevision"` //latest revision on snapshot + LastRevision int64 `json:"lastRevision"` // latest revision on snapshot CreatedOn time.Time `json:"createdOn"` SnapDir string `json:"snapDir"` SnapName string `json:"snapName"` IsChunk bool `json:"isChunk"` Prefix string `json:"prefix"` // Points to correct prefix of a snapshot in snapstore (Required for Backward Compatibility) - CompressionSuffix string `json:"compressionSuffix"` // CompressionSuffix depends on compessionPolicy + CompressionSuffix string `json:"compressionSuffix"` // CompressionSuffix depends on compression policy IsFinal bool `json:"isFinal"` + RetentionExpiry time.Time `json:"retentionExpiry"` +} + +// IsDeletable determines if the snapshot can be deleted. +// It checks if the retention expiry time is set and whether the current time is after the retention expiry time. +func (s *Snapshot) IsDeletable() bool { + // Check if RetentionExpiry is the zero value of time.Time, which means it is not set. + if s.RetentionExpiry.IsZero() { + // If RetentionExpiry is not set, assume the snapshot can be deleted. + return true + } + // Otherwise, check if the current time is after the retention expiry time. + return time.Now().After(s.RetentionExpiry) } // GenerateSnapshotName prepares the snapshot name from metadata From 46c9d69c848a210492bc3999e3afcdfa89839c63 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Wed, 7 Aug 2024 11:36:56 +0530 Subject: [PATCH 02/15] Call the correct method for GC. --- pkg/snapshot/snapshotter/garbagecollector.go | 4 ++-- pkg/snapstore/abs_snapstore.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/snapshot/snapshotter/garbagecollector.go b/pkg/snapshot/snapshotter/garbagecollector.go index f0e075a78..621a8ed6b 100644 --- a/pkg/snapshot/snapshotter/garbagecollector.go +++ b/pkg/snapshot/snapshotter/garbagecollector.go @@ -140,7 +140,7 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { } if deleteSnap { - if nextSnap.IsImmutable() { + if !nextSnap.IsDeletable() { ssr.logger.Infof("GC: Skipping : %s, since it is immutable", nextSnap.SnapName) continue } @@ -253,7 +253,7 @@ func (ssr *Snapshotter) GarbageCollectDeltaSnapshots(snapStream brtypes.SnapList snapPath := path.Join(snapStream[i].SnapDir, snapStream[i].SnapName) ssr.logger.Infof("GC: Deleting old delta snapshot: %s", snapPath) - if snapStream[i].IsImmutable() { + if !snapStream[i].IsDeletable() { ssr.logger.Infof("GC: Skipping : %s, since it is immutable", snapPath) continue } diff --git a/pkg/snapstore/abs_snapstore.go b/pkg/snapstore/abs_snapstore.go index 424ff15b0..54af36291 100644 --- a/pkg/snapstore/abs_snapstore.go +++ b/pkg/snapstore/abs_snapstore.go @@ -308,6 +308,7 @@ func (a *ABSSnapStore) List() (brtypes.SnapList, error) { if err != nil { logrus.Warnf("Invalid snapshot found. Ignoring it:%s\n", *blobItem.Name) } else { + // a := blob.BlobTags.BlobTagSet[0].Key snapList = append(snapList, s) } } From 8eb63e1422beca2cb4d1d52f802679b9a616e636 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Thu, 29 Aug 2024 13:55:37 +0530 Subject: [PATCH 03/15] Check immutability for chunk deletion in `GarbageCollectChunks()`. --- pkg/snapshot/snapshotter/garbagecollector.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/snapshot/snapshotter/garbagecollector.go b/pkg/snapshot/snapshotter/garbagecollector.go index 621a8ed6b..bee3e1ce4 100644 --- a/pkg/snapshot/snapshotter/garbagecollector.go +++ b/pkg/snapshot/snapshotter/garbagecollector.go @@ -62,6 +62,7 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { } else { // chunksDeleted stores the no of chunks deleted in the current iteration of GC. var chunksDeleted int + // GarbageCollectChunks returns a filtered SnapList which does not contain chunks chunksDeleted, snapList = ssr.GarbageCollectChunks(snapList) ssr.logger.Infof("GC: Total number garbage collected chunks: %d", chunksDeleted) } @@ -203,8 +204,8 @@ func getSnapStreamIndexList(snapList brtypes.SnapList) []int { } // GarbageCollectChunks removes obsolete chunks based on the latest recorded snapshot. -// It eliminates chunks associated with snapshots that have already been uploaded. -// Additionally, it avoids deleting chunks linked to snapshots currently being uploaded to prevent the garbage collector from removing chunks before the composite is formed. +// It eliminates chunks associated with snapshots that have already been uploaded, and returns a SnapList which does not include chunks. +// Additionally, it avoids deleting chunks linked to snapshots currently being uploaded to prevent the garbage collector from removing chunks before the composite is formed. This chunk garbage collection is required only for GCS. func (ssr *Snapshotter) GarbageCollectChunks(snapList brtypes.SnapList) (int, brtypes.SnapList) { var nonChunkSnapList brtypes.SnapList chunksDeleted := 0 @@ -220,6 +221,10 @@ func (ssr *Snapshotter) GarbageCollectChunks(snapList brtypes.SnapList) (int, br } // delete the chunk object snapPath := path.Join(snap.SnapDir, snap.SnapName) + if !snap.IsDeletable() { + ssr.logger.Infof("GC: Skipping : %s, since it is immutable", snap.SnapName) + continue + } ssr.logger.Infof("GC: Deleting chunk for old snapshot: %s", snapPath) if err := ssr.store.Delete(*snap); err != nil { ssr.logger.Warnf("GC: Failed to delete chunk %s: %v", snapPath, err) From 8d7acb30e5a9ebf9ea5b36ad7640c5cd7b85ae43 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Wed, 4 Sep 2024 10:23:14 +0530 Subject: [PATCH 04/15] Enable `List` calls to return untagged and tagged snapshots based on a `bool` argument. --- pkg/compactor/compactor_test.go | 4 +- pkg/miscellaneous/miscellaneous.go | 4 +- pkg/miscellaneous/miscellaneous_test.go | 2 +- pkg/snapshot/copier/copier.go | 2 +- pkg/snapshot/snapshotter/garbagecollector.go | 3 +- pkg/snapshot/snapshotter/snapshotter_test.go | 42 ++++++++++---------- pkg/snapstore/abs_snapstore.go | 2 +- pkg/snapstore/failed_snapstore.go | 2 +- pkg/snapstore/gcs_snapstore.go | 4 +- pkg/snapstore/local_snapstore.go | 2 +- pkg/snapstore/oss_snapstore.go | 2 +- pkg/snapstore/s3_snapstore.go | 2 +- pkg/snapstore/snapstore_test.go | 10 ++--- pkg/snapstore/swift_snapstore.go | 4 +- pkg/types/snapstore.go | 5 ++- test/e2e/integration/cloud_backup_test.go | 10 ++--- test/e2e/integrationcluster/backup_test.go | 4 +- test/e2e/integrationcluster/utils.go | 4 +- 18 files changed, 55 insertions(+), 53 deletions(-) diff --git a/pkg/compactor/compactor_test.go b/pkg/compactor/compactor_test.go index d459b2899..c076f30cf 100644 --- a/pkg/compactor/compactor_test.go +++ b/pkg/compactor/compactor_test.go @@ -129,7 +129,7 @@ var _ = Describe("Running Compactor", func() { Expect(err).ShouldNot(HaveOccurred()) // Check if the compacted full snapshot is really present - snapList, err := store.List() + snapList, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) compactedSnapshot = snapList[len(snapList)-1] @@ -210,7 +210,7 @@ var _ = Describe("Running Compactor", func() { Expect(err).ShouldNot(HaveOccurred()) // Check if the compacted full snapshot is really present - snapList, err := store.List() + snapList, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) compactedSnapshot = snapList[len(snapList)-1] diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 40ecc77e3..a24863c23 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -62,7 +62,7 @@ func GetLatestFullSnapshotAndDeltaSnapList(store brtypes.SnapStore) (*brtypes.Sn fullSnapshot *brtypes.Snapshot deltaSnapList brtypes.SnapList ) - snapList, err := store.List() + snapList, err := store.List(false) if err != nil { return nil, nil, err } @@ -97,7 +97,7 @@ type backup struct { // GetFilteredBackups returns sorted by date (new -> old) SnapList. It will also filter the snapshots that should be included or not using the filter function. // If the filter is nil it will return all snapshots. Also, maxBackups can be used to target only the last N snapshots (-1 = all). func GetFilteredBackups(store brtypes.SnapStore, maxBackups int, filter func(snaps brtypes.Snapshot) bool) (brtypes.SnapList, error) { - snapList, err := store.List() + snapList, err := store.List(false) if err != nil { return nil, err } diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index 1d5554747..1fd2dd281 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -817,7 +817,7 @@ func NewDummyStore(snapList brtypes.SnapList) DummyStore { return DummyStore{SnapList: snapList} } -func (ds *DummyStore) List() (brtypes.SnapList, error) { +func (ds *DummyStore) List(_ bool) (brtypes.SnapList, error) { return ds.SnapList, nil } diff --git a/pkg/snapshot/copier/copier.go b/pkg/snapshot/copier/copier.go index f27f0d793..c929529b3 100644 --- a/pkg/snapshot/copier/copier.go +++ b/pkg/snapshot/copier/copier.go @@ -123,7 +123,7 @@ func (c *Copier) copyBackups() error { // Get destination snapshots and build a map keyed by name c.logger.Info("Getting destination snapshots...") - destSnapshots, err := c.destSnapStore.List() + destSnapshots, err := c.destSnapStore.List(false) if err != nil { return fmt.Errorf("could not get destination snapshots: %v", err) } diff --git a/pkg/snapshot/snapshotter/garbagecollector.go b/pkg/snapshot/snapshotter/garbagecollector.go index bee3e1ce4..d38f2bbd6 100644 --- a/pkg/snapshot/snapshotter/garbagecollector.go +++ b/pkg/snapshot/snapshotter/garbagecollector.go @@ -41,7 +41,8 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { total := 0 ssr.logger.Info("GC: Executing garbage collection...") - snapList, err := ssr.store.List() + // List the tagged snapshots to garbage collect them + snapList, err := ssr.store.List(true) if err != nil { metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: err.Error()}).Inc() ssr.logger.Warnf("GC: Failed to list snapshots: %v", err) diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index a5d2a1e32..7788087c9 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -134,7 +134,7 @@ var _ = Describe("Snapshotter", func() { defer cancel() err = ssr.Run(ctx.Done(), true) Expect(err).Should(HaveOccurred()) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(BeZero()) }) @@ -172,7 +172,7 @@ var _ = Describe("Snapshotter", func() { }) It("should not take any snapshot", func() { - list, err := store.List() + list, err := store.List(false) count := 0 for _, snap := range list { if snap.Kind == brtypes.SnapshotKindFull { @@ -225,7 +225,7 @@ var _ = Describe("Snapshotter", func() { defer cancel() err = ssr.Run(ctx.Done(), true) Expect(err).ShouldNot(HaveOccurred()) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).ShouldNot(BeZero()) for _, snapshot := range list { @@ -286,7 +286,7 @@ var _ = Describe("Snapshotter", func() { ssrCtx := utils.ContextWithWaitGroup(testCtx, wg) err = ssr.Run(ssrCtx.Done(), false) Expect(err).ShouldNot(HaveOccurred()) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).ShouldNot(BeZero()) Expect(list[0].Kind).Should(Equal(brtypes.SnapshotKindDelta)) @@ -320,7 +320,7 @@ var _ = Describe("Snapshotter", func() { err = ssr.Run(ssrCtx.Done(), true) Expect(err).ShouldNot(HaveOccurred()) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).ShouldNot(BeZero()) Expect(list[0].Kind).Should(Equal(brtypes.SnapshotKindFull)) @@ -373,7 +373,7 @@ var _ = Describe("Snapshotter", func() { defer cancel() ssr.RunGarbageCollector(gcCtx.Done()) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(Equal(len(expectedSnapList))) @@ -403,7 +403,7 @@ var _ = Describe("Snapshotter", func() { defer cancel() ssr.RunGarbageCollector(gcCtx.Done()) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) incr := false @@ -449,7 +449,7 @@ var _ = Describe("Snapshotter", func() { Context("with all delta snapshots older than retention period", func() { It("should delete all delta snapshots", func() { store := prepareStoreWithDeltaSnapshots(testDir, deltaSnapshotCount) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(Equal(deltaSnapshotCount)) @@ -460,7 +460,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(deleted).To(Equal(deltaSnapshotCount)) - list, err = store.List() + list, err = store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(BeZero()) }) @@ -469,7 +469,7 @@ var _ = Describe("Snapshotter", func() { Context("with no delta snapshots", func() { It("should not delete any snapshots", func() { store := prepareStoreWithDeltaSnapshots(testDir, 0) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(BeZero()) @@ -481,7 +481,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(deleted).Should(BeZero()) - list, err = store.List() + list, err = store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(BeZero()) }) @@ -490,7 +490,7 @@ var _ = Describe("Snapshotter", func() { Context("with all delta snapshots younger than retention period", func() { It("should not delete any snapshots", func() { store := prepareStoreWithDeltaSnapshots(testDir, deltaSnapshotCount) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(Equal(6)) @@ -502,7 +502,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(deleted).Should(BeZero()) - list, err = store.List() + list, err = store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(Equal(deltaSnapshotCount)) }) @@ -511,7 +511,7 @@ var _ = Describe("Snapshotter", func() { Context("with a mix of delta snapshots, some older and some younger than retention period", func() { It("should delete only the delta snapshots older than the retention period", func() { store := prepareStoreWithDeltaSnapshots(testDir, deltaSnapshotCount) - list, err := store.List() + list, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(Equal(6)) @@ -523,7 +523,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(deleted).To(Equal(3)) - list, err = store.List() + list, err = store.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(len(list)).Should(Equal(3)) }) @@ -565,7 +565,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(chunkCount).To(BeZero()) - list, err := store.List() + list, err := store.List(false) Expect(err).NotTo(HaveOccurred()) Expect(len(list)).To(BeZero()) @@ -585,7 +585,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(chunkCount).To(Equal(4)) - list, err := store.List() + list, err := store.List(false) Expect(err).NotTo(HaveOccurred()) Expect(len(list)).To(Equal(4)) @@ -621,7 +621,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(chunkCount).To(Equal(9)) - list, err := store.List() + list, err := store.List(false) Expect(err).NotTo(HaveOccurred()) Expect(len(list)).To(Equal(10)) @@ -658,7 +658,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(chunkCount).To(Equal(9)) - list, err := store.List() + list, err := store.List(false) Expect(err).NotTo(HaveOccurred()) Expect(len(list)).To(Equal(10)) @@ -695,7 +695,7 @@ var _ = Describe("Snapshotter", func() { Expect(err).NotTo(HaveOccurred()) Expect(chunkCount).To(BeZero()) - list, err := store.List() + list, err := store.List(false) Expect(err).NotTo(HaveOccurred()) Expect(len(list)).To(Equal(3)) @@ -1266,7 +1266,7 @@ func addObjectsToStore(store brtypes.SnapStore, objectType string, kind string, // getObjectCount returns counts of chunk and composite objects in the store func getObjectCount(store brtypes.SnapStore) (int, int, error) { - list, err := store.List() + list, err := store.List(false) if err != nil { return 0, 0, err } diff --git a/pkg/snapstore/abs_snapstore.go b/pkg/snapstore/abs_snapstore.go index 54af36291..94d96e563 100644 --- a/pkg/snapstore/abs_snapstore.go +++ b/pkg/snapstore/abs_snapstore.go @@ -284,7 +284,7 @@ func (a *ABSSnapStore) Fetch(snap brtypes.Snapshot) (io.ReadCloser, error) { } // List will return sorted list with all snapshot files on store. -func (a *ABSSnapStore) List() (brtypes.SnapList, error) { +func (a *ABSSnapStore) List(_ bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(a.prefix, "/") // Last element of the tokens is backup version // Consider the parent of the backup version level (Required for Backward Compatibility) diff --git a/pkg/snapstore/failed_snapstore.go b/pkg/snapstore/failed_snapstore.go index 02e41288e..7af586c4d 100644 --- a/pkg/snapstore/failed_snapstore.go +++ b/pkg/snapstore/failed_snapstore.go @@ -32,7 +32,7 @@ func (f *FailedSnapStore) Save(snap brtypes.Snapshot, rc io.ReadCloser) error { } // List will list the snapshots from store -func (f *FailedSnapStore) List() (brtypes.SnapList, error) { +func (f *FailedSnapStore) List(_ bool) (brtypes.SnapList, error) { var snapList brtypes.SnapList return snapList, fmt.Errorf("failed to list the snapshots") } diff --git a/pkg/snapstore/gcs_snapstore.go b/pkg/snapstore/gcs_snapstore.go index 955a95b32..1d67f8e93 100644 --- a/pkg/snapstore/gcs_snapstore.go +++ b/pkg/snapstore/gcs_snapstore.go @@ -303,8 +303,8 @@ func (s *GCSSnapStore) List(includeTagged bool) (brtypes.SnapList, error) { return nil, err } - // Check if the snapshot should be ignored. - if attr.Metadata[brtypes.ExcludeSnapshotMetadataKey] == "true" { + // Check if the snapshot should be ignored + if !includeTagged && attr.Metadata[brtypes.ExcludeSnapshotMetadataKey] == "true" { logrus.Infof("Ignoring snapshot due to exclude metadata: %s", attr.Name) continue } diff --git a/pkg/snapstore/local_snapstore.go b/pkg/snapstore/local_snapstore.go index cda84b93d..9150d7829 100644 --- a/pkg/snapstore/local_snapstore.go +++ b/pkg/snapstore/local_snapstore.go @@ -68,7 +68,7 @@ func (s *LocalSnapStore) Save(snap brtypes.Snapshot, rc io.ReadCloser) error { } // List will return sorted list with all snapshot files on store. -func (s *LocalSnapStore) List() (brtypes.SnapList, error) { +func (s *LocalSnapStore) List(_ bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(s.prefix, "/") // Last element of the tokens is backup version // Consider the parent of the backup version level (Required for Backward Compatibility) diff --git a/pkg/snapstore/oss_snapstore.go b/pkg/snapstore/oss_snapstore.go index e8a9bf523..fb2fcb0ab 100644 --- a/pkg/snapstore/oss_snapstore.go +++ b/pkg/snapstore/oss_snapstore.go @@ -216,7 +216,7 @@ func (s *OSSSnapStore) uploadPart(imur oss.InitiateMultipartUploadResult, file * } // List will return sorted list with all snapshot files on store. -func (s *OSSSnapStore) List() (brtypes.SnapList, error) { +func (s *OSSSnapStore) List(_ bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(s.prefix, "/") // Last element of the tokens is backup version // Consider the parent of the backup version level (Required for Backward Compatibility) diff --git a/pkg/snapstore/s3_snapstore.go b/pkg/snapstore/s3_snapstore.go index 6e2c2a7eb..fa2737e94 100644 --- a/pkg/snapstore/s3_snapstore.go +++ b/pkg/snapstore/s3_snapstore.go @@ -511,7 +511,7 @@ func (s *S3SnapStore) partUploader(wg *sync.WaitGroup, stopCh <-chan struct{}, s } // List will return sorted list with all snapshot files on store. -func (s *S3SnapStore) List() (brtypes.SnapList, error) { +func (s *S3SnapStore) List(_ bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(s.prefix, "/") // Last element of the tokens is backup version // Consider the parent of the backup version level (Required for Backward Compatibility) diff --git a/pkg/snapstore/snapstore_test.go b/pkg/snapstore/snapstore_test.go index 87d5009f9..cc8d94c5f 100644 --- a/pkg/snapstore/snapstore_test.go +++ b/pkg/snapstore/snapstore_test.go @@ -176,7 +176,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { logrus.Infof("Running mock tests for %s when only v1 is present", provider) // List snap1 and snap2 - snapList, err := snapStore.List() + snapList, err := snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).To(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) Expect(snapList[0].SnapName).To(Equal(snap2.SnapName)) @@ -196,7 +196,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { prevLen := len(objectMap) err = snapStore.Delete(*snapList[secondSnapshotIndex]) Expect(err).ShouldNot(HaveOccurred()) - snapList, err = snapStore.List() + snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).To(Equal(prevLen - 1*snapStore.objectCountPerSnapshot)) @@ -228,7 +228,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { logrus.Infof("Running mock tests for %s when both v1 and v2 are present", provider) // List snap1, snap4, snap5 - snapList, err := snapStore.List() + snapList, err := snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).To(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) Expect(snapList[0].SnapName).To(Equal(snap1.SnapName)) @@ -297,7 +297,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { logrus.Infof("Running mock tests for %s when only v2 is present", provider) // List snap4 and snap5 - snapList, err := snapStore.List() + snapList, err := snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).To(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) Expect(snapList[0].SnapName).To(Equal(snap4.SnapName)) @@ -317,7 +317,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { prevLen := len(objectMap) err = snapStore.Delete(*snapList[0]) Expect(err).ShouldNot(HaveOccurred()) - snapList, err = snapStore.List() + snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).To(Equal(prevLen - snapStore.objectCountPerSnapshot)) diff --git a/pkg/snapstore/swift_snapstore.go b/pkg/snapstore/swift_snapstore.go index faa004001..da7f291f4 100644 --- a/pkg/snapstore/swift_snapstore.go +++ b/pkg/snapstore/swift_snapstore.go @@ -451,7 +451,7 @@ func (s *SwiftSnapStore) chunkUploader(wg *sync.WaitGroup, stopCh <-chan struct{ } // List will return sorted list with all snapshot files on store. -func (s *SwiftSnapStore) List() (brtypes.SnapList, error) { +func (s *SwiftSnapStore) List(_ bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(s.prefix, "/") // Last element of the tokens is backup version // Consider the parent of the backup version level (Required for Backward Compatibility) @@ -494,7 +494,7 @@ func (s *SwiftSnapStore) List() (brtypes.SnapList, error) { } func (s *SwiftSnapStore) getSnapshotChunks(snapshot brtypes.Snapshot) (brtypes.SnapList, error) { - snaps, err := s.List() + snaps, err := s.List(false) if err != nil { return nil, err } diff --git a/pkg/types/snapstore.go b/pkg/types/snapstore.go index a470c92fe..934bd0e82 100644 --- a/pkg/types/snapstore.go +++ b/pkg/types/snapstore.go @@ -57,6 +57,7 @@ const ( // MinChunkSize is set to 5Mib since it is lower chunk size limit for AWS. MinChunkSize int64 = 5 * (1 << 20) //5 MiB + // ExcludeSnapshotMetadataKey is the tag that is to be added on snapshots in the object store if they are not to be included in restoration ExcludeSnapshotMetadataKey = "x-etcd-snapshot-exclude" ) @@ -67,8 +68,8 @@ const ( type SnapStore interface { // Fetch should open reader for the snapshot file from store. Fetch(Snapshot) (io.ReadCloser, error) - // List will return sorted list with all snapshot files on store. - List() (SnapList, error) + // List will return sorted list with all snapshot files on store. Snapshots tagged with ExcludeSnapshotMetadataKey can be included in the List call by passing true. + List(bool) (SnapList, error) // Save will write the snapshot to store. Save(Snapshot, io.ReadCloser) error // Delete should delete the snapshot file from store. diff --git a/test/e2e/integration/cloud_backup_test.go b/test/e2e/integration/cloud_backup_test.go index a81a04c4a..550f9c0be 100644 --- a/test/e2e/integration/cloud_backup_test.go +++ b/test/e2e/integration/cloud_backup_test.go @@ -155,7 +155,7 @@ auto-compaction-retention: 30m` store, err = snapstore.GetSnapstore(snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) - snapList, err := store.List() + snapList, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) for _, snap := range snapList { Expect(store.Delete(*snap)).To(Succeed()) @@ -176,13 +176,13 @@ auto-compaction-retention: 30m` Context("taken at 1 minute interval", func() { It("should take periodic backups.", func() { - snaplist, err := store.List() + snaplist, err := store.List(false) Expect(snaplist).Should(BeEmpty()) Expect(err).ShouldNot(HaveOccurred()) time.Sleep(70 * time.Second) - snaplist, err = store.List() + snaplist, err = store.List(false) Expect(snaplist).ShouldNot(BeEmpty()) Expect(err).ShouldNot(HaveOccurred()) }) @@ -190,13 +190,13 @@ auto-compaction-retention: 30m` Context("taken at 1 minute interval", func() { It("should take periodic backups and limit based garbage collect backups over maxBackups configured", func() { - snaplist, err := store.List() + snaplist, err := store.List(false) Expect(snaplist).Should(BeEmpty()) Expect(err).ShouldNot(HaveOccurred()) time.Sleep(190 * time.Second) - snaplist, err = store.List() + snaplist, err = store.List(false) Expect(err).ShouldNot(HaveOccurred()) count := 0 for _, snap := range snaplist { diff --git a/test/e2e/integrationcluster/backup_test.go b/test/e2e/integrationcluster/backup_test.go index 0209be7a1..181ff6e0e 100644 --- a/test/e2e/integrationcluster/backup_test.go +++ b/test/e2e/integrationcluster/backup_test.go @@ -45,7 +45,7 @@ var _ = Describe("Backup", func() { Describe("Snapshotter", func() { It("should take full and delta snapshot", func() { - snapList, err := store.List() + snapList, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) numFulls, numDeltas := getTotalFullAndDeltaSnapshotCounts(snapList) Expect(numFulls).Should(Equal(1)) @@ -89,7 +89,7 @@ var _ = Describe("Backup", func() { cumulativeNumFulls, cumulativeNumDeltas := getTotalFullAndDeltaSnapshotCounts(cumulativeSnapList) - snapList, err := store.List() + snapList, err := store.List(false) Expect(err).ShouldNot(HaveOccurred()) numFulls, numDeltas := getTotalFullAndDeltaSnapshotCounts(snapList) diff --git a/test/e2e/integrationcluster/utils.go b/test/e2e/integrationcluster/utils.go index 730b56a7c..a34ed187e 100644 --- a/test/e2e/integrationcluster/utils.go +++ b/test/e2e/integrationcluster/utils.go @@ -311,7 +311,7 @@ func getTotalFullAndDeltaSnapshotCounts(snapList brtypes.SnapList) (int, int) { } func purgeSnapstore(store brtypes.SnapStore) error { - snapList, err := store.List() + snapList, err := store.List(false) if err != nil { return err } @@ -373,7 +373,7 @@ func recordCumulativeSnapList(logger *logrus.Logger, stopCh <-chan struct{}, res resultCh <- snapListResult return default: - snaps, err := store.List() + snaps, err := store.List(false) if err != nil { snapListResult.Error = err resultCh <- snapListResult From 7c097f3725617622a6835c87c2acd1844ebde750 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Tue, 10 Sep 2024 12:34:42 +0530 Subject: [PATCH 05/15] Added unit tests for `IsDeletable()`. --- pkg/snapstore/snapshot_test.go | 46 +++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/pkg/snapstore/snapshot_test.go b/pkg/snapstore/snapshot_test.go index 8e588d703..0f2477848 100644 --- a/pkg/snapstore/snapshot_test.go +++ b/pkg/snapstore/snapshot_test.go @@ -18,7 +18,6 @@ import ( ) var _ = Describe("Snapshot", func() { - Describe("Snapshot service", func() { Context("when provied with list of snapshot", func() { It("sorts snapshot by last revision number", func() { @@ -306,4 +305,49 @@ var _ = Describe("Snapshot", func() { }) }) }) + + Context("when provided with retention time periods", func() { + var ( + snap1 brtypes.Snapshot + snap2 brtypes.Snapshot + ) + BeforeEach(func() { + now := time.Now().UTC() + snapdir := fmt.Sprintf("Backup-%d", now.Second()) + snap1 = brtypes.Snapshot{ + CreatedOn: now, + StartRevision: 0, + LastRevision: 2088, + Kind: brtypes.SnapshotKindFull, + SnapDir: snapdir, + } + snap2 = brtypes.Snapshot{ + CreatedOn: now, + StartRevision: 1201, + LastRevision: 1500, + Kind: brtypes.SnapshotKindDelta, + SnapDir: snapdir, + } + + }) + It("should be deletable when its retention period is not set", func() { + // do not set the retention period + Expect(snap1.IsDeletable()).Should(BeTrue()) + Expect(snap2.IsDeletable()).Should(BeTrue()) + }) + It("should be deletable when its retention period has expired", func() { + // Setting expiry time to be a short time after creation + snap1.RetentionExpiry = snap1.CreatedOn.Add(time.Microsecond) + snap2.RetentionExpiry = snap2.CreatedOn.Add(time.Microsecond) + Expect(snap1.IsDeletable()).To(BeTrue()) + Expect(snap2.IsDeletable()).To(BeTrue()) + }) + It("should not be deletable when its retention period has not expired", func() { + // Setting the expiry time to be a long time after the creation + snap1.RetentionExpiry = snap1.CreatedOn.Add(time.Hour) + snap2.RetentionExpiry = snap2.CreatedOn.Add(time.Hour) + Expect(snap1.IsDeletable()).To(BeFalse()) + Expect(snap2.IsDeletable()).To(BeFalse()) + }) + }) }) From d7be1973ee6097eef8e019e41cf351c6e3743336 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Tue, 10 Sep 2024 15:45:02 +0530 Subject: [PATCH 06/15] Adapt unit tests for `List()` to handle listing tagged and untagged snapshots. --- pkg/snapstore/gcs_snapstore_test.go | 15 ++++++++---- pkg/snapstore/snapstore_test.go | 37 +++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/pkg/snapstore/gcs_snapstore_test.go b/pkg/snapstore/gcs_snapstore_test.go index a2203a210..899698be6 100644 --- a/pkg/snapstore/gcs_snapstore_test.go +++ b/pkg/snapstore/gcs_snapstore_test.go @@ -20,9 +20,10 @@ import ( // mockGCSClient is a mock client to be used in unit tests. type mockGCSClient struct { stiface.Client - objects map[string]*[]byte - prefix string - objectMutex sync.Mutex + objects map[string]*[]byte + prefix string + objectMetadata map[string]map[string]string + objectMutex sync.Mutex } func (m *mockGCSClient) Bucket(name string) stiface.BucketHandle { @@ -47,7 +48,7 @@ func (m *mockBucketHandle) Objects(context.Context, *storage.Query) stiface.Obje keys = append(keys, key) } sort.Strings(keys) - return &mockObjectIterator{keys: keys} + return &mockObjectIterator{keys: keys, metadata: m.client.objectMetadata} } type mockObjectHandle struct { @@ -82,6 +83,7 @@ func (m *mockObjectHandle) Delete(context.Context) error { defer m.client.objectMutex.Unlock() if _, ok := m.client.objects[m.object]; ok { delete(m.client.objects, m.object) + delete(m.client.objectMetadata, m.object) return nil } return fmt.Errorf("object %s not found", m.object) @@ -91,12 +93,15 @@ type mockObjectIterator struct { stiface.ObjectIterator currentIndex int keys []string + metadata map[string]map[string]string } func (m *mockObjectIterator) Next() (*storage.ObjectAttrs, error) { if m.currentIndex < len(m.keys) { + name := m.keys[m.currentIndex] obj := &storage.ObjectAttrs{ - Name: m.keys[m.currentIndex], + Name: name, + Metadata: m.metadata[name], } m.currentIndex++ return obj, nil diff --git a/pkg/snapstore/snapstore_test.go b/pkg/snapstore/snapstore_test.go index cc8d94c5f..db197b989 100644 --- a/pkg/snapstore/snapstore_test.go +++ b/pkg/snapstore/snapstore_test.go @@ -49,6 +49,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { snap4 brtypes.Snapshot snap5 brtypes.Snapshot snapstores map[string]testSnapStore + gcsClient *mockGCSClient ) BeforeEach(func() { @@ -101,6 +102,12 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { snap4.GenerateSnapshotName() snap5.GenerateSnapshotName() + gcsClient = &mockGCSClient{ + objects: objectMap, + prefix: prefixV2, + objectMetadata: make(map[string]map[string]string), + } + snapstores = map[string]testSnapStore{ "s3": { SnapStore: NewS3FromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, &mockS3Client{ @@ -123,10 +130,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { objectCountPerSnapshot: 1, }, "GCS": { - SnapStore: NewGCSSnapStoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, "", &mockGCSClient{ - objects: objectMap, - prefix: prefixV2, - }), + SnapStore: NewGCSSnapStoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, "", gcsClient), objectCountPerSnapshot: 1, }, "OSS": { @@ -303,6 +307,31 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { Expect(snapList[0].SnapName).To(Equal(snap4.SnapName)) Expect(snapList[secondSnapshotIndex].SnapName).To(Equal(snap5.SnapName)) + // List tests with false and true as arguments only implemented with GCS for now + if provider == "GCS" { + // the tagged snapshot should not be returned by the List() call + taggedSnapshot := snapList[0] + taggedSnapshotName := path.Join(taggedSnapshot.Prefix, taggedSnapshot.SnapDir, taggedSnapshot.SnapName) + gcsClient.objectMetadata[taggedSnapshotName] = map[string]string{"x-etcd-snapshot-exclude": "true"} + snapList, err = snapStore.List(false) + Expect(err).ShouldNot(HaveOccurred()) + Expect(snapList.Len()).Should(Equal((numberSnapshotsInObjectMap - 1) * snapStore.objectCountPerSnapshot)) + Expect(snapList[0].SnapName).ShouldNot(Equal(taggedSnapshot.SnapName)) + + // listing both tagged and untagged snapshots + snapList, err = snapStore.List(true) + Expect(err).ShouldNot(HaveOccurred()) + Expect(snapList.Len()).Should(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) + Expect(snapList[0].SnapName).Should(Equal(taggedSnapshot.SnapName)) + + // removing the tag will make the snapshot appear in the List call with false + delete(gcsClient.objectMetadata, taggedSnapshotName) + snapList, err = snapStore.List(false) + Expect(err).ShouldNot(HaveOccurred()) + Expect(snapList.Len()).Should(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) + Expect(snapList[0].SnapName).Should(Equal(taggedSnapshot.SnapName)) + } + // Fetch snap5 rc, err := snapStore.Fetch(*snapList[secondSnapshotIndex]) Expect(err).ShouldNot(HaveOccurred()) From 3545d03a0ad01b04038b72ba86a61241694b2f62 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Tue, 10 Sep 2024 15:56:20 +0530 Subject: [PATCH 07/15] Improve comments as suggested by @seshachalam-yv --- pkg/snapstore/gcs_snapstore.go | 6 +++--- pkg/types/snapstore.go | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/snapstore/gcs_snapstore.go b/pkg/snapstore/gcs_snapstore.go index 1d67f8e93..86e7d9bf8 100644 --- a/pkg/snapstore/gcs_snapstore.go +++ b/pkg/snapstore/gcs_snapstore.go @@ -283,8 +283,8 @@ func (s *GCSSnapStore) componentUploader(wg *sync.WaitGroup, stopCh <-chan struc } } -// List will return a sorted list with all snapshot files on store, excluding those marked with x-etcd-snapshot-exclude. -func (s *GCSSnapStore) List(includeTagged bool) (brtypes.SnapList, error) { +// List will return a sorted list with all snapshot files on store, excluding those marked with x-ignore-etcd-snapshot-exclude. Tagged snapshots can be included in the List call by passing true as the argument. +func (s *GCSSnapStore) List(includeAll bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(s.prefix, "/") // Consider the parent of the last element for backward compatibility. prefix := path.Join(strings.Join(prefixTokens[:len(prefixTokens)-1], "/")) @@ -304,7 +304,7 @@ func (s *GCSSnapStore) List(includeTagged bool) (brtypes.SnapList, error) { } // Check if the snapshot should be ignored - if !includeTagged && attr.Metadata[brtypes.ExcludeSnapshotMetadataKey] == "true" { + if !includeAll && attr.Metadata[brtypes.ExcludeSnapshotMetadataKey] == "true" { logrus.Infof("Ignoring snapshot due to exclude metadata: %s", attr.Name) continue } diff --git a/pkg/types/snapstore.go b/pkg/types/snapstore.go index 934bd0e82..9a568c285 100644 --- a/pkg/types/snapstore.go +++ b/pkg/types/snapstore.go @@ -68,8 +68,10 @@ const ( type SnapStore interface { // Fetch should open reader for the snapshot file from store. Fetch(Snapshot) (io.ReadCloser, error) - // List will return sorted list with all snapshot files on store. Snapshots tagged with ExcludeSnapshotMetadataKey can be included in the List call by passing true. - List(bool) (SnapList, error) + // List returns a sorted list of all snapshots in the store. + // includeAll specifies whether to include all snapshots while listing, including those with exclude tags. + // Snapshots with exclude tags are not listed unless includeAll is set to true. + List(includeAll bool) (SnapList, error) // Save will write the snapshot to store. Save(Snapshot, io.ReadCloser) error // Delete should delete the snapshot file from store. From f40496b2ffa26e7e5fa0f5c762c03df70e7d2a11 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Wed, 11 Sep 2024 14:42:37 +0530 Subject: [PATCH 08/15] Defined an interface which mock snapstores should implement to emulate tagged snapshots' behavior. --- pkg/snapstore/gcs_snapstore_test.go | 24 ++++++++++++++++-------- pkg/snapstore/snapstore_test.go | 23 ++++++++++++++++++----- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/pkg/snapstore/gcs_snapstore_test.go b/pkg/snapstore/gcs_snapstore_test.go index 899698be6..e4b66ee8a 100644 --- a/pkg/snapstore/gcs_snapstore_test.go +++ b/pkg/snapstore/gcs_snapstore_test.go @@ -20,16 +20,24 @@ import ( // mockGCSClient is a mock client to be used in unit tests. type mockGCSClient struct { stiface.Client - objects map[string]*[]byte - prefix string - objectMetadata map[string]map[string]string - objectMutex sync.Mutex + objects map[string]*[]byte + prefix string + objectTags map[string]map[string]string + objectMutex sync.Mutex } func (m *mockGCSClient) Bucket(name string) stiface.BucketHandle { return &mockBucketHandle{bucket: name, client: m} } +func (m *mockGCSClient) setTag(taggedSnapshotName string, tagMap map[string]string) { + m.objectTags[taggedSnapshotName] = map[string]string{"x-etcd-snapshot-exclude": "true"} +} + +func (m *mockGCSClient) deleteTag(taggedSnapshotName string) { + delete(m.objectTags, taggedSnapshotName) +} + type mockBucketHandle struct { stiface.BucketHandle bucket string @@ -48,7 +56,7 @@ func (m *mockBucketHandle) Objects(context.Context, *storage.Query) stiface.Obje keys = append(keys, key) } sort.Strings(keys) - return &mockObjectIterator{keys: keys, metadata: m.client.objectMetadata} + return &mockObjectIterator{keys: keys, tags: m.client.objectTags} } type mockObjectHandle struct { @@ -83,7 +91,7 @@ func (m *mockObjectHandle) Delete(context.Context) error { defer m.client.objectMutex.Unlock() if _, ok := m.client.objects[m.object]; ok { delete(m.client.objects, m.object) - delete(m.client.objectMetadata, m.object) + delete(m.client.objectTags, m.object) return nil } return fmt.Errorf("object %s not found", m.object) @@ -93,7 +101,7 @@ type mockObjectIterator struct { stiface.ObjectIterator currentIndex int keys []string - metadata map[string]map[string]string + tags map[string]map[string]string } func (m *mockObjectIterator) Next() (*storage.ObjectAttrs, error) { @@ -101,7 +109,7 @@ func (m *mockObjectIterator) Next() (*storage.ObjectAttrs, error) { name := m.keys[m.currentIndex] obj := &storage.ObjectAttrs{ Name: name, - Metadata: m.metadata[name], + Metadata: m.tags[name], } m.currentIndex++ return obj, nil diff --git a/pkg/snapstore/snapstore_test.go b/pkg/snapstore/snapstore_test.go index db197b989..6a9c0f24e 100644 --- a/pkg/snapstore/snapstore_test.go +++ b/pkg/snapstore/snapstore_test.go @@ -41,6 +41,14 @@ type testSnapStore struct { objectCountPerSnapshot int } +// tagI is the interface that is to be implemented by mock snapstores to set tags on snapshots +type tagI interface { + // Sets all of the tags for a mocked snapshot + setTag(string, map[string]string) + // Deletes all of the tags of a mocked snapshot + deleteTag(string) +} + var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { var ( snap1 brtypes.Snapshot @@ -103,9 +111,9 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { snap5.GenerateSnapshotName() gcsClient = &mockGCSClient{ - objects: objectMap, - prefix: prefixV2, - objectMetadata: make(map[string]map[string]string), + objects: objectMap, + prefix: prefixV2, + objectTags: make(map[string]map[string]string), } snapstores = map[string]testSnapStore{ @@ -308,11 +316,16 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { Expect(snapList[secondSnapshotIndex].SnapName).To(Equal(snap5.SnapName)) // List tests with false and true as arguments only implemented with GCS for now + var tag tagI + switch provider { + case "GCS": + tag = gcsClient + } if provider == "GCS" { // the tagged snapshot should not be returned by the List() call taggedSnapshot := snapList[0] taggedSnapshotName := path.Join(taggedSnapshot.Prefix, taggedSnapshot.SnapDir, taggedSnapshot.SnapName) - gcsClient.objectMetadata[taggedSnapshotName] = map[string]string{"x-etcd-snapshot-exclude": "true"} + tag.setTag(taggedSnapshotName, map[string]string{"x-etcd-snapshot-exclude": "true"}) snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).Should(Equal((numberSnapshotsInObjectMap - 1) * snapStore.objectCountPerSnapshot)) @@ -325,7 +338,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { Expect(snapList[0].SnapName).Should(Equal(taggedSnapshot.SnapName)) // removing the tag will make the snapshot appear in the List call with false - delete(gcsClient.objectMetadata, taggedSnapshotName) + tag.deleteTag(taggedSnapshotName) snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).Should(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) From 56753066a1608b7036fc05f3b83db1a794d81a22 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Thu, 12 Sep 2024 14:55:53 +0530 Subject: [PATCH 09/15] Remove hardcoding of tags in mock GCS tag implementation. * Remove hardcoding of tags in mock GCS tag implementation. * Rename the `tagI` interface to `tagger`. * Rename the methods of the `tagger` interface to plural since they operate on all tags of the snapshot. --- pkg/snapstore/gcs_snapstore_test.go | 6 +++--- pkg/snapstore/snapshot_test.go | 1 - pkg/snapstore/snapstore_test.go | 14 +++++++------- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/snapstore/gcs_snapstore_test.go b/pkg/snapstore/gcs_snapstore_test.go index e4b66ee8a..5d2bb7564 100644 --- a/pkg/snapstore/gcs_snapstore_test.go +++ b/pkg/snapstore/gcs_snapstore_test.go @@ -30,11 +30,11 @@ func (m *mockGCSClient) Bucket(name string) stiface.BucketHandle { return &mockBucketHandle{bucket: name, client: m} } -func (m *mockGCSClient) setTag(taggedSnapshotName string, tagMap map[string]string) { - m.objectTags[taggedSnapshotName] = map[string]string{"x-etcd-snapshot-exclude": "true"} +func (m *mockGCSClient) setTags(taggedSnapshotName string, tagMap map[string]string) { + m.objectTags[taggedSnapshotName] = tagMap } -func (m *mockGCSClient) deleteTag(taggedSnapshotName string) { +func (m *mockGCSClient) deleteTags(taggedSnapshotName string) { delete(m.objectTags, taggedSnapshotName) } diff --git a/pkg/snapstore/snapshot_test.go b/pkg/snapstore/snapshot_test.go index 0f2477848..6b00d8aa9 100644 --- a/pkg/snapstore/snapshot_test.go +++ b/pkg/snapstore/snapshot_test.go @@ -328,7 +328,6 @@ var _ = Describe("Snapshot", func() { Kind: brtypes.SnapshotKindDelta, SnapDir: snapdir, } - }) It("should be deletable when its retention period is not set", func() { // do not set the retention period diff --git a/pkg/snapstore/snapstore_test.go b/pkg/snapstore/snapstore_test.go index 6a9c0f24e..72cfba2ef 100644 --- a/pkg/snapstore/snapstore_test.go +++ b/pkg/snapstore/snapstore_test.go @@ -41,12 +41,12 @@ type testSnapStore struct { objectCountPerSnapshot int } -// tagI is the interface that is to be implemented by mock snapstores to set tags on snapshots -type tagI interface { +// tagger is the interface that is to be implemented by mock snapstores to set tags on snapshots +type tagger interface { // Sets all of the tags for a mocked snapshot - setTag(string, map[string]string) + setTags(string, map[string]string) // Deletes all of the tags of a mocked snapshot - deleteTag(string) + deleteTags(string) } var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { @@ -316,7 +316,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { Expect(snapList[secondSnapshotIndex].SnapName).To(Equal(snap5.SnapName)) // List tests with false and true as arguments only implemented with GCS for now - var tag tagI + var tag tagger switch provider { case "GCS": tag = gcsClient @@ -325,7 +325,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { // the tagged snapshot should not be returned by the List() call taggedSnapshot := snapList[0] taggedSnapshotName := path.Join(taggedSnapshot.Prefix, taggedSnapshot.SnapDir, taggedSnapshot.SnapName) - tag.setTag(taggedSnapshotName, map[string]string{"x-etcd-snapshot-exclude": "true"}) + tag.setTags(taggedSnapshotName, map[string]string{brtypes.ExcludeSnapshotMetadataKey: "true"}) snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).Should(Equal((numberSnapshotsInObjectMap - 1) * snapStore.objectCountPerSnapshot)) @@ -338,7 +338,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { Expect(snapList[0].SnapName).Should(Equal(taggedSnapshot.SnapName)) // removing the tag will make the snapshot appear in the List call with false - tag.deleteTag(taggedSnapshotName) + tag.deleteTags(taggedSnapshotName) snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).Should(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) From cc64835780ad1c03bb8f61a29192f0f8b38194f7 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Wed, 18 Sep 2024 09:38:59 +0530 Subject: [PATCH 10/15] Address review comments from @ishan16696 1. --- pkg/snapshot/snapshotter/garbagecollector.go | 6 +++--- pkg/snapstore/abs_snapstore.go | 1 - pkg/snapstore/gcs_snapstore.go | 2 +- pkg/snapstore/snapstore_test.go | 3 ++- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/snapshot/snapshotter/garbagecollector.go b/pkg/snapshot/snapshotter/garbagecollector.go index d38f2bbd6..62aec31d0 100644 --- a/pkg/snapshot/snapshotter/garbagecollector.go +++ b/pkg/snapshot/snapshotter/garbagecollector.go @@ -41,7 +41,7 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { total := 0 ssr.logger.Info("GC: Executing garbage collection...") - // List the tagged snapshots to garbage collect them + // List all (tagged and untagged) snapshots to garbage collect them according to the garbage collection policy. snapList, err := ssr.store.List(true) if err != nil { metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: err.Error()}).Inc() @@ -63,7 +63,7 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { } else { // chunksDeleted stores the no of chunks deleted in the current iteration of GC. var chunksDeleted int - // GarbageCollectChunks returns a filtered SnapList which does not contain chunks + // GarbageCollectChunks returns a filtered SnapList which does not contain chunks. chunksDeleted, snapList = ssr.GarbageCollectChunks(snapList) ssr.logger.Infof("GC: Total number garbage collected chunks: %d", chunksDeleted) } @@ -143,7 +143,7 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { if deleteSnap { if !nextSnap.IsDeletable() { - ssr.logger.Infof("GC: Skipping : %s, since it is immutable", nextSnap.SnapName) + ssr.logger.Infof("GC: Skipping the snapshot: %s, since its retention period hasn't expired yet", nextSnap.SnapName) continue } ssr.logger.Infof("GC: Deleting old full snapshot: %s %v", nextSnap.CreatedOn.UTC(), deleteSnap) diff --git a/pkg/snapstore/abs_snapstore.go b/pkg/snapstore/abs_snapstore.go index 94d96e563..8322a4d20 100644 --- a/pkg/snapstore/abs_snapstore.go +++ b/pkg/snapstore/abs_snapstore.go @@ -308,7 +308,6 @@ func (a *ABSSnapStore) List(_ bool) (brtypes.SnapList, error) { if err != nil { logrus.Warnf("Invalid snapshot found. Ignoring it:%s\n", *blobItem.Name) } else { - // a := blob.BlobTags.BlobTagSet[0].Key snapList = append(snapList, s) } } diff --git a/pkg/snapstore/gcs_snapstore.go b/pkg/snapstore/gcs_snapstore.go index 86e7d9bf8..5f4f5433a 100644 --- a/pkg/snapstore/gcs_snapstore.go +++ b/pkg/snapstore/gcs_snapstore.go @@ -283,7 +283,7 @@ func (s *GCSSnapStore) componentUploader(wg *sync.WaitGroup, stopCh <-chan struc } } -// List will return a sorted list with all snapshot files on store, excluding those marked with x-ignore-etcd-snapshot-exclude. Tagged snapshots can be included in the List call by passing true as the argument. +// List returns a sorted list of all snapshot files in the object store, excluding those snapshots tagged with `x-ignore-etcd-snapshot-exclude` in their object metadata/tags. To include these tagged snapshots in the List output, pass `true` as the argument. func (s *GCSSnapStore) List(includeAll bool) (brtypes.SnapList, error) { prefixTokens := strings.Split(s.prefix, "/") // Consider the parent of the last element for backward compatibility. diff --git a/pkg/snapstore/snapstore_test.go b/pkg/snapstore/snapstore_test.go index 72cfba2ef..bc3e0d49a 100644 --- a/pkg/snapstore/snapstore_test.go +++ b/pkg/snapstore/snapstore_test.go @@ -315,7 +315,8 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { Expect(snapList[0].SnapName).To(Equal(snap4.SnapName)) Expect(snapList[secondSnapshotIndex].SnapName).To(Equal(snap5.SnapName)) - // List tests with false and true as arguments only implemented with GCS for now + // The List method implemented for SnapStores which support immutable objects is tested for tagged and untagged snapshots. + // TODO @renormalize: ABS, S3 var tag tagger switch provider { case "GCS": From b78df5842fba20f02b5bcd541a18abde149ed841 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:25:56 +0530 Subject: [PATCH 11/15] Fix log strings, enhance `List` method comment, remove hardcoding. * Logs for skipping GC with immutable objects mention "immutability period" and not "retention period" since retention period as a phrase is already used for delta snapshot retention. * Enhance `ExcludeSnapshotMetadataKey` and `List` method comments. * Eliminate the potential for a flaky test in snapshot_test.go where later expiry time is only set to a nanosecond after the creation time instead of a microsecond. The test could flake if the test gets run within a microsecond. * Removed one hardcoding of a string from the proposed changed, and quite a bit of hardcoding in the existing codebase in snapstore_test.go --- pkg/snapshot/snapshotter/garbagecollector.go | 6 +-- pkg/snapstore/snapshot_test.go | 8 ++-- pkg/snapstore/snapstore_test.go | 45 +++++++------------- pkg/types/snapstore.go | 6 +-- 4 files changed, 26 insertions(+), 39 deletions(-) diff --git a/pkg/snapshot/snapshotter/garbagecollector.go b/pkg/snapshot/snapshotter/garbagecollector.go index 62aec31d0..6cee53405 100644 --- a/pkg/snapshot/snapshotter/garbagecollector.go +++ b/pkg/snapshot/snapshotter/garbagecollector.go @@ -143,7 +143,7 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { if deleteSnap { if !nextSnap.IsDeletable() { - ssr.logger.Infof("GC: Skipping the snapshot: %s, since its retention period hasn't expired yet", nextSnap.SnapName) + ssr.logger.Infof("GC: Skipping the snapshot: %s, since its immutability period hasn't expired yet", nextSnap.SnapName) continue } ssr.logger.Infof("GC: Deleting old full snapshot: %s %v", nextSnap.CreatedOn.UTC(), deleteSnap) @@ -223,7 +223,7 @@ func (ssr *Snapshotter) GarbageCollectChunks(snapList brtypes.SnapList) (int, br // delete the chunk object snapPath := path.Join(snap.SnapDir, snap.SnapName) if !snap.IsDeletable() { - ssr.logger.Infof("GC: Skipping : %s, since it is immutable", snap.SnapName) + ssr.logger.Infof("GC: Skipping the snapshot: %s, since its immutability period hasn't expired yet", snap.SnapName) continue } ssr.logger.Infof("GC: Deleting chunk for old snapshot: %s", snapPath) @@ -260,7 +260,7 @@ func (ssr *Snapshotter) GarbageCollectDeltaSnapshots(snapStream brtypes.SnapList snapPath := path.Join(snapStream[i].SnapDir, snapStream[i].SnapName) ssr.logger.Infof("GC: Deleting old delta snapshot: %s", snapPath) if !snapStream[i].IsDeletable() { - ssr.logger.Infof("GC: Skipping : %s, since it is immutable", snapPath) + ssr.logger.Infof("GC: Skipping the snapshot: %s, since its immutability period hasn't expired yet", snapPath) continue } if err := ssr.store.Delete(*snapStream[i]); err != nil { diff --git a/pkg/snapstore/snapshot_test.go b/pkg/snapstore/snapshot_test.go index 6b00d8aa9..4bd814880 100644 --- a/pkg/snapstore/snapshot_test.go +++ b/pkg/snapstore/snapshot_test.go @@ -323,8 +323,8 @@ var _ = Describe("Snapshot", func() { } snap2 = brtypes.Snapshot{ CreatedOn: now, - StartRevision: 1201, - LastRevision: 1500, + StartRevision: 2088, + LastRevision: 3088, Kind: brtypes.SnapshotKindDelta, SnapDir: snapdir, } @@ -336,8 +336,8 @@ var _ = Describe("Snapshot", func() { }) It("should be deletable when its retention period has expired", func() { // Setting expiry time to be a short time after creation - snap1.RetentionExpiry = snap1.CreatedOn.Add(time.Microsecond) - snap2.RetentionExpiry = snap2.CreatedOn.Add(time.Microsecond) + snap1.RetentionExpiry = snap1.CreatedOn.Add(time.Nanosecond) + snap2.RetentionExpiry = snap2.CreatedOn.Add(time.Nanosecond) Expect(snap1.IsDeletable()).To(BeTrue()) Expect(snap2.IsDeletable()).To(BeTrue()) }) diff --git a/pkg/snapstore/snapstore_test.go b/pkg/snapstore/snapstore_test.go index bc3e0d49a..639d960b8 100644 --- a/pkg/snapstore/snapstore_test.go +++ b/pkg/snapstore/snapstore_test.go @@ -117,7 +117,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { } snapstores = map[string]testSnapStore{ - "s3": { + brtypes.SnapstoreProviderS3: { SnapStore: NewS3FromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, &mockS3Client{ objects: objectMap, prefix: prefixV2, @@ -125,11 +125,11 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { }, SSECredentials{}), objectCountPerSnapshot: 1, }, - "swift": { + brtypes.SnapstoreProviderSwift: { SnapStore: NewSwiftSnapstoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, fake.ServiceClient()), objectCountPerSnapshot: 3, }, - "ABS": { + brtypes.SnapstoreProviderABS: { SnapStore: NewABSSnapStoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, &fakeABSContainerClient{ objects: objectMap, prefix: prefixV2, @@ -137,11 +137,11 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { }), objectCountPerSnapshot: 1, }, - "GCS": { + brtypes.SnapstoreProviderGCS: { SnapStore: NewGCSSnapStoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, "", gcsClient), objectCountPerSnapshot: 1, }, - "OSS": { + brtypes.SnapstoreProviderOSS: { SnapStore: NewOSSFromBucket(prefixV2, "/tmp", 5, brtypes.MinChunkSize, &mockOSSBucket{ objects: objectMap, prefix: prefixV2, @@ -150,7 +150,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { }), objectCountPerSnapshot: 1, }, - "ECS": { + brtypes.SnapstoreProviderECS: { SnapStore: NewS3FromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, &mockS3Client{ objects: objectMap, prefix: prefixV2, @@ -158,7 +158,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { }, SSECredentials{}), objectCountPerSnapshot: 1, }, - "OCS": { + brtypes.SnapstoreProviderOCS: { SnapStore: NewS3FromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, &mockS3Client{ objects: objectMap, prefix: prefixV2, @@ -317,16 +317,16 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { // The List method implemented for SnapStores which support immutable objects is tested for tagged and untagged snapshots. // TODO @renormalize: ABS, S3 - var tag tagger + var mockClient tagger switch provider { - case "GCS": - tag = gcsClient + case brtypes.SnapstoreProviderGCS: + mockClient = gcsClient } - if provider == "GCS" { + if provider == brtypes.SnapstoreProviderGCS { // the tagged snapshot should not be returned by the List() call taggedSnapshot := snapList[0] taggedSnapshotName := path.Join(taggedSnapshot.Prefix, taggedSnapshot.SnapDir, taggedSnapshot.SnapName) - tag.setTags(taggedSnapshotName, map[string]string{brtypes.ExcludeSnapshotMetadataKey: "true"}) + mockClient.setTags(taggedSnapshotName, map[string]string{brtypes.ExcludeSnapshotMetadataKey: "true"}) snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).Should(Equal((numberSnapshotsInObjectMap - 1) * snapStore.objectCountPerSnapshot)) @@ -339,7 +339,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { Expect(snapList[0].SnapName).Should(Equal(taggedSnapshot.SnapName)) // removing the tag will make the snapshot appear in the List call with false - tag.deleteTags(taggedSnapshotName) + mockClient.deleteTags(taggedSnapshotName) snapList, err = snapStore.List(false) Expect(err).ShouldNot(HaveOccurred()) Expect(snapList.Len()).Should(Equal(numberSnapshotsInObjectMap * snapStore.objectCountPerSnapshot)) @@ -377,7 +377,6 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { }) type CredentialTestConfig struct { - Provider string EnvVariable string SnapstoreProvider string CredentialType string // "file" or "directory" @@ -387,14 +386,12 @@ type CredentialTestConfig struct { var credentialTestConfigs = []CredentialTestConfig{ // AWS { - Provider: "AWS", EnvVariable: "AWS_APPLICATION_CREDENTIALS", SnapstoreProvider: brtypes.SnapstoreProviderS3, CredentialType: "directory", CredentialFiles: []string{"accessKeyID", "region", "secretAccessKey"}, }, { - Provider: "AWS", EnvVariable: "AWS_APPLICATION_CREDENTIALS_JSON", SnapstoreProvider: brtypes.SnapstoreProviderS3, CredentialType: "file", @@ -402,14 +399,12 @@ var credentialTestConfigs = []CredentialTestConfig{ }, // Azure { - Provider: "ABS", EnvVariable: "AZURE_APPLICATION_CREDENTIALS", SnapstoreProvider: brtypes.SnapstoreProviderABS, CredentialType: "directory", CredentialFiles: []string{"storageAccount", "storageKey"}, }, { - Provider: "ABS", EnvVariable: "AZURE_APPLICATION_CREDENTIALS_JSON", SnapstoreProvider: brtypes.SnapstoreProviderABS, CredentialType: "file", @@ -417,7 +412,6 @@ var credentialTestConfigs = []CredentialTestConfig{ }, // GCS { - Provider: "GCS", EnvVariable: "GOOGLE_APPLICATION_CREDENTIALS", SnapstoreProvider: brtypes.SnapstoreProviderGCS, CredentialType: "file", @@ -425,7 +419,6 @@ var credentialTestConfigs = []CredentialTestConfig{ }, // Swift V3ApplicationCredentials { - Provider: "Swift", EnvVariable: "OPENSTACK_APPLICATION_CREDENTIALS", SnapstoreProvider: brtypes.SnapstoreProviderSwift, CredentialType: "directory", @@ -433,7 +426,6 @@ var credentialTestConfigs = []CredentialTestConfig{ }, // Swift Password { - Provider: "Swift", EnvVariable: "OPENSTACK_APPLICATION_CREDENTIALS", SnapstoreProvider: brtypes.SnapstoreProviderSwift, CredentialType: "directory", @@ -441,7 +433,6 @@ var credentialTestConfigs = []CredentialTestConfig{ }, // Swift JSON { - Provider: "Swift", EnvVariable: "OPENSTACK_APPLICATION_CREDENTIALS_JSON", SnapstoreProvider: brtypes.SnapstoreProviderSwift, CredentialType: "file", @@ -449,14 +440,12 @@ var credentialTestConfigs = []CredentialTestConfig{ }, // OSS { - Provider: "OSS", EnvVariable: "ALICLOUD_APPLICATION_CREDENTIALS", SnapstoreProvider: brtypes.SnapstoreProviderOSS, CredentialType: "directory", CredentialFiles: []string{"accessKeyID", "accessKeySecret", "storageEndpoint"}, }, { - Provider: "OSS", EnvVariable: "ALICLOUD_APPLICATION_CREDENTIALS_JSON", SnapstoreProvider: brtypes.SnapstoreProviderOSS, CredentialType: "file", @@ -464,14 +453,12 @@ var credentialTestConfigs = []CredentialTestConfig{ }, // OCS { - Provider: "OCS", EnvVariable: "OPENSHIFT_APPLICATION_CREDENTIALS", SnapstoreProvider: brtypes.SnapstoreProviderOCS, CredentialType: "directory", CredentialFiles: []string{"accessKeyID", "region", "endpoint", "secretAccessKey"}, }, { - Provider: "OCS", EnvVariable: "OPENSHIFT_APPLICATION_CREDENTIALS_JSON", SnapstoreProvider: brtypes.SnapstoreProviderOCS, CredentialType: "file", @@ -482,7 +469,7 @@ var credentialTestConfigs = []CredentialTestConfig{ var _ = Describe("Dynamic access credential rotation test for each provider", func() { for _, config := range credentialTestConfigs { config := config - Describe(fmt.Sprintf("testing secret modification for %q with %q", config.Provider, config.EnvVariable), func() { + Describe(fmt.Sprintf("testing secret modification for %q with %q", config.SnapstoreProvider, config.EnvVariable), func() { Context("environment variable not set", func() { It("should return error", func() { newSecretModifiedTime, err := GetSnapstoreSecretModifiedTime(config.SnapstoreProvider) @@ -599,7 +586,7 @@ var _ = Describe("Blob Service URL construction for Azure", func() { var _ = Describe("Server Side Encryption Customer Managed Key for S3", func() { s3SnapstoreConfig := brtypes.SnapstoreConfig{ - Provider: "S3", + Provider: brtypes.SnapstoreProviderS3, Container: "etcd-test", Prefix: "v2", } @@ -700,7 +687,7 @@ func generateContentsForSnapshot(snapshot *brtypes.Snapshot) string { func setObjectMap(provider string, snapshots brtypes.SnapList) int { var numberSnapshotsAdded int for _, snapshot := range snapshots { - if provider == "swift" { + if provider == brtypes.SnapstoreProviderSwift { // contents of the snapshot split into segments generatedContents := generateContentsForSnapshot(snapshot) segmentBytes01 := []byte(generatedContents[:len(generatedContents)/2]) diff --git a/pkg/types/snapstore.go b/pkg/types/snapstore.go index 9a568c285..90f57fec4 100644 --- a/pkg/types/snapstore.go +++ b/pkg/types/snapstore.go @@ -57,7 +57,7 @@ const ( // MinChunkSize is set to 5Mib since it is lower chunk size limit for AWS. MinChunkSize int64 = 5 * (1 << 20) //5 MiB - // ExcludeSnapshotMetadataKey is the tag that is to be added on snapshots in the object store if they are not to be included in restoration + // ExcludeSnapshotMetadataKey is the tag that is to be added on snapshots in the object store if they are not to be included in SnapStore's List output. ExcludeSnapshotMetadataKey = "x-etcd-snapshot-exclude" ) @@ -68,7 +68,7 @@ const ( type SnapStore interface { // Fetch should open reader for the snapshot file from store. Fetch(Snapshot) (io.ReadCloser, error) - // List returns a sorted list of all snapshots in the store. + // List returns a sorted list (based on the last revision, ascending) of all snapshots in the store. // includeAll specifies whether to include all snapshots while listing, including those with exclude tags. // Snapshots with exclude tags are not listed unless includeAll is set to true. List(includeAll bool) (SnapList, error) @@ -97,8 +97,8 @@ type Snapshot struct { // It checks if the retention expiry time is set and whether the current time is after the retention expiry time. func (s *Snapshot) IsDeletable() bool { // Check if RetentionExpiry is the zero value of time.Time, which means it is not set. + // If RetentionExpiry is not set, assume the snapshot can be deleted. if s.RetentionExpiry.IsZero() { - // If RetentionExpiry is not set, assume the snapshot can be deleted. return true } // Otherwise, check if the current time is after the retention expiry time. From a6680d4ad3e2247f52374b5423b2ada4016e6b1c Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Fri, 20 Sep 2024 14:17:33 +0530 Subject: [PATCH 12/15] Rename `RetentionExpiry` to `ImmutabilityExpiryTime` * This rename makes the distinction clearer between retention expiry which is used for delta snapshots typically, and the immutability expiry time of a snapshot. --- pkg/snapstore/gcs_snapstore.go | 2 +- pkg/snapstore/snapshot_test.go | 18 +++++++++--------- pkg/types/snapstore.go | 34 +++++++++++++++++----------------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/pkg/snapstore/gcs_snapstore.go b/pkg/snapstore/gcs_snapstore.go index 5f4f5433a..e61d51ae0 100644 --- a/pkg/snapstore/gcs_snapstore.go +++ b/pkg/snapstore/gcs_snapstore.go @@ -319,7 +319,7 @@ func (s *GCSSnapStore) List(includeAll bool) (brtypes.SnapList, error) { logrus.Warnf("Invalid snapshot %s found, ignoring it: %v", v.Name, err) continue } - snap.RetentionExpiry = v.RetentionExpirationTime + snap.ImmutabilityExpiryTime = v.RetentionExpirationTime snapList = append(snapList, snap) } } diff --git a/pkg/snapstore/snapshot_test.go b/pkg/snapstore/snapshot_test.go index 4bd814880..9648fdf3c 100644 --- a/pkg/snapstore/snapshot_test.go +++ b/pkg/snapstore/snapshot_test.go @@ -306,7 +306,7 @@ var _ = Describe("Snapshot", func() { }) }) - Context("when provided with retention time periods", func() { + Context("when provided with immutability time periods", func() { var ( snap1 brtypes.Snapshot snap2 brtypes.Snapshot @@ -329,22 +329,22 @@ var _ = Describe("Snapshot", func() { SnapDir: snapdir, } }) - It("should be deletable when its retention period is not set", func() { - // do not set the retention period + It("should be deletable when its immutability period is not set", func() { + // do not set the immutability period Expect(snap1.IsDeletable()).Should(BeTrue()) Expect(snap2.IsDeletable()).Should(BeTrue()) }) - It("should be deletable when its retention period has expired", func() { + It("should be deletable when its immutability period has expired", func() { // Setting expiry time to be a short time after creation - snap1.RetentionExpiry = snap1.CreatedOn.Add(time.Nanosecond) - snap2.RetentionExpiry = snap2.CreatedOn.Add(time.Nanosecond) + snap1.ImmutabilityExpiryTime = snap1.CreatedOn.Add(time.Nanosecond) + snap2.ImmutabilityExpiryTime = snap2.CreatedOn.Add(time.Nanosecond) Expect(snap1.IsDeletable()).To(BeTrue()) Expect(snap2.IsDeletable()).To(BeTrue()) }) - It("should not be deletable when its retention period has not expired", func() { + It("should not be deletable when its immutability period has not expired", func() { // Setting the expiry time to be a long time after the creation - snap1.RetentionExpiry = snap1.CreatedOn.Add(time.Hour) - snap2.RetentionExpiry = snap2.CreatedOn.Add(time.Hour) + snap1.ImmutabilityExpiryTime = snap1.CreatedOn.Add(time.Hour) + snap2.ImmutabilityExpiryTime = snap2.CreatedOn.Add(time.Hour) Expect(snap1.IsDeletable()).To(BeFalse()) Expect(snap2.IsDeletable()).To(BeFalse()) }) diff --git a/pkg/types/snapstore.go b/pkg/types/snapstore.go index 90f57fec4..3551a36d8 100644 --- a/pkg/types/snapstore.go +++ b/pkg/types/snapstore.go @@ -80,29 +80,29 @@ type SnapStore interface { // Snapshot structure represents the metadata of snapshot. type Snapshot struct { - Kind string `json:"kind"` // incr:incremental, full:full - StartRevision int64 `json:"startRevision"` - LastRevision int64 `json:"lastRevision"` // latest revision on snapshot - CreatedOn time.Time `json:"createdOn"` - SnapDir string `json:"snapDir"` - SnapName string `json:"snapName"` - IsChunk bool `json:"isChunk"` - Prefix string `json:"prefix"` // Points to correct prefix of a snapshot in snapstore (Required for Backward Compatibility) - CompressionSuffix string `json:"compressionSuffix"` // CompressionSuffix depends on compression policy - IsFinal bool `json:"isFinal"` - RetentionExpiry time.Time `json:"retentionExpiry"` + Kind string `json:"kind"` // incr:incremental, full:full + StartRevision int64 `json:"startRevision"` + LastRevision int64 `json:"lastRevision"` // latest revision on snapshot + CreatedOn time.Time `json:"createdOn"` + SnapDir string `json:"snapDir"` + SnapName string `json:"snapName"` + IsChunk bool `json:"isChunk"` + Prefix string `json:"prefix"` // Points to correct prefix of a snapshot in snapstore (Required for Backward Compatibility) + CompressionSuffix string `json:"compressionSuffix"` // CompressionSuffix depends on compression policy + IsFinal bool `json:"isFinal"` + ImmutabilityExpiryTime time.Time `json:"immutabilityExpriyTime"` } // IsDeletable determines if the snapshot can be deleted. -// It checks if the retention expiry time is set and whether the current time is after the retention expiry time. +// It checks if the immutability expiry time is set and whether the current time is after the immutability expiry time. func (s *Snapshot) IsDeletable() bool { - // Check if RetentionExpiry is the zero value of time.Time, which means it is not set. - // If RetentionExpiry is not set, assume the snapshot can be deleted. - if s.RetentionExpiry.IsZero() { + // Check if ImmutabilityExpiryTime is the zero value of time.Time, which means it is not set. + // If ImmutabilityExpiryTime is not set, assume the snapshot can be deleted. + if s.ImmutabilityExpiryTime.IsZero() { return true } - // Otherwise, check if the current time is after the retention expiry time. - return time.Now().After(s.RetentionExpiry) + // Otherwise, check if the current time is after the immutability expiry time. + return time.Now().After(s.ImmutabilityExpiryTime) } // GenerateSnapshotName prepares the snapshot name from metadata From f2271985f71c3c7a57926be62dbb0ed840ee05fb Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Mon, 23 Sep 2024 11:28:15 +0530 Subject: [PATCH 13/15] Add `docs/usage/immutable_snapshots.md`. --- docs/usage/immutable_snapshots.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 docs/usage/immutable_snapshots.md diff --git a/docs/usage/immutable_snapshots.md b/docs/usage/immutable_snapshots.md new file mode 100644 index 000000000..54777884a --- /dev/null +++ b/docs/usage/immutable_snapshots.md @@ -0,0 +1,29 @@ +# Immutable Snapshots + +## Overview of Immutable Objects + +Several cloud providers offer functionality to create immutable objects within their storage services. Once an object is uploaded, it cannot be modified or deleted for a set period, known as the **immutability period**. These are referred to as **immutable objects**. + +Currently, etcd-backup-restore enables the use of immutable objects on the following cloud platforms: + +- Google Cloud Storage (currently supported) + +## Enabling and Using Immutable Snapshots with etcd-backup-restore + +Etcd-backup-restore supports immutable objects, typically at what cloud providers call the "bucket level." During the creation of a bucket, it is configured to render objects immutable for a specific duration from the moment of their upload. This feature can be enabled through: + +- **Google Cloud Storage**: [Bucket Lock](https://cloud.google.com/storage/docs/bucket-lock) + +It is also possible to enable immutability retroactively by making appropriate API calls to your cloud provider, allowing the immutable snapshots feature to be used with existing buckets. For information on such configurations, please refer to your cloud provider's documentation. + +The behavior of objects uploaded before a bucket is set to immutable varies among providers. Etcd-backup-restore manages these objects and will perform garbage collection according to the configured policy and the object's immutability expiry. + +## Current Capabilities + +Etcd-backup-restore does not require configurations regarding the immutability period of an object since this is inherited from the bucket’s immutability settings. The tool also checks the immutability expiry of an object before initiating its garbage collection. + +Therefore, it is advisable to configure your garbage collection policies based on the duration you want your objects to remain immutable. + +## Storage Considerations + +Making objects immutable for extended periods can increase storage costs since these objects cannot be removed once uploaded. Storing outdated snapshots beyond their utility does not significantly enhance recovery capabilities. Therefore, consider all factors before enabling immutability for buckets, as this feature is irreversible once set by cloud providers. From c0ea11f2036e3c237a03e5ec5643e4e891bc825f Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Tue, 24 Sep 2024 12:49:05 +0530 Subject: [PATCH 14/15] Address review comments from @ishan16696 on `docs/usage/immutable_snapshots.md` --- docs/usage/immutable_snapshots.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/usage/immutable_snapshots.md b/docs/usage/immutable_snapshots.md index 54777884a..6c031ea21 100644 --- a/docs/usage/immutable_snapshots.md +++ b/docs/usage/immutable_snapshots.md @@ -4,11 +4,11 @@ Several cloud providers offer functionality to create immutable objects within their storage services. Once an object is uploaded, it cannot be modified or deleted for a set period, known as the **immutability period**. These are referred to as **immutable objects**. -Currently, etcd-backup-restore enables the use of immutable objects on the following cloud platforms: +Currently, etcd-backup-restore supports the use of immutable objects on the following cloud platforms: - Google Cloud Storage (currently supported) -## Enabling and Using Immutable Snapshots with etcd-backup-restore +## Enabling and using Immutable Snapshots with etcd-backup-restore Etcd-backup-restore supports immutable objects, typically at what cloud providers call the "bucket level." During the creation of a bucket, it is configured to render objects immutable for a specific duration from the moment of their upload. This feature can be enabled through: @@ -16,11 +16,13 @@ Etcd-backup-restore supports immutable objects, typically at what cloud provider It is also possible to enable immutability retroactively by making appropriate API calls to your cloud provider, allowing the immutable snapshots feature to be used with existing buckets. For information on such configurations, please refer to your cloud provider's documentation. -The behavior of objects uploaded before a bucket is set to immutable varies among providers. Etcd-backup-restore manages these objects and will perform garbage collection according to the configured policy and the object's immutability expiry. +The behaviour of bucket's objects uploaded before a bucket is set to immutable varies among storage providers. Etcd-backup-restore manages these objects and will perform garbage collection according to the configured garbage collection policy and the object's immutability expiry. + +> Note: If immutable snapshots are not enabled then the object's immutability expiry will be considered as zero, hence causing no effect on current functionality. ## Current Capabilities -Etcd-backup-restore does not require configurations regarding the immutability period of an object since this is inherited from the bucket’s immutability settings. The tool also checks the immutability expiry of an object before initiating its garbage collection. +Etcd-backup-restore does not require configurations related to the immutability period of bucket's objects as this information is derived from the bucket's existing immutability settings. The etcd-backup-restore process also verifies the immutability expiry time of an object prior to initiating its garbage collection. Therefore, it is advisable to configure your garbage collection policies based on the duration you want your objects to remain immutable. From 040b23204c85183f9518b4e103390919584c3342 Mon Sep 17 00:00:00 2001 From: Saketh Kalaga <51327242+renormalize@users.noreply.github.com> Date: Tue, 24 Sep 2024 13:09:20 +0530 Subject: [PATCH 15/15] Address review comments from @anveshreddy18 --- pkg/snapstore/gcs_snapstore.go | 2 +- pkg/snapstore/snapshot_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/snapstore/gcs_snapstore.go b/pkg/snapstore/gcs_snapstore.go index e61d51ae0..aab6b3af3 100644 --- a/pkg/snapstore/gcs_snapstore.go +++ b/pkg/snapstore/gcs_snapstore.go @@ -305,7 +305,7 @@ func (s *GCSSnapStore) List(includeAll bool) (brtypes.SnapList, error) { // Check if the snapshot should be ignored if !includeAll && attr.Metadata[brtypes.ExcludeSnapshotMetadataKey] == "true" { - logrus.Infof("Ignoring snapshot due to exclude metadata: %s", attr.Name) + logrus.Infof("Ignoring snapshot due to exclude tag %q present in metadata on snapshot: %s", brtypes.ExcludeSnapshotMetadataKey, attr.Name) continue } attrs = append(attrs, attr) diff --git a/pkg/snapstore/snapshot_test.go b/pkg/snapstore/snapshot_test.go index 9648fdf3c..5919f3aa7 100644 --- a/pkg/snapstore/snapshot_test.go +++ b/pkg/snapstore/snapshot_test.go @@ -323,7 +323,7 @@ var _ = Describe("Snapshot", func() { } snap2 = brtypes.Snapshot{ CreatedOn: now, - StartRevision: 2088, + StartRevision: 2089, LastRevision: 3088, Kind: brtypes.SnapshotKindDelta, SnapDir: snapdir,