abh1sar commented on code in PR #12549:
URL: https://github.com/apache/cloudstack/pull/12549#discussion_r2772299010
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -215,7 +215,7 @@ private BackupVO createBackupObject(VirtualMachine vm,
String backupPath) {
public boolean restoreVMFromBackup(VirtualMachine vm, Backup backup) {
List<Backup.VolumeInfo> backedVolumes = backup.getBackedUpVolumes();
List<VolumeVO> volumes = backedVolumes.stream()
- .map(volume -> volumeDao.findByUuid(volume.getUuid()))
+ .map(volume -> volumeDao.findByUuid(volume.getPath()))
Review Comment:
I think we should not have path in the backed-up volumes metadata at all.
1. Backup files are saved by the volume uuid name
2. The path in backed-up volumes is not being referenced anywhere apart from
UI
volume.getPath() already gives us the path where we have to restore, no
point in maintaining it in backup metadata also and updating it whenever volume
path changes.
There was a PR merged on main that makes the UI reference uuid instead of
path. (https://github.com/apache/cloudstack/pull/12156)
So I propose removing `path` entirely from Backup.VolumeInfo in the main
branch. We don't need upgrade handling also. The `path` in the DB for older
backups will simply get ignored.
Now in the context of this PR, we should get the path from volume.getPath()
not from backup-up volumes metadata.
Thoughts? @sureshanaparti @Pearl1594
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
