Siyao Meng created HDDS-17228:
---------------------------------
Summary: A failed rewrite of a VERSION file publishes the empty
temporary file, so SCM, OM or a datanode cannot start again
Key: HDDS-17228
URL: https://issues.apache.org/jira/browse/HDDS-17228
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-13-keep-version-file-on-failed-write.patch,
TestBugCR13VersionFileLostOnFailedWrite.java
h3. Mechanism
{{IOUtils.writePropertiesToFile}} opens an {{AtomicFileOutputStream}} as a try
with resources resource and calls {{Properties.store}} inside the block.
{{Properties.store}} flushes its buffered writer before it returns, so a write
that fails (full disk, quota, failing device) throws from inside the block with
part of the content, or none of it, already in the temporary file. The implicit
{{close()}} then runs anyway, and {{AtomicFileOutputStream.close()}} moves the
temporary file over the target. The previous file is replaced by an empty or
truncated one, which is the opposite of what the atomic write is there for.
{{AtomicFileOutputStream.abort()}}, which deletes the temporary file instead,
is never called.
This is the helper that writes every {{VERSION}} file.
{{Storage.persistCurrentState}} and {{Storage.forceInitialize}} call it through
{{StorageInfo.writeTo}} on an existing file, so each of these rewrites can
destroy it: the datanode layout {{VERSION}} on every datanode start
({{OzoneContainer.initializeContainerServices}}, also
{{DatanodeVersionManager}}), OM and Recon certificate renewal
({{OzoneManager.saveNewCertId}}, {{ReconServer.saveNewCertId}}), SCM root CA
rotation ({{RootCARotationHandlerImpl}}), SCM and OM layout finalization
({{ScmVersionManager}}, {{OMVersionManager}}), and the SCM conversions that
call {{forceInitialize}} on a cluster that already holds data
({{StorageContainerManager.initializeSecurityIfNeeded}} when a non secure
cluster is made secure, {{initializeRatis}} when Ratis is enabled). Datanode
volume {{VERSION}} files are only created, never rewritten, so they are not
affected.
h3. Trigger
# A service rewrites its {{VERSION}} file. The most frequent case is routine:
every datanode start rewrites the datanode layout {{VERSION}} with the content
it has just read, from {{OzoneContainer.start}} on the first SCM handshake.
Certificate renewal (OM, Recon), root CA rotation and layout finalization do
the same for their own file.
# The write of that small file fails. A full or quota limited metadata volume,
or a device error, is enough.
# The helper commits the temporary file anyway, so {{VERSION}} is now empty or
holds a fragment.
No client request and no admin command is involved; both the write and the
failure are internal.
h3. Impact
* The service cannot read its own {{VERSION}} file after this. {{new
StorageInfo(type, versionFile)}} throws {{NullPointerException}} from
{{NodeType.valueOf(null)}}, so SCM, OM, Recon or the datanode layout storage
fails to start until an operator rebuilds the file by hand (run, see
Reproduction). A datanode whose metadata volume is full or failing therefore
destroys its layout {{VERSION}} on a restart and then cannot start at all,
because {{HddsDatanodeService.start}} constructs {{DatanodeStorage}} over the
same file.
* On the certificate paths the service also takes itself down right away:
{{OzoneManager.saveNewCertId}} calls {{shutDown}},
{{ReconServer.saveNewCertId}} calls {{terminateRecon}} and
{{RootCARotationHandlerImpl}} calls {{scm.shutDown}} when the write throws. The
node therefore stops and then cannot come back up, with no warning that the
file it was protecting is already gone (code reading).
* The helper has been in this form since HDDS-12456, so 2.0.0, 2.1.x and 2.2.x
are affected. Before that change {{StorageInfo.writeTo}} wrote in place, where
a failure before the first byte left the old content intact.
* No user data is lost; the damage is to service metadata, and the clusterID
and certificate serial id in a lost {{VERSION}} file have to be reconstructed
by the operator.
h3. Reproduction
PASS on an unfixed checkout, deterministic, no mocking framework.
[^TestBugCR13VersionFileLostOnFailedWrite.java] ({{hadoop-hdds/framework}})
creates a minimal {{Storage}} for an SCM style {{VERSION}} file, writes it (175
bytes), then rewrites it with a {{Properties}} whose {{store}} fails the way a
full disk does. The {{VERSION}} file is 8 bytes (or 0 bytes in the second test)
afterwards, no temporary file is left, and constructing the {{Storage}} again
throws {{NullPointerException}}, which is the start failure. A passing test
means the defect is present. The failure is injected in the test; a real out of
space condition was not exercised.
h3. Patch
[^CR-13-keep-version-file-on-failed-write.patch], against
b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. It also applies to master at
20d95ed37fe.
The helper now keeps the {{AtomicFileOutputStream}} out of the resource list,
writes, and closes it only on success, and calls {{abort()}} on any failure, so
the temporary file is deleted and the target keeps its previous content. The
success path writes the same bytes, and {{abort()}} swallows I/O errors while
deleting the temporary file, so the caller still sees the original failure.
Regression tests: {{TestIOUtils#failedPropertiesWriteKeepsPreviousFile}} and
{{TestIOUtils#uncheckedWriteFailureKeepsPreviousFile}}, both failing without
the change. The tests of the affected callers ({{TestDatanodeVersionFile}},
{{TestHddsVolume}}, {{TestDatanodeVersionManager}}, {{TestScmVersionManager}},
{{TestEndPoint}}, {{TestOMStorage}}, {{TestOMVersionManager}}) and checkstyle
pass. One behaviour note for review: after the change a failed certificate id
persist leaves the previous serial id in {{VERSION}}, so the service starts
again with the previous certificate instead of not starting at all.
HDDS-17227 fixes the same pattern in {{YamlUtils.dump}}, the helper that writes
the YAML files; the two changes touch different files and can go in in either
order.
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. The attached patch is
a proposal for review. Generated with Specula (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]
