HDDS-10374. Make container scanner generate merkle trees during the scan #7490

errose28 · 2024-11-26T17:03:37Z

What changes were proposed in this pull request?

Allow the container scanner to update the container Merkle Tree with the currently calculated hashes of what is on disk. If the tree does not already exist, it will be created. Even if the container is unhealthy, the merkle tree should still be updated to reflect the current state of the container. Hashes stored in RocksDB that were given by the client during write will not be modified.

Specific changes:

KeyValueContainerCheck has been modified to support building the merkle tree from the data the scanner sees.
Reworked the container checksum update methods in KeyValueHandler.
- In particular added a public updateContainerChecksum method for scanners to call so that ICRs are sent for checksum updates even if the container's state does not change.
- This also allows the KeyValueHandler to manage the in-memory data checksum update in the ContainerData object, so we don't need to do it from within ContainerChecksumTreeManager anymore.
A new ContainerScanHelper class was created to consolidate duplicate code among the scanners.
ContainerMerkleTree can now be built to contain blocks without any chunks, which is necessary to represent states like an empty block file.

What is the link to the Apache JIRA

HDDS-10374

How was this patch tested?

This patch contains unit, integration, and acceptance test additions and modifications. Major ones to note:

Unit tests for all container scanners updated to test that failure to update checksum does not impact moving a container to unhealthy.
The new KeyValueHandler#updateContainerChecksum method has a unit test added to check both ICR and disk updates happen correctly.
The TestContainerReconciliationWithMockDatanodes suite has been enabled after HDDS-12980. Add unit test framework for reconciliation. #8402
Container scanner integration tests have been updated to check container checksums after corruption is injected.
Existing tests have been migrated to use the on-demand scanner to generate merkle trees instead of manually building them. This removes numerous TODOs from the code.

More robust testing of fault combinations will be added later using the framework being developed under HDDS-11942

…anner-builds-mt * HDDS-10239-container-reconciliation: HDDS-10928. Implement container comparison logic within datanodes. (apache#7293)

Missing chunks are incorrectly reported as corrupted

Metadata scanner integration tests are passing

hadoop-ozone/dist/src/main/smoketest/admincli/container.robot

aswinshakil · 2025-01-09T20:52:11Z

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

    } finally {
      container.writeUnlock();
    }
-    createContainerMerkleTree(container);


If a container moves from OPEN to UNHEALTHY state we should try to build a merkle tree with whatever data we have at the moment before the scanner builds the actual merkle tree. If for some reason (either metadata/data error) we are unable to build it, then we can log the exception and move.

Yes, when we have an unhealthy container we should build the tree from the actual data, but this method only builds the tree from the metadata. I'm renaming it in the next commit to make that clearer. We should not depend on just the metadata if we suspect corruption. The way this should work is open -> unhealthy would trigger an on-demand scan of the container, but that code path is not in place right now. We may want to add that to our branch actually.

Yes, I think we should add the on-demand scan here to generate the Merkle tree.

...service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerCheck.java

aswinshakil

Overall the change looks good to me. I have left a minor comment to my previous comment.

kerneltime · 2025-01-22T07:03:57Z

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

+          .setContainerMerkleTree(treeProto);
      write(data, checksumInfoBuilder.build());
-      LOG.debug("Data merkle tree for container {} updated", containerID);
+      // If write succeeds, update the checksum in memory. Otherwise 0 will be used to indicate the metadata failure.


Should there be a way to tell if a checksum failed vs. scanner has not yet run? Should failure to generate checksum == -1?

As discussed, the in-memory hash should be what is written to disk as it is an in-memory cache. If updating the Merkle tree is failing, we can have a metric + log message and/or a status report to SCM or a new Admin API to Datanode to query the Merkle tree (I think there are going to be multiple debug scenarios where we would like to query a Datanode what it knows independent of SCM reports).

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

...rc/main/java/org/apache/hadoop/ozone/container/ozoneimpl/BackgroundContainerDataScanner.java

