HDDS-8207. [Snapshot] Fix bugs and add tests for SnapshotDeletingService. #4571

aswinshakil · 2023-04-14T20:30:50Z

What changes were proposed in this pull request?

Add tests for SnapshotDeletingService to cover all the Snapshot GC code paths.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8207

How was this patch tested?

This is a test change.

GeorgeJahad · 2023-04-19T23:27:06Z

It looks like checkDirReclaimable() returns true if the dir is reclaimable, but checkKeyReclaimable() returns false if the key is reclaimable. Is that correct?

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java

Lines 452 to 459 in 9cd92f4

    
           private boolean checkDirReclaimable( 
        
               Table.KeyValue<String, OmKeyInfo> deletedDir, 
        
               Table<String, OmDirectoryInfo> previousDirTable) throws IOException { 
        
             if (previousDirTable == null) { 
        
               return true; 
        
             }

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java

Lines 474 to 485 in 9cd92f4

    
           private boolean checkKeyReclaimable( 
        
               Table<String, OmKeyInfo> previousKeyTable, 
        
               Table<String, String> renamedKeyTable, 
        
               OmKeyInfo deletedKeyInfo, OmBucketInfo bucketInfo, 
        
               long volumeId, HddsProtos.KeyValue.Builder renamedKeyBuilder) 
        
               throws IOException { 
        
             String dbKey; 
        
             // Handle case when the deleted snapshot is the first snapshot. 
        
             if (previousKeyTable == null) { 
        
               return false; 
        
             }

GeorgeJahad · 2023-04-19T23:42:02Z

Do we need to change the snapshotInfo status to SNAPSHOT_RECLAIMED here?

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java

Line 282 in 9cd92f4

successRunCount.incrementAndGet();

or maybe here:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java

Lines 182 to 184 in 9cd92f4

    
           if (checkSnapshotReclaimable(snapshotDeletedTable, 
        
               snapshotDeletedDirTable, snapshotBucketKey, dbBucketKeyForDir)) { 
        
             purgeSnapshotKeys.add(snapInfo.getTableKey());

GeorgeJahad · 2023-04-19T23:48:32Z

I'd like to see unit tests that excercise all the if/else paths in the following methods:

checkSnapshotReclaimable
checkDirReclaimable
checkKeyReclaimable
handleDirectoryCleanUp

In addition, I'd like a unit test that confirms that we are correctly starting and stopping within the bucket scope.

aswinshakil · 2023-04-21T17:02:58Z

It looks like checkDirReclaimable() returns true if the dir is reclaimable, but checkKeyReclaimable() returns false if the key is reclaimable. Is that correct?

Yes you are correct. I'm going to make this consistent in the following patches or in this patch.

aswinshakil · 2023-04-21T17:05:46Z

Do we need to change the snapshotInfo status to SNAPSHOT_RECLAIMED here?

Do we need to store intermediate result here? Because the Snapshot is removed from the snapshotInfoTable in the next step immediately.

I'd like to see unit tests that excercise all the if/else paths in the following methods:

checkSnapshotReclaimable
checkDirReclaimable
checkKeyReclaimable
handleDirectoryCleanUp

In addition, I'd like a unit test that confirms that we are correctly starting and stopping within the bucket scope.

Sure I will add more tests to this patch.

GeorgeJahad · 2023-04-21T17:25:41Z

Do we need to store intermediate result here? Because the Snapshot is removed from the snapshotInfoTable in the next step immediately.

That is a good question. So maybe we can remove the RECLAIMED status from the enum, if it isn't used anywhere?

GeorgeJahad · 2023-04-21T17:28:27Z

Yes you are correct. I'm going to make this consistent in the following patches or in this patch.

Thanks, I found it very confusing.

...e/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java

smengcl · 2023-05-18T20:05:17Z

...ager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java

+      // If both next global and path snapshot are same, it may overwrite
+      // nextPathSnapInfo.setPathPreviousSnapshotID(), adding this check
+      // will prevent it.
+      if (nextGlobalSnapInfo != null && nextGlobalSnapInfo.getSnapshotID()
+          .equals(nextPathSnapInfo.getSnapshotID())) {
+        nextPathSnapInfo.setGlobalPreviousSnapshotID(
+            snapInfo.getPathPreviousSnapshotID());
+        metadataManager.getSnapshotInfoTable().putWithBatch(batchOperation,
+            nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
+      } else if (nextGlobalSnapInfo != null) {


In this case described in the comment, we need to update both nextPathSnapInfo and nextGlobalSnapInfo right? if so we shouldn't use else if imo

The reason for using else if because nextGlobalSnapInfo can be null if we are deleting the last snapshot, so we need that check.

smengcl · 2023-05-18T20:08:14Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java


-  public static void checkSnapshotActive(SnapshotInfo snapInfo)
+  public static void checkSnapshotActive(SnapshotInfo snapInfo,
+                                         boolean override)


Suggested change

boolean override)

boolean skipCheck)

Tho it sounds a bit weird to have a param to skip the check for the check method. If we fix the caller, how many places do we have to change? If not many we should do that instead.

I initially thought about this, but it looks like we should be changing in many places. I kept on getting Exceptions from multiple request.

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

...e/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java

smengcl · 2023-05-19T00:42:16Z

...e/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java

+    BucketArgs bucketArgs = new BucketArgs.Builder()
+        .setBucketLayout(BucketLayout.LEGACY)
+        .build();


This should works for all 3 types of buckets? Parameterize this later?

For FSO there is a separate test. Maybe we can parameterize it later. Anyways I'm planning on adding more tests in a separate JIRA which George requested for.

smengcl

Thanks @aswinshakil for the fixes and new tests. Looks good mostly.

smengcl · 2023-05-24T00:39:18Z

Looks like it's timing out on a relevant test:

https://github.com/apache/ozone/actions/runs/5060501663/jobs/9088775175

 Tests run: 3, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 209.808 s <<< FAILURE! - in org.apache.hadoop.fs.ozone.TestSnapshotDeletingService
Error:  org.apache.hadoop.fs.ozone.TestSnapshotDeletingService.testSnapshotWithFSO  Time elapsed: 158.164 s  <<< ERROR!
java.util.concurrent.TimeoutException: 
Timed out waiting for condition. Thread diagnostics:
Timestamp: 2023-05-24 12:20:12,141

"ContainerOp-2104f031-94b1-4091-a926-8a477d189e0d-9"  prio=5 tid=493 in Object.wait()
java.lang.Thread.State: WAITING (on object monitor)
        at sun.misc.Unsafe.park(Native Method)

This could be related to the bug @hemantk-12 just pointed out:

#4244 (comment)

aswinshakil · 2023-05-24T17:42:45Z

@smengcl and @hemantk-12 for pointing out, I have fixed it. The tests doesn't look like it's failing on this bug. I checked the log and it is failing on checkpoint creation.

2023-05-23 19:10:33,535 [OMDoubleBufferFlushThread] ERROR db.RDBCheckpointManager (RDBCheckpointManager.java:createCheckpoint(101)) - Unable to create RocksDB Snapshot.
java.io.IOException: org.apache.hadoop.hdds.utils.db.RocksDatabase$RocksCheckpoint@36eabcd1: Failed to createCheckpoint /home/runner/work/ozone/ozone/hadoop-ozone/integration-test/target/test-dir/MiniOzoneClusterImpl-f735872c-86ff-4001-aead-24a4ea2dd355/ozone-meta/db.snapshots/checkpointState/om.db-c9c89178-1f5c-4a38-981e-7730888542d5; status : InvalidArgument; message : Directory exists
	at org.apache.hadoop.hdds.utils.HddsServerUtil.toIOException(HddsServerUtil.java:588)
	at org.apache.hadoop.hdds.utils.db.RocksDatabase.toIOException(RocksDatabase.java:88)
	at org.apache.hadoop.hdds.utils.db.RocksDatabase$RocksCheckpoint.createCheckpoint(RocksDatabase.java:239)
	at org.apache.hadoop.hdds.utils.db.RDBCheckpointManager.createCheckpoint(RDBCheckpointManager.java:83)
	at org.apache.hadoop.hdds.utils.db.RDBStore.getSnapshot(RDBStore.java:325)
	at org.apache.hadoop.ozone.om.OmSnapshotManager.createOmSnapshotCheckpoint(OmSnapshotManager.java:392)
	at org.apache.hadoop.ozone.om.response.snapshot.OMSnapshotCreateResponse.addToDBBatch(OMSnapshotCreateResponse.java:81)
	at org.apache.hadoop.ozone.om.response.OMClientResponse.checkAndUpdateDB(OMClientResponse.java:73)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.lambda$5(OzoneManagerDoubleBuffer.java:414)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.addToBatchWithTrace(OzoneManagerDoubleBuffer.java:237)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.addToBatch(OzoneManagerDoubleBuffer.java:412)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.flushBatch(OzoneManagerDoubleBuffer.java:333)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.flushCurrentBuffer(OzoneManagerDoubleBuffer.java:312)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.flushTransactions(OzoneManagerDoubleBuffer.java:277)
	at java.lang.Thread.run(Thread.java:750)
Caused by: org.rocksdb.RocksDBException: Directory exists
	at org.rocksdb.Checkpoint.createCheckpoint(Native Method)
	at org.rocksdb.Checkpoint.createCheckpoint(Checkpoint.java:51)
	at org.apache.hadoop.hdds.utils.db.RocksDatabase$RocksCheckpoint.createCheckpoint(RocksDatabase.java:236)
	... 12 more

I'm not sure why the directory exists in the first place. Checking this out.

prashantpogde · 2023-05-24T20:39:46Z

Checkpoint creation is not an idempotent operation. In case of log replay also it can fail. We should figure this out but we need to make sure that the checkpoint directory doesn't exist. If it exists that means checkpoint must have been created in previous incarnation of log replay,

smengcl · 2023-05-25T18:08:03Z

The test timeout issue seems fixed now. Thanks @aswinshakil .

Some test failures still looks relevant (flaky?):

https://github.com/apache/ozone/actions/runs/5075772329/jobs/9117315098

[INFO] Running org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
Error:  Tests run: 245, Failures: 3, Errors: 0, Skipped: 9, Time elapsed: 381.878 s <<< FAILURE! - in org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
Error:  TestRootedOzoneFileSystem.testSnapshotRead  Time elapsed: 0.15 s  <<< FAILURE!
java.lang.AssertionError: Failed to read/list on snapshotPath, exception: java.io.FileNotFoundException: Unable to load snapshot. Snapshot directory for snapshot '/volume54269/bucket05827/snap1' does not exists.
	at org.junit.Assert.fail(Assert.java:89)
	at org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem.testSnapshotRead(TestRootedOzoneFileSystem.java:2450)
...

[INFO] Running org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystemWithFSO
Error:  Tests run: 104, Failures: 1, Errors: 0, Skipped: 16, Time elapsed: 152.944 s <<< FAILURE! - in org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystemWithFSO
Error:  TestRootedOzoneFileSystemWithFSO.testSnapshotRead  Time elapsed: 0.126 s  <<< FAILURE!
java.lang.AssertionError: Failed to read/list on snapshotPath, exception: java.io.FileNotFoundException: Unable to load snapshot. Snapshot directory for snapshot '/volume22951/bucket62128/snap1' does not exists.
	at org.junit.Assert.fail(Assert.java:89)
	at org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem.testSnapshotRead(TestRootedOzoneFileSystem.java:2450)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

This might be irrelevant:

Error:  Tests run: 32, Failures: 0, Errors: 1, Skipped: 4, Time elapsed: 51.412 s <<< FAILURE! - in org.apache.hadoop.fs.ozone.contract.ITestOzoneContractCreate
Error:  ITestOzoneContractCreate.testSyncable  Time elapsed: 0.308 s  <<< ERROR!
java.io.IOException: Inconsistent read for blockID=conID: 1 locID: 111677748019200007 bcsId: 0 length=2 position=1 numBytesToRead=1 numBytesRead=-1
	at org.apache.hadoop.ozone.client.io.KeyInputStream.checkPartBytesRead(KeyInputStream.java:176)
...

aswinshakil · 2023-05-25T20:08:14Z

Thanks for pointing this out @smengcl, I updated PR.

smengcl · 2023-05-25T21:00:06Z

...e/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java

+
+    assertTableRowCount(snapshotInfoTable, 2);
+    GenericTestUtils.waitFor(() -> snapshotDeletingService
+            .getSuccessfulRunCount() >= 1, 1000, 10000);


nit: indentation. Can fix this in the other PR: https://github.com/apache/ozone/pull/4571/files#r1199182257

Suggested change

.getSuccessfulRunCount() >= 1, 1000, 10000);

.getSuccessfulRunCount() >= 1, 1000, 10000);

smengcl · 2023-05-25T21:30:25Z

Thanks @aswinshakil for the fixes and tests. Thanks @GeorgeJahad for reviewing this as well.

HDDS-8207. [Snapshot] Add test for SnapshotDeletingService.

79947db

aswinshakil added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Apr 14, 2023

aswinshakil self-assigned this Apr 14, 2023

aswinshakil marked this pull request as draft April 14, 2023 20:30

HDDS-8207. [Snapshot] Add test for FSO.

9cd92f4

aswinshakil marked this pull request as ready for review April 18, 2023 18:16

GeorgeJahad reviewed Apr 27, 2023

View reviewed changes

...e/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java Show resolved Hide resolved

aswinshakil added 2 commits May 2, 2023 15:01

Merge apache/master into HDDS-8207

274bdce

Merge branch 'master' of https://github.com/apache/ozone into HDDS-8207

f9bd9e0

GeorgeJahad mentioned this pull request May 9, 2023

HDDS-8209. [SNAPSHOT] Synchronize tarball creation with background processes. #4680

Merged

aswinshakil added 3 commits May 9, 2023 12:41

Merge apache/master into HDDS-8207

bc02ab0

Fix bugs and update tests.

17e92ab

Merge master

9df54ad

aswinshakil changed the title ~~HDDS-8207. [Snapshot] Add tests for SnapshotDeletingService.~~ HDDS-8207. [Snapshot] Fix bugs and add tests for SnapshotDeletingService. May 17, 2023

aswinshakil requested review from GeorgeJahad and smengcl and removed request for GeorgeJahad May 17, 2023 16:42

smengcl added 2 commits May 18, 2023 11:44

Merge branch 'master' into HDDS-8207

9d0f254

One more param addition.

1bb0b51

smengcl reviewed May 18, 2023

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java Outdated Show resolved Hide resolved

smengcl reviewed May 18, 2023

View reviewed changes

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java Show resolved Hide resolved

smengcl reviewed May 19, 2023

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Show resolved Hide resolved

smengcl reviewed May 19, 2023

View reviewed changes

...e/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java Show resolved Hide resolved

smengcl reviewed May 19, 2023

View reviewed changes

Address Review comments.

0c798fa

aswinshakil force-pushed the HDDS-8207 branch from 3c0225c to 0c798fa Compare May 19, 2023 17:58

aswinshakil added 2 commits May 22, 2023 15:11

Merge master.

d2af54b

Fix test.

d90b18a

Fix bug in getNextActiveSnapshot()

7bf322f

aswinshakil added 2 commits May 24, 2023 18:58

Fix rocksdb directory already exists while loading snapshot cache.

7758af1

Fix NPE.

3478958

Move checkSnapshotDirExist check.

9473e4d

smengcl approved these changes May 25, 2023

View reviewed changes

smengcl merged commit 8e5b4bc into apache:master May 25, 2023

	.getSuccessfulRunCount() >= 1, 1000, 10000);
	.getSuccessfulRunCount() >= 1, 1000, 10000);

HDDS-8207. [Snapshot] Fix bugs and add tests for SnapshotDeletingService. #4571

HDDS-8207. [Snapshot] Fix bugs and add tests for SnapshotDeletingService. #4571

Uh oh!

Conversation

aswinshakil commented Apr 14, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

GeorgeJahad commented Apr 19, 2023

Uh oh!

GeorgeJahad commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeorgeJahad commented Apr 19, 2023

Uh oh!

aswinshakil commented Apr 21, 2023

Uh oh!

aswinshakil commented Apr 21, 2023

Uh oh!

GeorgeJahad commented Apr 21, 2023

Uh oh!

GeorgeJahad commented Apr 21, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smengcl left a comment

Choose a reason for hiding this comment

Uh oh!

smengcl commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aswinshakil commented May 24, 2023

Uh oh!

prashantpogde commented May 24, 2023

Uh oh!

smengcl commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aswinshakil commented May 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smengcl commented May 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GeorgeJahad commented Apr 19, 2023 •

edited

Loading

smengcl commented May 24, 2023 •

edited

Loading

smengcl commented May 25, 2023 •

edited

Loading