HDDS-12233. Atomically import a container by ptlrs · Pull Request #7934 · apache/ozone

ptlrs · 2025-02-19T17:00:21Z

What changes were proposed in this pull request?

This PR implements changes to atomically import a container.

Place a container in the RECOVERING state before it is imported.
Restore the container to its original state only after a successful import.
Import the SST files of a container in a single transaction.
Delete the partially-imported ratis-replicated recovering container upon datanode restart.
Updated tests to match the new behavior.

What is the link to the Apache JIRA

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

How was this patch tested?

CI: https://github.com/ptlrs/ozone/actions/runs/13518880080

Added some tests which check for the state of the container.

swamirishi

Thanks for working on the patch @ptlrs. This patch is incomplete from the sense where we are not updating the container state on disk and just doing it on memory and the problem could still persist on restarts.

ptlrs · 2025-02-24T18:56:53Z

+  private static final String TEST_DESCRIPTOR_FILE_CONTENT = "!<KeyValueContainerData>\n" +
+      "checksum: 2215d39f2ae1de89fec837d18dc6387d8cba22fb5943cf4616f80c4b34e2edfe\n" +
+      "chunksPath: target/test-dir/MiniOzoneClusterImpl-23c1bb30-d86a-4f79-88dc-574d8259a5b3/ozone-meta/datanode-4/data-0/hdds/23c1bb30-d86a-4f79-88dc-574d8259a5b3/current/containerDir0/1/chunks\n" +
+      "containerDBType: RocksDB\n" +
+      "containerID: 1\n" +
+      "containerType: KeyValueContainer\n" +
+      "layOutVersion: 2\n" +
+      "maxSize: 5368709120\n" +
+      "metadata: {}\n" +
+      "metadataPath: target/test-dir/MiniOzoneClusterImpl-23c1bb30-d86a-4f79-88dc-574d8259a5b3/ozone-meta/datanode-4/data-0/hdds/23c1bb30-d86a-4f79-88dc-574d8259a5b3/current/containerDir0/1/metadata\n" +
+      "originNodeId: 25a48afa-f8d8-44ff-b268-642167e5354b\n" +
+      "originPipelineId: d7faca81-407f-4a50-a399-bd478c9795e5\n" +
+      "schemaVersion: '3'\n" +
+      "state: CLOSED";


The dummy content for the descriptor file resulted in test failure as we now parse the descriptor file content and save to disk when performing the ATOMIC_MOVE. Updated the content to dummy parseable data.

ptlrs · 2025-02-26T19:16:01Z

Hi @ChenSammi, could you please take a look and review this PR. Thanks.

sumitagrawl

@ptlrs given a comment to handle RECOVERING in code, plz have a look and it will simplify code

sumitagrawl

LGTM

ptlrs · 2025-03-11T19:38:01Z

Hi @umamaheswararao, @swamirishi, @ChenSammi, @errose28, @kerneltime could you please review this PR. Thanks.

errose28 · 2025-03-13T23:55:42Z

The existing RECOVERING container handling will mark the failed import as unhealthy on restart. It will then be reported to SCM where it may or may not get deleted later if all the other replicas are also unhealthy. I think we should change this such that all RECOVERING containers (either Ratis imports or EC reconstructions) are deleted by the datanode on startup, since we know they are incomplete. cc @swamirishi

swamirishi

@ptlrs thanks for the patch changes overall look good to me, excepting for some minor nitpicky comments. You may choose to address them in another follow up jira or the same, I am letting it on you to decide.

swamirishi · 2025-03-24T17:38:53Z

    return descriptorFileContent;
  }

+  private void persistCustomContainerState(Container<KeyValueContainerData> container, byte[] descriptorContent,


Why explicitly Container ? Can this just not be any Container<? extends ContainerData>?

Since this class implements ContainerPacker<KeyValueContainerData> I kept it consistent with the other methods. I have now made it generic since it's now part of the interface.

swamirishi · 2025-03-24T17:41:30Z

+    }
+
+    KeyValueContainerData originalContainerData =
+        (KeyValueContainerData) ContainerDataYaml.readContainer(descriptorContent);


We needn't make this KeyValueContainerData specific. Do you think we can actually move this function ContainerPacker interface itself and make it a default function?

swamirishi · 2025-03-24T17:43:22Z