…anner-builds-mt * HDDS-10239-container-reconciliation: (646 commits) HDDS-11763. Implement container repair logic within datanodes. (apache#7474) HDDS-11887. Recon - Identify container replicas difference based on content checksums (apache#7942) HDDS-12397. Persist putBlock for closed container. (apache#7943) HDDS-12361. Mark testGetSnapshotDiffReportJob as flaky HDDS-12375. Random object created and used only once (apache#7933) HDDS-12335. Fix ozone admin namespace summary to give complete output (apache#7908) HDDS-12364. Require Override annotation for overridden methods (apache#7923) HDDS-11530. Support listMultipartUploads max uploads and markers (apache#7817) HDDS-11867. Remove code paths for non-Ratis SCM. (apache#7911) HDDS-12363. Add import options to IntelliJ IDEA style settings (apache#7921) HDDS-12215. Mark testContainerStateMachineRestartWithDNChangePipeline as flaky HDDS-12331. BlockOutputStream.failedServers is not thread-safe (apache#7885) HDDS-12188. Move server-only upgrade classes from hdds-common to hdds-server-framework (apache#7903) HDDS-12362. Remove temporary checkstyle suppression file (apache#7920) HDDS-12343. Fix spotbugs warnings in Recon (apache#7902) HDDS-12286. Fix license headers and imports for ozone-tools (apache#7919) HDDS-12284. Fix license headers and imports for ozone-s3-secret-store (apache#7917) HDDS-12275. Fix license headers and imports for ozone-integration-test (apache#7904) HDDS-12164. Rename and deprecate DFSConfigKeysLegacy config keys (apache#7803) HDDS-12283. Fix license headers and imports for ozone-recon-codegen (apache#7916) ... Conflicts: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeWriter.java hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerCheck.java hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerMerkleTreeWriter.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerDataScanner.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOnDemandContainerDataScanner.java hadoop-ozone/dist/src/main/smoketest/admincli/container.robot hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerMetadataScannerIntegration.java hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestOnDemandContainerDataScannerIntegration.java

hemantk-12

Thanks @errose28 for the patch.

hemantk-12 · 2025-04-11T17:48:42Z

...service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerCheck.java

                  StringUtils.bytes2Hex(expected.asReadOnlyByteBuffer()),
                  StringUtils.bytes2Hex(actual.asReadOnlyByteBuffer()),
                  block.getBlockID());
+          chunkHealthy = false;


We can break the loop here because smallest unit is chunk.

hemantk-12 · 2025-04-11T18:01:08Z

...er-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerScanError.java

    MISSING_CONTAINER_FILE,
    MISSING_CHUNKS_DIR,
-    MISSING_CHUNK_FILE,
+    MISSING_DATA_FILE,


qq: Is this for the cases when *.block file is missing?

Yes. I changed the name so it is agnostic of container layout.

…tate update

