Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2824529413 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13152 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2824239036 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2824241860 Packaging result [SF]: ✔️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 13151 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2824236902 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2824007636 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2824011178 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2814485921
> About the deviceId of datadiskoffering
>
> ```
> dataDiskOfferingInfo.setDeviceId(1L);
> ```
>
> should it be 0 if the vm is created from ISO ? this is not a valid
question if we do not support backup actions for vm from ISO.
For VM from ISOs createVirtualMachineFromScratch is called where deviceId
remains null in rootDiskOfferingInfo and in allocateRawVolume the deviceId for
volume is set to 0L.
```
if (deviceId != null) {
vol.setDeviceId(deviceId);
} else if (type.equals(Type.ROOT)) {
vol.setDeviceId(0l);
```
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2812198952
> Thanks @weizhouapache
>
> > * for vm created from ISO, the root volume uses diskofferingId ,
deviceId should be 0 I think.
>
> I tested this. The root volume is being assigned deviceId 0 for both
template and iso.
>
> > * for vm created with multiple data disks, it uses
`datadiskofferinglist`, but deviceId of volumes are not set in
dataDiskOfferingInfo.
>
> deviceIds are set here
>
> ```
> public List getDataDiskOfferingListFromBackup(Backup
backup) {
> ...
>diskOfferingInfoList.add(new DiskOfferingInfo(diskOffering, size,
minIops, maxIops, deviceId));
> ```
>
> Please let me know if this sounds ok.
thanks @abh1sar
good to know it
About the deviceId of datadiskoffering
```
dataDiskOfferingInfo.setDeviceId(1L);
```
should it be 0 if the vm is created from ISO ?
this is not a valid question if we do not support backup actions for vm from
ISO.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2810177222
Thanks @weizhouapache
> * for vm created from ISO, the root volume uses diskofferingId , deviceId
should be 0 I think.
I tested this. The root volume is being assigned deviceId 0 for both
template and iso.
> * for vm created with multiple data disks, it uses `datadiskofferinglist`,
but deviceId of volumes are not set in dataDiskOfferingInfo.
deviceIds are set here
```
public List getDataDiskOfferingListFromBackup(Backup
backup) {
...
diskOfferingInfoList.add(new DiskOfferingInfo(diskOffering, size,
minIops, maxIops, deviceId));
```
Please let me know if this sounds ok.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2808502266 overall lgtm note about the `setDeviceId` - for vm created from template, the data volume uses diskofferingId , so deviceId=1 is correct - for vm created from ISO, the root volume uses diskofferingId , deviceId should be 0 I think. - for vm created with multiple data disks, it uses `datadiskofferinglist`, but deviceId of volumes are not set in dataDiskOfferingInfo. is `deviceId` important for this feature ? cc @abh1sar -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2803448000 [SF] Trillian test result (tid-12978) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 54966 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10140-t12978-kvm-ol8.zip Smoke tests completed. 140 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below: Test | Result | Time (s) | Test File --- | --- | --- | --- ContextSuite context=TestClusterDRS>:setup | `Error` | 0.00 | test_cluster_drs.py -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2800938046 @blueorangutan test -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2800941364 @abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2800909050 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13040 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-280067 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2800703539 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2792406015 > > > this PR contains 3 changes > > > > > > * create new vm from backup > > > * threshold for object storage > > > * threshold for backup > > > > > > I would prefer to see 3 PRs (at least 2 PRs) but it looks difficult to seperate them now > > > > > > It's not ideal, but it was done this way so test effort could be minimised. > > I do not know how much QA work will be reduced. I think they will still have to test all the scenarios. yes, just in terms of deploying multiple envs. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037141264
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java:
##
@@ -213,7 +214,7 @@ public boolean removeVMFromBackupOffering(final
VirtualMachine vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
-return true;
+return false;
Review Comment:
good, thanks
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2795316046 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13013 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2792362931 > > this PR contains 3 changes > > > > * create new vm from backup > > * threshold for object storage > > * threshold for backup > > > > I would prefer to see 3 PRs (at least 2 PRs) but it looks difficult to seperate them now > > It's not ideal, but it was done this way so test effort could be minimised. I do not know how much QA work will be reduced. I think they will still have to test all the scenarios. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2795192381 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2795191555 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036998792
##
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java:
##
@@ -56,6 +56,9 @@ public class AddObjectStoragePoolCmd extends BaseCmd {
@Parameter(name = ApiConstants.TAGS, type = CommandType.STRING,
description = "the tags for the storage pool")
private String tags;
+@Parameter(name = ApiConstants.SIZE, type = CommandType.LONG, description
= "the total size of the object store in GiB. Used for tracking capacity and
sending alerts", since = "4.21")
Review Comment:
got it, thanks @abh1sar
##
api/src/main/java/com/cloud/vm/VirtualMachine.java:
##
@@ -128,7 +128,6 @@ public static StateMachine2 getStat
s_fsm.addTransition(new Transition(State.Error,
VirtualMachine.Event.DestroyRequested, State.Expunging, null));
s_fsm.addTransition(new Transition(State.Error,
VirtualMachine.Event.ExpungeOperation, State.Expunging, null));
s_fsm.addTransition(new Transition(State.Stopped,
Event.RestoringRequested, State.Restoring, null));
-s_fsm.addTransition(new Transition(State.Expunging,
Event.RestoringRequested, State.Restoring, null));
Review Comment:
ok, got it
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036932484
##
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##
@@ -138,6 +144,14 @@ public interface BackupManager extends BackupService,
Configurable, PluggableSer
ConfigKey.Scope.Global,
null);
+ConfigKey BackupStorageCapacityThreshold = new ConfigKey<>("Alert",
Float.class,
Review Comment:
such setting doesn't exist for other any resource as of now.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2035455775
##
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##
@@ -138,6 +144,14 @@ public interface BackupManager extends BackupService,
Configurable, PluggableSer
ConfigKey.Scope.Global,
null);
+ConfigKey BackupStorageCapacityThreshold = new ConfigKey<>("Alert",
Float.class,
Review Comment:
do we need to a setting to disable threshold ?
##
api/src/main/java/com/cloud/vm/VirtualMachine.java:
##
@@ -128,7 +128,6 @@ public static StateMachine2 getStat
s_fsm.addTransition(new Transition(State.Error,
VirtualMachine.Event.DestroyRequested, State.Expunging, null));
s_fsm.addTransition(new Transition(State.Error,
VirtualMachine.Event.ExpungeOperation, State.Expunging, null));
s_fsm.addTransition(new Transition(State.Stopped,
Event.RestoringRequested, State.Restoring, null));
-s_fsm.addTransition(new Transition(State.Expunging,
Event.RestoringRequested, State.Restoring, null));
Review Comment:
makes sense
is it needed for 4.20 ? (a trivial issue as I can see)
##
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##
@@ -1224,4 +1224,14 @@ public Map
getNameIdMapForVmIds(Collection ids) {
return vms.stream()
.collect(Collectors.toMap(VMInstanceVO::getInstanceName,
VMInstanceVO::getId));
}
+
+@Override
+public List listByIds(List ids) {
Review Comment:
1. `UserVmDaoImpl` has the same method
2. in case of `listIncludingRemovedBy` (not `listBy`), would it better to
rename the method ?
##
engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java:
##
@@ -0,0 +1,98 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.backup;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+
+import org.apache.cloudstack.api.ResourceDetail;
+
+@Entity
+@Table(name = "backup_details")
+public class BackupDetailVO implements ResourceDetail {
+@Id
+@GeneratedValue(strategy = GenerationType.IDENTITY)
+@Column(name = "id")
+private long id;
+
+@Column(name = "backup_id")
+private long resourceId;
+
+@Column(name = "name")
+private String name;
+
+@Column(name = "value", length = 1024)
Review Comment:
is 1024 big enough ?
##
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##
@@ -102,6 +111,18 @@ 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", since
= "4.21.0")
+private Map vmDetails;
+
+@SerializedName(ApiConstants.INTERVAL_TYPE)
+@Param(description = "the interval type of the backup schedule", since =
"4.21.0")
+private String intervalType;
+
+@SerializedName(ApiConstants.BACKUP_VM_OFFERING_REMOVED)
Review Comment:
is it used somewhere, except in the response ?
##
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java:
##
@@ -147,6 +148,13 @@ public class DeployVMCmd extends
BaseAsyncCreateCustomIdCmd implements SecurityG
since = "4.4")
private Long rootdisksize;
+@Parameter(name = ApiConstants.DATADISKS_DETAILS,
+type = CommandType.MAP,
+since = "4.21.0",
+description = "Disk offering details for creating multiple data
volumes. Mutually exclusibe with diskOfferingId." +
+" Example:
datadisksdetails[0].diskofferingid=1&datadisksdetails[0].size=10&datadisksdetails[0].miniops=100&datadisksdetails[0].maxiops=200")
Review Comment:
is `diskofferingid` a id (long) or uuid (string)?
in case of uuid, it would be good to change the value `1` to a uuid string
in this example
##
engi
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037335914
##
engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql:
##
@@ -37,3 +33,38 @@ WHERE rp.rule = 'quotaStatement'
AND NOT EXISTS(SELECT 1 FROM cloud.role_permissions rp_ WHERE rp.role_id =
rp_.role_id AND rp_.rule = 'quotaCreditsList');
CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host', 'last_mgmt_server_id',
'bigint unsigned DEFAULT NULL COMMENT "last management server this host is
connected to" AFTER `mgmt_server_id`');
+
+-- Add column max_backup to backup_schedule table
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backup_schedule', 'max_backups',
'int(8) default NULL COMMENT "maximum number of backups to maintain"');
+
+-- Add columns name, description and backup_interval_type to backup table
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'name', 'VARCHAR(255)
NOT NULL COMMENT "name of the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'description',
'VARCHAR(1024) COMMENT "description for the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'backup_interval_type',
'int(5) COMMENT "type of backup, e.g. manual, recurring - hourly, daily, weekly
or monthly"');
+
+-- Make the column vm_id in backups table nullable to handle orphan backups
+ALTER TABLE `cloud`.`backups` MODIFY COLUMN `vm_id` BIGINT UNSIGNED NULL;
+
+-- Create backup details table
+CREATE TABLE `cloud`.`backup_details` (
+ `id` bigint unsigned NOT NULL auto_increment,
+ `backup_id` bigint unsigned NOT NULL COMMENT 'backup id',
+ `name` varchar(255) NOT NULL,
+ `value` varchar(65536) NOT NULL,
Review Comment:
fixed.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2792328923 > this PR contains 3 changes > > * create new vm from backup > * threshold for object storage > * threshold for backup > > I would prefer to see 3 PRs (at least 2 PRs) but it looks difficult to seperate them now It's not ideal, but it was done this way so test effort could be minimised. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2794489574 [SF] Trillian test result (tid-12919) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 114554 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10140-t12919-kvm-ol8.zip Smoke tests completed. 114 look OK, 27 have errors, 0 did not run Only failed and skipped tests results shown below: Test | Result | Time (s) | Test File --- | --- | --- | --- ContextSuite context=TestInternalLb>:setup | `Error` | 0.00 | test_internal_lb.py ContextSuite context=TestIpv4Routing>:setup | `Error` | 0.00 | test_ipv4_routing.py test_01_1_create_iso_with_checksum_sha1_negative | `Error` | 66.60 | test_iso.py test_01_create_iso_with_checksum_sha1 | `Error` | 66.58 | test_iso.py test_01_create_iso_with_checksum_sha1 | `Error` | 66.58 | test_iso.py test_02_1_create_iso_with_checksum_sha256_negative | `Error` | 66.59 | test_iso.py test_02_create_iso_with_checksum_sha256 | `Error` | 66.56 | test_iso.py test_02_create_iso_with_checksum_sha256 | `Error` | 66.56 | test_iso.py test_03_1_create_iso_with_checksum_md5_negative | `Error` | 66.52 | test_iso.py test_03_create_iso_with_checksum_md5 | `Error` | 66.56 | test_iso.py test_03_create_iso_with_checksum_md5 | `Error` | 66.56 | test_iso.py test_04_create_iso_with_no_checksum | `Error` | 66.55 | test_iso.py test_04_create_iso_with_no_checksum | `Error` | 66.55 | test_iso.py test_01_create_iso | `Failure` | 1520.27 | test_iso.py ContextSuite context=TestISO>:setup | `Error` | 3038.15 | test_iso.py test_01_browser_migrate_template | `Error` | 65.78 | test_image_store_object_migration.py test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py test_02_upgrade_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py test_04_autoscale_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py test_05_basic_lifecycle_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py test_06_delete_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py test_10_vpc_tier_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py test_11_test_unmanaged_cluster_lifecycle | `Error` | 0.01 | test_kubernetes_clusters.py test_01_add_delete_kubernetes_supported_version | `Error` | 1801.81 | test_kubernetes_supported_versions.py ContextSuite context=TestListIdsParams>:setup | `Error` | 0.00 | test_list_ids_parameter.py ContextSuite context=TestLoadBalance>:setup | `Error` | 0.00 | test_loadbalance.py ContextSuite context=TestMetrics>:setup | `Error` | 0.00 | test_metrics_api.py test_01_native_to_native_network_migration | `Error` | 13.95 | test_migration.py test_02_native_to_native_vpc_migration | `Error` | 11.57 | test_migration.py test_nic_secondaryip_add_remove | `Error` | 1519.07 | test_multipleips_per_nic.py ContextSuite context=TestNestedVirtualization>:setup | `Error` | 0.00 | test_nested_virtualization.py ContextSuite context=TestNetworkACL>:setup | `Error` | 0.00 | test_network_acl.py ContextSuite context=TestPrivateGwACL>:setup | `Error` | 0.00 | test_privategw_acl.py ContextSuite context=TestIpv6Network>:setup | `Error` | 0.00 | test_network_ipv6.py test_03_network_operations_on_created_vm_of_otheruser | `Error` | 2.55 | test_network_permissions.py test_03_network_operations_on_created_vm_of_otheruser | `Error` | 2.55 | test_network_permissions.py test_04_deploy_vm_for_other_user_and_test_vm_operations | `Failure` | 1.39 | test_network_permissions.py ContextSuite context=TestNetworkPermissions>:teardown | `Error` | 1.43 | test_network_permissions.py test_delete_account | `Error` | 1515.93 | test_network.py test_delete_network_while_vm_on_it | `Error` | 1.17 | test_network.py test_deploy_vm_l2network | `Error` | 1.17 | test_network.py test_l2network_restart | `Error` | 2.29 | test_network.py ContextSuite context=TestPortForwarding>:setup | `Error` | 3.51 | test_network.py ContextSuite context=TestPublicIP>:setup | `Error` | 9.13 | test_network.py test_reboot_router | `Failure` | 0.08 | test_network.py test_releaseIP | `Error` | 4.72 | test_network.py test_releaseIP_using_IP | `Error` | 4.57 | test_network.py ContextSuite context=TestRouterRules>:setup | `Error` | 4.64 | test_network.py test_01_deployVMInSharedNetwork | `Failure` | 1.26 | test_network.py test_02_verifyRouterIpAfterNetworkRestart | `Failure` | 1.09 | test_network.py test_03_destroySharedNetwork | `Failure` | 1.09 | test_network.py ContextSuite context=TestSharedN
Re: [PR] Create new Instance from VM backup [cloudstack]
DaanHoogland commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037010182
##
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##
@@ -35,12 +37,14 @@
import com.cloud.vm.VirtualMachine;
public class DummyBackupProvider extends AdapterBase implements BackupProvider
{
-
+private static final Logger LOG =
LogManager.getLogger(DummyBackupProvider.class);
Review Comment:
naming convention says `private static final` means all capitals,
@weizhouapache . I think this is ok, unless we remove the `static` part.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037008932
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -659,9 +656,7 @@ public boolean setJobSchedule(final String jobName) {
public boolean deleteJobAndBackup(final String jobName) {
Pair result = executePowerShellCommands(Arrays.asList(
String.format("$job = Get-VBRJob -Name '%s'", jobName),
-"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }",
-String.format("$backup = Get-VBRBackup -Name '%s'", jobName),
-"if ($backup) { Remove-VBRBackup -Backup $backup -FromDisk
-Confirm:$false }"
+"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }"
Review Comment:
Yes.
new instance can be created from the old (and now orphan) backups.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037006429
##
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##
@@ -118,7 +110,7 @@ public boolean removeVMFromBackupOffering(VirtualMachine
vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
-return true;
+return false;
Review Comment:
Backup offering can now be removed from VM without deleting old backups. The
VM can be expunged or assigned another offering.
New instance can be created from the older backups.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037143721
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -855,55 +816,61 @@ public List
listRestorePointsLegacy(String backupName, Stri
}
logger.debug(String.format("Found restore points from [backupName:
%s, vmInternalName: %s] which is: [%s].", backupName, vmInternalName, block));
final String[] parts = block.split("\r\n");
-restorePoints.add(getRestorePointFromBlock(parts));
+restorePoints.add(getRestorePointFromBlock(parts, metricsMap));
}
return restorePoints;
}
-public List listRestorePoints(String backupName,
String vmInternalName) {
+public List listRestorePoints(String backupName,
String vmwareDcName, String vmInternalName, Map
metricsMap) {
if (isLegacyServer()) {
-return listRestorePointsLegacy(backupName, vmInternalName);
+return listRestorePointsLegacy(backupName, vmInternalName,
metricsMap);
} else {
-return listVmRestorePointsViaVeeamAPI(vmInternalName);
+return listVmRestorePointsViaVeeamAPI(vmwareDcName,
vmInternalName, metricsMap);
}
}
-public List listVmRestorePointsViaVeeamAPI(String
vmInternalName) {
+public List listVmRestorePointsViaVeeamAPI(String
vmwareDcName, String vmInternalName, Map metricsMap) {
logger.debug(String.format("Trying to list VM restore points via Veeam
B&R API for VM %s: ", vmInternalName));
try {
final HttpResponse response =
get(String.format("/vmRestorePoints?format=Entity"));
checkResponseOK(response);
-return
processHttpResponseForVmRestorePoints(response.getEntity().getContent(),
vmInternalName);
+return
processHttpResponseForVmRestorePoints(response.getEntity().getContent(),
vmwareDcName, vmInternalName, metricsMap);
} catch (final IOException e) {
logger.error("Failed to list VM restore points via Veeam B&R API
due to:", e);
checkResponseTimeOut(e);
}
return new ArrayList<>();
}
-public List
processHttpResponseForVmRestorePoints(InputStream content, String
vmInternalName) {
+public List
processHttpResponseForVmRestorePoints(InputStream content, String vmwareDcName,
String vmInternalName, Map metricsMap) {
List vmRestorePointList = new ArrayList<>();
try {
final ObjectMapper objectMapper = new XmlMapper();
final VmRestorePoints vmRestorePoints =
objectMapper.readValue(content, VmRestorePoints.class);
+final String hierarchyId = findDCHierarchy(vmwareDcName);
+final String hierarchyUuid =
hierarchyId.substring(hierarchyId.lastIndexOf(":") + 1);
if (vmRestorePoints == null) {
throw new CloudRuntimeException("Could not get VM restore
points via Veeam B&R API");
}
for (final VmRestorePoint vmRestorePoint :
vmRestorePoints.getVmRestorePoints()) {
logger.debug(String.format("Processing VM restore point
Name=%s, VmDisplayName=%s for vm name=%s",
vmRestorePoint.getName(),
vmRestorePoint.getVmDisplayName(), vmInternalName));
-if (!vmInternalName.equals(vmRestorePoint.getVmDisplayName()))
{
+if (!vmInternalName.equals(vmRestorePoint.getVmDisplayName())
|| !vmRestorePoint.getHierarchyObjRef().contains(hierarchyUuid)) {
continue;
}
boolean isReady = true;
+String backupFileId = null;
List links = vmRestorePoint.getLink();
for (Link link : links) {
if (Arrays.asList(BACKUP_FILE_REFERENCE,
RESTORE_POINT_REFERENCE).contains(link.getType()) &&
!link.getRel().equals("Up")) {
logger.info(String.format("The VM restore point is not
ready. Reference: %s, state: %s", link.getType(), link.getRel()));
isReady = false;
break;
}
+if (link.getType() != null &&
link.getType().equals(BACKUP_FILE_REFERENCE)) {
+backupFileId =
link.getHref().substring(link.getHref().lastIndexOf('/') + 1);
Review Comment:
ah ok, there is already a method `StringUtils.substringAfterLast`
good to know it
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037113772
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -8987,6 +9024,215 @@ public boolean unmanageUserVM(Long vmId) {
return true;
}
+@Override
+public UserVm allocateVMFromBackup(CreateVMFromBackupCmd cmd) throws
InsufficientCapacityException, ResourceAllocationException,
ResourceUnavailableException {
Review Comment:
@weizhouapache Thanks for the comments. I have addressed all of them apart
from this one and the one about iso. I'll get back to you on them.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037030743
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -659,9 +656,7 @@ public boolean setJobSchedule(final String jobName) {
public boolean deleteJobAndBackup(final String jobName) {
Pair result = executePowerShellCommands(Arrays.asList(
String.format("$job = Get-VBRJob -Name '%s'", jobName),
-"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }",
-String.format("$backup = Get-VBRBackup -Name '%s'", jobName),
-"if ($backup) { Remove-VBRBackup -Backup $backup -FromDisk
-Confirm:$false }"
+"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }"
Review Comment:
there was a PR #6589 . Is that included in this PR ?
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037150147
##
engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql:
##
@@ -37,3 +33,38 @@ WHERE rp.rule = 'quotaStatement'
AND NOT EXISTS(SELECT 1 FROM cloud.role_permissions rp_ WHERE rp.role_id =
rp_.role_id AND rp_.rule = 'quotaCreditsList');
CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host', 'last_mgmt_server_id',
'bigint unsigned DEFAULT NULL COMMENT "last management server this host is
connected to" AFTER `mgmt_server_id`');
+
+-- Add column max_backup to backup_schedule table
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backup_schedule', 'max_backups',
'int(8) default NULL COMMENT "maximum number of backups to maintain"');
+
+-- Add columns name, description and backup_interval_type to backup table
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'name', 'VARCHAR(255)
NOT NULL COMMENT "name of the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'description',
'VARCHAR(1024) COMMENT "description for the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'backup_interval_type',
'int(5) COMMENT "type of backup, e.g. manual, recurring - hourly, daily, weekly
or monthly"');
+
+-- Make the column vm_id in backups table nullable to handle orphan backups
+ALTER TABLE `cloud`.`backups` MODIFY COLUMN `vm_id` BIGINT UNSIGNED NULL;
+
+-- Create backup details table
+CREATE TABLE `cloud`.`backup_details` (
+ `id` bigint unsigned NOT NULL auto_increment,
+ `backup_id` bigint unsigned NOT NULL COMMENT 'backup id',
+ `name` varchar(255) NOT NULL,
+ `value` varchar(65536) NOT NULL,
Review Comment:
Yes, fixing it.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037148327
##
engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql:
##
@@ -37,3 +33,38 @@ WHERE rp.rule = 'quotaStatement'
AND NOT EXISTS(SELECT 1 FROM cloud.role_permissions rp_ WHERE rp.role_id =
rp_.role_id AND rp_.rule = 'quotaCreditsList');
CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host', 'last_mgmt_server_id',
'bigint unsigned DEFAULT NULL COMMENT "last management server this host is
connected to" AFTER `mgmt_server_id`');
+
+-- Add column max_backup to backup_schedule table
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backup_schedule', 'max_backups',
'int(8) default NULL COMMENT "maximum number of backups to maintain"');
+
+-- Add columns name, description and backup_interval_type to backup table
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'name', 'VARCHAR(255)
NOT NULL COMMENT "name of the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'description',
'VARCHAR(1024) COMMENT "description for the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'backup_interval_type',
'int(5) COMMENT "type of backup, e.g. manual, recurring - hourly, daily, weekly
or monthly"');
+
+-- Make the column vm_id in backups table nullable to handle orphan backups
+ALTER TABLE `cloud`.`backups` MODIFY COLUMN `vm_id` BIGINT UNSIGNED NULL;
+
+-- Create backup details table
+CREATE TABLE `cloud`.`backup_details` (
+ `id` bigint unsigned NOT NULL auto_increment,
+ `backup_id` bigint unsigned NOT NULL COMMENT 'backup id',
+ `name` varchar(255) NOT NULL,
+ `value` varchar(65536) NOT NULL,
Review Comment:
got an exception in simulator CI
```
java.sql.SQLSyntaxErrorException: Column length too big for column 'value'
(max = 21845); use BLOB or TEXT instead
```
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037024761
##
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##
@@ -118,7 +110,7 @@ public boolean removeVMFromBackupOffering(VirtualMachine
vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
-return true;
+return false;
Review Comment:
thanks
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036815904 ## api/src/main/java/com/cloud/vm/VirtualMachine.java: ## @@ -128,7 +128,6 @@ public static StateMachine2 getStat s_fsm.addTransition(new Transition(State.Error, VirtualMachine.Event.DestroyRequested, State.Expunging, null)); s_fsm.addTransition(new Transition(State.Error, VirtualMachine.Event.ExpungeOperation, State.Expunging, null)); s_fsm.addTransition(new Transition(State.Stopped, Event.RestoringRequested, State.Restoring, null)); -s_fsm.addTransition(new Transition(State.Expunging, Event.RestoringRequested, State.Restoring, null)); Review Comment: Currently VMs in expunging state can not be restored, so don't need to backport this change. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037055413
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -659,9 +656,7 @@ public boolean setJobSchedule(final String jobName) {
public boolean deleteJobAndBackup(final String jobName) {
Pair result = executePowerShellCommands(Arrays.asList(
String.format("$job = Get-VBRJob -Name '%s'", jobName),
-"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }",
-String.format("$backup = Get-VBRBackup -Name '%s'", jobName),
-"if ($backup) { Remove-VBRBackup -Backup $backup -FromDisk
-Confirm:$false }"
+"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }"
Review Comment:
No, but I see lots of overlaps 😨
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037028629
##
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##
@@ -35,12 +37,14 @@
import com.cloud.vm.VirtualMachine;
public class DummyBackupProvider extends AdapterBase implements BackupProvider
{
-
+private static final Logger LOG =
LogManager.getLogger(DummyBackupProvider.class);
Review Comment:
@DaanHoogland
there is already a LOGGER defined in `ComponentLifecycleBase`. we can just
use it
```
public class ComponentLifecycleBase implements ComponentLifecycle {
protected Logger logger = LogManager.getLogger(getClass());
```
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037023156
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -834,10 +794,11 @@ private Backup.RestorePoint
getRestorePointFromBlock(String[] parts) {
type = split[1].trim();
}
}
-return new Backup.RestorePoint(id, created, type);
+Backup.Metric metric = metricsMap.get(id);
Review Comment:
ideally not, but I'll add handling for npe.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037001160
##
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##
@@ -102,6 +111,18 @@ 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", since
= "4.21.0")
+private Map vmDetails;
+
+@SerializedName(ApiConstants.INTERVAL_TYPE)
+@Param(description = "the interval type of the backup schedule", since =
"4.21.0")
+private String intervalType;
+
+@SerializedName(ApiConstants.BACKUP_VM_OFFERING_REMOVED)
Review Comment:
ok
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037007942
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java:
##
@@ -213,7 +214,7 @@ public boolean removeVMFromBackupOffering(final
VirtualMachine vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
-return true;
+return false;
Review Comment:
Same as above.
In veeam as well, new instance can be created from backups, the original vm
of which is expunged.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2037003311
##
engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java:
##
@@ -0,0 +1,98 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.backup;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+
+import org.apache.cloudstack.api.ResourceDetail;
+
+@Entity
+@Table(name = "backup_details")
+public class BackupDetailVO implements ResourceDetail {
+@Id
+@GeneratedValue(strategy = GenerationType.IDENTITY)
+@Column(name = "id")
+private long id;
+
+@Column(name = "backup_id")
+private long resourceId;
+
+@Column(name = "name")
+private String name;
+
+@Column(name = "value", length = 1024)
Review Comment:
SQL needs to be updated as well
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036970174
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -834,10 +794,11 @@ private Backup.RestorePoint
getRestorePointFromBlock(String[] parts) {
type = split[1].trim();
}
}
-return new Backup.RestorePoint(id, created, type);
+Backup.Metric metric = metricsMap.get(id);
Review Comment:
is it possible metric is null ?
##
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##
@@ -35,12 +37,14 @@
import com.cloud.vm.VirtualMachine;
public class DummyBackupProvider extends AdapterBase implements BackupProvider
{
-
+private static final Logger LOG =
LogManager.getLogger(DummyBackupProvider.class);
Review Comment:
`logger` can be used in this class
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java:
##
@@ -213,7 +214,7 @@ public boolean removeVMFromBackupOffering(final
VirtualMachine vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
-return true;
+return false;
Review Comment:
why this is changed ?
##
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##
@@ -118,7 +110,7 @@ public boolean removeVMFromBackupOffering(VirtualMachine
vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
-return true;
+return false;
Review Comment:
can you explain a bit ?
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -8987,6 +9024,215 @@ public boolean unmanageUserVM(Long vmId) {
return true;
}
+@Override
+public UserVm allocateVMFromBackup(CreateVMFromBackupCmd cmd) throws
InsufficientCapacityException, ResourceAllocationException,
ResourceUnavailableException {
Review Comment:
since the process of CreateVMFromBackupCmd and DeployVMCmd are different, I
would suggest
- CreateVMFromBackupCmd is not extended from DeployVMCmd, some unsupported
parameters can be removed (for example userdata)
- do not change DeployVMCmd so there will be less impact. Changing the
properties of existing parameter will lead to some other changes, for example
in cloudstack-go project
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -659,9 +656,7 @@ public boolean setJobSchedule(final String jobName) {
public boolean deleteJobAndBackup(final String jobName) {
Pair result = executePowerShellCommands(Arrays.asList(
String.format("$job = Get-VBRJob -Name '%s'", jobName),
-"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }",
-String.format("$backup = Get-VBRBackup -Name '%s'", jobName),
-"if ($backup) { Remove-VBRBackup -Backup $backup -FromDisk
-Confirm:$false }"
+"if ($job) { Remove-VBRJob -Job $job -Confirm:$false }"
Review Comment:
so, only job is deleted, backups are not ?
##
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##
@@ -855,55 +816,61 @@ public List
listRestorePointsLegacy(String backupName, Stri
}
logger.debug(String.format("Found restore points from [backupName:
%s, vmInternalName: %s] which is: [%s].", backupName, vmInternalName, block));
final String[] parts = block.split("\r\n");
-restorePoints.add(getRestorePointFromBlock(parts));
+restorePoints.add(getRestorePointFromBlock(parts, metricsMap));
}
return restorePoints;
}
-public List listRestorePoints(String backupName,
String vmInternalName) {
+public List listRestorePoints(String backupName,
String vmwareDcName, String vmInternalName, Map
metricsMap) {
if (isLegacyServer()) {
-return listRestorePointsLegacy(backupName, vmInternalName);
+return listRestorePointsLegacy(backupName, vmInternalName,
metricsMap);
} else {
-return listVmRestorePointsViaVeeamAPI(vmInternalName);
+return listVmRestorePointsViaVeeamAPI(vmwareDcName,
vmInternalName, metricsMap);
}
}
-public List listVmRestorePointsViaVeeamAPI(String
vmInternalName) {
+public List listVmRestorePointsViaVeeamAPI(String
vmwareDcName, String vmInternalName, Map metricsMap) {
logger.debug(String.format("Trying to list VM restore points via Veeam
B&R API for VM %s: ", vmInternalName));
try {
final HttpResponse response =
get(String.format("/vmRestorePoints?format=Entity"));
checkResponseOK(response);
-return
processHttpResponseForVmRestorePoints(response.getEntity
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036958857
##
engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java:
##
@@ -0,0 +1,98 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.backup;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+
+import org.apache.cloudstack.api.ResourceDetail;
+
+@Entity
+@Table(name = "backup_details")
+public class BackupDetailVO implements ResourceDetail {
+@Id
+@GeneratedValue(strategy = GenerationType.IDENTITY)
+@Column(name = "id")
+private long id;
+
+@Column(name = "backup_id")
+private long resourceId;
+
+@Column(name = "name")
+private String name;
+
+@Column(name = "value", length = 1024)
Review Comment:
no, I'll change it to 65536 which is what BackupVO.backedUpVolumes also uses.
thanks for catching.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036958857
##
engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java:
##
@@ -0,0 +1,98 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.backup;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+
+import org.apache.cloudstack.api.ResourceDetail;
+
+@Entity
+@Table(name = "backup_details")
+public class BackupDetailVO implements ResourceDetail {
+@Id
+@GeneratedValue(strategy = GenerationType.IDENTITY)
+@Column(name = "id")
+private long id;
+
+@Column(name = "backup_id")
+private long resourceId;
+
+@Column(name = "name")
+private String name;
+
+@Column(name = "value", length = 1024)
Review Comment:
no, I'll change it to 65536 which is what BackupVO.backedUpVolumes also uses
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036948340
##
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##
@@ -1224,4 +1224,14 @@ public Map
getNameIdMapForVmIds(Collection ids) {
return vms.stream()
.collect(Collectors.toMap(VMInstanceVO::getInstanceName,
VMInstanceVO::getId));
}
+
+@Override
+public List listByIds(List ids) {
Review Comment:
Right, I'll rename this one to listByIdsIncludingRemoved
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036940973
##
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##
@@ -220,6 +220,14 @@ public interface StorageManager extends StorageService {
"storage.pool.host.connect.workers", "1",
"Number of worker threads to be used to connect hosts to a primary
storage", true);
+ConfigKey ObjectStorageCapacityThreshold = new ConfigKey<>("Alert",
Float.class,
+"zone.objectStorage.capacity.notificationthreshold",
+"0.75",
+"Percentage (as a value between 0 and 1) of object storage
utilization above which alerts will be sent about low storage available.",
+true,
+ConfigKey.Scope.Global,
Review Comment:
Object storage is currently not tied to a zone. So I'll remove `zone` from
the key name.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036923812
##
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##
@@ -102,6 +111,18 @@ 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", since
= "4.21.0")
+private Map vmDetails;
+
+@SerializedName(ApiConstants.INTERVAL_TYPE)
+@Param(description = "the interval type of the backup schedule", since =
"4.21.0")
+private String intervalType;
+
+@SerializedName(ApiConstants.BACKUP_VM_OFFERING_REMOVED)
Review Comment:
just in response, storage.js references it to decide whether to show
removeOffering button in backup list view.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036812654 ## api/src/main/java/org/apache/cloudstack/api/command/user/vm/CreateVMFromBackupCmd.java: ## @@ -0,0 +1,127 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.api.command.user.vm; + +import javax.inject.Inject; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ResponseObject; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.user.UserCmd; +import org.apache.cloudstack.api.response.BackupResponse; +import org.apache.cloudstack.api.response.UserVmResponse; +import org.apache.cloudstack.backup.BackupManager; + +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.InsufficientCapacityException; +import com.cloud.exception.InsufficientServerCapacityException; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.uservm.UserVm; +import com.cloud.vm.VirtualMachine; + +@APICommand(name = "createVMFromBackup", +description = "Creates and automatically starts a VM from a backup.", Review Comment: yes, it does. But it won't be like the new Instance is never started. This command will : 1. Create the new VM 2. Start it, so that volumes are created 3. Stop it 4. Restore the backed up data 5. Start the VM (or not start it if `start` parameter is set to false. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036813586 ## api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java: ## @@ -147,6 +148,13 @@ public class DeployVMCmd extends BaseAsyncCreateCustomIdCmd implements SecurityG since = "4.4") private Long rootdisksize; +@Parameter(name = ApiConstants.DATADISKS_DETAILS, +type = CommandType.MAP, +since = "4.21.0", +description = "Disk offering details for creating multiple data volumes. Mutually exclusibe with diskOfferingId." + +" Example: datadisksdetails[0].diskofferingid=1&datadisksdetails[0].size=10&datadisksdetails[0].miniops=100&datadisksdetails[0].maxiops=200") Review Comment: yes, its uuid. will make the change. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036791427
##
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java:
##
@@ -56,6 +56,9 @@ public class AddObjectStoragePoolCmd extends BaseCmd {
@Parameter(name = ApiConstants.TAGS, type = CommandType.STRING,
description = "the tags for the storage pool")
private String tags;
+@Parameter(name = ApiConstants.SIZE, type = CommandType.LONG, description
= "the total size of the object store in GiB. Used for tracking capacity and
sending alerts", since = "4.21")
Review Comment:
Yes, Object storage plugins don't have a proper way to return total
capacity. It's for admin to set and track.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2788714289 @blueorangutan test -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2788716490 @abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2785503701 @shwstppr 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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2785687953 @blueorangutan test-matrix -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2785677880 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12976 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
shwstppr commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2785499344 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar closed pull request #10140: Create new Instance from VM backup URL: https://github.com/apache/cloudstack/pull/10140 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
github-actions[bot] commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2760831529 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
github-actions[bot] commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2709730225 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1992650314
##
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##
@@ -35,6 +35,14 @@ public class BackupResponse extends BaseResponse {
@Param(description = "ID of the VM backup")
private String id;
+@SerializedName(ApiConstants.NAME)
+@Param(description = "name of the backup")
Review Comment:
yes, will add
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1992652792
##
api/src/main/java/org/apache/cloudstack/api/response/ObjectStoreResponse.java:
##
@@ -17,34 +17,40 @@
package org.apache.cloudstack.api.response;
import com.cloud.serializer.Param;
+
+import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.storage.object.ObjectStore;
import com.google.gson.annotations.SerializedName;
import org.apache.cloudstack.api.BaseResponseWithAnnotations;
import org.apache.cloudstack.api.EntityReference;
@EntityReference(value = ObjectStore.class)
public class ObjectStoreResponse extends BaseResponseWithAnnotations {
-@SerializedName("id")
+@SerializedName(ApiConstants.ID)
@Param(description = "the ID of the object store")
private String id;
-@SerializedName("name")
+@SerializedName(ApiConstants.NAME)
@Param(description = "the name of the object store")
private String name;
-@SerializedName("url")
+@SerializedName(ApiConstants.URL)
@Param(description = "the url of the object store")
private String url;
-@SerializedName("providername")
-@Param(description = "the provider name of the object store")
+@SerializedName(ApiConstants.PROVIDER)
+@Param(description = "the name of the object store provider")
private String providerName;
-@SerializedName("storagetotal")
+@SerializedName(ApiConstants.SIZE)
Review Comment:
you are right. I'll revert the name changes. They are not that significant
anyway
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
shwstppr commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1991041222
##
api/src/main/java/org/apache/cloudstack/api/response/ObjectStoreResponse.java:
##
@@ -17,34 +17,40 @@
package org.apache.cloudstack.api.response;
import com.cloud.serializer.Param;
+
+import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.storage.object.ObjectStore;
import com.google.gson.annotations.SerializedName;
import org.apache.cloudstack.api.BaseResponseWithAnnotations;
import org.apache.cloudstack.api.EntityReference;
@EntityReference(value = ObjectStore.class)
public class ObjectStoreResponse extends BaseResponseWithAnnotations {
-@SerializedName("id")
+@SerializedName(ApiConstants.ID)
@Param(description = "the ID of the object store")
private String id;
-@SerializedName("name")
+@SerializedName(ApiConstants.NAME)
@Param(description = "the name of the object store")
private String name;
-@SerializedName("url")
+@SerializedName(ApiConstants.URL)
@Param(description = "the url of the object store")
private String url;
-@SerializedName("providername")
-@Param(description = "the provider name of the object store")
+@SerializedName(ApiConstants.PROVIDER)
+@Param(description = "the name of the object store provider")
private String providerName;
-@SerializedName("storagetotal")
+@SerializedName(ApiConstants.SIZE)
Review Comment:
This changes the response param itself. Do we need some kind of deprecation
instead?
Similarly for other changed params. Could this cause any issues with
automation/SDKs?
##
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##
@@ -35,6 +35,14 @@ public class BackupResponse extends BaseResponse {
@Param(description = "ID of the VM backup")
private String id;
+@SerializedName(ApiConstants.NAME)
+@Param(description = "name of the backup")
Review Comment:
Do we need to add since key here and other new response params?
##
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##
@@ -2340,6 +2349,17 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) throws
throw new CloudRuntimeException("Unable to destroy " +
vm);
} else {
if (expunge) {
+if (vm.getBackupOfferingId() != null) {
Review Comment:
Possible to refactor to this a method?
##
plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/NetworkerBackupProvider.java:
##
@@ -615,6 +611,7 @@ public Backup
createNewBackupEntryForRestorePoint(Backup.RestorePoint restorePoi
backup.setAccountId(vm.getAccountId());
backup.setDomainId(vm.getDomainId());
backup.setZoneId(vm.getDataCenterId());
+backup.setName(vm.getHostName() + '-' + new
SimpleDateFormat("-MM-dd'T'HH:mmX").format(new Date()));
Review Comment:
This is a repeating code. Would it make sense to add method in
BackupProvider interface or some other common class to get the backup name?
##
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##
@@ -3428,11 +3438,19 @@ protected List
listCapacitiesWithDetails(final Long zoneId, final Lo
for (final Long zId : dcList) {
// op_host_Capacity contains only allocated stats and the real
time
// stats are stored "in memory".
-// List secondary storage capacity only when the api is
invoked for the zone layer.
-if ((capacityType == null || capacityType ==
Capacity.CAPACITY_TYPE_SECONDARY_STORAGE) &&
-podId == null && clusterId == null &&
-StringUtils.isEmpty(t)) {
-
taggedCapacities.add(_storageMgr.getSecondaryStorageUsedStats(null, zId));
+// List secondary and object storage capacities only when the
api is invoked for the zone layer.
+if (podId == null && clusterId == null &&
StringUtils.isEmpty(t)) {
+if (capacityType == null) {
Review Comment:
possible to move to a separate method?
##
server/src/main/java/com/cloud/configuration/Config.java:
##
@@ -134,6 +134,22 @@ public enum Config {
"0.75",
"Percentage (as a value between 0 and 1) of local storage
utilization above which alerts will be sent about low local storage available.",
null),
+BackupStorageCapacityThreshold(
Review Comment:
Would it be better to define them as ConfigKey in the respective
manager/service?
##
ui/src/views/compute/wizard/TemplateIsoSelection.vue:
##
@@ -114,6 +114,7 @@ export default {
}
}
if (this.preFillContent.t
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2716520837 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12746 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2716411432 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2716408965 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2716354954 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2714091713 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2714384779 Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 12737 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2714084453 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
DaanHoogland commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1988920461
##
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##
@@ -2345,6 +2349,17 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) throws
throw new CloudRuntimeException("Unable to destroy " +
vm);
} else {
if (expunge) {
+if (vm.getBackupOfferingId() != null) {
+List backupsForVm =
backupDao.listByVmId(vm.getDataCenterId(), vm.getId());
+if (CollectionUtils.isEmpty(backupsForVm)) {
+
backupManager.removeVMFromBackupOffering(vm.getId(), true);
+} else {
+throw new
CloudRuntimeException(String.format("This VM [uuid: %s, name: %s] has a "
++ "Backup Offering [id:
%s, external id: %s] with %s backups. Please, remove the backup offering "
++ "before proceeding to VM
exclusion!", vm.getUuid(), vm.getInstanceName(), vm.getBackupOfferingId(),
+vm.getBackupExternalId(),
backupsForVm.size()));
+}
+}
Review Comment:
extract method?
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
DaanHoogland commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1988656990
##
engine/schema/src/test/java/org/apache/cloudstack/backup/dao/BackupDaoImplTest.java:
##
@@ -0,0 +1,150 @@
+package org.apache.cloudstack.backup.dao;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.cloudstack.api.response.BackupResponse;
+import org.apache.cloudstack.backup.Backup;
+import org.apache.cloudstack.backup.BackupOfferingVO;
+import org.apache.cloudstack.backup.BackupVO;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterVO;
+import com.cloud.dc.dao.DataCenterDao;
+import com.cloud.domain.DomainVO;
+import com.cloud.domain.dao.DomainDao;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.user.AccountVO;
+import com.cloud.user.dao.AccountDao;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.VMInstanceDao;
+
+@RunWith(MockitoJUnitRunner.class)
+public class BackupDaoImplTest {
+@Spy
+@InjectMocks
+private BackupDaoImpl backupDao;
+
+@Mock
+BackupDetailsDao backupDetailsDao;
+
+@Mock
+SearchBuilder backupSearch;
+
+@Mock
+VMInstanceDao vmInstanceDao;
+
+@Mock
+AccountDao accountDao;
+
+@Mock
+DomainDao domainDao;
+
+@Mock
+DataCenterDao dataCenterDao;
+
+@Mock
+BackupOfferingDao backupOfferingDao;
+
+@Test
+public void testLoadDetails() {
+Long backupId = 1L;
+BackupVO backup = new BackupVO();
+ReflectionTestUtils.setField(backup, "id", backupId);
+Map details = new HashMap<>();
+details.put("key1", "value1");
+details.put("key2", "value2");
+
+
Mockito.when(backupDetailsDao.listDetailsKeyPairs(backupId)).thenReturn(details);
+
+backupDao.loadDetails(backup);
+
+Assert.assertEquals(details, backup.getDetails());
+Mockito.verify(backupDetailsDao).listDetailsKeyPairs(backupId);
+}
+
+@Test
+public void testSaveDetails() {
+Long backupId = 1L;
+BackupVO backup = new BackupVO();
+ReflectionTestUtils.setField(backup, "id", backupId);
+Map details = new HashMap<>();
+details.put("key1", "value1");
+details.put("key2", "value2");
+backup.setDetails(details);
+
+backupDao.saveDetails(backup);
+
+Mockito.verify(backupDetailsDao).saveDetails(Mockito.anyList());
+}
+
+@Test
+public void testNewBackupResponse() {
+Long vmId = 1L;
+Long accountId = 2L;
+Long domainId = 3L;
+Long zoneId = 4L;
+Long offeringId = 5L;
+Long backupId = 6L;
+BackupVO backup = new BackupVO();
+ReflectionTestUtils.setField(backup, "id", backupId);
+ReflectionTestUtils.setField(backup, "uuid", "backup-uuid");
+backup.setVmId(vmId);
+backup.setAccountId(accountId);
+backup.setDomainId(domainId);
+backup.setZoneId(zoneId);
+backup.setBackupOfferingId(offeringId);
+backup.setType("Full");
+backup.setBackupIntervalType((short) Backup.Type.MANUAL.ordinal());
+
+VMInstanceVO vm = new VMInstanceVO(vmId, 0L, "test-vm", "test-vm",
VirtualMachine.Type.User,
+0L, Hypervisor.HypervisorType.Simulator, 0L, domainId,
accountId, 0L, false);
+vm.setDataCenterId(zoneId);
+
+AccountVO account = new AccountVO();
+account.setUuid("account-uuid");
+account.setAccountName("test-account");
+
+DomainVO domain = new DomainVO();
+domain.setUuid("domain-uuid");
+domain.setName("test-domain");
+
+DataCenterVO zone = new DataCenterVO(1L, "test-zone", null, null,
null, null, null, null, null, null, DataCenter.NetworkType.Advanced, null,
null);
+zone.setUuid("zone-uuid");
+
+BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class);
+Mockito.when(offering.getUuid()).thenReturn("offering-uuid");
+Mockito.when(offering.getName()).thenReturn("test-offering");
+
+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
+
Mockito.when(accountDao.findByIdIncludingRemoved(accountId)).thenReturn(account);
+
Mockito.when(domainDao.findByIdIncludingRemoved(domainId)).thenReturn(domain);
+
Mockito.when(dataCenterDao.findByIdIncludingRemoved(zoneId)).thenReturn(zone);
+
Mockito.when(backupOfferingDao.findByIdIncludingRemoved(offeringId)).thenReturn(offering);
+
+BackupResponse response = backupDao.newBackupResponse(backup, false);
+
+
Re: [PR] Create new Instance from VM backup [cloudstack]
DaanHoogland commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1988656592 ## engine/schema/src/test/java/org/apache/cloudstack/backup/dao/BackupDaoImplTest.java: ## @@ -0,0 +1,150 @@ +package org.apache.cloudstack.backup.dao; Review Comment: license -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2712912023 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12734 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2712763171 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2712768339 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
github-actions[bot] commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2703022129 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
github-actions[bot] commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2679535756 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
github-actions[bot] commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2642759729 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2619796120 > 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 thanks @shwstppr I have fixed the disk-offering edge case and catch all exceptions plus some minor UI fixes. Also linked doc PR. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
shwstppr commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1922242112
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -748,6 +819,151 @@ private Backup.VolumeInfo
getVolumeInfo(List backedUpVolumes,
return null;
}
+@Override
+public void updateDiskOfferingSizeFromBackup(List
dataDiskOfferingsInfo, Backup backup) {
+List dataDiskOfferingsInfoFromBackup =
getDataDiskOfferingListFromBackup(backup);
+int index = 0;
+for(DiskOfferingInfo diskOfferingInfo : dataDiskOfferingsInfo) {
+diskOfferingInfo.setSize(Math.max(diskOfferingInfo.getSize(),
dataDiskOfferingsInfoFromBackup.get(index).getSize()));
+index++;
+}
+}
+
+@Override
+public DiskOfferingInfo getRootDiskOfferingInfoFromBackup(Backup backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
+if (diskOfferingIdsDetail == null) {
+return null;
+}
+String [] diskOfferingIds = diskOfferingIdsDetail.split(",");
+String [] deviceIds =
backup.getDetail(ApiConstants.DEVICE_IDS).split(",");
+String [] diskSizes =
backup.getDetail(ApiConstants.DISK_SIZES).split(",");
+
+for (int i = 0; i < diskOfferingIds.length; i++) {
+if (deviceIds[i].equals("0")) {
+DiskOfferingVO diskOffering =
diskOfferingDao.findByUuid(diskOfferingIds[i]);
+if (diskOffering == null) {
+throw new CloudRuntimeException("Unable to find the root
disk offering with uuid (" + diskOfferingIds[i] + ") stored in backup. Please
specify a valid root disk offering id while creating the instance");
+}
+Long size = Long.parseLong(diskSizes[i]) / (1024 * 1024 *
1024);
+return new DiskOfferingInfo(diskOffering, size, null, null,
0L);
+}
+}
+return null;
+}
+
+@Override
+public List getDataDiskOfferingListFromBackup(Backup
backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
+if (diskOfferingIdsDetail == null) {
+return null;
+}
+
+String [] diskOfferingIds = diskOfferingIdsDetail.split(",");
+String [] deviceIds =
backup.getDetail(ApiConstants.DEVICE_IDS).split(",");
+String [] diskSizes =
backup.getDetail(ApiConstants.DISK_SIZES).split(",");
+String [] minIopsList =
backup.getDetail(ApiConstants.MIN_IOPS).split(",");
+String [] maxIopsList =
backup.getDetail(ApiConstants.MAX_IOPS).split(",");
+
+List diskOfferingInfoList = new ArrayList<>();
+for (int i = 0; i < diskOfferingIds.length; i++) {
+Long deviceId = Long.parseLong(deviceIds[i]);
+if (deviceId == 0) {
+continue;
+}
+DiskOfferingVO diskOffering =
diskOfferingDao.findByUuid(diskOfferingIds[i]);
+if (diskOffering == null) {
+throw new CloudRuntimeException("Unable to find the disk
offering with uuid (" + diskOfferingIds[i] + ") stored in backup. Please
specify a valid disk offering id while creating the instance");
+}
+Long size = Long.parseLong(diskSizes[i]) / (1024 * 1024 * 1024);
+Long minIops = (diskOffering.isCustomizedIops() != null &&
diskOffering.isCustomizedIops() && !minIopsList[i].equals("null")) ?
+Long.parseLong(minIopsList[i]) : null;
+Long maxIops = (diskOffering.isCustomizedIops() != null &&
diskOffering.isCustomizedIops() && !maxIopsList[i].equals("null")) ?
+Long.parseLong(maxIopsList[i]) : null;
+diskOfferingInfoList.add(new DiskOfferingInfo(diskOffering, size,
minIops, maxIops, deviceId));
+}
+return diskOfferingInfoList;
+}
+
+@Override
+public boolean restoreBackupToVM(final Long backupId, final Long vmId)
throws ResourceUnavailableException {
+final BackupVO backup = backupDao.findById(backupId);
+if (backup == null) {
+throw new CloudRuntimeException("Backup " + backupId + " does not
exist");
+}
+validateBackupForZone(backup.getZoneId());
+
+VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(vmId);
+if (vm == null) {
+throw new CloudRuntimeException("VM ID " + backup.getVmId() + "
couldn't be found on existing or removed VMs");
+}
+accountManager.checkAccess(CallContext.current().getCallingAccount(),
null, true, vm);
+
+if (vm.getRemoved() != null) {
+throw new CloudRuntimeException("VM is removed from CloudStack");
+}
+if (vm.getRemoved() != null) {
+throw new CloudRuntimeException("VM should be in the stopped
state");
+}
+if (!vm.getState().equals(VirtualMa
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918164498
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -6139,6 +6186,11 @@ public UserVm createVirtualMachine(DeployVMCmd cmd)
throws InsufficientCapacityE
}
}
+List dataDiskOfferingsInfo =
cmd.getDataDiskOfferingsInfo();
+if (dataDiskOfferingsInfo != null && diskOfferingId != null) {
Review 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.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
github-actions[bot] commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2596333025 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918392131
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -748,6 +819,151 @@ private Backup.VolumeInfo
getVolumeInfo(List backedUpVolumes,
return null;
}
+@Override
+public void updateDiskOfferingSizeFromBackup(List
dataDiskOfferingsInfo, Backup backup) {
+List dataDiskOfferingsInfoFromBackup =
getDataDiskOfferingListFromBackup(backup);
+int index = 0;
+for(DiskOfferingInfo diskOfferingInfo : dataDiskOfferingsInfo) {
+diskOfferingInfo.setSize(Math.max(diskOfferingInfo.getSize(),
dataDiskOfferingsInfoFromBackup.get(index).getSize()));
+index++;
+}
+}
+
+@Override
+public DiskOfferingInfo getRootDiskOfferingInfoFromBackup(Backup backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
+if (diskOfferingIdsDetail == null) {
+return null;
+}
+String [] diskOfferingIds = diskOfferingIdsDetail.split(",");
+String [] deviceIds =
backup.getDetail(ApiConstants.DEVICE_IDS).split(",");
+String [] diskSizes =
backup.getDetail(ApiConstants.DISK_SIZES).split(",");
+
+for (int i = 0; i < diskOfferingIds.length; i++) {
+if (deviceIds[i].equals("0")) {
+DiskOfferingVO diskOffering =
diskOfferingDao.findByUuid(diskOfferingIds[i]);
+if (diskOffering == null) {
+throw new CloudRuntimeException("Unable to find the root
disk offering with uuid (" + diskOfferingIds[i] + ") stored in backup. Please
specify a valid root disk offering id while creating the instance");
+}
+Long size = Long.parseLong(diskSizes[i]) / (1024 * 1024 *
1024);
+return new DiskOfferingInfo(diskOffering, size, null, null,
0L);
+}
+}
+return null;
+}
+
+@Override
+public List getDataDiskOfferingListFromBackup(Backup
backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
+if (diskOfferingIdsDetail == null) {
+return null;
+}
+
+String [] diskOfferingIds = diskOfferingIdsDetail.split(",");
+String [] deviceIds =
backup.getDetail(ApiConstants.DEVICE_IDS).split(",");
+String [] diskSizes =
backup.getDetail(ApiConstants.DISK_SIZES).split(",");
+String [] minIopsList =
backup.getDetail(ApiConstants.MIN_IOPS).split(",");
+String [] maxIopsList =
backup.getDetail(ApiConstants.MAX_IOPS).split(",");
+
+List diskOfferingInfoList = new ArrayList<>();
+for (int i = 0; i < diskOfferingIds.length; i++) {
+Long deviceId = Long.parseLong(deviceIds[i]);
+if (deviceId == 0) {
+continue;
+}
+DiskOfferingVO diskOffering =
diskOfferingDao.findByUuid(diskOfferingIds[i]);
+if (diskOffering == null) {
+throw new CloudRuntimeException("Unable to find the disk
offering with uuid (" + diskOfferingIds[i] + ") stored in backup. Please
specify a valid disk offering id while creating the instance");
+}
+Long size = Long.parseLong(diskSizes[i]) / (1024 * 1024 * 1024);
+Long minIops = (diskOffering.isCustomizedIops() != null &&
diskOffering.isCustomizedIops() && !minIopsList[i].equals("null")) ?
+Long.parseLong(minIopsList[i]) : null;
+Long maxIops = (diskOffering.isCustomizedIops() != null &&
diskOffering.isCustomizedIops() && !maxIopsList[i].equals("null")) ?
+Long.parseLong(maxIopsList[i]) : null;
+diskOfferingInfoList.add(new DiskOfferingInfo(diskOffering, size,
minIops, maxIops, deviceId));
+}
+return diskOfferingInfoList;
+}
+
+@Override
+public boolean restoreBackupToVM(final Long backupId, final Long vmId)
throws ResourceUnavailableException {
+final BackupVO backup = backupDao.findById(backupId);
+if (backup == null) {
+throw new CloudRuntimeException("Backup " + backupId + " does not
exist");
+}
+validateBackupForZone(backup.getZoneId());
+
+VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(vmId);
+if (vm == null) {
+throw new CloudRuntimeException("VM ID " + backup.getVmId() + "
couldn't be found on existing or removed VMs");
+}
+accountManager.checkAccess(CallContext.current().getCallingAccount(),
null, true, vm);
+
+if (vm.getRemoved() != null) {
+throw new CloudRuntimeException("VM is removed from CloudStack");
+}
+if (vm.getRemoved() != null) {
+throw new CloudRuntimeException("VM should be in the stopped
state");
+}
+if (!vm.getState().equals(VirtualMac
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918199072
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -748,6 +819,151 @@ private Backup.VolumeInfo
getVolumeInfo(List backedUpVolumes,
return null;
}
+@Override
+public void updateDiskOfferingSizeFromBackup(List
dataDiskOfferingsInfo, Backup backup) {
+List dataDiskOfferingsInfoFromBackup =
getDataDiskOfferingListFromBackup(backup);
+int index = 0;
+for(DiskOfferingInfo diskOfferingInfo : dataDiskOfferingsInfo) {
+diskOfferingInfo.setSize(Math.max(diskOfferingInfo.getSize(),
dataDiskOfferingsInfoFromBackup.get(index).getSize()));
Review Comment:
A volume with fixed size disk offering of 5gb can be resized later to 7gb.
In that case backup details table will have the disk size as 7GB.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918196373
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -748,6 +819,151 @@ private Backup.VolumeInfo
getVolumeInfo(List backedUpVolumes,
return null;
}
+@Override
+public void updateDiskOfferingSizeFromBackup(List
dataDiskOfferingsInfo, Backup backup) {
+List dataDiskOfferingsInfoFromBackup =
getDataDiskOfferingListFromBackup(backup);
+int index = 0;
+for(DiskOfferingInfo diskOfferingInfo : dataDiskOfferingsInfo) {
+diskOfferingInfo.setSize(Math.max(diskOfferingInfo.getSize(),
dataDiskOfferingsInfoFromBackup.get(index).getSize()));
+index++;
+}
+}
+
+@Override
+public DiskOfferingInfo getRootDiskOfferingInfoFromBackup(Backup backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
Review Comment:
For older backups User will get an error "Backup doesn't contain service
offering uuid. Please specify a valid service offering id while creating the
instance"
User will have to click on Configure Instance button and provide inputs.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918196798
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -748,6 +819,151 @@ private Backup.VolumeInfo
getVolumeInfo(List backedUpVolumes,
return null;
}
+@Override
+public void updateDiskOfferingSizeFromBackup(List
dataDiskOfferingsInfo, Backup backup) {
+List dataDiskOfferingsInfoFromBackup =
getDataDiskOfferingListFromBackup(backup);
+int index = 0;
+for(DiskOfferingInfo diskOfferingInfo : dataDiskOfferingsInfo) {
+diskOfferingInfo.setSize(Math.max(diskOfferingInfo.getSize(),
dataDiskOfferingsInfoFromBackup.get(index).getSize()));
+index++;
+}
+}
+
+@Override
+public DiskOfferingInfo getRootDiskOfferingInfoFromBackup(Backup backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
+if (diskOfferingIdsDetail == null) {
+return null;
+}
+String [] diskOfferingIds = diskOfferingIdsDetail.split(",");
+String [] deviceIds =
backup.getDetail(ApiConstants.DEVICE_IDS).split(",");
+String [] diskSizes =
backup.getDetail(ApiConstants.DISK_SIZES).split(",");
+
+for (int i = 0; i < diskOfferingIds.length; i++) {
+if (deviceIds[i].equals("0")) {
+DiskOfferingVO diskOffering =
diskOfferingDao.findByUuid(diskOfferingIds[i]);
+if (diskOffering == null) {
+throw new CloudRuntimeException("Unable to find the root
disk offering with uuid (" + diskOfferingIds[i] + ") stored in backup. Please
specify a valid root disk offering id while creating the instance");
+}
+Long size = Long.parseLong(diskSizes[i]) / (1024 * 1024 *
1024);
+return new DiskOfferingInfo(diskOffering, size, null, null,
0L);
+}
+}
+return null;
+}
+
+@Override
+public List getDataDiskOfferingListFromBackup(Backup
backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
+if (diskOfferingIdsDetail == null) {
+return null;
+}
+
+String [] diskOfferingIds = diskOfferingIdsDetail.split(",");
+String [] deviceIds =
backup.getDetail(ApiConstants.DEVICE_IDS).split(",");
+String [] diskSizes =
backup.getDetail(ApiConstants.DISK_SIZES).split(",");
+String [] minIopsList =
backup.getDetail(ApiConstants.MIN_IOPS).split(",");
+String [] maxIopsList =
backup.getDetail(ApiConstants.MAX_IOPS).split(",");
+
+List diskOfferingInfoList = new ArrayList<>();
+for (int i = 0; i < diskOfferingIds.length; i++) {
+Long deviceId = Long.parseLong(deviceIds[i]);
+if (deviceId == 0) {
+continue;
+}
+DiskOfferingVO diskOffering =
diskOfferingDao.findByUuid(diskOfferingIds[i]);
Review Comment:
Will test it
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918189120
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -273,10 +292,62 @@ public boolean deleteBackupOffering(final Long
offeringId) {
throw new CloudRuntimeException("Backup offering is assigned to
VMs, remove the assignment(s) in order to remove the offering.");
}
-validateForZone(offering.getZoneId());
+validateBackupForZone(offering.getZoneId());
return backupOfferingDao.remove(offering.getId());
}
+@Override
+public Map getVmDetailsForBackup(VirtualMachine vm) {
+HashMap details = new HashMap<>();
+details.put(ApiConstants.HYPERVISOR,
vm.getHypervisorType().toString());
+ServiceOffering serviceOffering =
entityManager.findById(ServiceOffering.class, vm.getServiceOfferingId());
Review Comment:
No, will use the respective Daos.
##
plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/NetworkerBackupProvider.java:
##
@@ -117,6 +122,12 @@ public class NetworkerBackupProvider extends AdapterBase
implements BackupProvid
@Inject
private VMInstanceDao vmInstanceDao;
+@Inject
+private EntityManager entityManager;
Review Comment:
will do
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918175515 ## ui/src/views/compute/DeployVMFromBackup.vue: ## @@ -0,0 +1,2211 @@ +// Licensed to the Apache Software Foundation (ASF) under one Review Comment: @shwstppr I had put this here as it is similar to DeployVM.vue but I guess components make more sense as it is being used as a component -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918168554 ## ui/public/locales/en.json: ## @@ -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", Review Comment: Not needed. Will remove one. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918164498
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -6139,6 +6186,11 @@ public UserVm createVirtualMachine(DeployVMCmd cmd)
throws InsufficientCapacityE
}
}
+List dataDiskOfferingsInfo =
cmd.getDataDiskOfferingsInfo();
+if (dataDiskOfferingsInfo != null && diskOfferingId != null) {
Review 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, but only if you think it is still worth
doing.
--
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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1918117662 ## api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java: ## @@ -147,6 +148,13 @@ public class DeployVMCmd extends BaseAsyncCreateCustomIdCmd implements SecurityG since = "4.4") private Long rootdisksize; +@Parameter(name = ApiConstants.DATADISKS_DETAILS, Review 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) -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
shwstppr commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1917875143
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -6139,6 +6186,11 @@ public UserVm createVirtualMachine(DeployVMCmd cmd)
throws InsufficientCapacityE
}
}
+List dataDiskOfferingsInfo =
cmd.getDataDiskOfferingsInfo();
+if (dataDiskOfferingsInfo != null && diskOfferingId != null) {
Review 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?
##
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java:
##
@@ -147,6 +148,13 @@ public class DeployVMCmd extends
BaseAsyncCreateCustomIdCmd implements SecurityG
since = "4.4")
private Long rootdisksize;
+@Parameter(name = ApiConstants.DATADISKS_DETAILS,
Review Comment:
@abh1sar is it possible to use the existing details param here?
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -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 names = cmd.getNames();
+if (names == null || names.isEmpty()) {
Review Comment:
use StringUtils?
##
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##
@@ -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")
Review Comment:
since attribute here
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -273,10 +292,62 @@ public boolean deleteBackupOffering(final Long
offeringId) {
throw new CloudRuntimeException("Backup offering is assigned to
VMs, remove the assignment(s) in order to remove the offering.");
}
-validateForZone(offering.getZoneId());
+validateBackupForZone(offering.getZoneId());
return backupOfferingDao.remove(offering.getId());
}
+@Override
+public Map getVmDetailsForBackup(VirtualMachine vm) {
+HashMap details = new HashMap<>();
+details.put(ApiConstants.HYPERVISOR,
vm.getHypervisorType().toString());
+ServiceOffering serviceOffering =
entityManager.findById(ServiceOffering.class, vm.getServiceOfferingId());
Review Comment:
Any reason ServiceOfferingDao, VmTemplateDao can't be used here?
##
plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/NetworkerBackupProvider.java:
##
@@ -117,6 +122,12 @@ public class NetworkerBackupProvider extends AdapterBase
implements BackupProvid
@Inject
private VMInstanceDao vmInstanceDao;
+@Inject
+private EntityManager entityManager;
Review Comment:
Can we use `*Daos` instead?
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -748,6 +819,151 @@ private Backup.VolumeInfo
getVolumeInfo(List backedUpVolumes,
return null;
}
+@Override
+public void updateDiskOfferingSizeFromBackup(List
dataDiskOfferingsInfo, Backup backup) {
+List dataDiskOfferingsInfoFromBackup =
getDataDiskOfferingListFromBackup(backup);
+int index = 0;
+for(DiskOfferingInfo diskOfferingInfo : dataDiskOfferingsInfo) {
+diskOfferingInfo.setSize(Math.max(diskOfferingInfo.getSize(),
dataDiskOfferingsInfoFromBackup.get(index).getSize()));
+index++;
+}
+}
+
+@Override
+public DiskOfferingInfo getRootDiskOfferingInfoFromBackup(Backup backup) {
+String diskOfferingIdsDetail =
backup.getDetail(ApiConstants.DISK_OFFERING_IDS);
Review Comment:
These details will be stored only for new backups, what happens if an older
backup is used? Does the user need to provide inputs in that case?
##
plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/networker/NetworkerClient.java:
##
@@ -22,6 +22,7 @@
import com.cloud.vm.VirtualMachine;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
+
Review Comment:
Changes in this file can be reverted maybe
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -748,6 +819,151 @@ private Backup.VolumeInfo
getVolumeInfo(List backedUpVolumes,
return null;
}
+@Override
+public void updateDiskOfferingSizeFromBackup(List
dataDiskO
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2594696479 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12095 -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
blueorangutan commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2594610247 @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. -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
abh1sar commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2594607510 @blueorangutan package -- 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
Re: [PR] Create new Instance from VM backup [cloudstack]
github-actions[bot] commented on PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#issuecomment-2594426112 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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
