Skip to content

HDDS-17360. Datanode asks SCM to close an open EC container when it loses the container's replica - #11727

Open
yandrey321 wants to merge 4 commits into
apache:masterfrom
yandrey321:HDDS-17360
Open

yandrey321 wants to merge 4 commits into
apache:masterfrom
yandrey321:HDDS-17360

Conversation

@yandrey321

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Problem. When a datanode loses the replica of an open EC container, for example because its volume failed, nothing closes the container, and SCM keeps allocating it:

  • the datanode moves the container to its missing set and sends a full container report, and writes to it fail with CONTAINER_MISSING ("has been lost and cannot be recreated");
  • SCM drops the replica, but OpenContainerHandler only checks that each remaining replica is OPEN, so an open EC container with a missing index counts as healthy;
  • the container never fills, because the writes fail, so it isn't closed for being full either.

Ratis containers on the same volume are closed within seconds: the failed write makes the Ratis state machine unhealthy, and the datanode asks SCM to close the pipeline. EC writes go through the gRPC path, which has no such handling.

Implementation idea. The datanode asks SCM to close the container, through the existing ContainerAction CLOSE
in the heartbeat, in two places:

  • When a volume fails (ContainerSet.handleVolumeFailures), for each OPEN EC container on it (replica index > 0).
    The existing code already triggers a heartbeat after the full container report, so SCM closes the container
    within seconds, whether or not anyone writes to it.
  • When a write over gRPC to a missing container fails with CONTAINER_MISSING (HddsDispatcher), if it is an EC
    write (replica index > 0 on WriteChunk or PutBlock). Only the first such write per container triggers an immediate
    heartbeat; later ones re-add the action, deduplicated while pending.

SCM's existing path does the rest:

  • ContainerActionsHandler fires CLOSE_CONTAINER;
  • the container moves to CLOSING and is detached from its pipeline, so it's no longer allocated, and the EC pipeline
    is closed at the next allocation;
  • the replicas are force-closed;
  • once CLOSED, the missing index is reconstructed.

Key decisions.

  • Datanode-driven, like Ratis. Only the datanode that lost the replica reports it, so a newly allocated container
    whose replicas haven't been created yet is never closed by mistake. That risk would come with an SCM-side check on
    replica counts.
    handles every CLOSE
    action the same way, and the datanode log says why.
  • Two triggers. The volume-failure hook closes containers that nobody writes to. The write hook covers an action
    lost with a failed heartbeat, SCM failover, and containers that went missing in other ways.

Side effects.

  • More, harmless CLOSE actions. Each failed EC write to a lost container re-adds the action. SCM ignores CLOSE
    for containers that are no longer OPEN, so repeats and reports from several datanodes are harmless.
  • Earlier close. Open EC containers on a failed volume are now closed even if no one writes to them, so their
    missing index gets reconstructed sooner.

Generated-by: Claude Code (Claude Opus 5.5)

What is the link to the Apache JIRA

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

How was this patch tested?

Integration and unit tests.

…oses the container's replica

Generated-by: Claude Code (Claude Opus 5.5)

@smengcl smengcl 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.

+1. Thanks @yandrey321 for the patch.

@jojochuang jojochuang 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.

The Ratis and EC paths address the same allocation problem at different levels:

Step Ratis EC with this PR
Volume fails Replica is removed locally, marked missing, and omitted from the full container report Same, plus a CLOSE action for each affected OPEN EC container, followed by a heartbeat
Subsequent write Goes through the Ratis state machine and fails on the missing replica Goes through gRPC and returns CONTAINER_MISSING
Failure handling State machine becomes unhealthy; DataNode requests pipeline close Dispatcher re-adds a container-close action; the first such failure per container triggers an immediate heartbeat
SCM response Closes the pipeline and finalizes its open containers Finalizes the affected container, stopping further allocation; the EC pipeline is closed later during allocation
Replica closing Normal Ratis close handling, potentially involving QUASI_CLOSED Force-close, avoiding QUASI_CLOSED
Repair Ratis replica recovery/replication handling Missing EC index becomes eligible for reconstruction after closure

One timing distinction worth preserving in the description: the Ratis missing-container path depends on a subsequent write encountering the failure. Volume failure alone does not immediately close an idle Ratis pipeline. This patch's EC volume-failure hook acts even without further writes.

Could we also extend the integration test through the remaining recovery steps? Currently, waiting for the container to leave OPEN proves that allocation stops, and reading the existing key verifies availability with surviving fragments. It does not establish that closure completes or reconstruction occurs.

Waiting for SCM's container state to reach CLOSED, then verifying restoration of all required EC replica indexes, would cover those claims. The test may need a spare DataNode to satisfy reconstruction placement constraints.

@peterxcli
peterxcli self-requested a review October 25, 2026 11:44
@yandrey321

Copy link
Copy Markdown
Contributor Author

Could we also extend the integration test through the remaining recovery steps? Currently, waiting for the container to leave OPEN proves that allocation stops, and reading the existing key verifies availability with surviving fragments. It does not establish that closure completes or reconstruction occurs.

Waiting for SCM's container state to reach CLOSED, then verifying restoration of all required EC replica indexes, would cover those claims. The test may need a spare DataNode to satisfy reconstruction placement constraints.

