Create new Instance from VM backup #10140
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10140 +/- ##
============================================
+ Coverage 16.17% 16.20% +0.02%
- Complexity 13039 13075 +36
============================================
Files 5645 5651 +6
Lines 494749 496247 +1498
Branches 59946 60247 +301
============================================
+ Hits 80042 80414 +372
- Misses 405877 406909 +1032
- Partials 8830 8924 +94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Yes, we need this ! |
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11996 |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
boring-cyborg
bot
added
component:compute
component:kubernetes
component:orchestration
labels
abh1sar
changed the title
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12095 |
| @@ -147,6 +148,13 @@ public class DeployVMCmd extends BaseAsyncCreateCustomIdCmd implements SecurityG | |||
| since = "4.4") | |||
| private Long rootdisksize; | |||
|
|
|||
| @Parameter(name = ApiConstants.DATADISKS_DETAILS, | |||
There was a problem hiding this comment.
@abh1sar is it possible to use the existing details param here?
There was a problem hiding this comment.
@shwstppr I am not sure how we can input a list of datadiskdetails using the details Map.
Anyway, It seems better to me to keep the parameter separate like ipToNetworkList for easier usage and understanding (this could be used for deployvm in general to create instances with multiple volumes)
| @@ -102,6 +103,10 @@ public class BackupResponse extends BaseResponse { | |||
| @Param(description = "zone name") | |||
| private String zone; | |||
|
|
|||
| @SerializedName(ApiConstants.VM_DETAILS) | |||
| @Param(description = "Lists the vm specific details for the backup") | |||
| @@ -1009,32 +1011,40 @@ public UserVm resetVMSSHKey(ResetVMSSHKeyCmd cmd) throws ResourceUnavailableExce | |||
| throw new InvalidParameterValueException("Vm " + userVm + " should be stopped to do SSH Key reset"); | |||
| } | |||
|
|
|||
| if (cmd.getNames() == null || cmd.getNames().isEmpty()) { | |||
| List<String> names = cmd.getNames(); | |||
| if (names == null || names.isEmpty()) { | |||
| @@ -6139,6 +6186,11 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE | |||
| } | |||
| } | |||
|
|
|||
| List<DiskOfferingInfo> dataDiskOfferingsInfo = cmd.getDataDiskOfferingsInfo(); | |||
| if (dataDiskOfferingsInfo != null && diskOfferingId != null) { | |||
There was a problem hiding this comment.
If diskOfferingId is the ID of just another data disk, is it better to generate a diskOfferingInfo using it and use a single variable dataDiskOfferingsInfo in the subsequent method calls?
There was a problem hiding this comment.
diskOfferingId also corresponds to root disk if vm is created from iso.
It is definitely better if we have a single argument for all data disk creation, but we can't avoid passing diskOfferingId to other methods due to the iso limitation.
I can check the ISO condition and put the diskoffering details for single data disk in diskOfferingInfoList if you think it is still worth doing.
| @@ -2626,11 +2628,13 @@ | |||
| "label.bucket.policy": "Bucket Policy", | |||
| "label.usersecretkey": "Secret Key", | |||
| "label.create.bucket": "Create Bucket", | |||
| "label.create.instance.from.backup": "Create new instance from backup", | |||
There was a problem hiding this comment.
why two different keys? label.create.new.instance.from.backup and label.create.instance.from.backup
There was a problem hiding this comment.
Not needed. Will remove one.
|
|
||
| List<Backup.VolumeInfo> backupVolumes = backup.getBackedUpVolumes(); | ||
| if (backupVolumes == null) { | ||
| throw new CloudRuntimeException("Backed-up volumes not found"); |
There was a problem hiding this comment.
Better message can be used
|
|
||
| BackupOffering offering = backupOfferingDao.findByIdIncludingRemoved(backup.getBackupOfferingId()); | ||
| if (offering == null) { | ||
| String errorMessage = "Failed to find backup offering of the VM backup."; |
| String errorMessage = "Failed to find backup offering of the VM backup."; | ||
| throw new CloudRuntimeException("Failed to find backup offering of the VM backup."); | ||
| } | ||
| if ("networker".equals(offering.getProvider())) { |
There was a problem hiding this comment.
maybe better to define in the provider itself - offering.getProvider().supportsInstanceFromBackup()
| // The restore process is executed by a backup provider outside of ACS, I am using the catch-all (Exception) to | ||
| // ensure that no provider-side exception is missed. Therefore, we have a proper handling of exceptions, and rollbacks if needed. | ||
| } catch (Exception e) { | ||
| logger.error(String.format("Failed to restore backup [%s] due to: [%s].", backupDetailsInMessage, e.getMessage()), e); |
There was a problem hiding this comment.
ActionEventUtils.onFailedActionEvent?
| } | ||
| // The restore process is executed by a backup provider outside of ACS, I am using the catch-all (Exception) to | ||
| // ensure that no provider-side exception is missed. Therefore, we have a proper handling of exceptions, and rollbacks if needed. | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
possible to avoid catch all?
There was a problem hiding this comment.
@shwstppr I can catch specific exceptions, but will it be better to catch all exceptions - handle them and then rethrow the same exception ? In case any unknown exceptions are thrown by the providers (doesn't seem possible now, but maybe in the future)
There was a problem hiding this comment.
@abh1sar up to you. Catching all will also mask any NPEs.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
There was a problem hiding this comment.
Code LGTM
@abh1sar only one diskoffering - zone link edge case seems to be remaining for testing/changes
Please see if we need to add some documentation for the feature
@blueorangutan package
Description
This PR adds the ability to create a new Instance from a VM backup for Dummy, NAS and Veeam backup Providers.
This will still work if the original Instance used to create the backup was expunged.
New API
UI
DB changes
Other Changes
Map datadisksdetailsto deployVirtualMachine api to create multiple data volumes at the time of instance creation.Plugins related changes
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?