…anner-builds-mt * HDDS-10239-container-reconciliation: (433 commits) Revert "HDDS-11232. Spare InfoBucket RPC call for the FileSystem#getFileStatus calls for the general case. (apache#6988)" (apache#8358) HDDS-12993. Use DatanodeID in ReconNodeManager. (apache#8456) HDDS-13024. HTTP connect to 0.0.0.0 failed (apache#8439) HDDS-12914. Bump awssdk to 2.31.40, test ResumableFileDownload (apache#8455) HDDS-4677. Document Ozone Ports and Connection End Points (apache#8226) HDDS-13044. Remove DatanodeDetails#getUuid usages from hdds-common/client/container-service (apache#8462) HDDS-12568. Implement MiniOzoneCluster.Service for Recon (apache#8452) HDDS-13046. Add .vscode to .gitignore (apache#8461) HDDS-13022. Split up exclusive size tracking for key and directory cleanup in SnapshotInfo (apache#8433) HDDS-12966. DBDefinitionFactory should not throw InvalidArnException (apache#8454) HDDS-12948. [Snapshot] Increase `ozone.om.fs.snapshot.max.limit` default value to 10k (apache#8376) HDDS-11904. Fix HealthyPipelineSafeModeRule logic. (apache#8386) HDDS-12989. Throw CodecException for the Codec byte[] methods (apache#8444) HDDS-11298. Support owner field in the S3 listBuckets request (apache#8315) HDDS-12810. Check and reserve space atomically in VolumeChoosingPolicy (apache#8360) HDDS-13016. Add a getAllNodeCount() method to NodeManager. (apache#8445) HDDS-12299. Merge OzoneAclConfig into OmConfig (apache#8383) HDDS-12501. Remove OMKeyInfo from SST files in backup directory. (apache#8214) HDDS-13021. Make callId unique for each request in AbstractKeyDeletingService (apache#8432) HDDS-13000. [Snapshot] listSnapshotDiffJobs throws NPE. (apache#8418) ...

aswinshakil

One thing that struck my mind is that it's not related to this PR, but we can have it as a separate JIRA. We shouldn't allow reconciliation of a container that is already reconciling. This can lead to some other bugs where we update the metadata or the data in parallel.

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

...va/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerMetadataScannerIntegration.java

...er-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java

errose28 · 2025-05-19T21:56:58Z

We shouldn't allow reconciliation of a container that is already reconciling. This can lead to some other bugs where we update the metadata or the data in parallel.

Most of this should be handled for us by the ReplicationSupervisor and tested generally here. However this depends on how we define the uniqueness of the reconciliation commands and here I think we do have a problem as you mentioned, because we can get multiple simultaneous reconciliation tasks for the same container if the peer list is different. We should fix this in a follow-up so that ReconcileContainerCommand equality is based only on container ID, and add a test for this that is unique to reconciliation commands.

Update: Filed HDDS-13086 for this.

errose28 · 2025-05-19T22:37:51Z

Thanks for the review @aswinshakil. All comments should be addressed. I still need to stabilize this test on our branch before trying to get green CI with this change.

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

...r-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerScanHelper.java

Still save ICR for the end of reconciliation

aswinshakil · 2025-05-27T07:14:29Z

The changes for the scan gap look good. I see there are some checkstyle-related CI failures.

aswinshakil

TestContainerCommandReconciliation#testContainerChecksumChunkCorruption seems to be failing, I tried to look into it. There seems to be an issue with the scan container logic. Can you take a look at it?

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

errose28 · 2025-06-03T23:35:02Z

@aswinshakil I added the test fixes you suggested for the flaky tests on the branch to this PR:

TestFilePerBlockStrategy running out of space
TestContainerCommandReconciliation#testDataChecksumReportedAtSCM timing out when SCM was restarted

I also fixed TestContainerCommandReconciliation#testContainerChecksumChunkCorruption which started failing in this PR. The test was using a non-standard way of introducing corruption by changing the stored client checksums of the replica, not the data, which is something we can't recover from. I changed it to corrupt the actual data instead.

aswinshakil

Thanks for working on this @errose28. LGTM+1, we can get this merged.

errose28 · 2025-06-04T17:12:45Z

Thanks for the reviews @aswinshakil @kerneltime

errose28 added 5 commits November 22, 2024 17:15

Merge branch 'HDDS-10239-container-reconciliation' into HDDS-10374-sc…

a8b8dbc

…anner-builds-mt * HDDS-10239-container-reconciliation: HDDS-10928. Implement container comparison logic within datanodes. (apache#7293)

Add code to build and write the tree from the data scanners

999a913

Update todo in acceptance test

b0d1ba9

Add unit tests for tree generation by scanners based on container state

382bce2

Add initial (failing) unit test for KeyValueContaienrCheck

28b1889

Missing chunks are incorrectly reported as corrupted

github-actions bot added the container-reconciliation label Nov 26, 2024

errose28 added 12 commits November 26, 2024 12:45

Update container data checksum when building the tree

dc182e8

Fix handling of fully truncated block of 0 size

a3401a9

Add unit tests for new addBlock method in tree

a25d44d

Test that SCM gets a checksum with the container report

7550a3c

Add (failing) tests that SCM sees updated checksums

847f8d8

Update acceptance test

452c294

Add javadoc for tree generation from metadata

dc45eca

Data integration tests passing

1cb291f

Don't generate tree from metadata for unhealthy container

d6b21d2

Metadata scanner integration tests are passing

Checkstyle

2a2dbbd

Marking container unhealthy should not write a merkle tree (test fix)

c9a077c

Checkstyle

0bbbdc5

errose28 marked this pull request as ready for review January 9, 2025 17:45

errose28 requested review from aswinshakil and kerneltime January 9, 2025 17:45

aswinshakil reviewed Jan 9, 2025

View reviewed changes

Address review comments

7b971a9

aswinshakil reviewed Jan 21, 2025

View reviewed changes

kerneltime reviewed Feb 10, 2025

View reviewed changes

errose28 mentioned this pull request Apr 4, 2025

HDDS-11763. Implement container repair logic within datanodes. #7474

Merged

aswinshakil reviewed Apr 11, 2025

View reviewed changes

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

...rc/main/java/org/apache/hadoop/ozone/container/ozoneimpl/BackgroundContainerDataScanner.java Outdated Show resolved Hide resolved

errose28 marked this pull request as draft April 11, 2025 17:15

hemantk-12 reviewed Apr 11, 2025

View reviewed changes

errose28 added 6 commits May 14, 2025 18:58

Improve and update scanner integration tests

6c8be07

Add unit tests that checksum update failure does not stop container s…

60a1a6e

…tate update

Checkstyle

d035c17

Fix scan gap for unit test

53336ae

Fix metadata scan test

2504638

aswinshakil reviewed May 19, 2025

View reviewed changes

errose28 added 2 commits May 19, 2025 18:28

Update based on review

4be9992

pmd

c0b89dd

aswinshakil reviewed May 20, 2025

View reviewed changes

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

...r-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerScanHelper.java Show resolved Hide resolved

errose28 added 2 commits May 22, 2025 17:12

Update ContainerData checksum info after reconcile with each peer

e24a24e

Still save ICR for the end of reconciliation

Support bypassing scan gap (tests are failing)

dc27f74

errose28 added 2 commits May 27, 2025 11:40

Checkstyle

e2974b4

Fix scan gap bug. All tests expected to pass

34b4b9a

aswinshakil reviewed Jun 2, 2025

View reviewed changes

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

errose28 added 5 commits June 2, 2025 16:25

Fix scan gap call

5fda700

Use temp dir for test, fix space overflow in CI

de6a757

Add configs in test to support restarting DN and SCM quickly

1574cdd

Use standard corruption injection in failing test

7ff972c

Checkstyle

02a3ac6

Findbugs

54cbf92

aswinshakil marked this pull request as ready for review June 4, 2025 10:50

aswinshakil approved these changes Jun 4, 2025

View reviewed changes

errose28 merged commit 9a445ed into apache:HDDS-10239-container-reconciliation Jun 4, 2025
53 checks passed

HDDS-10374. Make container scanner generate merkle trees during the scan #7490

HDDS-10374. Make container scanner generate merkle trees during the scan #7490

Uh oh!

Conversation

errose28 commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

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!

aswinshakil left a comment

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!

hemantk-12 left a comment

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!

aswinshakil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

errose28 commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

errose28 commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

aswinshakil commented May 27, 2025

Uh oh!

aswinshakil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

errose28 commented Jun 3, 2025

Uh oh!

aswinshakil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

errose28 commented Jun 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

errose28 commented Nov 26, 2024 •

edited

Loading

errose28 commented May 19, 2025 •

edited

Loading