HDDS-13955. Handle empty datanode.id file gracefully #9479

cchung100m · 2025-12-11T15:07:44Z

What changes were proposed in this pull request?

Fetch the datanode UUID from the VERSION file under hdds.datanode.dir directory
Write the UUID to the datanode.id file

What is the link to the Apache JIRA

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

How was this patch tested?

Existing unit and integration tests

Gargi-jais11

Thanks @cchung100m for wokring on this.
Left some comments below.

Gargi-jais11 · 2025-12-13T17:21:59Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java

+      String dataNodeDir = conf.get(ScmConfigKeys.HDDS_DATANODE_DIR_KEY);
+      if (dataNodeDir == null || dataNodeDir.isEmpty()) {
+        throw new IOException("hdds.datanode.dir is not configured.", e);
+      }
+      File versionFile = new File(dataNodeDir, "hdds/VERSION");


hdds.datanode.dir can be a comma-separated list (e.g., /data/hdds1,/data/hdds2,/data/hdds3). conf.get() returns the entire string, creating an invalid path like /data/hdds1,/data/hdds2,/data/hdds3/hdds/VERSION.

Solution: Follow the pattern used in GeneratorDatanode.java

use HddsServerUtil.getDatanodeStorageDirs() to parse the comma-separated list and read from the first directory (all volumes VERSION file share the same datanode UUID).

Step 1: Get all storage directories (handles comma-separated list)
Step 2: Try each storage directory until we find a valid VERSION file with datanodeUUID.
Step 3: Create a new DaatnodeDetails with UUID found.
Step 4: Write the recovered Datanode ID to the datanode.id file

Gargi-jais11 · 2025-12-13T17:27:05Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/DatanodeIdYaml.java

+      if (datanodeDetailsYaml == null) {
+        throw new EmptyDatanodeIdFileException(
+            "Datanode ID file is empty: " + path.getAbsolutePath());
+      }


It may happen that object exists but UUID might be null or empty. So we can have these checks as well.

Suggested change

if (datanodeDetailsYaml == null) {

throw new EmptyDatanodeIdFileException(

"Datanode ID file is empty: " + path.getAbsolutePath());

}

if (datanodeDetailsYaml == null

|| datanodeDetailsYaml.getUuid() == null

|| datanodeDetailsYaml.getUuid().isEmpty()) {

throw new EmptyDatanodeIdFileException(

"Datanode ID file is empty: " + path.getAbsolutePath());

}

Gargi-jais11 · 2025-12-13T17:31:15Z

...main/java/org/apache/hadoop/ozone/container/common/helpers/EmptyDatanodeIdFileException.java

+/**
+ * Exception thrown when a Datanode ID file is empty.
+ */
+public class EmptyDatanodeIdFileException extends IOException {


I don't think we need to add a new exception class. Let's remove this and it also matches the existing codebase pattern.
Use IOException.

Hi @Gargi-jais11

Thanks for the prompt reply and suggestions. I've updated the part you mentioned.

Gargi-jais11 · 2025-12-13T17:33:02Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/DatanodeIdYaml.java

      }

