[
https://issues.apache.org/jira/browse/IGNITE-28870?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Oleg Valuyskiy updated IGNITE-28870:
------------------------------------
Description:
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
was:
Title:
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:
{code}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yamlsft.tempFileTree().root()
<work_dir>/db/<node_storage>/snp/<snapshot_name>
h1. sft.tempFileTree().nodeStorage()
<work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>
{code}
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:
{code}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yaml<work_dir>/db/<node_storage>/snp/<snapshot_name> -- root()
└── db/<node_storage> -- nodeStorage()
{code}
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}
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}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yaml<work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>
{code}
and then checking:
{code}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yaml<work_dir>/db/<node_storage>/snp/<snapshot_name>
{code}
the current implementation deletes:
{code}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yaml<work_dir>/db/<node_storage>/snp/<snapshot_name>
{code}
and then calls {{U.fileCount(...)}} for the same already deleted path.
This changes the original cleanup semantics. 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. Observable consequence
One visible consequence is a false ERROR in the log after a successful snapshot
creation:
{code}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yamlSnapshot directory doesn't exist [snpName=<snapshot_name>,
dir=<work_dir>/db/<node_storage>/snp]
{code}
This happens because {{U.fileCount(dir.toPath())}} is called after {{dir}} has
already been removed by {{{}U.delete(dir){}}}.
The snapshot itself may be created successfully. The error is produced by the
temporary files cleanup code.
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. Proposed fix
The helper should receive both directories explicitly:
{code:java}
private static void removeTmpDir(File nodeStorage, File root, boolean err,
IgniteLogger log) {
U.delete(nodeStorage);
{{// Delete snapshot temporary root if no other files exist or snapshot cleanup
is performed after an error.
try \{
if (err || U.fileCount(root.toPath()) == 0)
U.delete(root.toPath());
}
catch (NoSuchFileException ignored) \{
// Temporary snapshot root has already been removed.
}
catch (IOException e) \{
log.warning("Failed to clean up snapshot temporary directory " +
"[snpName=" + root.getName() + ", dir=" + root + ']', e);
}}}
}
{code}
And the cleanup should pass the node-specific storage and the corresponding
root separately:
{code:java}
public static void removeTmpSnapshotFiles(SnapshotFileTree sft, boolean err,
IgniteLogger log) {
NodeFileTree tmpFt = sft.tempFileTree();
{{removeTmpDir(tmpFt.nodeStorage(), tmpFt.root(), err, log);
for (File tmpDrStorage : tmpFt.extraStorages().values())
removeTmpDir(tmpDrStorage, tmpDrStorage.getParentFile(), err, log);}}
}
{code}
h3. Why the {{err}} flag is kept
The {{err}} flag is part of the original cleanup semantics and should be
preserved.
On the success path, the snapshot temporary root should be removed only if it
becomes empty after the node-specific temporary storage is removed.
On the error path, the temporary root of the current snapshot may contain
incomplete or invalid files. Therefore, it is reasonable to remove it
regardless of its contents.
The condition should check {{err}} first:
{code:java}
if (err || U.fileCount(root.toPath()) == 0)
{code}
This avoids unnecessary {{U.fileCount(...)}} calls when the root directory
should be removed anyway due to an error.
h3. Testing
The regression can be covered in {{IgniteSnapshotManagerSelfTest}} by checking
that snapshot temporary cleanup does not produce cleanup-related error messages
after snapshot manager scenarios.
The existing {{AbstractSnapshotSelfTest}} cleanup check verifies that snapshot
temporary roots are empty after test execution. However, it does not catch this
regression because the broken implementation still removes the temporary
directory, but then logs an error when trying to inspect the already deleted
path.
Therefore, the test should also verify that cleanup does not log errors such as:
{code}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yamlSnapshot directory doesn't exist [snpName=
{code}
or the new cleanup failure message:
{code}
Unable to find source-code formatter for language: text. Available languages
are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go,
groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl,
php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml,
yamlFailed to clean up snapshot temporary directory
{code}
h3. Impact
The issue does not necessarily mean that snapshot creation has failed. In the
common case, the snapshot may be created successfully.
However, the cleanup logic is currently incorrect: it deletes the snapshot
temporary root before performing the check that was originally intended to
decide whether that root should be removed.
The false ERROR in the log is one visible consequence of this broken cleanup
sequence.
> 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
>
>
> 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)
