[
https://issues.apache.org/jira/browse/IGNITE-28870?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Oleg Valuyskiy updated IGNITE-28870:
------------------------------------
Description:
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:
h1. {code:text}
sft.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:text}
<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:text}
<work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>
{code}
and then checking:
{code:text}
<work_dir>/db/<node_storage>/snp/<snapshot_name>
{code}
the current implementation deletes:
{code:text}
<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:text}
Snapshot 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:text}
Snapshot directory doesn't exist [snpName=
{code}
or the new cleanup failure message:
{code:text}
Failed 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.
was:
After the changes introduced in IGNITE-24130 / PR #11896, Ignite may log the
following ERROR after a snapshot is successfully created:
{code:java}
Snapshot directory doesn't exist [snpName=<snapshot_name>,
dir=<work_dir>/db/<node_storage>/snp]{code}
The snapshot itself may be successfully created in the configured snapshot
directory. The error is logged during cleanup of temporary snapshot files.
Reproducer: [^SnapshotTmpDirCleanupReproducer.patch]
h3. Original cleanup logic
Before IGNITE-24130 / PR #11896, snapshot cleanup used two different temporary
directories ({*}SnapshotFutureTask#onDone{*}):
{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 filesystem levels:
* 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 logic was:
* remove the node-specific temporary snapshot storage directory;
* then check the temporary root directory of the current snapshot;
* if the root directory has no other files left, remove it as well;
* if snapshot processing finished with an error, remove the temporary root
directory regardless of its contents.
h3. Regression after IGNITE-24130 / PR #11896
After the data region storage path support changes, 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 receives 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. The method now:
* removes the snapshot temporary root directory;
* then tries to count files in that same directory;
* then tries to delete the same directory again.
So after:
{code:java}
U.delete(dir);{code}
the following call may fail with NoSuchFileException:
{code:java}
U.fileCount(dir.toPath()){code}
This results in a false ERROR in the log even though the snapshot temporary
directory has already been successfully removed.
h3. Expected behavior
The cleanup logic should preserve the original two-level semantics:
* remove the node-specific temporary snapshot storage directory;
* check the snapshot temporary root directory;
* remove the root directory if it becomes empty;
* remove the root directory 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 directory has already been removed successfully.
> 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
>
>
> 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:
> h1. {code:text}
> sft.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:text}
> <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:text}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>
> {code}
> and then checking:
> {code:text}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>
> {code}
> the current implementation deletes:
> {code:text}
> <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:text}
> Snapshot 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:text}
> Snapshot directory doesn't exist [snpName=
> {code}
> or the new cleanup failure message:
> {code:text}
> Failed 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
