Added tests for compaction backward compatibility #1

aaronfern · 2021-06-04T05:58:39Z

What this PR does / why we need it:
Adds local store test cases to verify the backward compatibility of compaction with respect to garbage collection and restorer

Which issue(s) this PR fixes:
Partially Fixes gardener/etcd-druid#88

Special notes for your reviewer:

Release note:

NONE

abdasgupta · 2021-06-04T07:56:04Z

pkg/snapshot/restorer/restorer_test.go

@@ -645,6 +662,222 @@ var _ = Describe("Running Restorer", func() {

 	})

+	Describe("NEGATIVE:For scenarios involving both old as well as updated directory structures being present", func() {


Why did you mark it as NEGATIVE ? It should be like other test cases.

Removed the NEGATIVE tag

amshuman-kr

Thanks @aaronfern for the test cases! Seems to be already a very good first cut. A few suggestions and questions below.

amshuman-kr · 2021-06-05T06:16:46Z

pkg/snapshot/restorer/restorer_suite_test.go

@@ -51,7 +52,9 @@ func TestRestorer(t *testing.T) {
 	RunSpecs(t, "Restorer Suite")
 }

-var _ = SynchronizedBeforeSuite(func() []byte {
+var _ = SynchronizedBeforeSuite(prepare, func(data []byte) {})


What is the purpose of the second empty func?

It's actually part of the syntax of the SynchronizedBeforeSuite call, it needs two functions as arguments.
However as I was adding a couple of test cases, I realised that this change wasn't necessarily required. Hence this change has been reverted.

amshuman-kr · 2021-06-05T10:02:02Z

pkg/snapshot/restorer/restorer_test.go

+	files, err := ioutil.ReadDir(path.Join(snapstoreDir, "v2"))
+	Expect(err).ShouldNot(HaveOccurred())
+	oldPath := path.Join(snapstoreDir, "v2")
+	newPath := path.Join(path.Join(snapstoreDir, "v1"), fmt.Sprintf("Backup-%d", curTime))
+	for _, f := range files {
+		err = os.Rename(path.Join(oldPath, f.Name()), path.Join(newPath, f.Name()))
+		Expect(err).ShouldNot(HaveOccurred())
+	}


I like this approach of taking snapshots into v2 and then moving to v1. This is a good way to test backward compatibility for restoration when we have stopped support for uploading to v1 👍

The code above moves everything under v2 to v1/Backup-XXX, including the snapshots taken before this function is called which would include the base full snapshot. This has the good effect that restoration from base snapshot from the pure v1 structure is tested.

To test the mixed case, could you please support the case where the first few snapshots (full snapshot and some incremental snapshots) are in the v1/Backup-XXX and some more incremental snapshots (at least one) are in v2. This could possibly be controlled via a parameter to the function.

Added a test case to support this scenario

amshuman-kr · 2021-06-05T10:04:13Z

pkg/snapshot/restorer/restorer_test.go

+		//Test to check backward compatibility of restorer
+		//Tests restorer behaviour when local database has to be restored from snapstore with old (v1) as well as updated (v2) directory structures
+		//TODO: Consider removing when backward compatibility no longer needed
+		Context("With snapshots in v1 as well as v2 dir", func() {


There is another mixed case where the first few snapshots (full and some incremental) are in v1 structure and the remaining (at least one) are in v2 format. Restoration from this also should succeed. Can you please add this case too?

For completeness, there is the opposite case too where the first few snapshots (including the full snapshot) are in v2 structure and the remaining ones are in v1 structure. Strictly speaking, this should never happen but could be possible if we are forced to revert the folder structure change release of etcd-backup-restore for some reason. Covering this case, might protect against that case too.

Added a test case to support this scenario

amshuman-kr · 2021-06-05T10:23:15Z

pkg/snapshot/snapshotter/snapshotter_test.go

+					snap := &brtypes.Snapshot{
+						Kind:          brtypes.SnapshotKindFull,
+						CreatedOn:     snapTime,
+						StartRevision: 0,
+						LastRevision:  1001,
+					}


After the garbage collection, is the whole store expected to be in the v2 format? This is fine, I am just asking because I did not find SnapDir field being initialised for any of the expected snapshots.

Also, is there a commonality in the initialisation of the expected snapshot lists between the cases? If so, can you please convert the logic into a function and reuse?

No, not really. Snaps that are not garbage collected would remain in their original directory.
Added changes to support SnapDir in the expected snapshots and using that to decide test case result

amshuman-kr

Thanks for the changes @aaronfern! Looks good now. Just a few more comments.

amshuman-kr · 2021-06-08T04:49:13Z

pkg/snapshot/restorer/restorer_test.go

+
+//takeValidV1Snaps saves valid snaps in the v1 prefix dir of snapstore so that restorer could restore from them
+//TODO: Consider removing when backward compatibility no longer needed
+func takeValidV1Snaps(logger *logrus.Entry, container string, deltaSnapshotPeriod time.Duration, endpoints []string, stopCh <-chan struct{}, startWithFullSnapshot bool, compressionConfig *compressor.CompressionConfig, mode string) error {


Is startWithFullSnapshot required here? It doesn't seem to be used.

amshuman-kr · 2021-06-08T04:49:22Z

pkg/snapshot/restorer/restorer_test.go

+
+//takeInvalidV1Snaps saves an invalid snap in the v1 prefix dir of the snapstore so that restorer can't restore from it
+//TODO: Consider removing when backward compatibility no longer needed
+func takeInvalidV1Snaps(logger *logrus.Entry, container string, deltaSnapshotPeriod time.Duration, endpoints []string, stopCh <-chan struct{}, startWithFullSnapshot bool, compressionConfig *compressor.CompressionConfig) error {


Is startWithFullSnapshot required here? It doesn't seem to be used.

amshuman-kr · 2021-06-08T04:50:47Z

pkg/snapshot/restorer/restorer_test.go

+	allSnapsInv1 = "allSnapsInv1"
+	fullSnapInv1 = "fullSnapInv1"
+	fullSnapInv2 = "fullSnapInv2"


Better to name these constants xxxInV1 or xxxInV2 with capital V otherwise Inv might be confused to mean invalid.

amshuman-kr · 2021-06-08T04:51:18Z

pkg/snapshot/snapshotter/snapshotter_test.go

+	snapsInv1 = "v1"
+	snapsInv2 = "v2"


Better to use caps V in the constant names as mentioned above.

amshuman-kr · 2021-06-08T04:55:23Z

pkg/snapshot/snapshotter/snapshotter_test.go

+				for _, snap := range list {
+					if incr == false {
+						if snap.Kind == brtypes.SnapshotKindDelta {
+							incr = true


If I understood right, the snapshots are prepared in such a away that everything that does not have incremental snapshots should be in v1 structure and everything that has incremental snapshots should be in v2 structure?

Possibly there is a commonality between this case and the next too from the expectation perspective? The only difference seems to be the SnapDir check. Or am I oversimplifying it?

Moved this to a function

The logic there is to make sure that no full snaps are present in the list after a incr snap post garbage collection. The SnapDir check is done based on the test itself, and that's passed to the func in the form of mode now.

amshuman-kr · 2021-06-08T05:00:06Z

pkg/snapshot/snapshotter/snapshotter_test.go

+		// if mode == "v1" {
+		// 	dir = fmt.Sprintf("Backup-%d", snapTime.Unix())
+		// } else {
+		// 	dir = ""
+		// }


Please remove these lines if not required.

amshuman-kr

Thanks for the changes @aaronfern! Looks good.

One last change to make debugging failed tests. Sorry, I missed it in the laster review.

amshuman-kr · 2021-06-08T11:16:09Z

pkg/snapshot/snapshotter/snapshotter_test.go

+					if snap.CreatedOn != expectedSnapList[index].CreatedOn || snap.Kind != expectedSnapList[index].Kind || snap.SnapDir != expectedSnapList[index].SnapDir {
+						Fail("Expected snap list doesn't match with output snap list")
+					}


Might be better to change this section to 3 Expects for the corresponding fields. This will help debug when the tests fail.

The same holds in other cases below.

amshuman-kr

Thanks for the changes @aaronfern!

/lgtm

abdasgupta reviewed Jun 4, 2021

View reviewed changes

aaronfern force-pushed the working branch from 8bbb08b to 76f3624 Compare June 5, 2021 04:17

amshuman-kr suggested changes Jun 5, 2021

View reviewed changes

aaronfern force-pushed the working branch from 76f3624 to ddf0a48 Compare June 7, 2021 11:36

amshuman-kr suggested changes Jun 8, 2021

View reviewed changes

aaronfern force-pushed the working branch from ddf0a48 to 5cd41de Compare June 8, 2021 07:31

amshuman-kr suggested changes Jun 8, 2021

View reviewed changes

aaronfern force-pushed the working branch from 5cd41de to f77b48f Compare June 8, 2021 15:04

Added tests for compaction backward compatibility

bf01c49

aaronfern force-pushed the working branch from f77b48f to bf01c49 Compare June 8, 2021 15:07

amshuman-kr approved these changes Jun 9, 2021

View reviewed changes

abdasgupta merged commit bb6dc59 into abdasgupta:working Jun 9, 2021

aaronfern deleted the working branch June 28, 2021 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for compaction backward compatibility #1

Added tests for compaction backward compatibility #1

aaronfern commented Jun 4, 2021

abdasgupta Jun 4, 2021

aaronfern Jun 7, 2021

amshuman-kr left a comment

amshuman-kr Jun 5, 2021

aaronfern Jun 7, 2021

amshuman-kr Jun 5, 2021 •

edited

Loading

aaronfern Jun 7, 2021

amshuman-kr Jun 5, 2021

aaronfern Jun 7, 2021

amshuman-kr Jun 5, 2021

aaronfern Jun 7, 2021

amshuman-kr left a comment

amshuman-kr Jun 8, 2021

aaronfern Jun 8, 2021

amshuman-kr Jun 8, 2021

aaronfern Jun 8, 2021

amshuman-kr Jun 8, 2021

aaronfern Jun 8, 2021

amshuman-kr Jun 8, 2021

aaronfern Jun 8, 2021

amshuman-kr Jun 8, 2021

amshuman-kr Jun 8, 2021

aaronfern Jun 8, 2021

amshuman-kr Jun 8, 2021

aaronfern Jun 8, 2021

amshuman-kr left a comment

amshuman-kr Jun 8, 2021

aaronfern Jun 8, 2021

amshuman-kr left a comment

		@@ -645,6 +662,222 @@ var _ = Describe("Running Restorer", func() {

		})

		Describe("NEGATIVE:For scenarios involving both old as well as updated directory structures being present", func() {

Added tests for compaction backward compatibility #1

Added tests for compaction backward compatibility #1

Conversation

aaronfern commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr Jun 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

amshuman-kr Jun 5, 2021 •

edited

Loading