-    if (VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.HBASE_SUPPORT)) {
-      getLastChunkInfoTable().loadFromFile(
-          getTableDumpFile(getLastChunkInfoTable(), dumpDir));
+      throws IOException, RocksDBException {


nit: Do you want to throw RockDBException here? Why not wrap RocksDBException inside IOException? Technically RocksDBException for ozone is actually like an IO error.

Fixed by throwing IOException instead

swamirishi · 2025-03-24T17:46:14Z

+      sstFileReader.open(tableDumpFile.getAbsolutePath());
+      try (ManagedReadOptions managedReadOptions = new ManagedReadOptions();
+           ManagedSstFileReaderIterator iterator =
+               ManagedSstFileReaderIterator.managed(sstFileReader.newIterator(managedReadOptions))) {


Are we sure there are no tombstones in this sst file?

Can you confirm this? How we are generating this sst file.

We dump the files by reading all the keys via an iterator so there should be no tombstones.

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

Line 124 in 1a69500

public void dumpKVContainerData(long containerID, File dumpDir)

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

Lines 276 to 284 in 1a69500

public void dumpToFileWithPrefix(File externalFile, byte[] prefix)

throws IOException {

try (TableIterator<byte[], KeyValue<byte[], byte[]>> iter

= iterator(prefix);

DumpFileWriter fileWriter = new RDBSstFileWriter()) {

fileWriter.open(externalFile);

while (iter.hasNext()) {

final KeyValue<byte[], byte[]> entry = iter.next();

fileWriter.put(entry.getKey(), entry.getValue());

swamirishi · 2025-03-24T17:51:27Z



+  @Test
+  public void testImportedContainerIsClosed() throws Exception {


Can we have miniOzoneCluster test case for the container cleanup on DN restart?

…-container-replication # Conflicts: # hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java

ptlrs · 2025-04-01T00:15:47Z

Hi @swamirishi, can you please review the latest changes.

swamirishi

LGTM @ptlrs thanks for addressing all the review comments.

ghost · 2025-04-01T18:19:11Z

Thanks for the patch @ptlrs and @sumitagrawl for reviewing the patch

ghost · 2025-04-01T18:32:45Z

Thank you for the extensive reviews @umamaheswararao @sumitagrawl @ChenSammi @swamirishi @errose28.

atomically import a container

78da63b

umamaheswararao reviewed Feb 19, 2025

View reviewed changes

Comment thread ...e/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaThreeImpl.java Outdated

HDDS-12233. throw the RocksDBException

9ccda66

swamirishi requested changes Feb 20, 2025

View reviewed changes

Comment thread ...iner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java Outdated

ptlrs added 6 commits February 20, 2025 19:19

HDDS-12233. save custom container state

920e5ad

persist state before atomic move, add rocksdb checks, fix tests

c9f1741

persist state before atomic move

b3abc37

fix KeyValueContainer test

a151bd2

remove log lines

e68c85e

add test to confirm the state of the imported container

6d9b112

ptlrs commented Feb 24, 2025

View reviewed changes

Comment thread ...service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestTarContainerPacker.java

ptlrs added 2 commits February 24, 2025 11:19

fix checkstyle error

c9907f8

fix checkstyle error

8d09dcb

ptlrs requested a review from swamirishi February 24, 2025 23:37

ptlrs and others added 2 commits February 24, 2025 15:38

Merge branch 'master' into HDDS-12233-atomic-container-replication

3e1cb0e

fix checkstyle error

8e3f941

ptlrs marked this pull request as ready for review February 25, 2025 15:20

swamirishi reviewed Feb 27, 2025

View reviewed changes

Comment thread ...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java Outdated

ptlrs requested a review from swamirishi February 27, 2025 01:26

sumitagrawl reviewed Feb 27, 2025

View reviewed changes

Comment thread ...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java

ChenSammi reviewed Mar 3, 2025

View reviewed changes

Comment thread ...e/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaThreeImpl.java Outdated

ChenSammi reviewed Mar 3, 2025

View reviewed changes

Comment thread ...e/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaThreeImpl.java Outdated

ptlrs added 2 commits March 3, 2025 20:27

fix review comments

06e90e2

fix review comments to reduce the number of container file updates

436d151

ptlrs commented Mar 11, 2025

View reviewed changes

Comment thread ...iner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java

ptlrs requested a review from sumitagrawl March 11, 2025 04:00

fix review comments to reduce the number of container file updates

db1ac27

sumitagrawl approved these changes Mar 11, 2025

View reviewed changes

ptlrs requested review from ChenSammi and umamaheswararao March 11, 2025 17:15

ptlrs added 2 commits March 19, 2025 12:35

delete Ratis replicated recovering containers upon datanode start

35361d1

fix unit test

1a69500

sumitagrawl approved these changes Mar 21, 2025

View reviewed changes

swamirishi approved these changes Mar 24, 2025

View reviewed changes

ptlrs added 2 commits March 28, 2025 10:50

HDDS-12233. fix review comments

f7108cb

Merge remote-tracking branch 'upstream/master' into HDDS-12233-atomic…

1b60599

…-container-replication # Conflicts: # hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java

ptlrs marked this pull request as draft March 28, 2025 17:58

HDDS-12233. remove unused field

a3f661b

ptlrs requested a review from swamirishi April 1, 2025 00:14

swamirishi approved these changes Apr 1, 2025

View reviewed changes

swamirishi marked this pull request as ready for review April 1, 2025 14:07

swamirishi merged commit af5301e into apache:master Apr 1, 2025

ptlrs deleted the HDDS-12233-atomic-container-replication branch April 1, 2025 18:18

Gargi-jais11 mentioned this pull request May 15, 2025

HDDS-13023. Container checksum is missing after container import #8459

Merged

	public void dumpToFileWithPrefix(File externalFile, byte[] prefix)
	throws IOException {
	try (TableIterator<byte[], KeyValue<byte[], byte[]>> iter
	= iterator(prefix);
	DumpFileWriter fileWriter = new RDBSstFileWriter()) {
	fileWriter.open(externalFile);
	while (iter.hasNext()) {
	final KeyValue<byte[], byte[]> entry = iter.next();
	fileWriter.put(entry.getKey(), entry.getValue());



		@Test
		public void testImportedContainerIsClosed() throws Exception {

Uh oh!

Conversation

ptlrs commented Feb 19, 2025 • 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!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ptlrs Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ptlrs commented Feb 26, 2025

Uh oh!

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

ptlrs commented Mar 11, 2025

Uh oh!

errose28 commented Mar 13, 2025

Uh oh!

swamirishi 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!

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!

ptlrs Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptlrs commented Apr 1, 2025

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 1, 2025

Uh oh!

ghost commented Apr 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ptlrs commented Feb 19, 2025 •

edited

Loading

ptlrs Feb 24, 2025 •

edited

Loading

ptlrs Mar 28, 2025 •

edited

Loading