Skip to content

HDDS-16425. Fail datanode startup cleanly on initialization errors - #11246

Open
smengcl wants to merge 4 commits into
apache:masterfrom
smengcl:HDDS-16425
Open

smengcl wants to merge 4 commits into
apache:masterfrom
smengcl:HDDS-16425

Conversation

@smengcl

@smengcl smengcl commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Datanode startup can hang indefinitely when container initialization fails, for example because a corrupted Raft log cannot be read.

OzoneContainer.start() already propagates the startup exception, but the existing failure handling has several gaps:

  • Initialization remains INITIALIZING, leaving subsequent callers waiting indefinitely for INITIALIZED.
  • VersionEndpointTask does not consistently treat local initialization failures as fatal: runtime exceptions can escape, while most IOExceptions are handled as retryable communication failures.
  • If the caller has already timed out, the exceptional task completion can go unobserved.

Proposed changes:

  • Log the full cause and trigger datanode shutdown for local initialization failures. Partially started services cannot safely be initialized again because startup is not idempotent. SCM communication failures remain retryable.
  • Retain InitializingStatus and add FAILED, preserving the original exception so waiting and subsequent callers fail promptly without repeating initialization.
  • Preserve pending endpoint completions across heartbeat cycles and make endpoint state changes visible across threads, allowing delayed failures to trigger shutdown.
  • Replace startup polling with a dedicated lock so concurrent callers share the same initialization result.

What is the link to the Apache JIRA?

HDDS-16425

How was this patch tested?

  • All 46 tests passed across TestOzoneContainer, TestRunningDatanodeState, and TestDatanodeStateMachine.
  • Coverage includes concurrent initialization, checked and unchecked failures, late completions, slow successful startup, and retryable SCM communication failures.
  • An integrated test injects a delayed Ratis startup failure after replication starts, verifying original-cause logging, the fatal shutdown callback, waiting-caller release, and no initialization retry.
  • Checkstyle passed.

Generated-by: Codex (GPT-6)

Copilot AI lite review requested due to automatic review settings September 15, 2026 22:23
@smengcl smengcl added the AI-gen label Sep 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR makes datanode container initialization failure handling fail-fast and preserves delayed endpoint failures across heartbeat cycles.

Changes:

  • Adds a serialized initialization state machine with retained failure causes.
  • Routes local startup failures toward datanode shutdown while preserving SCM retry behavior.
  • Adds concurrency, timeout, and integration coverage.
File summaries
File Description
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java Updated as part of this pull request.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestDatanodeStateMachine.java Updated as part of this pull request.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/states/datanode/TestRunningDatanodeState.java Updated as part of this pull request.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java Updated as part of this pull request.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/VersionEndpointTask.java Updated as part of this pull request.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java Updated as part of this pull request.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/EndpointStateMachine.java Updated as part of this pull request.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@smengcl smengcl added the bug Something isn't working label Sep 15, 2026
Catch both Exception and Error during container service startup.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation consistently handles initialization failures and includes focused concurrency and integration coverage.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@smengcl
smengcl marked this pull request as ready for review September 16, 2026 02:00

@devmadhuu devmadhuu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @smengcl for the patch. Largely LGTM. But overall I have few questions:

  1. Does RATIS currently cleanly handles exception errors separately for each raft group. I mean if two raft groups are corrupted or some issue like volume failure while loading them, should DN continue to initialize and load remaining raft groups/pipelines and for problematic ones, ozone gets the status code etc ? So that ozone DN can eventually takes care of closure of those pipelines and let other OPEN and valid pipelines functional ? We currently run with raft.server.log.corruption.policy = EXCEPTION (not overridden), so a corrupt log throws during server.start(). Would a per‑group skip/report path be safer than failing the entire node? I mean that would not be the part of this PR, but just wondering if this is how RATIS should handle ?

try {
initializeContainerServices(clusterId);
initializingStatus.set(InitializingStatus.INITIALIZED);
} catch (IOException | RuntimeException | Error ex) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not too generic to handle all errors ? What if any OOM. It will not let InterruptedException also to interrupt the thread.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch only records the terminal failure and immediately rethrows the same throwable, including OutOfMemoryError. It does not attempt recovery. Recording FAILED ensures other startup callers do not remain waiting for successful initialization.

At the endpoint boundary, the failure triggers fatal shutdown. Executor tasks capture Errors in their futures, so simply letting an Error escape would not reliably terminate the DN. Actual heap exhaustion can still prevent logging or orderly shutdown.

InterruptedException is not caught by this union. Acquiring a synchronized monitor is noninterruptible, which is a separate limitation.


hddsDispatcher.init();
hddsDispatcher.setClusterId(clusterId);
writeChannel.start();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we know some known issue in RATIS, but what if in future some other regression in RATIS makes hang forever, should we decouple this by having some timeout to avoid an infinite hold of initializationLock ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup this patch can't handle an initializer that never returns. The heartbeat timeout limits how long the caller waits, but does not cancel the underlying startup operation.

A startup watchdog would need an overall deadline and a terminal shutdown policy, accounting for legitimate long container and Raft log loading. Timing out lock acquisition alone would leave the original initializer running, and retrying partially initialized services would be unsafe.

I suggest handling that separately from this PR's exception-propagation fix.

@smengcl

smengcl commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @devmadhuu for taking a look.

Does RATIS currently cleanly handles exception errors separately for each raft group.

Ratis 3.2.1 does not provide a per-group skip/report startup contract. It starts the groups in parallel and waits for their combined completion before starting the RPC servers. If a group’s startup fails, the overall server startup fails, even if other groups initialized successfully.

ref: RaftServerProxy.startImpl()

We currently run with raft.server.log.corruption.policy = EXCEPTION (not overridden), so a corrupt log throws during server.start().

Correct for the default EXCEPTION policy. Ratis also supports WARN_AND_RETURN, which logs covered segment-read failures and returns the readable entries before the corruption. If Ratis then completes startup successfully, this PR allows the DN to continue normally.

The PR preserves both policies: it does not override the configured corruption policy or treat warnings as fatal. It triggers shutdown when startup actually throws. WARN_AND_RETURN can still encounter other failures, such as consistency-check failures, and it does not provide per-group isolation.

Would a per‑group skip/report path be safer than failing the entire node?

It could improve availability for corruption isolated to one group. However, Ratis would need to prevent the failed group from serving requests, clean up its partially initialized resources, and report the failure so Ozone/SCM can handle the affected pipeline. Shared volume or server failures would also need to be distinguished from isolated group failures.

Agreed that this would be a useful separate enhancement. This PR handles the existing server-start failure by terminating the DN with the original cause logged, rather than leaving initialization stuck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-gen bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants