[jira] [Created] (HDDS-17228) A failed rewrite of a VERSION file publishes the empty temporary file, so SCM, OM or a datanode cannot start again

["Siyao Meng (Jira)" <ji...@apache.org>]

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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org