+      if (datanodeDetailsYaml == null) {
+        throw new EmptyDatanodeIdFileException(


Remove EmptyDatanodeIdFileException and use

IOException("Datanode ID file is empty or has null UUID: " + path.getAbsolutePath());

Gargi-jais11 · 2025-12-13T17:44:58Z

@cchung100m Please add a unit test in TestContainerUtils for the following scenario:

Setup storage directory structure
Create VERSION file with datanode UUID
Simulate corrupted/empty datanode.id file. Creates an empty datanode.id file (0 bytes). Verifies the file is empty
Calls ContainerUtils.readDatanodeDetailsFrom(datanodeIdFile, testConf)
Verify recovery results

Assertions:
Recovered UUID matches the UUID in the VERSION file
datanode.id is recreated with content (file size > 0)
The recreated file can be read normally and contains the correct UUID

Gargi-jais11 · 2025-12-15T12:56:52Z

@ChenSammi and @peterxcli Could you please review the patch?

adoroszlai

Thanks @cchung100m for the patch.

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java

ChenSammi · 2025-12-29T07:34:54Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java

+    String dnUuid = null;
+    Collection<String> dataNodeDirs =
+        HddsServerUtil.getDatanodeStorageDirs(conf);
+    if (dataNodeDirs.isEmpty()) {


HddsServerUtil.getDatanodeStorageDirs checks whether return list is empty before return, so the double check here can be omitted.

Hi @ChenSammi
Thanks for the suggestions. I've updated the part you mentioned.

ChenSammi · 2025-12-29T07:49:10Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java

+      throw new IOException("hdds.datanode.dir is not configured.");
+    }
+    for (String dataNodeDir : dataNodeDirs) {
+      File versionFile = new File(dataNodeDir, "hdds/VERSION");


Please use VERSION_FILE and HDDS_VOLUME_DIR to replace the plain string.

ChenSammi · 2025-12-29T07:59:41Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java

-      } catch (IOException io) {
-        throw new IOException("Failed to parse DatanodeDetails from "
-            + path.getAbsolutePath(), io);
+      LOG.warn("Failed to read Datanode ID file as YAML. Reason: {}. " +


Please use "e" instead of e.getMessage() here. Stack trace will reveal more info than e.getMessage() during debug. e.getMessage() is usually used when constructing error message propagated from client to server. Same for L185.

ChenSammi · 2025-12-29T08:01:44Z

Thanks @cchung100m for working on this task. Overall it looks good to me. Just a few minor comments.

Gargi-jais11

Thanks @cchung100m for updating the patch. Once the above few comments are resolved it looks good to go.

cchung100m added 3 commits December 11, 2025 23:03

HDDS-13955: Handle empty datanode.id file gracefully

5152562

HDDS-13955: Fix checkstyle

68904d3

HDDS-13955: Fix compile error

ee916ad

cchung100m force-pushed the HDDS-13955 branch from 3f842d5 to ee916ad Compare December 12, 2025 11:30

cchung100m marked this pull request as ready for review December 12, 2025 12:56

Gargi-jais11 suggested changes Dec 13, 2025

View reviewed changes

cchung100m marked this pull request as draft December 14, 2025 05:07

cchung100m added 2 commits December 14, 2025 14:09

HDDS-13955: Refactor by review comments

3a923cd

HDDS-13955: Combine the recovery handling for Datanode ID file not found

6768196

cchung100m marked this pull request as ready for review December 14, 2025 08:20

cchung100m marked this pull request as draft December 14, 2025 08:25

cchung100m force-pushed the HDDS-13955 branch from 201536c to bd9c536 Compare December 14, 2025 08:36

HDDS-13955: Add test case: testDatanodeIdRecovery

89fec25

cchung100m force-pushed the HDDS-13955 branch from bd9c536 to 89fec25 Compare December 14, 2025 08:39

cchung100m marked this pull request as ready for review December 14, 2025 09:52

HDDS-13955: Add IOException to handle DatanodeDetails.getFromProtoBuf

6112982

adoroszlai reviewed Dec 19, 2025

View reviewed changes

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java Outdated Show resolved Hide resolved

HDDS-13955: refactor the recovery code into separate methods

46afa5b

adoroszlai requested review from ChenSammi, Gargi-jais11 and errose28 and removed request for Gargi-jais11 December 21, 2025 07:50

ChenSammi reviewed Dec 29, 2025

View reviewed changes

Gargi-jais11 reviewed Dec 30, 2025

View reviewed changes

Remove the unnecessary HddsServerUtil.getDatanodeStorageDirs check

6ad2e09

cchung100m marked this pull request as draft December 30, 2025 15:12

cchung100m marked this pull request as ready for review December 30, 2025 15:45

HDDS-13955. Handle empty datanode.id file gracefully #9479

Are you sure you want to change the base?

HDDS-13955. Handle empty datanode.id file gracefully #9479

Conversation

cchung100m commented Dec 11, 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!

Gargi-jais11 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!

Gargi-jais11 commented Dec 13, 2025

Uh oh!

Gargi-jais11 commented Dec 15, 2025

Uh oh!

adoroszlai left a comment

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!

ChenSammi Dec 29, 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!

ChenSammi commented Dec 29, 2025

Uh oh!

Gargi-jais11 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cchung100m commented Dec 11, 2025 •

edited

Loading

ChenSammi Dec 29, 2025 •

edited

Loading