HDDS-10376. Add a Datanode API to supply a merkle tree for a given container. #6945

aswinshakil · 2024-07-15T15:51:55Z

What changes were proposed in this pull request?

Add an API to the datanode server that can be called to retrieve the merkle tree of a given container. Added DNContainerOperationClient which can be further used during the reconciliation process to readChunk, readBlock and other operations that we might require later.

What is the link to the Apache JIRA

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

How was this patch tested?

This patch was tested with integration test.

…ntainer.

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/audit/DNAction.java

errose28

Thanks for working on this @aswinshakil. Adding a summary of what we discussed:

Zero-copy is not required in phase 1 and is deferred to HDDS-11217.
We plan to merge the clients for reconciliation and EC reconstruction after merging to master. Doing it here will create too many merge conflicts. HDDS-11218
We are currently seeing if there's a way to implement this without making the modifications to KeyValueHandler that are causing widespread changes here.

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

...r-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/TokenHelper.java

...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java

errose28

I left a few comments on the existing implementation, but I have some more guidance that I think would help on this PR.

I think the end vision is that reconciliation will work similarly to EC reconstruction. This means that our ReconcileContainerCommandHandler would be given the client from the DatanodeStateMachine constructor. The implementation would not go to KeyValueHandler at all and that (currently placeholder) code will be removed. Instead the ReconcileContainerCommandHandler will submit to the ReplicationSupervisor for scheduling reconciliation the same way that replication and reconstruction are.

However, all of the above are out of scope for this PR. Those will be implemented as part of the actual reconciliation in HDDS-10928. For this PR we just need:

A new API
A client to call it
Tests for the client and the API.

For now the KeyValueHandler implementation is just a placeholder anyways so we don't need to touch it here. This should remove the sweeping KeyValueHandler changes in this PR and reduce it's size quite a bit.

...on/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/ContainerCommandResponseBuilders.java

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/TokenHelper.java

...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java

...ice/src/main/java/org/apache/hadoop/ozone/container/checksum/DNContainerOperationClient.java

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java

errose28

Thanks for the updates @aswinshakil I have a few more comments on the new changes. Additionally:

It would be good to test the container tokens on this new API by adding a test similar to this
After the last set of changes there's some extra files like ContainerCommands that have only whitespace diff. It would be good to revert those to give this PR a more accurate file count.
It looks like some tests are failing due to duplicate metrics registration. On first glance I'm not sure how it relates to this change exactly.

...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java

...ainer-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Handler.java

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java

...st/src/test/java/org/apache/hadoop/ozone/dn/checksum/TestContainerCommandReconciliation.java

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java

errose28

Thanks for the updates. Looks good, we are just missing the secure testing I mentioned in my previous comment. It would be good to do the following secure tests to TestOzoneContainerWithTLS or somewhere similar to make sure this PR does not have problems with the container token:

Read works in secure env
Read fails in secure env with invalid token

kerneltime · 2024-07-30T20:42:51Z

...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java

