Skip to content

Update error message when no snapshot strategy is found #11455

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
sureshanaparti merged 1 commit into from
Aug 18, 2025
Merged

Update error message when no snapshot strategy is found #11455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions .../storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2795,6 +2795,8 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) {
} catch (CloudRuntimeException cre) {
logger.error("Take snapshot: {} failed", volume, cre);
throw cre;
} catch (UnsupportedOperationException ex) {
throw ex;
} catch (Exception e) {
if (logger.isDebugEnabled()) {
logger.debug("unknown exception while taking snapshot for volume {} was caught", volume, e);
Expand Down
6 changes: 2 additions & 4 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3814,10 +3814,8 @@ protected Volume liveMigrateVolume(Volume volume, StoragePool destPool) throws S

@Override
@ActionEvent(eventType = EventTypes.EVENT_SNAPSHOT_CREATE, eventDescription = "taking snapshot", async = true)
public Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account account, boolean quiescevm,
Snapshot.LocationType locationType, boolean asyncBackup, Map<String, String> tags, List<Long> zoneIds, List<Long> poolIds, Boolean useStorageReplication)

throws ResourceAllocationException {
public Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account account, boolean quiescevm, Snapshot.LocationType locationType, boolean asyncBackup,
Map<String, String> tags, List<Long> zoneIds, List<Long> poolIds, Boolean useStorageReplication) throws ResourceAllocationException {
final Snapshot snapshot = takeSnapshotInternal(volumeId, policyId, snapshotId, account, quiescevm, locationType, asyncBackup, zoneIds, poolIds, useStorageReplication);
if (snapshot != null && MapUtils.isNotEmpty(tags)) {
taggedResourceService.createTags(Collections.singletonList(snapshot.getUuid()), ResourceTag.ResourceObjectType.Snapshot, tags, null);
Expand Down
5 changes: 3 additions & 2 deletions server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1621,7 +1621,8 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
if (snapshotStrategy == null) {
_snapshotDao.remove(snapshotId);
logger.debug("No strategy found for creation of snapshot [{}], removing its record from the database.", snapshot);
throw new CloudRuntimeException(String.format("Can't find snapshot strategy to deal with snapshot:%s", snapshot.getSnapshotVO()));
throw new UnsupportedOperationException(String.format("Unable to find a snapshot strategy to create snapshot [%s] of volume [%s]. Please check the logs.",
Copy link
Member

@weizhouapache weizhouapache Aug 15, 2025

Choose a reason for hiding this comment

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

@JoaoJandre
thanks for the PR

this is not user-friendly I think, only administrator can access the check the logs

Copy link
Contributor Author

@JoaoJandre JoaoJandre Aug 15, 2025

Choose a reason for hiding this comment

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

@weizhouapache How would you change the message so that it is still generic (this is an error when no snapshot strategy is found; thus this message will be shown for other situations as well) and does not reveal the infrastructure information to the end user?

Copy link
Member

@weizhouapache weizhouapache Aug 15, 2025

Choose a reason for hiding this comment

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

I think more details but does not reveal the critical information would be better

for example, when the management server log says
2025-08-15 05:58:04,557 WARN [o.a.c.s.s.DefaultSnapshotStrategy] (Work-Job-Executor-6:[ctx-bcc76f85, job-179/job-180, ctx-5bbd5fed]) (logid:2d361233) VM [b50bdd89-1b6b-4ef3-bf85-c97d8cfa01a2] already has KVM File-Based storage VM snapshots. These VM snapshots and volume snapshots are not supported together for KVM. As restoring volume snapshots will erase the VM snapshots and cause data loss.

end users could get an error when create a volume snapshot, like
Volume snapshot is unsupported as this VM has a disk-only VM snapshots

btw: would it be possible to check the condition in a ealier stage ?

Copy link
Contributor

@sureshanaparti sureshanaparti Aug 18, 2025

Choose a reason for hiding this comment

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

@JoaoJandre please raise separate PR if error message can be improved. thanks.

Copy link
Contributor Author

@JoaoJandre JoaoJandre Aug 18, 2025

Choose a reason for hiding this comment

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

@weizhouapache We currently have 5 snapshot strategies. This error is thrown when no snapshot strategy supports the snapshot being taken. Thus, the message cannot be specific to a single strategy.

Depending on the storage/strategy being used, it might be possible to create volume snapshots and disk-only VM snapshots.

Copy link
Member

@weizhouapache weizhouapache Aug 18, 2025

Choose a reason for hiding this comment

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

@JoaoJandre
ok, ignore it if it is difficult to improve

snapshot.getSnapshotVO(), volume.getUuid()));
}

SnapshotInfo snapshotOnPrimary = snapshotStrategy.takeSnapshot(snapshot);
Expand Down Expand Up @@ -1672,7 +1673,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
} catch (Exception e) {
logger.debug("post process snapshot failed", e);
}
} catch (CloudRuntimeException cre) {
} catch (CloudRuntimeException | UnsupportedOperationException cre) {
if (logger.isDebugEnabled()) {
logger.debug("Failed to create snapshot" + cre.getLocalizedMessage());
}
Expand Down
Loading