[
https://issues.apache.org/jira/browse/IGNITE-28870?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18099606#comment-18099606
]
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
(12)|borderStyle=dashed|borderColor=#ccc|titleBGColor=#D6F7C1}
{color:#00008b}Snapshots 7{color} [[tests
12|https://ci2.ignite.apache.org/viewLog.html?buildId=9232779]]
* {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}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpExtraStorageCleanupPreservesTempRoot[encryption=false,
onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpExtraStorageCleanupPreservesTempRoot[encryption=false,
onlyPrimay=false] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpExtraStorageCleanupPreservesTempRoot[encryption=true,
onlyPrimay=true] - PASSED{color}
... and 1 new tests
{panel}
[TeamCity *--> Run :: All*
Results|https://ci2.ignite.apache.org/viewLog.html?buildId=9232787&buildTypeId=IgniteTests24Java8_RunAll]
{color:#ffffff}tcbot-analysis-comment chainBuildId=9232787
rerunBuildIds=9233278,9233280,9233282,9233284,9233286,9233288{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: 1h 10m
> 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
> h3. Extra-storage cleanup logic
> The temporary directory layouts for the default storage and extra storages
> are asymmetric.
> For the default storage, the snapshot-specific directory contains a nested
> nodeStorage:
> {code:java}
> snp/<snapshot-name>/db/<node-storage>{code}
> Therefore, cleanup first removes nodeStorage and then removes
> *snp/<snapshot-name>* only if it becomes empty or snapshot processing
> finishes with an error. The common *snp* root is preserved.
> For an extra storage, an *extraStorages()* value already points directly to
> the snapshot-specific directory:
> {code:java}
> snp/<snapshot-name>{code}
> There are two possible approaches:
> # Align extra-storage cleanup with the default-storage cleanup semantics:
> remove only *snp/<snapshot-name>* and preserve the empty common *snp* root.
> This approach is used in the fix.
> # Preserve the previous behavior that also removes the common snp root for
> extra storages, but then change the overall cleanup contract and the
> assertions in {*}afterTestSnapshot(){*}, which currently expect snapshot
> temporary roots to exist and contain no files.
> The first approach was selected because it provides consistent semantics for
> both storage types: snapshot-specific temporary data is removed, while the
> common snp root is preserved.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
