Copilot commented on code in PR #11258:
URL: https://github.com/apache/cloudstack/pull/11258#discussion_r2222141372
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -62,16 +62,21 @@ public Answer execute(RestoreBackupCommand command,
LibvirtComputingResource ser
String restoreVolumeUuid = command.getRestoreVolumeUUID();
String newVolumeId = null;
- if (Objects.isNull(vmExists)) {
- String volumePath = volumePaths.get(0);
- int lastIndex = volumePath.lastIndexOf("/");
- newVolumeId = volumePath.substring(lastIndex + 1);
- restoreVolume(backupPath, backupRepoType, backupRepoAddress,
volumePath, diskType, restoreVolumeUuid,
- new Pair<>(vmName, command.getVmState()), mountOptions);
- } else if (Boolean.TRUE.equals(vmExists)) {
- restoreVolumesOfExistingVM(volumePaths, backupPath,
backupRepoType, backupRepoAddress, mountOptions);
- } else {
- restoreVolumesOfDestroyedVMs(volumePaths, vmName, backupPath,
backupRepoType, backupRepoAddress, mountOptions);
+ try {
+ if (Objects.isNull(vmExists)) {
+ String volumePath = volumePaths.get(0);
+ int lastIndex = volumePath.lastIndexOf("/");
+ newVolumeId = volumePath.substring(lastIndex + 1);
+ restoreVolume(backupPath, backupRepoType, backupRepoAddress,
volumePath, diskType, restoreVolumeUuid,
+ new Pair<>(vmName, command.getVmState()),
mountOptions);
+ } else if (Boolean.TRUE.equals(vmExists)) {
+ restoreVolumesOfExistingVM(volumePaths, backupPath,
backupRepoType, backupRepoAddress, mountOptions);
+ } else {
+ restoreVolumesOfDestroyedVMs(volumePaths, vmName, backupPath,
backupRepoType, backupRepoAddress, mountOptions);
+ }
+ } catch (CloudRuntimeException e) {
+ logger.error("Failed to restore backup for VM: " + vmName, e);
+ return new BackupAnswer(command, false, e.getMessage());
Review Comment:
The error message returned uses e.getMessage() which may be null or not
user-friendly. Consider providing a more descriptive error message that
includes context about the backup restoration failure.
```suggestion
String errorMessage = "Failed to restore backup for VM: " +
vmName + ".";
if (e.getMessage() != null && !e.getMessage().isEmpty()) {
errorMessage += " Details: " + e.getMessage();
}
return new BackupAnswer(command, false, errorMessage);
```
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -277,7 +278,8 @@ public boolean deleteBackupOffering(final Long offeringId) {
public static String createVolumeInfoFromVolumes(List<VolumeVO> vmVolumes)
{
Review Comment:
The parameter type has been changed from VolumeVO to Volume, but the method
signature still accepts List<VolumeVO>. Consider updating the method signature
to accept List<Volume> for consistency, or cast appropriately to maintain type
safety.
```suggestion
public static String createVolumeInfoFromVolumes(List<Volume> vmVolumes)
{
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