+      try (FileInputStream inStream = new FileInputStream(checksumFile)) {
+        return ByteString.readFrom(inStream);
+      } catch (FileNotFoundException ex) {
+        // TODO: Build the container checksum tree when it doesn't exist.


We should not build it here, also this can be a WARN as post upgrade all containers won't have a checksum tree file

Right, building the tree would be handled in the layer above based on the result of this method. We can update the comment. I'm actually thinking we should debug log here because this will get printed for every container the scanner encounters in the first run.

...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java

...ice/src/main/java/org/apache/hadoop/ozone/container/checksum/DNContainerOperationClient.java

kerneltime · 2024-07-30T20:46:10Z

...ice/src/main/java/org/apache/hadoop/ozone/container/checksum/DNContainerOperationClient.java

+ * This class wraps necessary container-level rpc calls for container reconcilitaion.
+ *   - GetContainerMerkleTree
+ */
+public class DNContainerOperationClient implements AutoCloseable {


Suggested change

public class DNContainerOperationClient implements AutoCloseable {

public class DNContainerReconciliationOperationClient implements AutoCloseable {

We want to keep this generic so that we can merge this with ECContainerOperationClient

We can rename it then.

...ice/src/main/java/org/apache/hadoop/ozone/container/checksum/DNContainerOperationClient.java

...c/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java

kerneltime · 2024-07-30T21:03:24Z

...ice/src/main/java/org/apache/hadoop/ozone/container/checksum/DNContainerOperationClient.java

+            .setMaxCacheSize(dnConf.getContainerClientCacheSize())
+            .setStaleThresholdMs(dnConf.getContainerClientCacheStaleThreshold())


What is the expected behavior to cache clients ? Is the client per Datanode? We do not expect much concurrency on the client, so should the threshold be longer ?

errose28

Thanks for adding the test @aswinshakil. The current test is called testDNContainerOperationClient, but in addition to testing the client's token generation, it is also testing the server side token validation. The existing test names in this class also don't reflect exactly what they test which doesn't help. For this test I think we should split this into more specific tests:

testDNContainerOperationClient would use DNContainerOperationClient to test that it generates correct tokens. The server should validate them and the call would succeed.
testGetContainerMerkleTree would use XceiverClientManager directly to test that the server side API accepts valid tokens and rejects invalid ones.

I don't think we need a test of the underlying TLS connection while calling the API, similar to downloadContainer. This is a layer below any code that was changed in this PR so downloadContainer should already cover it.

...est/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainerWithTLS.java

errose28 · 2024-08-06T16:23:26Z

...est/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainerWithTLS.java

+      assertThrows(IOException.class, () -> ContainerProtocolCalls.getContainerMerkleTree(
+          finalClient, containerId, "invalidContainerToken"));


We should specifically test for StorageContainerException with result code BLOCK_TOKEN_VERIFICATION_FAILED. Also there's some info missing from the exception message which shows up as null:

org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException: BLOCK_TOKEN_VERIFICATION_FAILED for null: Failed to decode token : invalidContainerToken

null here is the dispatcherContext. We don't have it for getContainerMerkleTree() operation.

errose28 · 2024-08-06T22:25:01Z

Thanks for the continued updates @aswinshakil. Just needs a rebase then I think we are good to go.

errose28 · 2024-08-07T15:17:38Z

@aswinshakil Why was the checksum manager added as a parameter to the key value handler after the branch update? This is not in the branch, and was not in the PR at commit 7e57538 prior to the merge commit. The implementation I have in HDDS-11254 does not pass this through the constructor either so it is a sweeping change we would just have to undo.

errose28

Thanks for the updates @aswinshakil

…p-supervisor Merge conflicts are resolved but the change does not yet build. * HDDS-10239-container-reconciliation: (183 commits) HDDS-10376. Add a Datanode API to supply a merkle tree for a given container. (apache#6945) HDDS-11289. Bump docker-maven-plugin to 0.45.0 (apache#7024) HDDS-11287. Code cleanup in XceiverClientSpi (apache#7043) HDDS-11283. Refactor KeyValueStreamDataChannel to avoid spurious IDE build issues (apache#7040) HDDS-11257. Ozone write does not work when http proxy is set for the JVM. (apache#7036) HDDS-11249. Bump ozone-runner to 20240729-jdk17-1 (apache#7003) HDDS-10517. Recon - Add a UI component for showing DN decommissioning detailed status and info (apache#6724) HDDS-10926. Block deletion should update container merkle tree. (apache#6875) HDDS-11270. [hsync] Add DN layout version (HBASE_SUPPORT/version 8) upgrade test. (apache#7021) HDDS-11272. Statistics some node status information (apache#7025) HDDS-11278. Move code out of Hadoop util package (apache#7028) HDDS-11274. (addendum) Replace Hadoop annotations/configs with Ozone-specific ones HDDS-11274. Replace Hadoop annotations/configs with Ozone-specific ones (apache#7026) HDDS-11280. Add Synchronize in AbstractCommitWatcher.addAckDataLength (apache#7032) HDDS-11235. Spare InfoBucket RPC call in FileSystem#mkdir() call. (apache#6990) HDDS-11273. Bump commons-compress to 1.26.2 (apache#7023) HDDS-11225. Increase ipc.server.read.threadpool.size (apache#7007) HDDS-11224. Increase hdds.datanode.handler.count (apache#7011) HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. (apache#7012) HDDS-11214. Added config to set rocksDB's max log file size and num of log files (apache#7014) ... Conflicts: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java

…rrupt-files * HDDS-10239-container-reconciliation: (183 commits) HDDS-10376. Add a Datanode API to supply a merkle tree for a given container. (apache#6945) HDDS-11289. Bump docker-maven-plugin to 0.45.0 (apache#7024) HDDS-11287. Code cleanup in XceiverClientSpi (apache#7043) HDDS-11283. Refactor KeyValueStreamDataChannel to avoid spurious IDE build issues (apache#7040) HDDS-11257. Ozone write does not work when http proxy is set for the JVM. (apache#7036) HDDS-11249. Bump ozone-runner to 20240729-jdk17-1 (apache#7003) HDDS-10517. Recon - Add a UI component for showing DN decommissioning detailed status and info (apache#6724) HDDS-10926. Block deletion should update container merkle tree. (apache#6875) HDDS-11270. [hsync] Add DN layout version (HBASE_SUPPORT/version 8) upgrade test. (apache#7021) HDDS-11272. Statistics some node status information (apache#7025) HDDS-11278. Move code out of Hadoop util package (apache#7028) HDDS-11274. (addendum) Replace Hadoop annotations/configs with Ozone-specific ones HDDS-11274. Replace Hadoop annotations/configs with Ozone-specific ones (apache#7026) HDDS-11280. Add Synchronize in AbstractCommitWatcher.addAckDataLength (apache#7032) HDDS-11235. Spare InfoBucket RPC call in FileSystem#mkdir() call. (apache#6990) HDDS-11273. Bump commons-compress to 1.26.2 (apache#7023) HDDS-11225. Increase ipc.server.read.threadpool.size (apache#7007) HDDS-11224. Increase hdds.datanode.handler.count (apache#7011) HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. (apache#7012) HDDS-11214. Added config to set rocksDB's max log file size and num of log files (apache#7014) ... Conflicts: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java

HDDS-10376. Add a Datanode API to supply a merkle tree for a given co…

e600d15

…ntainer.

aswinshakil added the container-reconciliation label Jul 15, 2024

aswinshakil requested review from kerneltime and errose28 July 15, 2024 15:51

aswinshakil self-assigned this Jul 15, 2024

HDDS-10376. Fix findbugs.

737361f

kerneltime reviewed Jul 16, 2024

View reviewed changes

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/audit/DNAction.java Outdated Show resolved Hide resolved

errose28 reviewed Jul 22, 2024

View reviewed changes

Move TokenHelper and change readMerkleTree to getMerkleTree

76119bc

errose28 reviewed Jul 24, 2024

View reviewed changes

aswinshakil added 3 commits July 25, 2024 21:15

Address review comments.

688353f

Resolve conflicts.

ca38c36

remove datanode client

1737194

errose28 reviewed Jul 25, 2024

View reviewed changes

aswinshakil added 2 commits July 27, 2024 18:46

Address Review Comments.

16a084a

Unregister ContainerMerkleTreeMetrics.

46ddedb

errose28 reviewed Jul 29, 2024

View reviewed changes

kerneltime reviewed Jul 30, 2024

View reviewed changes

...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java Outdated Show resolved Hide resolved

errose28 reviewed Jul 30, 2024

View reviewed changes

...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java Outdated Show resolved Hide resolved

kerneltime reviewed Jul 30, 2024

View reviewed changes

errose28 mentioned this pull request Jul 31, 2024

HDDS-10926. Block deletion should update container merkle tree. #6875

Merged

Add test for container token.

e3afa53

errose28 reviewed Aug 5, 2024

View reviewed changes

...est/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainerWithTLS.java Outdated Show resolved Hide resolved

aswinshakil added 2 commits August 5, 2024 21:12

Updates test as per review comments.

6d55777

Close and release clients for test.

afac35a

errose28 reviewed Aug 6, 2024

View reviewed changes

Update test with proper client release.

7e57538

aswinshakil force-pushed the HDDS-10376-datanode-api branch from 662af28 to 7e57538 Compare August 7, 2024 16:35

Resolve conflicts

4ae022d

errose28 approved these changes Aug 7, 2024

View reviewed changes

jojochuang mentioned this pull request Aug 7, 2024

HDDS-10874. Freon tool DNRPCLoadGenerator should not cache client objects. #6705

Closed

errose28 merged commit ab35173 into apache:HDDS-10239-container-reconciliation Aug 7, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-10376. Add a Datanode API to supply a merkle tree for a given container. #6945

HDDS-10376. Add a Datanode API to supply a merkle tree for a given container. #6945

aswinshakil commented Jul 15, 2024

errose28 left a comment

errose28 left a comment

errose28 left a comment

errose28 left a comment

kerneltime Jul 30, 2024

errose28 Jul 30, 2024

kerneltime Jul 30, 2024

aswinshakil Jul 31, 2024

kerneltime Jul 31, 2024

kerneltime Jul 30, 2024

errose28 left a comment

errose28 Aug 6, 2024

aswinshakil Aug 6, 2024

errose28 commented Aug 6, 2024

errose28 commented Aug 7, 2024

errose28 left a comment

	public class DNContainerOperationClient implements AutoCloseable {
	public class DNContainerReconciliationOperationClient implements AutoCloseable {

		.setMaxCacheSize(dnConf.getContainerClientCacheSize())
		.setStaleThresholdMs(dnConf.getContainerClientCacheStaleThreshold())

		assertThrows(IOException.class, () -> ContainerProtocolCalls.getContainerMerkleTree(
		finalClient, containerId, "invalidContainerToken"));

HDDS-10376. Add a Datanode API to supply a merkle tree for a given container. #6945

HDDS-10376. Add a Datanode API to supply a merkle tree for a given container. #6945

Conversation

aswinshakil commented Jul 15, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

errose28 left a comment

Choose a reason for hiding this comment

errose28 left a comment

Choose a reason for hiding this comment

errose28 left a comment

Choose a reason for hiding this comment

errose28 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

errose28 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

errose28 commented Aug 6, 2024

errose28 commented Aug 7, 2024

errose28 left a comment

Choose a reason for hiding this comment