From 1724dc6fe4d9bab6350132b41648f52b56f7d74c Mon Sep 17 00:00:00 2001 From: Shreyas Rao Date: Sun, 7 Jan 2024 01:47:12 +0530 Subject: [PATCH 1/5] Fix bug in chunk deletion for openstack --- pkg/snapshot/snapshotter/garbagecollector.go | 21 ++++++-- pkg/snapstore/swift_snapstore.go | 55 ++++++++++++++++---- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/pkg/snapshot/snapshotter/garbagecollector.go b/pkg/snapshot/snapshotter/garbagecollector.go index 949dc91f7..c51a5445c 100644 --- a/pkg/snapshot/snapshotter/garbagecollector.go +++ b/pkg/snapshot/snapshotter/garbagecollector.go @@ -58,10 +58,23 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) { continue } - // chunksDeleted stores the no of chunks deleted in the current iteration of GC - var chunksDeleted int - chunksDeleted, snapList = ssr.GarbageCollectChunks(snapList) - ssr.logger.Infof("GC: Total number garbage collected chunks: %d", chunksDeleted) + // Skip chunk deletion for openstack swift provider, since the manifest object is a virtual + // representation of the object, and the actual data is stored in the segment objects, aka chunks + // Chunk deletion for this provider is handled in regular snapshot deletion + if ssr.snapstoreConfig.Provider == brtypes.SnapstoreProviderSwift { + var filteredSnapList brtypes.SnapList + for _, snap := range snapList { + if !snap.IsChunk { + filteredSnapList = append(filteredSnapList, snap) + } + } + snapList = filteredSnapList + } else { + // chunksDeleted stores the no of chunks deleted in the current iteration of GC. + var chunksDeleted int + chunksDeleted, snapList = ssr.GarbageCollectChunks(snapList) + ssr.logger.Infof("GC: Total number garbage collected chunks: %d", chunksDeleted) + } snapStreamIndexList := getSnapStreamIndexList(snapList) diff --git a/pkg/snapstore/swift_snapstore.go b/pkg/snapstore/swift_snapstore.go index 820ea28e8..6efa8e066 100644 --- a/pkg/snapstore/swift_snapstore.go +++ b/pkg/snapstore/swift_snapstore.go @@ -330,22 +330,23 @@ func (s *SwiftSnapStore) Fetch(snap brtypes.Snapshot) (io.ReadCloser, error) { return resp.Body, resp.Err } -// Save will write the snapshot to store +// Save will write the snapshot to store, as a DLO (dynamic large object), as described +// in https://docs.openstack.org/swift/latest/overview_large_objects.html func (s *SwiftSnapStore) Save(snap brtypes.Snapshot, rc io.ReadCloser) error { // Save it locally - tmpfile, err := os.CreateTemp(s.tempDir, tmpBackupFilePrefix) + tempFile, err := os.CreateTemp(s.tempDir, tmpBackupFilePrefix) if err != nil { rc.Close() return fmt.Errorf("failed to create snapshot tempfile: %v", err) } defer func() { - tmpfile.Close() - os.Remove(tmpfile.Name()) + tempFile.Close() + os.Remove(tempFile.Name()) }() - size, err := io.Copy(tmpfile, rc) + size, err := io.Copy(tempFile, rc) rc.Close() if err != nil { - return fmt.Errorf("failed to save snapshot to tmpfile: %v", err) + return fmt.Errorf("failed to save snapshot to tempFile: %v", err) } var ( @@ -365,7 +366,7 @@ func (s *SwiftSnapStore) Save(snap brtypes.Snapshot, rc io.ReadCloser) error { for i := uint(0); i < s.maxParallelChunkUploads; i++ { wg.Add(1) - go s.chunkUploader(&wg, cancelCh, &snap, tmpfile, chunkUploadCh, resCh) + go s.chunkUploader(&wg, cancelCh, &snap, tempFile, chunkUploadCh, resCh) } logrus.Infof("Uploading snapshot of size: %d, chunkSize: %d, noOfChunks: %d", size, chunkSize, noOfChunks) @@ -483,13 +484,47 @@ func (s *SwiftSnapStore) List() (brtypes.SnapList, error) { sort.Sort(snapList) return snapList, nil +} + +func (s *SwiftSnapStore) getSnapshotChunks(snapshot brtypes.Snapshot) (brtypes.SnapList, error) { + snaps, err := s.List() + if err != nil { + return nil, err + } + var chunkList brtypes.SnapList + for _, snap := range snaps { + if snap.IsChunk { + chunkParentSnapPath, _ := path.Split(path.Join(snap.Prefix, snap.SnapDir, snap.SnapName)) + if strings.TrimSuffix(chunkParentSnapPath, "/") == path.Join(snapshot.Prefix, snapshot.SnapDir, snapshot.SnapName) { + chunkList = append(chunkList, snap) + } + } + } + return chunkList, nil } -// Delete should delete the snapshot file from store +// Delete deletes the objects related to the DLO (dynamic large object) from the store. +// This includes the manifest object as well as the segment objects, as +// described in https://docs.openstack.org/swift/latest/overview_large_objects.html func (s *SwiftSnapStore) Delete(snap brtypes.Snapshot) error { - result := objects.Delete(s.client, s.bucket, path.Join(snap.Prefix, snap.SnapDir, snap.SnapName), nil) - return result.Err + chunks, err := s.getSnapshotChunks(snap) + if err != nil { + return err + } + + var chunkObjectNames []string + for _, chunk := range chunks { + chunkObjectNames = append(chunkObjectNames, path.Join(chunk.Prefix, chunk.SnapDir, chunk.SnapName)) + } + + manifestObjectDeleteResult := objects.Delete(s.client, s.bucket, path.Join(snap.Prefix, snap.SnapDir, snap.SnapName), nil) + if manifestObjectDeleteResult.Err != nil { + return manifestObjectDeleteResult.Err + } + + chunkObjectsDeleteResult := objects.BulkDelete(s.client, s.bucket, chunkObjectNames) + return chunkObjectsDeleteResult.Err } // GetSwiftCredentialsLastModifiedTime returns the latest modification timestamp of the Swift credential file(s) From a94e2ae3dfb2a2e01bbab072acfa6a5d09b026d3 Mon Sep 17 00:00:00 2001 From: Shreyas Rao Date: Sun, 7 Jan 2024 02:50:00 +0530 Subject: [PATCH 2/5] Remove backward compatability tests for GC, other minor fixes in snapshotter_test.go --- pkg/snapshot/snapshotter/snapshotter_test.go | 251 ++----------------- 1 file changed, 22 insertions(+), 229 deletions(-) diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index 5c27ffac6..611591dac 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -38,7 +38,6 @@ import ( ) const ( - mixed = "mixed" snapsInV1 = "v1" snapsInV2 = "v2" ) @@ -120,7 +119,7 @@ var _ = Describe("Snapshotter", func() { It("should timeout & not take any snapshot", func() { maxBackups = 2 - testTimeout := time.Duration(time.Minute * time.Duration(maxBackups+1)) + testTimeout := time.Minute * time.Duration(maxBackups+1) snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "snapshotter_2.bkp")} store, err = snapstore.GetSnapstore(snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) @@ -156,7 +155,7 @@ var _ = Describe("Snapshotter", func() { BeforeEach(func() { schedule = "* * 31 2 *" maxBackups = 2 - testTimeout := time.Duration(time.Minute * time.Duration(maxBackups+1)) + testTimeout := time.Minute * time.Duration(maxBackups+1) snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "snapshotter_3.bkp")} store, err = snapstore.GetSnapstore(snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) @@ -202,13 +201,13 @@ var _ = Describe("Snapshotter", func() { schedule = "*/1 * * * *" maxBackups = 2 // We will wait for maxBackups+1 times schedule period - testTimeout = time.Duration(time.Minute * time.Duration(maxBackups+1)) + testTimeout = time.Minute * time.Duration(maxBackups+1) }) Context("with delta snapshot interval set to zero seconds", func() { BeforeEach(func() { deltaSnapshotInterval = 0 - testTimeout = time.Duration(time.Minute * time.Duration(maxBackups)) + testTimeout = time.Minute * time.Duration(maxBackups) }) It("should take periodic backups without delta snapshots", func() { @@ -263,7 +262,7 @@ var _ = Describe("Snapshotter", func() { Context("with delta snapshots enabled", func() { BeforeEach(func() { deltaSnapshotInterval = 10 * time.Second - testTimeout = time.Duration(time.Minute * time.Duration(maxBackups+1)) + testTimeout = time.Minute * time.Duration(maxBackups+1) }) Context("with snapshotter starting without first full snapshot", func() { @@ -390,94 +389,6 @@ var _ = Describe("Snapshotter", func() { } }) - //Test to check backward compatibility of garbage collector - //Checks garbage collector behaviour (in exponential config) when both v1 and v2 directories are present - //TODO: Consider removing when backward compatibility no longer needed - It("should garbage collect exponentially from both v1 and v2 dir structures (backward compatible)", func() { - logger.Infoln("creating expected output") - - // Prepare expected resultant snapshot list - var ( - store, snapstoreConfig = prepareStoreForBackwardCompatibleGC(now, "gc_exponential_backward_compatible.bkp") - snapTime = time.Date(now.Year(), now.Month(), now.Day()-35, 0, -30, 0, 0, now.Location()) - expectedSnapList = brtypes.SnapList{} - ) - - expectedSnapList = prepareExpectedSnapshotsList(snapTime, now, expectedSnapList, mixed) - - //start test - snapshotterConfig := &brtypes.SnapshotterConfig{ - FullSnapshotSchedule: schedule, - DeltaSnapshotPeriod: wrappers.Duration{Duration: 10 * time.Second}, - DeltaSnapshotMemoryLimit: brtypes.DefaultDeltaSnapMemoryLimit, - GarbageCollectionPeriod: wrappers.Duration{Duration: garbageCollectionPeriod}, - GarbageCollectionPolicy: brtypes.GarbageCollectionPolicyExponential, - MaxBackups: maxBackups, - } - - ssr, err := NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - gcCtx, cancel := context.WithTimeout(testCtx, testTimeout) - defer cancel() - ssr.RunGarbageCollector(gcCtx.Done()) - - list, err := store.List() - Expect(err).ShouldNot(HaveOccurred()) - Expect(len(list)).Should(Equal(len(expectedSnapList))) - - for index, snap := range list { - fmt.Println("Snap day: ", snap.CreatedOn.Day()) - fmt.Println("Expected snap day: ", expectedSnapList[index].CreatedOn.Day()) - Expect(snap.CreatedOn).Should(Equal(expectedSnapList[index].CreatedOn)) - Expect(snap.Kind).Should(Equal(expectedSnapList[index].Kind)) - Expect(snap.SnapDir).Should(Equal(expectedSnapList[index].SnapDir)) - } - }) - - //Test to check backward compatibility of garbage collector - //Tests garbage collector behaviour (in exponential config) when only v1 directory is present - //TODO: Consider removing when backward compatibility no longer needed - It("should garbage collect exponentially with only v1 dir structure present (backward compatible test)", func() { - logger.Infoln("creating expected output") - - // Prepare expected resultant snapshot list - var ( - store, snapstoreConfig = prepareStoreForGarbageCollection(now, "gc_exponential_backward_compatiblev1.bkp", "v1") - snapTime = time.Date(now.Year(), now.Month(), now.Day()-35, 0, -30, 0, 0, now.Location()) - expectedSnapList = brtypes.SnapList{} - ) - - expectedSnapList = prepareExpectedSnapshotsList(snapTime, now, expectedSnapList, snapsInV1) - - //start test - snapshotterConfig := &brtypes.SnapshotterConfig{ - FullSnapshotSchedule: schedule, - DeltaSnapshotPeriod: wrappers.Duration{Duration: 10 * time.Second}, - DeltaSnapshotMemoryLimit: brtypes.DefaultDeltaSnapMemoryLimit, - GarbageCollectionPeriod: wrappers.Duration{Duration: garbageCollectionPeriod}, - GarbageCollectionPolicy: brtypes.GarbageCollectionPolicyExponential, - MaxBackups: maxBackups, - } - - ssr, err := NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - gcCtx, cancel := context.WithTimeout(testCtx, testTimeout) - defer cancel() - ssr.RunGarbageCollector(gcCtx.Done()) - - list, err := store.List() - Expect(err).ShouldNot(HaveOccurred()) - Expect(len(list)).Should(Equal(len(expectedSnapList))) - - for index, snap := range list { - Expect(snap.CreatedOn).Should(Equal(expectedSnapList[index].CreatedOn)) - Expect(snap.Kind).Should(Equal(expectedSnapList[index].Kind)) - Expect(snap.SnapDir).Should(Equal(expectedSnapList[index].SnapDir)) - } - }) - It("should garbage collect limitBased", func() { now := time.Now().UTC() store, snapstoreConfig := prepareStoreForGarbageCollection(now, "garbagecollector_limit_based.bkp", "v2") @@ -516,34 +427,6 @@ var _ = Describe("Snapshotter", func() { } }) - // Test to check backward compatibility of garbage collector - // Tests garbage collector behaviour (in limit based config) when only v1 directory is present - // TODO: Consider removing when backward compatibility no longer needed - It("should garbage collect limitBased with only v1 dir structure present (backward compatible test)", func() { - now := time.Now().UTC() - store, snapstoreConfig := prepareStoreForGarbageCollection(now, "gc_limit_based_backward_compatiblev1.bkp", "v1") - snapshotterConfig := &brtypes.SnapshotterConfig{ - FullSnapshotSchedule: schedule, - DeltaSnapshotPeriod: wrappers.Duration{Duration: 10 * time.Second}, - DeltaSnapshotMemoryLimit: brtypes.DefaultDeltaSnapMemoryLimit, - GarbageCollectionPeriod: wrappers.Duration{Duration: garbageCollectionPeriod}, - GarbageCollectionPolicy: brtypes.GarbageCollectionPolicyLimitBased, - MaxBackups: maxBackups, - } - - ssr, err := NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - gcCtx, cancel := context.WithTimeout(testCtx, testTimeout) - defer cancel() - ssr.RunGarbageCollector(gcCtx.Done()) - - list, err := store.List() - Expect(err).ShouldNot(HaveOccurred()) - - validateLimitBasedSnapshots(list, maxBackups, snapsInV1) - }) - Describe("###GarbageCollectDeltaSnapshots", func() { const ( deltaSnapshotCount = 6 @@ -699,7 +582,7 @@ var _ = Describe("Snapshotter", func() { }) Context("no previous snapshots, first snapshot upload is underway with few chunks uploaded", func() { It("should not delete any chunks", func() { - // Add 4 chunks of kind Full with startRevision=0, lastRevision=1 + // Add 4 chunks of kind Full, with startRevision=0, lastRevision=1 err := addObjectsToStore(store, "Chunk", "Full", 0, 1, 4, time.Now()) Expect(err).NotTo(HaveOccurred()) @@ -761,7 +644,7 @@ var _ = Describe("Snapshotter", func() { }) Context("previous snapshot upload is complete, current snapshot upload underway", func() { It("should not delete chunks of currently uploading snapshot", func() { - // add 5 chunks of kind Full with startRevision = 1, lastRevision = 3 which corresponds to the below full snapshot + // add 5 chunks of kind Full, with startRevision = 1, lastRevision = 3 which corresponds to the below full snapshot err := addObjectsToStore(store, "Chunk", "Full", 1, 3, 5, time.Now()) Expect(err).NotTo(HaveOccurred()) // add the full snapshot with startRevision = 1, lastRevision = 3 @@ -911,7 +794,7 @@ var _ = Describe("Snapshotter", func() { ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) - // Previous full snapshot was taken 1 days before at exactly at scheduled time + // Previous full snapshot was taken 1 day before at exactly at scheduled time ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-1, (currentHour+2)%24, (currentMin+1)%60, 0, 0, time.Local), } @@ -1022,11 +905,11 @@ var _ = Describe("Snapshotter", func() { }) }) -// prepareExpectedSnapshotsList prepares the expected snapshotlist based on directory structure +// prepareExpectedSnapshotsList prepares the expected snapshot list based on directory structure func prepareExpectedSnapshotsList(snapTime time.Time, now time.Time, expectedSnapList brtypes.SnapList, directoryStruct string) brtypes.SnapList { // weekly snapshot for i := 1; i <= 4; i++ { - snapTime = snapTime.Add(time.Duration(time.Hour * 24 * 7)) + snapTime = snapTime.Add(time.Hour * 24 * 7) snap := &brtypes.Snapshot{ Kind: brtypes.SnapshotKindFull, CreatedOn: snapTime, @@ -1045,7 +928,7 @@ func prepareExpectedSnapshotsList(snapTime time.Time, now time.Time, expectedSna // daily snapshot // in case of mixed directory structure, we would list snaps of first 3 days from v1 structure and rest of all snaps from v2 structure for i := 1; i <= 7; i++ { - snapTime = snapTime.Add(time.Duration(time.Hour * 24)) + snapTime = snapTime.Add(time.Hour * 24) snap := &brtypes.Snapshot{ Kind: brtypes.SnapshotKindFull, CreatedOn: snapTime, @@ -1066,7 +949,7 @@ func prepareExpectedSnapshotsList(snapTime time.Time, now time.Time, expectedSna fmt.Println("Daily snapshot list prepared") // hourly snapshot - snapTime = snapTime.Add(time.Duration(time.Hour)) + snapTime = snapTime.Add(time.Hour) for now.Truncate(time.Hour).Sub(snapTime) > 0 { snap := &brtypes.Snapshot{ Kind: brtypes.SnapshotKindFull, @@ -1079,7 +962,7 @@ func prepareExpectedSnapshotsList(snapTime time.Time, now time.Time, expectedSna snap.GenerateSnapshotDirectory() } expectedSnapList = append(expectedSnapList, snap) - snapTime = snapTime.Add(time.Duration(time.Hour)) + snapTime = snapTime.Add(time.Hour) } fmt.Println("Hourly snapshot list prepared") @@ -1096,7 +979,7 @@ func prepareExpectedSnapshotsList(snapTime time.Time, now time.Time, expectedSna snap.GenerateSnapshotDirectory() } expectedSnapList = append(expectedSnapList, snap) - snapTime = snapTime.Add(time.Duration(time.Minute * 30)) + snapTime = snapTime.Add(time.Minute * 30) for now.Sub(snapTime) >= 0 { snap := &brtypes.Snapshot{ Kind: brtypes.SnapshotKindFull, @@ -1109,13 +992,13 @@ func prepareExpectedSnapshotsList(snapTime time.Time, now time.Time, expectedSna snap.GenerateSnapshotDirectory() } expectedSnapList = append(expectedSnapList, snap) - snapTime = snapTime.Add(time.Duration(time.Minute * 30)) + snapTime = snapTime.Add(time.Minute * 30) } fmt.Println("Current hour full snapshot list prepared") // delta snapshots - snapTime = snapTime.Add(time.Duration(-time.Minute * 30)) - snapTime = snapTime.Add(time.Duration(time.Minute * 10)) + snapTime = snapTime.Add(-time.Minute * 30) + snapTime = snapTime.Add(time.Minute * 10) for now.Sub(snapTime) >= 0 { snap := &brtypes.Snapshot{ Kind: brtypes.SnapshotKindDelta, @@ -1128,7 +1011,7 @@ func prepareExpectedSnapshotsList(snapTime time.Time, now time.Time, expectedSna snap.GenerateSnapshotDirectory() } expectedSnapList = append(expectedSnapList, snap) - snapTime = snapTime.Add(time.Duration(time.Minute * 10)) + snapTime = snapTime.Add(time.Minute * 10) } fmt.Println("Incremental snapshot list prepared") return expectedSnapList @@ -1169,102 +1052,12 @@ func prepareStoreForGarbageCollection(forTime time.Time, storeContainer string, if storePrefix == "v1" { snap.GenerateSnapshotDirectory() } - snapTime = snapTime.Add(time.Duration(time.Minute * 10)) - store.Save(snap, io.NopCloser(strings.NewReader(fmt.Sprintf("dummy-snapshot-content for snap created on %s", snap.CreatedOn)))) + snapTime = snapTime.Add(time.Minute * 10) + Expect(store.Save(snap, io.NopCloser(strings.NewReader(fmt.Sprintf("dummy-snapshot-content for snap created on %s", snap.CreatedOn))))).ShouldNot(HaveOccurred()) } return store, snapstoreConf } -// prepareStoreForBackwardCompatibleGC populates the store with dummy snapshots in both v1 and v2 drectory structures for backward compatible garbage collection tests -// Tied up with backward compatibility tests -// TODO: Consider removing when backward compatibility no longer needed -func prepareStoreForBackwardCompatibleGC(forTime time.Time, storeContainer string) (brtypes.SnapStore, *brtypes.SnapstoreConfig) { - var ( - // Divide the forTime into two period. First period is during when snapshots would be collected in v1 and second period is when snapshots would be collected in v2. - snapTimev1 = time.Date(forTime.Year(), forTime.Month(), forTime.Day()-36, 0, 0, 0, 0, forTime.Location()) - snapTimev2 = time.Date(forTime.Year(), forTime.Month(), forTime.Day()-4, 0, 0, 0, 0, forTime.Location()) - count = 0 - noOfDeltaSnapshots = 3 - ) - fmt.Println("setting up garbage collection test") - // Prepare store - snapstoreConfig := &brtypes.SnapstoreConfig{Container: path.Join(outputDir, storeContainer), Prefix: "v1"} - store, err := snapstore.GetSnapstore(snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - for snapTimev2.Sub(snapTimev1) >= 0 { - var kind = brtypes.SnapshotKindDelta - if count == 0 { - kind = brtypes.SnapshotKindFull - } - count = (count + 1) % noOfDeltaSnapshots - snap := brtypes.Snapshot{ - Kind: kind, - CreatedOn: snapTimev1, - StartRevision: 0, - LastRevision: 1001, - } - snap.GenerateSnapshotName() - snap.GenerateSnapshotDirectory() - snapTimev1 = snapTimev1.Add(time.Duration(time.Minute * 10)) - store.Save(snap, io.NopCloser(strings.NewReader(fmt.Sprintf("dummy-snapshot-content for snap created on %s", snap.CreatedOn)))) - } - - count = 0 - // Prepare store - snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, storeContainer), Prefix: "v2"} - store, err = snapstore.GetSnapstore(snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - for forTime.Sub(snapTimev2) >= 0 { - var kind = brtypes.SnapshotKindDelta - if count == 0 { - kind = brtypes.SnapshotKindFull - } - count = (count + 1) % noOfDeltaSnapshots - snapv2 := brtypes.Snapshot{ - Kind: kind, - CreatedOn: snapTimev2, - StartRevision: 0, - LastRevision: 1001, - } - snapv2.GenerateSnapshotName() - snapTimev2 = snapTimev2.Add(time.Duration(time.Minute * 10)) - store.Save(snapv2, io.NopCloser(strings.NewReader(fmt.Sprintf("dummy-snapshot-content for snap created on %s", snapv2.CreatedOn)))) - } - return store, snapstoreConfig -} - -// validateLimitBasedSnapshots verifies whether the snapshot list after being garbage collected using the limit-based configuration is a valid snapshot list -func validateLimitBasedSnapshots(list brtypes.SnapList, maxBackups uint, mode string) { - incr := false - fullSnapCount := 0 - for _, snap := range list { - if incr == false { - if snap.Kind == brtypes.SnapshotKindDelta { - //Indicates that no full snapshot can occur after a incr snapshot in an already garbage collected list - incr = true - } else { - //Number of full snapshots in garbage collected list cannot be more than the maxBackups configuration - fullSnapCount++ - Expect(fullSnapCount).Should(BeNumerically("<=", maxBackups)) - } - if mode == snapsInV2 { - Expect(snap.SnapDir).Should(Equal("")) - } else if mode == snapsInV1 { - Expect(snap.SnapDir).Should(ContainSubstring("Backup")) - } - } else { - Expect(snap.Kind).Should(Equal(brtypes.SnapshotKindDelta)) - if mode == snapsInV2 { - Expect(snap.SnapDir).Should(Equal("")) - } else if mode == snapsInV1 { - Expect(snap.SnapDir).Should(ContainSubstring("Backup")) - } - } - } -} - /* prepareStoreWithDeltaSnapshots prepares a snapshot store with a specified number of delta snapshots. @@ -1293,8 +1086,8 @@ func prepareStoreWithDeltaSnapshots(storeContainer string, numDeltaSnapshots int LastRevision: int64(i+1) * 10, } snap.GenerateSnapshotName() - snapTime = snapTime.Add(time.Duration(time.Minute * 10)) - store.Save(snap, io.NopCloser(strings.NewReader(fmt.Sprintf("dummy-snapshot-content for snap created on %s", snap.CreatedOn)))) + snapTime = snapTime.Add(time.Minute * 10) + Expect(store.Save(snap, io.NopCloser(strings.NewReader(fmt.Sprintf("dummy-snapshot-content for snap created on %s", snap.CreatedOn))))).ShouldNot(HaveOccurred()) } return store From 21c5a2d6a0002649da572ee51735be9562d247c9 Mon Sep 17 00:00:00 2001 From: Shreyas Rao Date: Tue, 9 Jan 2024 13:49:46 +0530 Subject: [PATCH 3/5] Fix unit tests --- pkg/snapstore/snapstore_test.go | 2 +- pkg/snapstore/swift_snapstore.go | 38 ++++++++++++++++----------- pkg/snapstore/swift_snapstore_test.go | 9 ++++--- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/pkg/snapstore/snapstore_test.go b/pkg/snapstore/snapstore_test.go index dc96dd434..38b68e079 100644 --- a/pkg/snapstore/snapstore_test.go +++ b/pkg/snapstore/snapstore_test.go @@ -109,7 +109,7 @@ var _ = Describe("Save, List, Fetch, Delete from mock snapstore", func() { prefix: prefixV2, multiPartUploads: map[string]*[][]byte{}, }), - "swift": NewSwiftSnapstoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, fake.ServiceClient()), + "swift": NewSwiftSnapstoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, fake.ServiceClient(), false), "ABS": newFakeABSSnapstore(), "GCS": NewGCSSnapStoreFromClient(bucket, prefixV2, "/tmp", 5, brtypes.MinChunkSize, &mockGCSClient{ objects: objectMap, diff --git a/pkg/snapstore/swift_snapstore.go b/pkg/snapstore/swift_snapstore.go index 6efa8e066..f86baf3df 100644 --- a/pkg/snapstore/swift_snapstore.go +++ b/pkg/snapstore/swift_snapstore.go @@ -56,6 +56,10 @@ type SwiftSnapStore struct { maxParallelChunkUploads uint minChunkSize int64 tempDir string + // deleteChunksForObject decides whether deletion of a (manifest) object should also delete the + // associated chunks (segment objects). + // Default: true + deleteChunksForObject bool } type applicationCredential struct { @@ -110,10 +114,9 @@ func NewSwiftSnapStore(config *brtypes.SnapstoreConfig) (*SwiftSnapStore, error) }) if err != nil { return nil, err - } - return NewSwiftSnapstoreFromClient(config.Container, config.Prefix, config.TempDir, config.MaxParallelChunkUploads, config.MinChunkSize, client), nil + return NewSwiftSnapstoreFromClient(config.Container, config.Prefix, config.TempDir, config.MaxParallelChunkUploads, config.MinChunkSize, client, true), nil } @@ -313,7 +316,7 @@ func readSwiftCredentialDir(dirName string) (*swiftCredentials, error) { } // NewSwiftSnapstoreFromClient will create the new Swift snapstore object from Swift client -func NewSwiftSnapstoreFromClient(bucket, prefix, tempDir string, maxParallelChunkUploads uint, minChunkSize int64, cli *gophercloud.ServiceClient) *SwiftSnapStore { +func NewSwiftSnapstoreFromClient(bucket, prefix, tempDir string, maxParallelChunkUploads uint, minChunkSize int64, cli *gophercloud.ServiceClient, deleteChunksForObject bool) *SwiftSnapStore { return &SwiftSnapStore{ bucket: bucket, prefix: prefix, @@ -321,6 +324,7 @@ func NewSwiftSnapstoreFromClient(bucket, prefix, tempDir string, maxParallelChun maxParallelChunkUploads: maxParallelChunkUploads, minChunkSize: minChunkSize, tempDir: tempDir, + deleteChunksForObject: deleteChunksForObject, } } @@ -508,23 +512,25 @@ func (s *SwiftSnapStore) getSnapshotChunks(snapshot brtypes.Snapshot) (brtypes.S // This includes the manifest object as well as the segment objects, as // described in https://docs.openstack.org/swift/latest/overview_large_objects.html func (s *SwiftSnapStore) Delete(snap brtypes.Snapshot) error { - chunks, err := s.getSnapshotChunks(snap) - if err != nil { - return err - } + if s.deleteChunksForObject { + chunks, err := s.getSnapshotChunks(snap) + if err != nil { + return err + } - var chunkObjectNames []string - for _, chunk := range chunks { - chunkObjectNames = append(chunkObjectNames, path.Join(chunk.Prefix, chunk.SnapDir, chunk.SnapName)) - } + var chunkObjectNames []string + for _, chunk := range chunks { + chunkObjectNames = append(chunkObjectNames, path.Join(chunk.Prefix, chunk.SnapDir, chunk.SnapName)) + } - manifestObjectDeleteResult := objects.Delete(s.client, s.bucket, path.Join(snap.Prefix, snap.SnapDir, snap.SnapName), nil) - if manifestObjectDeleteResult.Err != nil { - return manifestObjectDeleteResult.Err + chunkObjectsDeleteResult := objects.BulkDelete(s.client, s.bucket, chunkObjectNames) + if chunkObjectsDeleteResult.Err != nil { + return chunkObjectsDeleteResult.Err + } } - chunkObjectsDeleteResult := objects.BulkDelete(s.client, s.bucket, chunkObjectNames) - return chunkObjectsDeleteResult.Err + manifestObjectDeleteResult := objects.Delete(s.client, s.bucket, path.Join(snap.Prefix, snap.SnapDir, snap.SnapName), nil) + return manifestObjectDeleteResult.Err } // GetSwiftCredentialsLastModifiedTime returns the latest modification timestamp of the Swift credential file(s) diff --git a/pkg/snapstore/swift_snapstore_test.go b/pkg/snapstore/swift_snapstore_test.go index 69db171be..1e341ddc2 100644 --- a/pkg/snapstore/swift_snapstore_test.go +++ b/pkg/snapstore/swift_snapstore_test.go @@ -70,7 +70,9 @@ func handleCreateTextObject(w http.ResponseWriter, r *http.Request) { logrus.Errorf("object name cannot be empty") w.WriteHeader(http.StatusBadRequest) } + if len(r.Header.Get("X-Object-Manifest")) == 0 { + // segment object buf := new(bytes.Buffer) if _, err = io.Copy(buf, r.Body); err != nil { logrus.Errorf("failed to read content %v", err) @@ -78,12 +80,13 @@ func handleCreateTextObject(w http.ResponseWriter, r *http.Request) { return } content = buf.Bytes() + objectMapMutex.Lock() + objectMap[key] = &content + objectMapMutex.Unlock() } else { content = make([]byte, 0) } - objectMapMutex.Lock() - objectMap[key] = &content - objectMapMutex.Unlock() + hash := md5.New() io.WriteString(hash, string(content)) localChecksum := hash.Sum(nil) From f0dbb046c78495732ce9123f6a37e3272dd2f16f Mon Sep 17 00:00:00 2001 From: Shreyas Rao Date: Tue, 9 Jan 2024 14:17:23 +0530 Subject: [PATCH 4/5] Address review comments from @ishan16696 --- pkg/snapstore/swift_snapstore.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/snapstore/swift_snapstore.go b/pkg/snapstore/swift_snapstore.go index f86baf3df..587c98ce5 100644 --- a/pkg/snapstore/swift_snapstore.go +++ b/pkg/snapstore/swift_snapstore.go @@ -523,14 +523,13 @@ func (s *SwiftSnapStore) Delete(snap brtypes.Snapshot) error { chunkObjectNames = append(chunkObjectNames, path.Join(chunk.Prefix, chunk.SnapDir, chunk.SnapName)) } - chunkObjectsDeleteResult := objects.BulkDelete(s.client, s.bucket, chunkObjectNames) - if chunkObjectsDeleteResult.Err != nil { + if chunkObjectsDeleteResult := objects.BulkDelete(s.client, s.bucket, chunkObjectNames); chunkObjectsDeleteResult.Err != nil { return chunkObjectsDeleteResult.Err } } - manifestObjectDeleteResult := objects.Delete(s.client, s.bucket, path.Join(snap.Prefix, snap.SnapDir, snap.SnapName), nil) - return manifestObjectDeleteResult.Err + // delete manifest object + return objects.Delete(s.client, s.bucket, path.Join(snap.Prefix, snap.SnapDir, snap.SnapName), nil).Err } // GetSwiftCredentialsLastModifiedTime returns the latest modification timestamp of the Swift credential file(s) From cd8debbc7dcd16e72f5d04b7ce504f8fdf5dca5e Mon Sep 17 00:00:00 2001 From: Shreyas Rao Date: Tue, 9 Jan 2024 16:07:52 +0530 Subject: [PATCH 5/5] Address review comments from @anveshreddy18 --- pkg/snapstore/swift_snapstore.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/snapstore/swift_snapstore.go b/pkg/snapstore/swift_snapstore.go index 587c98ce5..772089d3c 100644 --- a/pkg/snapstore/swift_snapstore.go +++ b/pkg/snapstore/swift_snapstore.go @@ -518,14 +518,17 @@ func (s *SwiftSnapStore) Delete(snap brtypes.Snapshot) error { return err } - var chunkObjectNames []string - for _, chunk := range chunks { - chunkObjectNames = append(chunkObjectNames, path.Join(chunk.Prefix, chunk.SnapDir, chunk.SnapName)) - } + if len(chunks) > 0 { + var chunkObjectNames []string + for _, chunk := range chunks { + chunkObjectNames = append(chunkObjectNames, path.Join(chunk.Prefix, chunk.SnapDir, chunk.SnapName)) + } - if chunkObjectsDeleteResult := objects.BulkDelete(s.client, s.bucket, chunkObjectNames); chunkObjectsDeleteResult.Err != nil { - return chunkObjectsDeleteResult.Err + if chunkObjectsDeleteResult := objects.BulkDelete(s.client, s.bucket, chunkObjectNames); chunkObjectsDeleteResult.Err != nil { + return chunkObjectsDeleteResult.Err + } } + } // delete manifest object