[
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
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.
was:
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
> 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
> 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)