Done. The integration test now waits for SCM to report the container CLOSED, and then until every RS(3,2) index (1–5) has a CLOSED replica again, before reading the key a second time. The cluster has 6 datanodes, so the reconstruction target doesn't have to be the datanode that lost the replica. The close-container wait and the replication manager and report intervals are shortened to keep the test at about 20 s. In the test log the replication manager sends a reconstructECContainersCommand and the datanode completes it about 5 s after the volume failure.

@umamaheswararao

Copy link
Copy Markdown
Contributor

I have a question here: When volume failed, why it is not triggering FCR? According to https://issues.apache.org/jira/browse/HDDS-8448, it should trigger FCR right. Upon processing FCR, aren't we closing the open containers for replication?

@yandrey321

Copy link
Copy Markdown
Contributor Author

When volume failed, why it is not triggering FCR? According to https://issues.apache.org/jira/browse/HDDS-8448, it should trigger FCR right. Upon processing FCR, aren't we closing the open containers for replication?

It does trigger the FCR: ContainerSet.handleVolumeFailures still sends it right away, as HDDS-8448 added (getFullContainerReportDiscardPendingICR() → refreshFullReport → triggerHeartbeat), and this PR doesn't change that. The problem is that processing the FCR doesn't close an OPEN EC container:

  1. The FCR only removes the replica. The lost container is missing from the report, so ContainerReportHandler.processMissingReplicas removes that datanode's replica from SCM (removeContainerReplica) and from the node's container set, and stops there. There is no close and no replication command.
  2. The replication manager doesn't treat the container as unhealthy. For an OPEN container, OpenContainerHandler closes it only if it has no healthy pipeline, or if one of the remaining replicas isn't OPEN (isOpenContainerHealthy checks allMatch over the replicas that are left). After the FCR, the 4 remaining replicas of the RS(3,2) container are all OPEN. The EC pipeline is still healthy too, because its datanodes are alive and heartbeating, so the container counts as healthy and stays OPEN.
  3. An OPEN container isn't replicated anyway. OpenContainerHandler.handle returns true for any OPEN container, which stops the handler chain, so under-replication isn't checked until the container is CLOSED. That is why it has to be closed first.

For Ratis the close doesn't come from the FCR either. The next write to the lost container fails in the Ratis state machine, and the datanode asks SCM to close the pipeline. EC writes go over gRPC and have no such path, so SCM kept allocating the container, and every write to the missing index failed with CONTAINER_MISSING.

The integration test shows this. With the volume-failure hook removed, TestECContainerCloseOnVolumeFailure still sends the FCR, yet times out with the container still OPEN in SCM. With the hook, the CLOSE action goes out in the same heartbeat as the FCR. The container then moves to CLOSING and CLOSED, and the missing index is reconstructed.

An alternative would be for SCM to close an OPEN EC container whose replica count falls below the required nodes. But SCM can't tell a lost replica from a newly allocated container whose replicas haven't been reported yet, so it could close new containers by mistake. Only the datanode knows that it lost the replica, so it reports it, the same way the Ratis path does.

@jojochuang

Copy link
Copy Markdown
Contributor

I screened all 324 unresolved issues returned by the Jira filter for Generated with Specula. These are the closest related reports; all were Open when checked:

Existing report Relationship to this PR
HDDS-17267 — Lost CLOSE actions Direct reliability dependency: can lose this PR's volume-failure notification.
HDDS-17269 — Queued heartbeats run after re-registration is requested Another way to drain and lose pending CLOSE actions.
HDDS-17064 — Failed heartbeat loses the full container report SCM can retain the lost replica in its accounting, delaying repair even if closure occurs.
HDDS-17062, HDDS-17063 — Stale ICRs follow the failure FCR Can re-add the lost replica to SCM's view and delay reconstruction.
HDDS-17018 — Container added after the failed-volume sweep Such a container escapes this PR's sweep and its proactive CLOSE action.
HDDS-17019 — DB-volume failure not propagated to data volumes The data-volume failure hook may never run for affected containers.
HDDS-16906 — Dead-member EC pipeline survives SCM leadership changes Similar allocation/repair symptoms, but a different trigger. A dead datanode cannot send this PR's notification.

These are existing adjacent defects, not regressions introduced by this PR. HDDS-17267 is the most important companion fix for making closure without subsequent writes dependable: the volume-failure hook queues the CLOSE action once and removes the replica locally; a failed heartbeat or SCM re-registration response can discard the action. A later failed EC write re-adds it, but an idle container can remain OPEN. Failure/re-registration coverage would help establish the retry behavior.

The earlier recovery-test concern has been addressed: the integration test now waits for CLOSED state and restoration of all five EC indexes. On head cbb84e256b4, the 24 focused unit tests and TestECContainerCloseOnVolumeFailure passed locally.

Generated-by: Codex (GPT-6)

@yandrey321

yandrey321 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

@jojochuang I'll pick up these Specula JIRAs and fix them as follow ups.

Co-authored-by: Codex (GPT-6)
Change-Id: Iedb9578a1589a9b25b855df381a148ab9913fc38
Co-authored-by: Codex (GPT-6)
Change-Id: If7d406e019f499fb2e1f6873be957d70d52129c1

@jojochuang jojochuang 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.

The EC write detection now has an explanatory comment, and master has been merged. Local validation on JDK 21 passed: TestHddsDispatcher (18 tests), TestVolumeSetDiskChecks (6 tests), TestECContainerCloseOnVolumeFailure (1 integration test), and repository checkstyle. No blocking findings from the source review. CI on this updated head is not yet confirmed green.

Generated-by: Codex (GPT-6)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants