HDDS-11408. Snapshot rename table entries are propagated incorrectly on snapshot deletes #7200

swamirishi · 2024-09-13T22:44:10Z

What changes were proposed in this pull request?

On snapshot deletes, not all rename table entries are propogated to the next snapshot. This could lead to an incorrect reclamation of blocks on certain edge case scenarios.

Snapshot deletion steps:

Has the snapshot been flushed to disk
If next snapshot is not in deleted state
If the next snapshot is AOS, then check key deletion service is not running
Move all renameTableEntries/deleted/deletedDirectoryTable keys to next snapshot/AOS
Check all tables empty
Purge snapshot from the chain.

What is the link to the Apache JIRA

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

How was this patch tested?

Adding unit tests

…on snapshot deletes Change-Id: I79bcf24d6517bd740aae0c7cf9915146b34d0931

hemantk-12

Thanks @swamirishi for working on this.

I'm half done with the review and leaving some comments.

hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java

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

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

hemantk-12 · 2024-09-14T22:11:14Z

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

+      int currentCount = 0;
+      while (renamedKeyIter.hasNext() && currentCount < count) {
+        Table.KeyValue<String, String> kv = renamedKeyIter.next();
+        if (kv != null) {


Can it be null? hasNext() should do this check?

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

...ain/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotMoveDeletedKeysResponse.java

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

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

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto

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

Change-Id: Ib9e08a7bdf603a4a26e7631c7b5a978d89c32a5a

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

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

...rc/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotMoveTableKeysRequest.java

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

Change-Id: Ifc9b97ffb05aa079bec5de9b08739c662d18c9ec

aswinshakil

Thanks for working on the patch @swamirishi. I'm posting my comments halfway through the code, I have yet to review the core logic.

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

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

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

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

Change-Id: I7b5d16029296b45fe49709121b0608c37ccc634b

Change-Id: Id6ce92feed88029bfbea9fdcdb77e123d097519f

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

Change-Id: I501790e90bd6005dde625f286db3969dbf0c99aa

Change-Id: Idd1c12d2a457ff024f798df4833deeb9fb73f4eb

Change-Id: I4503a43edef21e0511e5d9517217ff7fd9ee6a0b

Change-Id: I746be2343c3aa075ae8470a2b58587bab6c72f89

.../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/KeyManagerImpl.java

...rc/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotMoveTableKeysRequest.java

Change-Id: I3353211ab7269cc4506489019a6c2992554da658

Change-Id: I485ed971c9f9cb2240cfc807ff29188c9ba90f7e

Change-Id: If9552a29f9a6b78ee828841b113d015f6f51199d

Change-Id: I7f76fdd5b7c8fc8b9477ff1cd96dd4eefdc47d20

Change-Id: I12c2ce8da14fd8d30c78953be06efd96429ed190

Change-Id: I43a67c1ae02d9a89391aa48dfec0238debb1c5c5

Change-Id: I0aa53260461fe306cb3b9c95fe0366d7764d6531

Change-Id: Iedfa1dbc2294ffcb486a941599a73bc0e35acf44

Change-Id: Iffea07415eea4c5a071264f66516b8f041cce290

Change-Id: I0fb68ee048e9b85ce412b10b8791ca4bfe74d3aa

Change-Id: Ia6efe2b4ef67e1b4bd847cd1a3301b3ffbe585d0

Change-Id: I9c7d92c66fad94166e6b5da5973a404e4fca1c58

prashantpogde · 2024-09-19T18:40:54Z

...tion-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDeletingService.java

+        om.getKeyManager().getDeletedDirEntries(testBucket.getVolumeName(),
+            testBucket.getName(), 1000);
+    renamesKeyEntries.forEach(entry -> Assertions.assertTrue(aosRenamesKeyEntries.contains(entry)));
+    deletedKeyEntries.forEach(entry -> Assertions.assertTrue(aosDeletedKeyEntries.contains(entry)));


Can we also assert that these keys are removed from the snapshot to be deleted ?

...tion-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDeletingService.java

hemantk-12 · 2024-09-19T17:47:14Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java

+      @Override
+      public boolean equals(Object obj) {
+        if (!(obj instanceof KeyValue)) {
+          return false;
+        }
+        KeyValue<?, ?> kv = (KeyValue<?, ?>) obj;
+        try {
+          return getKey().equals(kv.getKey()) && getValue().equals(kv.getValue());
+        } catch (IOException e) {
+          throw new RuntimeException(e);
+        }
+      }
+
+      @Override
+      public int hashCode() {
+        return Objects.hash(getKey(), getValue());
+      }


qq: Is this for testing?

@swamirishi , This seems incorrect. Filed HDDS-13235

BTW, testing code should reside in tests. Otherwise, it may introduce real bugs to production code.

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java

hemantk-12 · 2024-09-19T17:51:53Z

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

+   * underlying metadataManager.
+   * @throws IOException
+   */
+  List<Table.KeyValue<String, String>> getRenamesKeyEntries(


I'm not asking to create another map. I'm saying the return type could be a map rather than a list of pairs.

hemantk-12 · 2024-09-19T17:58:06Z

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

+    int currentCount = 0;
+    while (tableIterator.hasNext() && currentCount < count) {
+      Table.KeyValue<String, V> kv = tableIterator.next();
+      if (kv != null) {


Yes, there is no harm but it isn't how an iterator is supposed to be used unless null is allowed value in the collection.

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

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

hemantk-12 · 2024-09-19T18:18:10Z

...rc/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotMoveTableKeysRequest.java

+    List<SnapshotMoveKeyInfos> deletedDirs = new ArrayList<>(moveTableKeysRequest.getDeletedDirsList().size());
+    //validate deleted key starts with bucket FSO path prefix.[/<volId>/<bucketId>/]
+    for (SnapshotMoveKeyInfos deletedDir : moveTableKeysRequest.getDeletedDirsList()) {
+      // Filter deleted directories with exactly one keyInfo per key.


Isn't it the same as line 120?

...rc/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotMoveTableKeysRequest.java

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

...rc/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotMoveTableKeysRequest.java

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

.../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/snapshot/SnapshotUtils.java

...ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotChain.java

Change-Id: Ia56d8cd8c11ab74fd9f9076516d939bcc88f3559

hemantk-12

Thanks, @swamirishi for finding this critical issue and fixing it.

LGTM+1.

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

Change-Id: Ia69b99aef696fa20f299670b1fdcc3f790ca8e19

swamirishi · 2024-09-20T20:27:02Z

Thank you for the reviews on the patch @aswinshakil @hemantk-12 @prashantpogde

swamirishi requested review from aswinshakil, hemantk-12 and prashantpogde September 13, 2024 22:44

HDDS-11408. Snapshot rename table entries are propogated incorrectly …

0335ed2

…on snapshot deletes Change-Id: I79bcf24d6517bd740aae0c7cf9915146b34d0931

swamirishi force-pushed the HDDS-11408 branch from 4ce3b36 to 0335ed2 Compare September 13, 2024 22:58

adoroszlai added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Sep 15, 2024

hemantk-12 reviewed Sep 16, 2024

View reviewed changes

prashantpogde reviewed Sep 16, 2024

View reviewed changes

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto Show resolved Hide resolved

prashantpogde reviewed Sep 16, 2024

View reviewed changes

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

HDDS-11408. Add test cases

53a5b5d

Change-Id: Ib9e08a7bdf603a4a26e7631c7b5a978d89c32a5a

prashantpogde reviewed Sep 16, 2024

View reviewed changes

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

prashantpogde reviewed Sep 16, 2024

View reviewed changes

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

prashantpogde reviewed Sep 16, 2024

View reviewed changes

...rc/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotMoveTableKeysRequest.java Show resolved Hide resolved

prashantpogde reviewed Sep 16, 2024

View reviewed changes

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

HDDS-11408. Address review comments

87ea520

Change-Id: Ifc9b97ffb05aa079bec5de9b08739c662d18c9ec

aswinshakil reviewed Sep 16, 2024

View reviewed changes

swamirishi added 2 commits September 16, 2024 15:50

HDDS-11408. Address review comments

6675538

Change-Id: I7b5d16029296b45fe49709121b0608c37ccc634b

HDDS-11408. Address review comments

96c3daa

Change-Id: Id6ce92feed88029bfbea9fdcdb77e123d097519f

aswinshakil reviewed Sep 16, 2024

View reviewed changes

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

HDDS-11408. Address review comments

535a6cd

Change-Id: I501790e90bd6005dde625f286db3969dbf0c99aa

hemantk-12 changed the title ~~HDDS-11408. Snapshot rename table entries are propogated incorrectly on snapshot deletes~~ HDDS-11408. Snapshot rename table entries are propagated incorrectly on snapshot deletes Sep 16, 2024

swamirishi added 3 commits September 16, 2024 18:07

HDDS-11408. Address review comments

7d49b64

Change-Id: Idd1c12d2a457ff024f798df4833deeb9fb73f4eb

HDDS-11408. Address review comments

f79c5c7

Change-Id: I4503a43edef21e0511e5d9517217ff7fd9ee6a0b

HDDS-11408. Address review comments

7124b1e

Change-Id: I746be2343c3aa075ae8470a2b58587bab6c72f89

prashantpogde reviewed Sep 17, 2024

View reviewed changes

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

prashantpogde reviewed Sep 17, 2024

View reviewed changes

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

prashantpogde reviewed Sep 17, 2024

View reviewed changes

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

prashantpogde reviewed Sep 17, 2024

View reviewed changes

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

prashantpogde reviewed Sep 17, 2024

View reviewed changes

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

hemantk-12 reviewed Sep 17, 2024

View reviewed changes

swamirishi added 12 commits September 18, 2024 17:04

HDDS-11408. Address review comments

075441b

Change-Id: I3353211ab7269cc4506489019a6c2992554da658

HDDS-11408. Address review comments

c3a11a5

Change-Id: I485ed971c9f9cb2240cfc807ff29188c9ba90f7e

HDDS-11408. Address review comments

bd28a4c

Change-Id: If9552a29f9a6b78ee828841b113d015f6f51199d

HDDS-11408. Address review comments

1fd5f1d

Change-Id: I7f76fdd5b7c8fc8b9477ff1cd96dd4eefdc47d20

HDDS-11408. Debug test failure

d1c0a77

Change-Id: I12c2ce8da14fd8d30c78953be06efd96429ed190

HDDS-11408. Debug test failure

0d0e3e7

Change-Id: I43a67c1ae02d9a89391aa48dfec0238debb1c5c5

HDDS-11408. Debug test failure

554e452

Change-Id: I0aa53260461fe306cb3b9c95fe0366d7764d6531

HDDS-11408. Debug test failure

1aa55ff

Change-Id: Iedfa1dbc2294ffcb486a941599a73bc0e35acf44

HDDS-11408. Debug test failure

364663c

Change-Id: Iffea07415eea4c5a071264f66516b8f041cce290

HDDS-11408. Add log

9af1e99

Change-Id: I0fb68ee048e9b85ce412b10b8791ca4bfe74d3aa

HDDS-11408. Add log

0a8183d

Change-Id: Ia6efe2b4ef67e1b4bd847cd1a3301b3ffbe585d0

HDDS-11408. Add log

73572ad

Change-Id: I9c7d92c66fad94166e6b5da5973a404e4fca1c58

prashantpogde reviewed Sep 19, 2024

View reviewed changes

...tion-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDeletingService.java Show resolved Hide resolved

hemantk-12 reviewed Sep 19, 2024

View reviewed changes

aswinshakil reviewed Sep 19, 2024

View reviewed changes

HDDS-11408. Address review comments

063f7b7

Change-Id: Ia56d8cd8c11ab74fd9f9076516d939bcc88f3559

swamirishi marked this pull request as ready for review September 20, 2024 04:08

hemantk-12 reviewed Sep 20, 2024

View reviewed changes

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

HDDS-11408. Address review comments

302491d

Change-Id: Ia69b99aef696fa20f299670b1fdcc3f790ca8e19

swamirishi merged commit 8c0b54e into apache:master Sep 20, 2024

HDDS-11408. Snapshot rename table entries are propagated incorrectly on snapshot deletes #7200

HDDS-11408. Snapshot rename table entries are propagated incorrectly on snapshot deletes #7200

Uh oh!

Conversation

swamirishi commented Sep 13, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aswinshakil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!