Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7221 +/- ##
============================================
- Coverage 15.42% 4.18% -11.24%
============================================
Files 5469 369 -5100
Lines 478292 30284 -448008
Branches 58134 5359 -52775
============================================
- Hits 73775 1268 -72507
+ Misses 396389 28872 -367517
+ Partials 8128 144 -7984
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
SonarCloud Quality Gate failed. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
harikrishna-patnala
left a comment
harikrishna-patnala
left a comment
There was a problem hiding this comment.
Thanks @SadiJr, except the suggested few changes, the remaining code LGTM.
| @Parameter(name = ApiConstants.START_VM, | ||
| type = CommandType.BOOLEAN, | ||
| required = false, | ||
| description = "start VM after the restore is finished") |
There was a problem hiding this comment.
| @Parameter(name = ApiConstants.START_VM, | |
| type = CommandType.BOOLEAN, | |
| required = false, | |
| description = "start VM after the restore is finished") | |
| @Parameter(name = ApiConstants.START_VM, | |
| type = CommandType.BOOLEAN, | |
| required = false, | |
| description = "start VM after the restore is finished", | |
| since = "4.19.0") |
There was a problem hiding this comment.
this will power on the vm from veeam, looks like a out-of-band operation (the power state, host_id, etc are not managed by cloudstack).
@SadiJr
I think we should avoid it. cc @DaanHoogland
There was a problem hiding this comment.
@weizhouapache when using the option to power on VM from Veeam, it will only start the VM in the last host the VM was allocated to. Also, there is a process in ACS that updates the VM state from time to time, according to what is collected from vCenter. Additionally, the HA process in VMWare is also not managed by ACS. Having said all that, I believe that if Veeam has this option, we should leave it up to the user to decide, not us.
There was a problem hiding this comment.
@SadiJr
You can do lots of operations on vcenter or veeam, but cloudstack does not support OOB operations perfectly.
We should avoid the OOB operations as much as possible, unless it is really neccessary.
I think it is unnecessary to add the option. If you insist to support, I would rather starting the vm by invoking cloudstack methods/API (as cloudstack will check the proper host by available resources, affinity groups, and apply network rules, etc), than starting the vm via veeam.
Let me give you an example, if you stop a vm, and the network is shutdown (VR is stopped), when you start the vm in vcenter or veeam, the vm has no internet connection as the VR is still stopped.
We have actually faced several issues caused by OOB operations (start/migration), or DRS.
There was a problem hiding this comment.
@weizhouapache I have removed this feature from this PR as it is not in the core of it. In the future I might open a PR to add it separately and we can discuss it further on that PR.
|
@SadiJr can you please check the review comments? |
|
@SadiJr are you still looking to get this merged? |
|
@shwstppr @DaanHoogland Sorry for the delay, I will check the suggestions and respond/work on this PR |
@JoaoJandre |
|
@weizhouapache a [SL] Trillian-Jenkins test job (rocky8 mgmt + vmware-80) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-10542)
|
@weizhouapache Ok, I'll try to test this and see if everything still looks good after merging with main. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
hi @SadiJr please check and resolve any conflicts in the branch. |
|
@weizhouapache @hsato03 addressing you , as you seem the only ones involved in veeam atm. I’m closing this one as it has conflicts and is old. please update and reopen if you think it is still relevant. |
9 participants
Description
Using VMWare with Veeam on ACS, when restoring a volume from a backup, ACS creates a new VM from the backup, detaches all volumes, and attaches the selected volume to the original VM.
Veeam allows restoring only the selected volume, which is faster and more performant than the current approach. This PR aims to improve the volume restore process using this native option of Veeam.
Also, when restoring and attaching one volume to one VM, the target VM needs to be stopped and then started. However, the entire process is manual; even Veeam allows the VM to be started automatically if the restore is successful. A new parameter,
startvm, has been added to the APIrestoreVolumeFromBackupAndAttachToVM, allowing operators to specify whether or not the VM should be started automatically when volume restoration is complete.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
It was tested in a local lab:
startvm, and check if, in Veeam and vCenter, only the selected volume are restored, which results in success, but i had to start the VM manually;startvmas true, and the VM is automatically started by Veeam when restore finishes.