[jira] [Commented] (IGNITE-28870) Restore snapshot temporary files cleanup logic

["Ignite TC Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/IGNITE-28870?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095669#comment-18095669 ] 

Ignite TC Bot commented on IGNITE-28870:
----------------------------------------

{panel:title=Branch: [pull/13358/head] Base: [master] : No blockers found!|borderStyle=dashed|borderColor=#ccc|titleBGColor=#D6F7C1}{panel}
{panel:title=Branch: [pull/13358/head] Base: [master] : New Tests (8)|borderStyle=dashed|borderColor=#ccc|titleBGColor=#D6F7C1}
{color:#00008b}Snapshots 7{color} [[tests 8|https://ci2.ignite.apache.org/viewLog.html?buildId=9201595]]
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=false, onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=false, onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=false, onlyPrimay=false] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=false, onlyPrimay=false] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=true, onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=true, onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=true, onlyPrimay=false] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7: IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=true, onlyPrimay=false] - PASSED{color}

{panel}
[TeamCity *--> Run :: All* Results|https://ci2.ignite.apache.org/viewLog.html?buildId=9201603&buildTypeId=IgniteTests24Java8_RunAll]
{color:#ffffff}tcbot-analysis-comment chainBuildId=9201603 rerunBuildIds=9202192{color}

> Restore snapshot temporary files cleanup logic
> ----------------------------------------------
>
>                 Key: IGNITE-28870
>                 URL: https://issues.apache.org/jira/browse/IGNITE-28870
>             Project: Ignite
>          Issue Type: Bug
>            Reporter: Oleg Valuyskiy
>            Assignee: Oleg Valuyskiy
>            Priority: Minor
>              Labels: ise
>         Attachments: SnapshotTmpDirCleanupReproducer.patch
>
>          Time Spent: 50m
>  Remaining Estimate: 0h
>
> h3. Problem
> After the changes introduced in IGNITE-24130 / PR #11896, the temporary snapshot files cleanup logic was accidentally changed. Previously, snapshot cleanup used two different directories:
> {code:java}
> U.delete(sft.tempFileTree().nodeStorage());
> // Delete snapshot directory if no other files exists.
> try {
>     if (U.fileCount(sft.tempFileTree().root().toPath()) == 0 || err != null)
>         U.delete(sft.tempFileTree().root().toPath());
> }
> catch (IOException e) {
>     log.error("Snapshot directory doesn't exist [snpName=" + snpName + ", dir=" + sft.tempFileTree().root() + ']');
> }
> {code}
> These two directories represent different levels of the temporary snapshot file tree. For example:
>  * sft.tempFileTree().root() - <work_dir>/db/<node_storage>/snp/<snapshot_name>
>  * sft.tempFileTree().nodeStorage() - <work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>
> So the original cleanup logic was:
>  * remove the node-specific temporary snapshot storage directory
>  * then check the temporary root directory of the current snapshot
>  * remove the root directory only if it becomes empty
>  * remove the root directory regardless of its contents if snapshot processing finished with an error, because the temporary state of the current snapshot is considered invalid
> In other words, the original logic intentionally worked with two different filesystem levels.
> h3. Regression
> After IGNITE-24130 / PR #11896, the cleanup logic was refactored into a helper method:
> {code:java}
> public static void removeTmpSnapshotFiles(SnapshotFileTree sft, boolean err, IgniteLogger log) {
>     NodeFileTree tmpFt = sft.tempFileTree();
>     ```
>     removeTmpDir(tmpFt.root(), err, log);
>     for (File tmpDrStorage : tmpFt.extraStorages().values())
>         removeTmpDir(tmpDrStorage.getParentFile(), err, log);
>     ```
> }
> {code}
> And the helper currently accepts only one directory:
> {code:java}
> private static void removeTmpDir(File dir, boolean err, IgniteLogger log) {
>     U.delete(dir);
>     ```
>     // Delete snapshot directory if no other files exists.
>     try {
>         if (U.fileCount(dir.toPath()) == 0 || err)
>             U.delete(dir.toPath());
>     }
>     catch (IOException e) {
>         log.error("Snapshot directory doesn't exist [snpName=" + dir.getName() + ", dir=" + dir.getParentFile() + ']');
>     }
>     ```
> }
> {code}
> This accidentally collapses the original two-level cleanup logic into a single directory. As a result, the code now:
>  * removes the snapshot temporary root directory
>  * then tries to count files in the same directory
>  * then tries to delete the same directory again
> For example, instead of deleting:
> {code:java}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>{code}
> and then checking:
> {code:java}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>{code}
> the current implementation deletes:
> {code:java}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>{code}
> and then calls *U.fileCount(...)* for the same already deleted path. This leads to a  false ERROR in the log after a successful snapshot creation:
> {code:java}
> Snapshot directory doesn't exist [snpName=<snapshot_name>, dir=<work_dir>/db/<node_storage>/snp]{code}
> (this is how the issue was identified in the first place, reproducer: [^SnapshotTmpDirCleanupReproducer.patch])
> Thus, because of the cahnges introduced in IGNITE-24130 the original cleanup logic was changed. The success path became more aggressive: the whole snapshot temporary root is removed before checking whether it still contains any unexpected files. Previously, such a non-empty root was not removed on the success path.
> h3. Expected behavior
> The cleanup logic should restore the original two-level behavior:
>  * remove the node-specific temporary snapshot storage directory
>  * then check the snapshot temporary root directory
>  * remove the root directory if it becomes empty
>  * remove the root directory regardless of its contents if snapshot processing finished with an error
>  * do not call *U.fileCount(...)* on the directory that has just been deleted
>  * do not log an error when the temporary directory has already been successfully removed



--
This message was sent by Atlassian Jira
(v8.20.10#820010)