Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14255 +/- ##
============================================
+ Coverage 19.91% 20.40% +0.48%
- Complexity 20200 21289 +1089
============================================
Files 6373 6482 +109
Lines 577230 588079 +10849
Branches 70696 72022 +1326
============================================
+ Hits 114974 119985 +5011
- Misses 449690 455073 +5383
- Partials 12566 13021 +455
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I forgot to change the test, so I will leave this one as draft for a bit before I can fix it. |
| response.setToCheckpointId(backup.getToCheckpointId()); | ||
| } | ||
|
|
||
| if (KBOSS_BACKUP_PROVIDER.equals(offering.getProvider()) && isCallerRootAdmin) { |
There was a problem hiding this comment.
you are fetching offering from dao layer and, what if it would be null? we can have null check here for offering to ignore NPE
| Map<String, String> backupVolumePaths = new HashMap<>(); | ||
|
|
||
| backupJoins.forEach(b -> { | ||
| backupVolumePaths.put(b.getVolumeName(), b.getImageStorePath()); |
There was a problem hiding this comment.
CloudStack does not enforce uniqueness for volume names. The same volume name can be used across multiple VMs, and users can also rename volumes after creation. Given this, if multiple volumes end up sharing the same name, this put() operation could silently overwrite the path mapping for an unintended volume, potentially resulting in incorrect path associations. you can count on volumeUUID instead.
Additionally, considering that multiple secondary storage pools can exist within a zone, we could optimise the map value by storing a composite identifier that includes both the image path and the image UUID/name, rather than just the imageStorePath. This would provide a more reliable way to uniquely identify image locations while also improving maintainability and future scalability.
There was a problem hiding this comment.
Current tests are only happy paths, can we add few negative ones also for example,
- what would happen for non-admin user
- anything other than KBOSS
- null offerings
|
@rajiv-jain-netapp, thanks for the reviews! I made the following changes to this PR: Implemented null check for offerings, Changed how the backup path is stored in the response map and improved the test cases. For the backup path map, thank you for pointing out about the same volume name replacement scenario. I changed the map to now use the volume UUID as key. Also, I created a new structure for the map value that stores the volume name and its image storage path as attributes. Regarding the usage of storage data for the response, as the only value stored by For the tests, I extracted the
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
6 open issues
v.name/v.uuidbut does not aggregate them or include them inGROUP BY. On… · NewObject.values(dataResource[item])will throw ifdataResource.backuppathsisnull/undefined… · NewcreateBackupResponseis called in a loop forlistBackups, and this adds a DAO call per backup… · New
Review effort: Lite
| String getBackupNameFromVM(VirtualMachine vm); | ||
|
|
||
| BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails); | ||
| BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails, boolean isCallerRootAdmin); |
| MAX(CASE WHEN bd.name = 'current' THEN bd.value END) current, | ||
| COALESCE(MAX(CASE WHEN bd.name = 'isolated' THEN bd.value END), 'false') isolated, | ||
| nbpr.volume_id, | ||
| v.name as volume_name, | ||
| v.uuid as volume_uuid, | ||
| nbsr.path image_store_path | ||
| FROM backups b | ||
| LEFT JOIN backup_details bd ON b.id = bd.backup_id | ||
| LEFT JOIN backup_offering bo ON b.backup_offering_id = bo.id | ||
| LEFT JOIN internal_backup_store_ref nbsr ON b.id = nbsr.backup_id | ||
| LEFT JOIN internal_backup_pool_ref nbpr ON nbpr.volume_id = nbsr.volume_id | ||
| LEFT JOIN volumes v ON v.id = nbsr.volume_id | ||
| WHERE bo.provider='kboss' | ||
| GROUP BY b.id, nbsr.volume_id; |
| <div v-else-if="$route.meta.name === 'backup' && item === 'backuppaths'"> | ||
| <div v-for="(backupVolumeInfo, idx) in Object.values(dataResource[item])" :key="idx"> | ||
| <span>{{ backupVolumeInfo.volumeName }} - {{ backupVolumeInfo.backupPath }}</span> | ||
| </div> | ||
| </div> |
| public class BackupVolumeNameAndPath { | ||
| private String volumeName; | ||
| private String backupPath; | ||
|
|
||
| public BackupVolumeNameAndPath(String volumeName, String backupPath) { | ||
| this.volumeName = volumeName; | ||
| this.backupPath = backupPath; | ||
| } |
| if (offering != null && KBOSS_BACKUP_PROVIDER.equals(offering.getProvider()) && isCallerRootAdmin) { | ||
| List<InternalBackupJoinVO> backupJoins = internalBackupJoinDao.listById(backup.getId()); | ||
| Map<String, BackupVolumeNameAndPath> backupVolumePaths = new HashMap<>(); | ||
|
|
||
| backupJoins.forEach(b -> { | ||
| backupVolumePaths.put(b.getVolumeUuid(), new BackupVolumeNameAndPath(b.getVolumeName(), b.getImageStorePath())); | ||
| }); | ||
|
|
||
| response.setBackupPaths(backupVolumePaths); | ||
| } |
| @SerializedName(ApiConstants.BACKUP_PATHS) | ||
| @Param(description = "The backups paths in the secondary storage") | ||
| private Map<String, BackupVolumeNameAndPath> backupPaths; |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
12 open issues
dataResource[item]can benullfor non-Root Admin users (per the intended behavior on the API… · NewObject.values(dataResource[item])will throw ifdataResource.backuppathsisnull/undefined…v.name/v.uuidbut does not aggregate them or include them inGROUP BY. On…v.nameandv.uuidwhile grouping by onlyb.id, nbsr.volume_id.… · NewlistBackupsreturns many results for a Root Admin on KBOSS, this adds a per-backup DAO call… · NewcreateBackupResponseis called in a loop forlistBackups, and this adds a DAO call per backup…keycan cause incorrect re-rendering if the object entries… · New
Review effort: Lite
| <div v-else-if="$route.meta.name === 'backup' && item === 'backuppaths'"> | ||
| <div v-for="(backupVolumeInfo, idx) in Object.values(dataResource[item])" :key="idx"> | ||
| <span>{{ backupVolumeInfo.volumeName }} - {{ backupVolumeInfo.backupPath }}</span> | ||
| </div> | ||
| </div> |
| String getBackupNameFromVM(VirtualMachine vm); | ||
|
|
||
| BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails); | ||
| BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails, boolean isCallerRootAdmin); |
| MAX(CASE WHEN bd.name = 'current' THEN bd.value END) current, | ||
| COALESCE(MAX(CASE WHEN bd.name = 'isolated' THEN bd.value END), 'false') isolated, | ||
| nbpr.volume_id, | ||
| v.name as volume_name, | ||
| v.uuid as volume_uuid, | ||
| nbsr.path image_store_path | ||
| FROM backups b | ||
| LEFT JOIN backup_details bd ON b.id = bd.backup_id | ||
| LEFT JOIN backup_offering bo ON b.backup_offering_id = bo.id | ||
| LEFT JOIN internal_backup_store_ref nbsr ON b.id = nbsr.backup_id | ||
| LEFT JOIN internal_backup_pool_ref nbpr ON nbpr.volume_id = nbsr.volume_id | ||
| LEFT JOIN volumes v ON v.id = nbsr.volume_id | ||
| WHERE bo.provider='kboss' | ||
| GROUP BY b.id, nbsr.volume_id; |
| if (offering != null && KBOSS_BACKUP_PROVIDER.equals(offering.getProvider()) && isCallerRootAdmin) { | ||
| List<InternalBackupJoinVO> backupJoins = internalBackupJoinDao.listById(backup.getId()); | ||
| Map<String, BackupVolumeNameAndPath> backupVolumePaths = new HashMap<>(); | ||
|
|
||
| backupJoins.forEach(b -> { | ||
| backupVolumePaths.put(b.getVolumeUuid(), new BackupVolumeNameAndPath(b.getVolumeName(), b.getImageStorePath())); | ||
| }); | ||
|
|
||
| response.setBackupPaths(backupVolumePaths); | ||
| } |
| @SerializedName(ApiConstants.BACKUP_PATHS) | ||
| @Param(description = "The backups paths in the secondary storage") | ||
| private Map<String, BackupVolumeNameAndPath> backupPaths; |
| </div> | ||
| </div> | ||
| <div v-else-if="$route.meta.name === 'backup' && item === 'backuppaths'"> | ||
| <div v-for="(backupVolumeInfo, idx) in Object.values(dataResource[item])" :key="idx"> |
Description
Currently, the backup's path in the secondary storage is not displayed in the GUI, being necessary to query it directly from the database. Therefore, the
backuppathsattribute, with the path of the backup of each VM's disk was added to thelistBackupsAPI response, and to the backup details in the GUI. Only Root Admin accounts are allowed to view the backup paths in the GUI and in the API response. Currently only supported byKBOSSprovider.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
Backup paths in the GUI
How Has This Been Tested?
I configured KBOSS as the backup provider, and created a new backup of an existing VM. Then, I accessed the backup's details tab and validated that the new field was added successfully. I also validated that the
backuppathsattribute was present at thelistBackupsAPI response.