Siyao Meng created HDDS-17227:
---------------------------------
Summary: A failed rewrite of datanode.id leaves it empty, so the
datanode loses its operational state and initial version on the next start
Key: HDDS-17227
URL: https://issues.apache.org/jira/browse/HDDS-17227
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-6-replace-datanode-id-atomically.patch,
TestBugCR6DatanodeIdLostOnFailedWrite.java
h3. Mechanism
{{ContainerUtils.writeDatanodeDetailsTo}} deletes an existing {{datanode.id}}
and creates it again empty before {{DatanodeIdYaml.createDatanodeIdFile}}
writes the new content through {{YamlUtils.dump}}. That helper writes to a
temporary file with Ratis {{AtomicFileOutputStream}} and moves it over the
target when the stream is closed. Two things defeat the atomic write:
* the delete: any write that does not complete leaves an empty file, or no file
if the process dies between the delete and the create;
* {{YamlUtils.dump}} closes the stream in a try with resources also when
{{yaml.dump}} throws, so after a failed write, for example on a full disk, the
empty or partial temporary file is still moved over the target. The stream's
{{abort()}} is never called.
On the next start {{ContainerUtils.readDatanodeDetailsFrom}} finds the empty
file and recovers only the UUID from the volume VERSION file (HDDS-13955). The
operational state becomes IN_SERVICE, its expiry 0, the certificate serial ID
null and the initial version {{DEFAULT_VERSION}}. Hostname and software version
are set again on every start, so their loss has no effect.
h3. Trigger
No crash is needed, a write that fails while the datanode keeps running is
enough:
* an I/O error while the content is written, for example a full metadata disk
({{File too large}} under a file size limit was run as well);
* an I/O error when the temporary file is created;
* an interrupted command thread: {{DatanodeStateMachine.close}} interrupts the
thread that runs {{SetNodeOperationalStateCommandHandler}}, and the write fails
with {{ClosedByInterruptException}}.
The file is rewritten through this method for every
{{SetNodeOperationalStateCommand}}, on every datanode start
({{InitDatanodeState}}) and on certificate rotation in secure mode
({{HddsDatanodeService.saveNewCertId}}). The missing file case needs a crash
between two consecutive system calls and was not run.
h3. Impact
>From 2.2.0 the datanode starts after an empty {{datanode.id}}, with these
>losses:
* The initial version does not heal: it is only set when {{datanode.id}} is
created, and {{XceiverServerRatis.assignPorts}} uses the separate Ratis server
and admin ports only for an initial version that supports them. After one such
start the datanode serves Raft on its client port (9858 by default) instead of
the server port (9856) for good. By code reading, existing pipelines whose Raft
groups hold the old server address then cannot reach this datanode until SCM
closes them; not run on a cluster.
* The persisted operational state is IN_SERVICE with no maintenance end time. A
stable leader SCM still holds the state and sends it again on the next
heartbeat. By code reading the state is lost for good when no SCM holds it at
that point, for example after all SCMs restarted, or when leadership moves to a
follower that adopted the reported IN_SERVICE before the correction reached the
datanode; not run.
* The certificate serial ID in the file is lost.
In 2.0.x and 2.1.x, which have the atomic write (HDDS-12531, first released in
2.0.0) but not the recovery (HDDS-13955), an empty {{datanode.id}} stops the
datanode from starting with the {{NullPointerException}} quoted in HDDS-13955
until an operator repairs the file. Releases before 2.0.0 wrote the file in
place and emptied it on a failed write as well. No container data is lost.
h3. Reproduction
PASS on unmodified source, deterministic.
[^TestBugCR6DatanodeIdLostOnFailedWrite.java]
({{hadoop-hdds/container-service}}) writes {{datanode.id}} with
ENTERING_MAINTENANCE, an end time, a certificate serial ID and the current
software version as initial version, next to a VERSION file. It then makes one
write fail in three ways: the real {{SetNodeOperationalStateCommandHandler}}
with an interrupted thread, the same handler with an I/O error when the
temporary file is created, and {{YamlUtils.dump}} called directly with a
{{Yaml}} whose writer fails every write, as a full disk does. After each
failure the file has length 0, and reading it with
{{ContainerUtils.readDatanodeDetailsFrom}}, as a restart does, gives the right
UUID with {{persistedOpState=IN_SERVICE}}, expiry 0, null certificate serial ID
and {{initialVersion=DEFAULT_VERSION}}. A passing test means the defect is
present.
h3. Patch
[^CR-6-replace-datanode-id-atomically.patch], against
b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. It also applies to master at
20d95ed37fe.
{{writeDatanodeDetailsTo}} no longer deletes and recreates an existing file; it
only creates the parent directory when the file does not exist yet.
{{YamlUtils.dump}} now flushes and closes the stream only after {{yaml.dump}}
succeeded, which moves the temporary file over the target, and calls
{{abort()}} on any failure, which deletes the temporary file and leaves the
target alone. A failed write now keeps the previous content and leaves no
temporary file. A successful write produces the same file as before. The change
is in the shared {{YamlUtils.dump}}, so its other callers get the same
behaviour.
Covered by the new {{TestYamlUtils}} ({{hadoop-hdds/framework}}): a write error
inside {{yaml.dump}} must keep the previous content and leave no temporary
file, and a later write must succeed; a second case covers an {{Error}} during
serialization. Also by {{testFailedDatanodeIdWriteKeepsPreviousFile}} in the
existing {{TestContainerUtils}}, for an error when the temporary file is
created. Both fail without the change because the file is empty. With the
change the tests of {{YamlUtils}}, {{ContainerUtils}}, {{DatanodeIdYaml}} and
the other {{YamlUtils.dump}} callers, {{TestDecommissionAndMaintenance}} on a
mini cluster (9 tests) and checkstyle pass. With the patch all three
reproduction tests fail because the file keeps its 346 bytes and the old state.
Under a file size limit of 0, a real {{File too large}} write error left the
file at 0 bytes before the change and kept its 35 bytes after it.
Not changed: the parent directory is not synced after the rename, so after a
power loss some file systems may show the old file, never an empty one. A write
error inside {{yaml.dump}} surfaces as an unchecked {{YAMLException}} that gets
past the callers' {{catch (IOException)}}, so their handling of a failed write,
such as the shutdown in {{InitDatanodeState}}, is skipped.
Found by code review of the SCM datanode decommission and maintenance admin
paths, as part of the TLA+ verification effort under HDDS-15926, on commit
b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-13955 (recovers the UUID from an empty file, the symptom),
HDDS-5309 (empty {{datanode.id}} after a restart, closed without a cause),
HDDS-12843 (empty {{datanode.id}} in {{TestDecommissionAndMaintenance}}, fixed
the in memory update order) and HDDS-12608 (VERSION file creation race). The
attached patch is a proposal for review. Generated with Specula (Claude Opus
5.5 and Claude Opus 5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
