Skip to content

Fix of delete of Ceph's snapshots from secondary storage #5130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rohityadavcloud merged 2 commits into from
Jun 25, 2021
Merged

Fix of delete of Ceph's snapshots from secondary storage #5130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eed2cd4
Fix of delete of Ceph's snapshots from secondary storage
slavkap Jun 18, 2021
178dfeb
Unit tests: remove HIGEST priority for delete operation
slavkap Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions ...e/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/CephSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,6 @@ public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
return StrategyPriority.HIGHEST;
}

if (SnapshotOperation.DELETE.equals(op)) {
return StrategyPriority.HIGHEST;
}

Copy link
Contributor

@sureshanaparti sureshanaparti Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, as delete op is not handled here, it shouldn't consider this strategy.

Copy link
Member

@GabrielBrascher GabrielBrascher Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sureshanaparti thanks for the code review. I would like to just add a note to avoid misunderstandings.

Delete operation for Ceph has always been supported by CephSnapshotStrategy due to the fact that the parent class of CephSnapshotStrategy is StorageSystemSnapshotStrategy, which handles the deletion of Ceph/RBD snapshots.

However, StorageSystemSnapshotStrategy does not handle backed snapshots via NFS in this context and when Ceph had features introduced the backup on secondary storage was not taken into account.

With the fix proposed by @slavkap, it was possible to check that nowadays DefaultSnapshotStrategy supports deletions of Snapshots on Ceph (with or without backup on NFS).

return StrategyPriority.CANT_HANDLE;
}

Expand Down
2 changes: 0 additions & 2 deletions ...apshot/src/test/java/org/apache/cloudstack/storage/snapshot/CephSnapshotStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ private void configureAndVerifyCanHandle(Date removed, boolean isSnapshotStoredO
StrategyPriority strategyPriority = cephSnapshotStrategy.canHandle(snapshot, snapshotOps[i]);
if (snapshotOps[i] == SnapshotOperation.REVERT && isSnapshotStoredOnRbdStoragePool) {
Assert.assertEquals(StrategyPriority.HIGHEST, strategyPriority);
} else if (snapshotOps[i] == SnapshotOperation.DELETE && isSnapshotStoredOnRbdStoragePool) {
Assert.assertEquals(StrategyPriority.HIGHEST, strategyPriority);
} else {
Assert.assertEquals(StrategyPriority.CANT_HANDLE, strategyPriority);
}
Expand Down