[
https://issues.apache.org/jira/browse/HDDS-17228?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17228:
------------------------------
Attachment: TestBugCR13VersionFileLostOnFailedWrite.java
> 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
> Priority: Major
> 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]
