You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/09/29 19:17:20 UTC
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4307: [VMware] vSphere advanced capabilities and Full OVF properties support
DaanHoogland commented on a change in pull request #4307:
URL: https://github.com/apache/cloudstack/pull/4307#discussion_r496704106
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/template/ListTemplatesCmd.java
##########
@@ -82,10 +86,35 @@
@Parameter(name = ApiConstants.PARENT_TEMPLATE_ID, type = CommandType.UUID, entityType = TemplateResponse.class, description = "list datadisk templates by parent template id", since = "4.4")
private Long parentTemplateId;
+ @Parameter(name = ApiConstants.DETAILS,
Review comment:
add a `since = "4.15"`
##########
File path: api/src/main/java/org/apache/cloudstack/api/response/TemplateResponse.java
##########
@@ -194,12 +195,22 @@
@Param(description = "KVM Only: true if template is directly downloaded to Primary Storage bypassing Secondary Storage")
private Boolean directDownload;
+ @SerializedName(ApiConstants.DEPLOY_AS_IS)
+ @Param(description = "VMware only: true if template is deployed without orchestrating disks and networks but \"as-is\" defined in the template.")
+ private Boolean deployAsIs;
+
+ @SerializedName(ApiConstants.DEPLOY_AS_IS_DETAILS)
+ @Param(description = "VMware only: additional key/value details tied with deploy-as-is template")
Review comment:
add `since = "4.15"`
##########
File path: api/src/main/java/com/cloud/agent/api/to/DeployAsIsInfoTO.java
##########
@@ -0,0 +1,58 @@
+// 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 com.cloud.agent.api.to;
Review comment:
new class should be in org.apache.cloudstack
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFNetworkTO.java
##########
@@ -0,0 +1,124 @@
+// 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 com.cloud.agent.api.to.deployasis;
Review comment:
new class should be in org.apache.cloudstack
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFEulaSectionTO.java
##########
@@ -0,0 +1,49 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
+
+import com.cloud.agent.api.LogLevel;
+
+/**
Review comment:
👍 🎉
##########
File path: engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateService.java
##########
@@ -72,4 +75,6 @@ public TemplateInfo getTemplate() {
void associateCrosszoneTemplatesToZone(long dcId);
AsyncCallFuture<TemplateApiResult> createDatadiskTemplateAsync(TemplateInfo parentTemplate, TemplateInfo dataDiskTemplate, String path, String diskId, long fileSize, boolean bootable);
+
+ List<DatadiskTO> getTemplateDatadisksOnImageStore(TemplateInfo templateInfo, String configurationId);
Review comment:
here the string is `configurationId` instead of `configuration`. would be nice to javadoc the difference
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1085,7 +1170,7 @@ boolean isNetworkImplemented(final NetworkVO network) {
}
Pair<NetworkGuru, NetworkVO> implementNetwork(final long networkId, final DeployDestination dest, final ReservationContext context, final boolean isRouter) throws ConcurrentOperationException,
- ResourceUnavailableException, InsufficientCapacityException {
+ ResourceUnavailableException, InsufficientCapacityException {
Review comment:
with throws?
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1661,7 +1746,7 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
@Override
public void prepare(final VirtualMachineProfile vmProfile, final DeployDestination dest, final ReservationContext context) throws InsufficientCapacityException, ConcurrentOperationException,
- ResourceUnavailableException {
+ ResourceUnavailableException {
Review comment:
with throws?
##########
File path: engine/schema/src/main/java/com/cloud/storage/VMTemplateStoragePoolVO.java
##########
@@ -168,17 +171,18 @@ public Status getDownloadState() {
return downloadState;
}
- public VMTemplateStoragePoolVO(long poolId, long templateId) {
+ public VMTemplateStoragePoolVO(long poolId, long templateId, String configuration) {
super();
this.poolId = poolId;
this.templateId = templateId;
this.downloadState = Status.NOT_DOWNLOADED;
this.state = ObjectInDataStoreStateMachine.State.Allocated;
this.markedForGC = false;
+ this.deploymentOption = configuration;
Review comment:
why this difference in name? what do either mean?
##########
File path: engine/schema/src/main/resources/META-INF/db/schema-41400to41500.sql
##########
@@ -220,3 +516,22 @@ ALTER VIEW `cloud`.`image_store_view` AS
`cloud`.`data_center` ON image_store.data_center_id = data_center.id
left join
`cloud`.`image_store_details` ON image_store_details.store_id = image_store.id;
+
+-- OVF configured OS while registering deploy-as-is templates Linux 3.x Kernel OS
+INSERT IGNORE INTO `cloud`.`guest_os` (id, uuid, category_id, display_name, created) VALUES (305, UUID(), 11, 'OVF Configured OS', now());
+INSERT IGNORE INTO `cloud`.`guest_os` (id, uuid, category_id, display_name, created) VALUES (306, UUID(), 2, 'Linux 3.x Kernel (64 bit)', now());
+INSERT IGNORE INTO `cloud`.`guest_os` (id, uuid, category_id, display_name, created) VALUES (307, UUID(), 2, 'Linux 3.x Kernel (32 bit)', now());
+
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.0', 'other3xLinux64Guest', 306, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.5', 'other3xLinux64Guest', 306, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7', 'other3xLinux64Guest', 306, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7.1', 'other3xLinux64Guest', 306, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7.2', 'other3xLinux64Guest', 306, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7.3', 'other3xLinux64Guest', 306, now(), 0);
+
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.0', 'other3xLinuxGuest', 307, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.5', 'other3xLinuxGuest', 307, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7', 'other3xLinuxGuest', 307, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7.1', 'other3xLinuxGuest', 307, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7.2', 'other3xLinuxGuest', 307, now(), 0);
+INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) VALUES (UUID(), 'VMware', '6.7.3', 'other3xLinuxGuest', 307, now(), 0);
Review comment:
why are these part of this PR?
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFEulaSectionTO.java
##########
@@ -0,0 +1,49 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
Review comment:
new class should be in org.apache.cloudstack
##########
File path: core/src/main/java/com/cloud/agent/api/ValidateVcenterDetailsCommand.java
##########
@@ -0,0 +1,40 @@
+// 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 com.cloud.agent.api;
Review comment:
new class should be in org.apache.cloudstack
##########
File path: api/src/main/java/org/apache/cloudstack/api/response/TemplateResponse.java
##########
@@ -194,12 +195,22 @@
@Param(description = "KVM Only: true if template is directly downloaded to Primary Storage bypassing Secondary Storage")
private Boolean directDownload;
+ @SerializedName(ApiConstants.DEPLOY_AS_IS)
+ @Param(description = "VMware only: true if template is deployed without orchestrating disks and networks but \"as-is\" defined in the template.")
Review comment:
add `since = "4.15"`
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1773,22 +1824,55 @@ public static void importVmFromOVF(VmwareHypervisorHost host, String ovfFilePath
}
}
- DatacenterMO dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
- ManagedObjectReference morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), dcMo.getVmFolder(), morHost);
+ DatacenterMO dcMo = null;
+ try {
+ dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
+ } catch (Exception e) {
+ throw new CloudRuntimeException(String.format("no datacenter for host '%s' available in context", context.getServerAddress()), e);
+ }
+ ManagedObjectReference folderMO = null;
+ try {
+ folderMO = dcMo.getVmFolder();
+ } catch (Exception e) {
+ throw new CloudRuntimeException("no management handle for VmFolder", e);
+ }
+ ManagedObjectReference morLease = null;
+ try {
+ morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), folderMO, morHost);
+ } catch (DuplicateNameFaultMsg
+ | FileFaultFaultMsg
+ | InsufficientResourcesFaultFaultMsg
+ | InvalidDatastoreFaultMsg
+ | InvalidNameFaultMsg
+ | OutOfBoundsFaultMsg
+ | RuntimeFaultFaultMsg
+ | VmConfigFaultFaultMsg fault) {
+ throw new CloudRuntimeException("import vApp failed",fault);
+ }
if (morLease == null) {
String msg = "importVApp() failed. ovfFilePath: " + ovfFilePath + ", vmName: " + vmName + ", diskOption: " + diskOption;
s_logger.error(msg);
- throw new Exception(msg);
+ throw new CloudRuntimeException(msg);
}
boolean importSuccess = true;
final HttpNfcLeaseMO leaseMo = new HttpNfcLeaseMO(context, morLease);
- HttpNfcLeaseState state = leaseMo.waitState(new HttpNfcLeaseState[] {HttpNfcLeaseState.READY, HttpNfcLeaseState.ERROR});
+ HttpNfcLeaseState state = null;
+ try {
+ state = leaseMo.waitState(new HttpNfcLeaseState[] {HttpNfcLeaseState.READY, HttpNfcLeaseState.ERROR});
+ } catch (Exception e) {
+ throw new CloudRuntimeException("exception while waiting for leaseMO", e);
+ }
Review comment:
new method
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFConfigurationTO.java
##########
@@ -0,0 +1,61 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
Review comment:
new class should be in org.apache.cloudstack
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1773,22 +1824,55 @@ public static void importVmFromOVF(VmwareHypervisorHost host, String ovfFilePath
}
}
- DatacenterMO dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
- ManagedObjectReference morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), dcMo.getVmFolder(), morHost);
+ DatacenterMO dcMo = null;
+ try {
+ dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
+ } catch (Exception e) {
+ throw new CloudRuntimeException(String.format("no datacenter for host '%s' available in context", context.getServerAddress()), e);
+ }
+ ManagedObjectReference folderMO = null;
+ try {
+ folderMO = dcMo.getVmFolder();
+ } catch (Exception e) {
+ throw new CloudRuntimeException("no management handle for VmFolder", e);
+ }
+ ManagedObjectReference morLease = null;
+ try {
+ morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), folderMO, morHost);
+ } catch (DuplicateNameFaultMsg
+ | FileFaultFaultMsg
+ | InsufficientResourcesFaultFaultMsg
+ | InvalidDatastoreFaultMsg
+ | InvalidNameFaultMsg
+ | OutOfBoundsFaultMsg
+ | RuntimeFaultFaultMsg
+ | VmConfigFaultFaultMsg fault) {
+ throw new CloudRuntimeException("import vApp failed",fault);
+ }
if (morLease == null) {
String msg = "importVApp() failed. ovfFilePath: " + ovfFilePath + ", vmName: " + vmName + ", diskOption: " + diskOption;
s_logger.error(msg);
- throw new Exception(msg);
+ throw new CloudRuntimeException(msg);
}
boolean importSuccess = true;
final HttpNfcLeaseMO leaseMo = new HttpNfcLeaseMO(context, morLease);
- HttpNfcLeaseState state = leaseMo.waitState(new HttpNfcLeaseState[] {HttpNfcLeaseState.READY, HttpNfcLeaseState.ERROR});
+ HttpNfcLeaseState state = null;
+ try {
+ state = leaseMo.waitState(new HttpNfcLeaseState[] {HttpNfcLeaseState.READY, HttpNfcLeaseState.ERROR});
+ } catch (Exception e) {
+ throw new CloudRuntimeException("exception while waiting for leaseMO", e);
+ }
try {
if (state == HttpNfcLeaseState.READY) {
final long totalBytes = HttpNfcLeaseMO.calcTotalBytes(ovfImportResult);
File ovfFile = new File(ovfFilePath);
- HttpNfcLeaseInfo httpNfcLeaseInfo = leaseMo.getLeaseInfo();
+ HttpNfcLeaseInfo httpNfcLeaseInfo = null;
+ try {
+ httpNfcLeaseInfo = leaseMo.getLeaseInfo();
+ } catch (Exception e) {
+ throw new CloudRuntimeException("error waiting for lease info", e);
+ }
Review comment:
new method
##########
File path: api/src/main/java/com/cloud/agent/api/to/OVFInformationTO.java
##########
@@ -0,0 +1,95 @@
+//
+// 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 com.cloud.agent.api.to;
Review comment:
new class should be in org.apache.cloudstack
##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -745,20 +752,38 @@ private Answer execute(ResizeVolumeCommand cmd) {
} else if (newSize == oldSize) {
return new ResizeVolumeAnswer(cmd, true, "success", newSize * ResourceType.bytesToKiB);
}
+ /*
+ // FR41 this is yet to fix
+ ManagedObjectReference morDS1 = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, cmd.getPoolUuid());
+ DatastoreMO dsMo1 = new DatastoreMO(hyperHost.getContext(), morDS1);
+ vmdkDataStorePath = VmwareStorageLayoutHelper.getLegacyDatastorePathFromVmdkFileName(dsMo1, path + VMDK_EXTENSION);
+ DatastoreFile dsFile1 = new DatastoreFile(vmdkDataStorePath);
+
+ s_logger.debug("vDiskid does not exist for volume " + vmdkDataStorePath + " registering the disk now");
+ VirtualStorageObjectManagerMO vStorageObjectManagerMO = new VirtualStorageObjectManagerMO(getServiceContext());
+ try {
+ VStorageObject vStorageObject = vStorageObjectManagerMO.registerVirtualDisk(dsFile1, null, dsMo1.getOwnerDatacenter().second());
+ VStorageObjectConfigInfo diskConfigInfo = vStorageObject.getConfig();
+ ID vdiskId = diskConfigInfo.getId();
+ } catch (Throwable e) {
+ if (e instanceof AlreadyExistsFaultMsg) {
+
+ }
+ }*/
Review comment:
code in comment please remove
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFConfigurationTO.java
##########
@@ -0,0 +1,61 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
+
+import java.util.List;
+
+public class OVFConfigurationTO implements TemplateDeployAsIsInformationTO {
Review comment:
small javadoc ?
```suggestion
/**
* contains lists of configurations items, parsed from the OVF
*/
public class OVFConfigurationTO implements TemplateDeployAsIsInformationTO {
```
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFVirtualHardwareItemTO.java
##########
@@ -0,0 +1,365 @@
+// 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 com.cloud.agent.api.to.deployasis;
+
+// From: https://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSettingData.xsd
Review comment:
please change to javadoc, adding two or three words? nothing special
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFVirtualHardwareItemTO.java
##########
@@ -0,0 +1,365 @@
+// 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 com.cloud.agent.api.to.deployasis;
Review comment:
new class should be in `org.apache.cloudstack`
##########
File path: engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java
##########
@@ -117,8 +120,11 @@ DiskProfile allocateRawVolume(Type type, String name, DiskOffering offering, Lon
boolean canVmRestartOnAnotherServer(long vmId);
- DiskProfile allocateTemplatedVolume(Type type, String name, DiskOffering offering, Long rootDisksize, Long minIops, Long maxIops, VirtualMachineTemplate template, VirtualMachine vm,
- Account owner);
+ /**
Review comment:
👍
##########
File path: plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/api/command/admin/zone/ListVsphereStoragePoliciesCmd.java
##########
@@ -0,0 +1,109 @@
+// 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.admin.zone;
+
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.VsphereStoragePolicy;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.hypervisor.vmware.VmwareDatacenterService;
+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.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.api.response.VsphereStoragePoliciesResponse;
+import org.apache.cloudstack.api.response.ZoneResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import javax.inject.Inject;
+import java.util.ArrayList;
+import java.util.List;
+
+@APICommand(name = ListVsphereStoragePoliciesCmd.APINAME, description = "List vSphere storage policies",
+ responseObject = VsphereStoragePoliciesResponse.class,
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+ authorized = {RoleType.Admin})
+public class ListVsphereStoragePoliciesCmd extends BaseCmd {
Review comment:
```suggestion
@APICommand(name = ListVsphereStoragePoliciesCmd.APINAME, description = "List vSphere storage policies",
responseObject = VsphereStoragePoliciesResponse.class,
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
since = "4.15", authorized = {RoleType.Admin})
public class ListVsphereStoragePoliciesCmd extends BaseCmd {
```
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/TemplateDeployAsIsInformationTO.java
##########
@@ -0,0 +1,24 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
+
+import java.io.Serializable;
+
+public interface TemplateDeployAsIsInformationTO extends Serializable {
Review comment:
what is this needed for?
##########
File path: api/src/main/java/com/cloud/agent/api/to/OVFInformationTO.java
##########
@@ -0,0 +1,95 @@
+//
+// 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 com.cloud.agent.api.to;
+
+import com.cloud.agent.api.LogLevel;
+import com.cloud.agent.api.to.deployasis.OVFEulaSectionTO;
+import com.cloud.agent.api.to.deployasis.OVFNetworkTO;
+import com.cloud.agent.api.to.deployasis.OVFPropertyTO;
+import com.cloud.agent.api.to.deployasis.OVFVirtualHardwareSectionTO;
+import com.cloud.utils.Pair;
+
+import java.util.List;
+
+public class OVFInformationTO {
Review comment:
small javadoc?
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFVirtualHardwareSectionTO.java
##########
@@ -0,0 +1,50 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
+
+import java.util.List;
+
+public class OVFVirtualHardwareSectionTO implements TemplateDeployAsIsInformationTO {
Review comment:
good name ==> no javadoc needed
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFVirtualHardwareSectionTO.java
##########
@@ -0,0 +1,50 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
Review comment:
new class should be in `org.apache.cloudstack`
##########
File path: core/src/main/java/com/cloud/agent/api/ValidateVcenterDetailsCommand.java
##########
@@ -0,0 +1,40 @@
+// 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 com.cloud.agent.api;
+
+public class ValidateVcenterDetailsCommand extends Command {
Review comment:
what is it for?
##########
File path: core/src/main/java/org/apache/cloudstack/storage/command/CheckDataStoreStoragePolicyComplainceCommand.java
##########
@@ -0,0 +1,61 @@
+//
+// 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.storage.command;
+
+import com.cloud.agent.api.to.StorageFilerTO;
+
+public class CheckDataStoreStoragePolicyComplainceCommand extends StorageSubSystemCommand {
+
+ String storagePolicyId;
+ private StorageFilerTO storagePool;
+
+ public CheckDataStoreStoragePolicyComplainceCommand(String storagePolicyId, StorageFilerTO storagePool) {
Review comment:
short javadoc with descrition of this command-answer pattern? at least what is returned under what condition? just by the name it might be a `boolean` or a `List<StoragePolicy>` that is expected.
##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
##########
@@ -1124,6 +1203,12 @@ private void postCreatePrivateTemplate(String installFullPath, long templateId,
throw new Exception(msg);
}
+ DatacenterMO dcMo = new DatacenterMO(context, hyperHost.getHyperHostDatacenter());
+ ManagedObjectReference morPool = hyperHost.getHyperHostOwnerResourcePool();
+ vmMo.createFullCloneWithSpecificDisk(templateUniqueName, dcMo.getVmFolder(), morPool, VmwareHelper.getDiskDeviceDatastore(volumeDeviceInfo.first()), volumeDeviceInfo);
+ clonedVm = dcMo.findVm(templateUniqueName);
+
+ /* FR41 THIS IS OLD way of creating template using snapshot
Review comment:
internal project code in open source version, please remove.
##########
File path: api/src/main/java/com/cloud/agent/api/to/DeployAsIsInfoTO.java
##########
@@ -0,0 +1,58 @@
+// 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 com.cloud.agent.api.to;
+
+import com.cloud.agent.api.LogLevel;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class DeployAsIsInfoTO {
Review comment:
javadoc, what on earth is an `DeployAsIsInfoTO`?
##########
File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java
##########
@@ -1498,6 +1610,16 @@ public PrimaryDataStoreInfo cancelPrimaryStorageForMaintenance(CancelPrimaryStor
DataStoreProvider provider = _dataStoreProviderMgr.getDataStoreProvider(primaryStorage.getStorageProviderName());
DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle();
DataStore store = _dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary);
+ if (primaryStorage.getPoolType() == StoragePoolType.DatastoreCluster) {
+ primaryStorage.setStatus(StoragePoolStatus.Up);
+ _storagePoolDao.update(primaryStorage.getId(), primaryStorage);
+ //FR41 need to handle when one of the primary stores is unable to cancel the maintenance mode
+ List<StoragePoolVO> childDatastores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(primaryStorageId);
+ for (StoragePoolVO childDatastore : childDatastores) {
+ DataStore childStore = _dataStoreMgr.getDataStore(childDatastore.getId(), DataStoreRole.Primary);
+ lifeCycle.cancelMaintain(childStore);
+ }
+ }
Review comment:
separate method
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -747,87 +751,168 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
@Override
@DB
public void allocate(final VirtualMachineProfile vm, final LinkedHashMap<? extends Network, List<? extends NicProfile>> networks, final Map<String, Map<Integer, String>> extraDhcpOptions) throws InsufficientCapacityException,
Review comment:
fromat request
```suggestion
public void allocate(final VirtualMachineProfile vm, final LinkedHashMap<? extends Network, List<? extends NicProfile>> networks, final Map<String, Map<Integer, String>> extraDhcpOptions)
throws InsufficientCapacityException, ConcurrentOperationException {
```
but of course delete the next line (github sugestions are almost smart enough for this.
##########
File path: core/src/test/java/com/cloud/agent/api/storage/DownloadAnswerTest.java
##########
@@ -0,0 +1,62 @@
+// 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 com.cloud.agent.api.storage;
Review comment:
new class should be in `org.apache.cloudstack`
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -747,87 +751,168 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
@Override
@DB
public void allocate(final VirtualMachineProfile vm, final LinkedHashMap<? extends Network, List<? extends NicProfile>> networks, final Map<String, Map<Integer, String>> extraDhcpOptions) throws InsufficientCapacityException,
- ConcurrentOperationException {
+ ConcurrentOperationException {
Review comment:
```suggestion
```
but only in combination with above
##########
File path: engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateDataFactory.java
##########
@@ -27,7 +27,7 @@
TemplateInfo getReadyTemplateOnImageStore(long templateId, Long zoneId);
- TemplateInfo getTemplate(DataObject obj, DataStore store);
+ TemplateInfo getTemplate(DataObject obj, DataStore store, String configuration);
Review comment:
i would like a javadoc about what this string entails
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -592,6 +597,13 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti
//remove the overcommit details from the uservm details
userVmDetailsDao.removeDetails(vm.getId());
+ // Remove details if VM deploy as-is
Review comment:
this comment could be the method-name of a member being called here.
`long templateId` should be scope protected to avoid confusion in this 114! line method.
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/TemplateDeployAsIsInformationTO.java
##########
@@ -0,0 +1,24 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
Review comment:
new class should be in `org.apache.cloudstack`
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1105,7 +1190,7 @@ boolean isNetworkImplemented(final NetworkVO network) {
@Override
@DB
public Pair<NetworkGuru, NetworkVO> implementNetwork(final long networkId, final DeployDestination dest, final ReservationContext context) throws ConcurrentOperationException,
- ResourceUnavailableException, InsufficientCapacityException {
+ ResourceUnavailableException, InsufficientCapacityException {
Review comment:
with throws?
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -3056,7 +3141,7 @@ public boolean startNetwork(final long networkId, final DeployDestination dest,
@Override
public boolean restartNetwork(final Long networkId, final Account callerAccount, final User callerUser, final boolean cleanup) throws ConcurrentOperationException, ResourceUnavailableException,
- InsufficientCapacityException {
+ InsufficientCapacityException {
Review comment:
and this one (with `throws`)
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -296,6 +315,34 @@ public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Lo
return null;
}
+ @Override
+ public StoragePool findChildDataStoreInDataStoreCluster(DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, Long datastoreClusterId) {
+ Long podId = null;
+ if (pod != null) {
+ podId = pod.getId();
+ } else if (clusterId != null) {
+ Cluster cluster = _entityMgr.findById(Cluster.class, clusterId);
+ if (cluster != null) {
+ podId = cluster.getPodId();
+ }
+ }
+ List<StoragePoolVO> childDatastores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(datastoreClusterId);
+ List<StoragePool> suitablePools = new ArrayList<StoragePool>();
+
+ for (StoragePoolVO childDatastore: childDatastores)
+ suitablePools.add((StoragePool)dataStoreMgr.getDataStore(childDatastore.getId(), DataStoreRole.Primary));
+
+ VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm);
+ for (StoragePoolAllocator allocator : _storagePoolAllocators) {
+ DataCenterDeployment plan = new DataCenterDeployment(dc.getId(), podId, clusterId, hostId, null, null);
+ final List<StoragePool> poolList = allocator.reorderPools(suitablePools, profile, plan);
+
+ if (poolList != null && !poolList.isEmpty()) {
+ return (StoragePool)dataStoreMgr.getDataStore(poolList.get(0).getId(), DataStoreRole.Primary);
+ }
+ }
+ return null;
+ }
Review comment:
newline:
```suggestion
}
```
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -747,87 +751,168 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
@Override
@DB
public void allocate(final VirtualMachineProfile vm, final LinkedHashMap<? extends Network, List<? extends NicProfile>> networks, final Map<String, Map<Integer, String>> extraDhcpOptions) throws InsufficientCapacityException,
- ConcurrentOperationException {
+ ConcurrentOperationException {
Transaction.execute(new TransactionCallbackWithExceptionNoReturn<InsufficientCapacityException>() {
@Override
public void doInTransactionWithoutResult(final TransactionStatus status) throws InsufficientCapacityException {
- int deviceId = 0;
- int size = 0;
- for (final Network ntwk : networks.keySet()) {
- final List<? extends NicProfile> profiles = networks.get(ntwk);
- if (profiles != null && !profiles.isEmpty()) {
- size = size + profiles.size();
- } else {
- size = size + 1;
- }
+ if (s_logger.isTraceEnabled()) {
+ s_logger.trace(String.format("allocating networks for %s(template %s); %d networks",vm.getInstanceName(), vm.getTemplate().getUuid(), networks.size()));
}
+ int deviceId = 0;
+ int size;
+ size = determineNumberOfNicsRequired();
final boolean[] deviceIds = new boolean[size];
Arrays.fill(deviceIds, false);
+ List<Pair<Network, NicProfile>> profilesList = getOrderedNetworkNicProfileMapping(networks);
final List<NicProfile> nics = new ArrayList<NicProfile>(size);
NicProfile defaultNic = null;
+ Network nextNetwork = null;
+ for (Pair <Network, NicProfile> networkNicPair : profilesList) {
+ nextNetwork = networkNicPair.first();
+ Pair<NicProfile, Integer> newDeviceInfo = addRequestedNicToNicListWithDeviceNumberAndRetrieveDefaultDevice(networkNicPair.second(), deviceIds, deviceId, nextNetwork, nics, defaultNic);
+ defaultNic = newDeviceInfo.first();
+ deviceId = newDeviceInfo.second();
+ }
+ createExtraNics(size, nics, nextNetwork);
+
+ if (nics.size() == 1) {
+ nics.get(0).setDefaultNic(true);
+ }
+ }
+
+ /**
+ * private transaction method to check and add devices to the nic list and update the info
+ */
+ Pair<NicProfile,Integer> addRequestedNicToNicListWithDeviceNumberAndRetrieveDefaultDevice(NicProfile requested, boolean[] deviceIds, int deviceId, Network nextNetwork, List<NicProfile> nics, NicProfile defaultNic)
+ throws InsufficientAddressCapacityException, InsufficientVirtualNetworkCapacityException {
Review comment:
👍 this is the kind of formatting i'd like to see for all methods (i.e. throws on a new line if the signature is (too) long
##########
File path: engine/schema/src/main/java/com/cloud/dc/dao/VsphereStoragePolicyDao.java
##########
@@ -14,18 +14,17 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
+package com.cloud.dc.dao;
Review comment:
new classes should be in org.apache.cloudstack
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -747,87 +751,168 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
@Override
@DB
public void allocate(final VirtualMachineProfile vm, final LinkedHashMap<? extends Network, List<? extends NicProfile>> networks, final Map<String, Map<Integer, String>> extraDhcpOptions) throws InsufficientCapacityException,
- ConcurrentOperationException {
+ ConcurrentOperationException {
Transaction.execute(new TransactionCallbackWithExceptionNoReturn<InsufficientCapacityException>() {
@Override
public void doInTransactionWithoutResult(final TransactionStatus status) throws InsufficientCapacityException {
- int deviceId = 0;
- int size = 0;
- for (final Network ntwk : networks.keySet()) {
- final List<? extends NicProfile> profiles = networks.get(ntwk);
- if (profiles != null && !profiles.isEmpty()) {
- size = size + profiles.size();
- } else {
- size = size + 1;
- }
+ if (s_logger.isTraceEnabled()) {
+ s_logger.trace(String.format("allocating networks for %s(template %s); %d networks",vm.getInstanceName(), vm.getTemplate().getUuid(), networks.size()));
}
+ int deviceId = 0;
+ int size;
+ size = determineNumberOfNicsRequired();
final boolean[] deviceIds = new boolean[size];
Arrays.fill(deviceIds, false);
+ List<Pair<Network, NicProfile>> profilesList = getOrderedNetworkNicProfileMapping(networks);
final List<NicProfile> nics = new ArrayList<NicProfile>(size);
NicProfile defaultNic = null;
+ Network nextNetwork = null;
+ for (Pair <Network, NicProfile> networkNicPair : profilesList) {
+ nextNetwork = networkNicPair.first();
+ Pair<NicProfile, Integer> newDeviceInfo = addRequestedNicToNicListWithDeviceNumberAndRetrieveDefaultDevice(networkNicPair.second(), deviceIds, deviceId, nextNetwork, nics, defaultNic);
+ defaultNic = newDeviceInfo.first();
+ deviceId = newDeviceInfo.second();
+ }
+ createExtraNics(size, nics, nextNetwork);
+
+ if (nics.size() == 1) {
+ nics.get(0).setDefaultNic(true);
+ }
+ }
+
+ /**
+ * private transaction method to check and add devices to the nic list and update the info
+ */
+ Pair<NicProfile,Integer> addRequestedNicToNicListWithDeviceNumberAndRetrieveDefaultDevice(NicProfile requested, boolean[] deviceIds, int deviceId, Network nextNetwork, List<NicProfile> nics, NicProfile defaultNic)
+ throws InsufficientAddressCapacityException, InsufficientVirtualNetworkCapacityException {
+ Pair<NicProfile, Integer> rc = new Pair<>(null,null);
+ Boolean isDefaultNic = false;
+ if (vm != null && requested != null && requested.isDefaultNic()) {
+ isDefaultNic = true;
+ }
+
+ while (deviceIds[deviceId] && deviceId < deviceIds.length) {
+ deviceId++;
+ }
+
+ final Pair<NicProfile, Integer> vmNicPair = allocateNic(requested, nextNetwork, isDefaultNic, deviceId, vm);
+ NicProfile vmNic = null;
+ if (vmNicPair != null) {
+ vmNic = vmNicPair.first();
+ if (vmNic == null) {
+ return rc;
+ }
+ deviceId = vmNicPair.second();
+ }
+
+ final int devId = vmNic.getDeviceId();
+ if (devId >= deviceIds.length) {
+ throw new IllegalArgumentException("Device id for nic is too large: " + vmNic);
+ }
+ if (deviceIds[devId]) {
+ throw new IllegalArgumentException("Conflicting device id for two different nics: " + vmNic);
+ }
+
+ deviceIds[devId] = true;
+ if (vmNic.isDefaultNic()) {
+ if (defaultNic != null) {
+ throw new IllegalArgumentException("You cannot specify two nics as default nics: nic 1 = " + defaultNic + "; nic 2 = " + vmNic);
+ }
+ defaultNic = vmNic;
+ }
+
+ nics.add(vmNic);
+ vm.addNic(vmNic);
+ saveExtraDhcpOptions(nextNetwork.getUuid(), vmNic.getId(), extraDhcpOptions);
+ rc.first(defaultNic);
+ rc.second(deviceId);
+ return rc;
+ }
+
+ /**
+ * private transaction method to get oredered list of Network and NicProfile pair
+ * @return ordered list of Network and NicProfile pair
+ * @param networks the map od networks to nic profiles list
+ */
+ private List<Pair<Network, NicProfile>> getOrderedNetworkNicProfileMapping(final LinkedHashMap<? extends Network, List<? extends NicProfile>> networks) {
+ List<Pair<Network, NicProfile>> profilesList = new ArrayList<>();
for (final Map.Entry<? extends Network, List<? extends NicProfile>> network : networks.entrySet()) {
- final Network config = network.getKey();
List<? extends NicProfile> requestedProfiles = network.getValue();
if (requestedProfiles == null) {
requestedProfiles = new ArrayList<NicProfile>();
}
if (requestedProfiles.isEmpty()) {
requestedProfiles.add(null);
}
-
for (final NicProfile requested : requestedProfiles) {
- Boolean isDefaultNic = false;
- if (vm != null && requested != null && requested.isDefaultNic()) {
- isDefaultNic = true;
- }
-
- while (deviceIds[deviceId] && deviceId < deviceIds.length) {
- deviceId++;
- }
-
- final Pair<NicProfile, Integer> vmNicPair = allocateNic(requested, config, isDefaultNic, deviceId, vm);
- NicProfile vmNic = null;
- if (vmNicPair != null) {
- vmNic = vmNicPair.first();
- if (vmNic == null) {
- continue;
- }
- deviceId = vmNicPair.second();
- }
-
- final int devId = vmNic.getDeviceId();
- if (devId >= deviceIds.length) {
- throw new IllegalArgumentException("Device id for nic is too large: " + vmNic);
- }
- if (deviceIds[devId]) {
- throw new IllegalArgumentException("Conflicting device id for two different nics: " + vmNic);
+ profilesList.add(new Pair<Network, NicProfile>(network.getKey(), requested));
+ }
+ }
+ profilesList.sort(new Comparator<Pair<Network, NicProfile>>() {
+ @Override
+ public int compare(Pair<Network, NicProfile> pair1, Pair<Network, NicProfile> pair2) {
+ int profile1Order = Integer.MAX_VALUE;
+ int profile2Order = Integer.MAX_VALUE;
+ if (pair1 != null && pair1.second() != null && pair1.second().getOrderIndex() != null) {
+ profile1Order = pair1.second().getOrderIndex();
}
-
- deviceIds[devId] = true;
-
- if (vmNic.isDefaultNic()) {
- if (defaultNic != null) {
- throw new IllegalArgumentException("You cannot specify two nics as default nics: nic 1 = " + defaultNic + "; nic 2 = " + vmNic);
- }
- defaultNic = vmNic;
+ if (pair2 != null && pair2.second() != null && pair2.second().getOrderIndex() != null) {
+ profile2Order = pair2.second().getOrderIndex();
}
+ return profile1Order - profile2Order;
+ }
+ });
+ return profilesList;
+ }
- nics.add(vmNic);
- vm.addNic(vmNic);
- saveExtraDhcpOptions(config.getUuid(), vmNic.getId(), extraDhcpOptions);
+ /**
+ * private transaction method to run over the objects and determine nic requirements
+ * @return the total numer of nics required
+ */
+ private int determineNumberOfNicsRequired() {
+ int size = 0;
+ for (final Network ntwk : networks.keySet()) {
+ final List<? extends NicProfile> profiles = networks.get(ntwk);
+ if (profiles != null && !profiles.isEmpty()) {
+ size = size + profiles.size();
+ } else {
+ size = size + 1;
}
}
+
+ List<OVFNetworkTO> netprereqs = templateDeployAsIsDetailsDao.listNetworkRequirementsByTemplateId(vm.getTemplate().getId());
+ if (size < netprereqs.size()) {
+ size = netprereqs.size();
+ }
+ return size;
+ }
+
+ /**
+ * private transaction method to add nics as required
+ * @param size the number needed
+ * @param nics the list of nics present
+ * @param finalNetwork the network to add the nics to
+ * @throws InsufficientVirtualNetworkCapacityException great
+ * @throws InsufficientAddressCapacityException also magnificent, as the name sugests
+ */
+ private void createExtraNics(int size, List<NicProfile> nics, Network finalNetwork) throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException {
if (nics.size() != size) {
s_logger.warn("Number of nics " + nics.size() + " doesn't match number of requested nics " + size);
- throw new CloudRuntimeException("Number of nics " + nics.size() + " doesn't match number of requested networks " + size);
- }
-
- if (nics.size() == 1) {
- nics.get(0).setDefaultNic(true);
+ if (nics.size() > size) {
+ throw new CloudRuntimeException("Number of nics " + nics.size() + " doesn't match number of requested networks " + size);
+ } else {
+ if (finalNetwork == null) {
+ throw new CloudRuntimeException(String.format("can not assign network to %d remaining required NICs", size - nics.size()));
+ }
+ // create extra
Review comment:
method name?
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -3033,7 +3118,7 @@ public void reallyRun() {
@Override
public boolean startNetwork(final long networkId, final DeployDestination dest, final ReservationContext context) throws ConcurrentOperationException, ResourceUnavailableException,
- InsufficientCapacityException {
+ InsufficientCapacityException {
Review comment:
`throws`
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -1704,4 +1801,4 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
}
});
}
-}
+}
Review comment:
```suggestion
}
```
##########
File path: engine/schema/src/main/java/com/cloud/dc/dao/VsphereStoragePolicyDaoImpl.java
##########
@@ -0,0 +1,64 @@
+// 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 com.cloud.dc.dao;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/schema/src/main/java/com/cloud/dc/VsphereStoragePolicyVO.java
##########
@@ -0,0 +1,126 @@
+// 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 com.cloud.dc;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/schema/src/main/java/com/cloud/deployasis/dao/TemplateDeployAsIsDetailsDao.java
##########
@@ -0,0 +1,32 @@
+// 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 com.cloud.deployasis.dao;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/schema/src/main/java/com/cloud/deployasis/dao/TemplateDeployAsIsDetailsDaoImpl.java
##########
@@ -0,0 +1,83 @@
+// 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 com.cloud.deployasis.dao;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/schema/src/main/java/com/cloud/storage/dao/VMTemplateDetailsDaoImpl.java
##########
@@ -30,4 +30,4 @@
public void addDetail(long resourceId, String key, String value, boolean display) {
super.addDetail(new VMTemplateDetailVO(resourceId, key, value, display));
}
-}
+}
Review comment:
```suggestion
}
```
##########
File path: engine/schema/src/main/java/com/cloud/deployasis/UserVmDeployAsIsDetailVO.java
##########
@@ -0,0 +1,94 @@
+// 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 com.cloud.deployasis;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/schema/src/main/java/com/cloud/deployasis/TemplateDeployAsIsDetailVO.java
##########
@@ -0,0 +1,94 @@
+// 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 com.cloud.deployasis;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java
##########
@@ -1280,4 +1290,4 @@ protected Void createDatadiskTemplateCallback(AsyncCallbackDispatcher<TemplateSe
future.complete(dataDiskTemplateResult);
return null;
}
-}
+}
Review comment:
```suggestion
}
```
##########
File path: engine/schema/src/main/java/com/cloud/deployasis/dao/UserVmDeployAsIsDetailsDao.java
##########
@@ -0,0 +1,24 @@
+// 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 com.cloud.deployasis.dao;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/schema/src/main/java/com/cloud/deployasis/dao/UserVmDeployAsIsDetailsDaoImpl.java
##########
@@ -0,0 +1,30 @@
+// 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 com.cloud.deployasis.dao;
Review comment:
new classes should be in `org.apache.cloudstack`
##########
File path: engine/storage/src/main/java/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java
##########
@@ -178,6 +185,13 @@ public DataStore attachZone(DataStore store, HypervisorType hypervisor) {
pool.setHypervisor(hypervisor);
pool.setStatus(StoragePoolStatus.Up);
this.dataStoreDao.update(pool.getId(), pool);
+ if(pool.getPoolType() == StoragePoolType.DatastoreCluster && pool.getParent() == 0) {
+ List<StoragePoolVO> childDatastores = dataStoreDao.listChildStoragePoolsInDatastoreCluster(pool.getId());
+ for (StoragePoolVO child : childDatastores) {
+ child.setScope(ScopeType.ZONE);
+ this.dataStoreDao.update(child.getId(), child);
+ }
+ }
Review comment:
call of `doSomeChildClusterStuff(ScopeType.ZONE, ...)`
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -2125,4 +2132,4 @@ public void unmanageVolume(long volumeId) {
volDao.remove(vol.getId());
}
}
-}
+}
Review comment:
```suggestion
}
```
##########
File path: engine/storage/src/main/java/org/apache/cloudstack/storage/image/deployasis/DeployAsIsHelper.java
##########
@@ -0,0 +1,34 @@
+// 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.storage.image.deployasis;
+
+import com.cloud.agent.api.storage.DownloadAnswer;
+import com.cloud.agent.api.to.NicTO;
+import com.cloud.vm.VirtualMachineProfile;
+
+import java.util.Map;
+
+public interface DeployAsIsHelper {
+
+ void persistTemplateDeployAsIsDetails(long templateId, DownloadAnswer answer);
+ Map<String, String> getVirtualMachineDeployAsIsProperties(VirtualMachineProfile vmId);
+
+ String getAllocatedVirtualMachineTemplatePath(VirtualMachineProfile vm, String configuration, String destStoragePool);
+ String getAllocatedVirtualMachineDestinationStoragePool(VirtualMachineProfile vm);
+
+ Map<Integer, String> getAllocatedVirtualMachineNicsAdapterMapping(VirtualMachineProfile vm, NicTO[] nics);
+}
Review comment:
can we have some javadoc here? these are helper functions for use in... (storage drivers/network orchestrators/Virtualmachien orchestrators) ?
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualStorageObjectManagerMO.java
##########
@@ -0,0 +1,94 @@
+// 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 com.cloud.hypervisor.vmware.mo;
Review comment:
new classes should go in `org.apache.cloudstack`
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VcenterSessionHandler.java
##########
@@ -0,0 +1,88 @@
+// 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 com.cloud.hypervisor.vmware.util;
Review comment:
new classes should go in `org.apache.cloudstack`
##########
File path: engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java
##########
@@ -413,25 +383,16 @@ private Answer sendToLeastBusyEndpoint(List<EndPoint> eps, CopyCommand cmd) {
return answer;
} catch (AgentUnavailableException e) {
errMsg = e.toString();
- s_logger.debug("Failed to send command, due to Agent:" + endPoint.getId() + ", " + e.toString());
+ LOGGER.debug("Failed to send command, due to Agent:" + endPoint.getId() + ", " + e.toString());
} catch (OperationTimedoutException e) {
errMsg = e.toString();
- s_logger.debug("Failed to send command, due to Agent:" + endPoint.getId() + ", " + e.toString());
+ LOGGER.debug("Failed to send command, due to Agent:" + endPoint.getId() + ", " + e.toString());
}
throw new CloudRuntimeException("Failed to send command, due to Agent:" + endPoint.getId() + ", " + errMsg);
}
@Override
public boolean canCopy(DataObject srcData, DataObject destData) {
- DataStore srcStore = srcData.getDataStore();
- DataStore destStore = destData.getDataStore();
- if ((srcData.getDataStore().getTO() instanceof NfsTO && destData.getDataStore().getTO() instanceof NfsTO) &&
- (srcStore.getRole() == DataStoreRole.Image && destStore.getRole() == DataStoreRole.Image) &&
- ((srcData.getType() == DataObjectType.TEMPLATE && destData.getType() == DataObjectType.TEMPLATE) ||
- (srcData.getType() == DataObjectType.SNAPSHOT && destData.getType() == DataObjectType.SNAPSHOT) ||
- (srcData.getType() == DataObjectType.VOLUME && destData.getType() == DataObjectType.VOLUME))) {
- return true;
- }
Review comment:
kind of surprising that the BaseImageStoreDriverImpl would be able to copy anything anymore in a PR that is Vmware specific... Why is this?
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1738,33 +1775,47 @@ public static String removeOVFNetwork(final String ovfString) {
return ovfString;
}
+ /**
+ * deploys a new VM from a ovf spec. It ignores network, defaults locale to 'US'
+ * @throws Exception shoud be a VmwareResourceException
Review comment:
```suggestion
* @throws CloudRuntimeException should be a VmwareResourceException
* @throws IOException if ovfFilePath not valid?
```
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1738,33 +1775,47 @@ public static String removeOVFNetwork(final String ovfString) {
return ovfString;
}
+ /**
Review comment:
👍
##########
File path: engine/storage/src/main/java/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java
##########
@@ -161,6 +161,13 @@ public DataStore attachCluster(DataStore store) {
pool.setScope(ScopeType.CLUSTER);
pool.setStatus(StoragePoolStatus.Up);
this.dataStoreDao.update(pool.getId(), pool);
+ if(pool.getPoolType() == StoragePoolType.DatastoreCluster && pool.getParent() == 0) {
+ List<StoragePoolVO> childDatastores = dataStoreDao.listChildStoragePoolsInDatastoreCluster(pool.getId());
+ for (StoragePoolVO child : childDatastores) {
+ child.setScope(ScopeType.CLUSTER);
+ this.dataStoreDao.update(child.getId(), child);
+ }
+ }
Review comment:
new method `doSomeChildClusterStuff(ScopeType scopeType, ...)`
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/PbmProfileManagerMO.java
##########
@@ -0,0 +1,94 @@
+// 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 com.cloud.hypervisor.vmware.mo;
Review comment:
new classes should go in `org.apache.cloudstack`
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/StoragepodMO.java
##########
@@ -0,0 +1,48 @@
+// 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 com.cloud.hypervisor.vmware.mo;
Review comment:
new classes should go in `org.apache.cloudstack`
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1773,22 +1824,55 @@ public static void importVmFromOVF(VmwareHypervisorHost host, String ovfFilePath
}
}
- DatacenterMO dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
- ManagedObjectReference morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), dcMo.getVmFolder(), morHost);
+ DatacenterMO dcMo = null;
+ try {
+ dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
+ } catch (Exception e) {
+ throw new CloudRuntimeException(String.format("no datacenter for host '%s' available in context", context.getServerAddress()), e);
+ }
Review comment:
factor in new method
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1738,33 +1775,47 @@ public static String removeOVFNetwork(final String ovfString) {
return ovfString;
}
+ /**
+ * deploys a new VM from a ovf spec. It ignores network, defaults locale to 'US'
+ * @throws Exception shoud be a VmwareResourceException
+ */
public static void importVmFromOVF(VmwareHypervisorHost host, String ovfFilePath, String vmName, DatastoreMO dsMo, String diskOption, ManagedObjectReference morRp,
- ManagedObjectReference morHost) throws Exception {
+ ManagedObjectReference morHost, String configurationId) throws CloudRuntimeException, IOException {
assert (morRp != null);
OvfCreateImportSpecParams importSpecParams = new OvfCreateImportSpecParams();
importSpecParams.setHostSystem(morHost);
importSpecParams.setLocale("US");
importSpecParams.setEntityName(vmName);
- importSpecParams.setDeploymentOption("");
+ String deploymentOption = StringUtils.isNotBlank(configurationId) ? configurationId : "";
+ importSpecParams.setDeploymentOption(deploymentOption);
importSpecParams.setDiskProvisioning(diskOption); // diskOption: thin, thick, etc
String ovfDescriptor = removeOVFNetwork(HttpNfcLeaseMO.readOvfContent(ovfFilePath));
VmwareContext context = host.getContext();
- OvfCreateImportSpecResult ovfImportResult =
- context.getService().createImportSpec(context.getServiceContent().getOvfManager(), ovfDescriptor, morRp, dsMo.getMor(), importSpecParams);
-
+ OvfCreateImportSpecResult ovfImportResult = null;
+ try {
+ ovfImportResult = context.getService().createImportSpec(context.getServiceContent().getOvfManager(), ovfDescriptor, morRp, dsMo.getMor(), importSpecParams);
+ } catch (ConcurrentAccessFaultMsg
+ | FileFaultFaultMsg
+ | InvalidDatastoreFaultMsg
+ | InvalidStateFaultMsg
+ | RuntimeFaultFaultMsg
+ | TaskInProgressFaultMsg
+ | VmConfigFaultFaultMsg error) {
+ throw new CloudRuntimeException("ImportSpec creation failed", error);
+ }
Review comment:
factorred out in new method?
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1773,22 +1824,55 @@ public static void importVmFromOVF(VmwareHypervisorHost host, String ovfFilePath
}
}
- DatacenterMO dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
- ManagedObjectReference morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), dcMo.getVmFolder(), morHost);
+ DatacenterMO dcMo = null;
+ try {
+ dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
+ } catch (Exception e) {
+ throw new CloudRuntimeException(String.format("no datacenter for host '%s' available in context", context.getServerAddress()), e);
+ }
+ ManagedObjectReference folderMO = null;
+ try {
+ folderMO = dcMo.getVmFolder();
+ } catch (Exception e) {
+ throw new CloudRuntimeException("no management handle for VmFolder", e);
+ }
+ ManagedObjectReference morLease = null;
+ try {
+ morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), folderMO, morHost);
+ } catch (DuplicateNameFaultMsg
+ | FileFaultFaultMsg
+ | InsufficientResourcesFaultFaultMsg
+ | InvalidDatastoreFaultMsg
+ | InvalidNameFaultMsg
+ | OutOfBoundsFaultMsg
+ | RuntimeFaultFaultMsg
+ | VmConfigFaultFaultMsg fault) {
+ throw new CloudRuntimeException("import vApp failed",fault);
+ }
Review comment:
new method
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/PbmPlacementSolverMO.java
##########
@@ -0,0 +1,68 @@
+// 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 com.cloud.hypervisor.vmware.mo;
Review comment:
new classes should go in `org.apache.cloudstack`
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1704,6 +1736,11 @@ public static String resolveHostNameInUrl(DatacenterMO dcMo, String url) {
return url;
}
+ /**
Review comment:
👍
##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
##########
@@ -1720,13 +1801,19 @@ public Answer createTemplateFromSnapshot(CopyCommand cmd) {
String disks[] = vmMo.getCurrentSnapshotDiskChainDatastorePaths(diskDevice);
if (clonedWorkerVMNeeded) {
// 4 MB is the minimum requirement for VM memory in VMware
Review comment:
is this comment still valid? not by hte looks of the code below.
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1818,31 +1903,44 @@ public void action(Long param) {
String erroMsg = "File upload task failed to complete due to: " + e.getMessage();
s_logger.error(erroMsg);
importSuccess = false; // Set flag to cleanup the stale template left due to failed import operation, if any
- throw new Exception(erroMsg, e);
+ throw new CloudRuntimeException(erroMsg, e);
} catch (Throwable th) {
String errorMsg = "throwable caught during file upload task: " + th.getMessage();
s_logger.error(errorMsg);
importSuccess = false; // Set flag to cleanup the stale template left due to failed import operation, if any
- throw new Exception(errorMsg, th);
+ throw new CloudRuntimeException(errorMsg, th);
} finally {
progressReporter.close();
}
if (bytesAlreadyWritten == totalBytes) {
- leaseMo.updateLeaseProgress(100);
+ try {
+ leaseMo.updateLeaseProgress(100);
+ } catch (Exception e) {
+ throw new CloudRuntimeException("error while waiting for lease update", e);
+ }
}
} else if (state == HttpNfcLeaseState.ERROR) {
- LocalizedMethodFault error = leaseMo.getLeaseError();
+ LocalizedMethodFault error = null;
+ try {
+ error = leaseMo.getLeaseError();
+ } catch (Exception e) {
+ throw new CloudRuntimeException("error getting lease error", e);
+ }
MethodFault fault = error.getFault();
String erroMsg = "Object creation on vCenter failed due to: Exception: " + fault.getClass().getName() + ", message: " + error.getLocalizedMessage();
s_logger.error(erroMsg);
- throw new Exception(erroMsg);
+ throw new CloudRuntimeException(erroMsg);
}
} finally {
- if (!importSuccess) {
- s_logger.error("Aborting the lease on " + vmName + " after import operation failed.");
- leaseMo.abortLease();
- } else {
- leaseMo.completeLease();
+ try {
+ if (!importSuccess) {
+ s_logger.error("Aborting the lease on " + vmName + " after import operation failed.");
+ leaseMo.abortLease();
+ } else {
+ leaseMo.completeLease();
+ }
Review comment:
cleanup/finalize method
##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
##########
@@ -891,6 +886,92 @@ public Answer cloneVolumeFromBaseTemplate(CopyCommand cmd) {
}
}
+ private String cloneVMwithVMname(VmwareContext context, VmwareHypervisorHost hyperHost, TemplateObjectTO template,
+ VirtualMachineMO vmTemplate, VolumeObjectTO volume, DatacenterMO dcMo, DatastoreMO dsMo) throws Exception {
+ ManagedObjectReference morDatastore = dsMo.getMor();
+ ManagedObjectReference morPool = hyperHost.getHyperHostOwnerResourcePool();
+ ManagedObjectReference morCluster = hyperHost.getHyperHostCluster();
+ if (template.getSize() != null) {
+ _fullCloneFlag = volume.getSize() > template.getSize() ? true : _fullCloneFlag;
+ }
+ if (!_fullCloneFlag) {
+ createVMLinkedClone(vmTemplate, dcMo, volume.getVmName(), morDatastore, morPool);
+ } else {
+ createVMFullClone(vmTemplate, dcMo, dsMo, volume.getVmName(), morDatastore, morPool);
+ }
+
+ VirtualMachineMO vmMo = new ClusterMO(context, morCluster).findVmOnHyperHost(volume.getVmName());
+ assert (vmMo != null);
+
+ return vmMo.getVmdkFileBaseNames().get(0);
+ }
+
+ private String createVMFolderWithVMName(VmwareContext context, VmwareHypervisorHost hyperHost, TemplateObjectTO template,
Review comment:
long method, can you split it?
##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
##########
@@ -1773,22 +1824,55 @@ public static void importVmFromOVF(VmwareHypervisorHost host, String ovfFilePath
}
}
- DatacenterMO dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
- ManagedObjectReference morLease = context.getService().importVApp(morRp, ovfImportResult.getImportSpec(), dcMo.getVmFolder(), morHost);
+ DatacenterMO dcMo = null;
+ try {
+ dcMo = new DatacenterMO(context, host.getHyperHostDatacenter());
+ } catch (Exception e) {
+ throw new CloudRuntimeException(String.format("no datacenter for host '%s' available in context", context.getServerAddress()), e);
+ }
+ ManagedObjectReference folderMO = null;
+ try {
+ folderMO = dcMo.getVmFolder();
+ } catch (Exception e) {
+ throw new CloudRuntimeException("no management handle for VmFolder", e);
+ }
Review comment:
factor in new method
##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -745,20 +752,38 @@ private Answer execute(ResizeVolumeCommand cmd) {
} else if (newSize == oldSize) {
return new ResizeVolumeAnswer(cmd, true, "success", newSize * ResourceType.bytesToKiB);
}
+ /*
+ // FR41 this is yet to fix
Review comment:
internal project code in open source version. please remove/reformulate this comment.
##########
File path: api/src/main/java/com/cloud/agent/api/to/deployasis/OVFVirtualHardwareSectionTO.java
##########
@@ -0,0 +1,50 @@
+//
+// 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 com.cloud.agent.api.to.deployasis;
+
+import java.util.List;
+
+public class OVFVirtualHardwareSectionTO implements TemplateDeployAsIsInformationTO {
Review comment:
though it has contents that might need explanation to the new...
##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
##########
@@ -1134,7 +1219,7 @@ private void postCreatePrivateTemplate(String installFullPath, long templateId,
Pair<VirtualMachineMO, String[]> cloneResult =
vmMo.cloneFromCurrentSnapshot(workerVmName, 0, 4, volumeDeviceInfo.second(), VmwareHelper.getDiskDeviceDatastore(volumeDeviceInfo.first()));
clonedVm = cloneResult.first();
-
+ * */
Review comment:
code in comment, please remove.
##########
File path: plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/api/command/admin/zone/ImportVsphereStoragePoliciesCmd.java
##########
@@ -0,0 +1,111 @@
+// 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.admin.zone;
+
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.VsphereStoragePolicy;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.hypervisor.vmware.VmwareDatacenterService;
+
+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.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.VsphereStoragePoliciesResponse;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.api.response.ZoneResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import javax.inject.Inject;
+import java.util.ArrayList;
+import java.util.List;
+
+@APICommand(name = ImportVsphereStoragePoliciesCmd.APINAME, description = "Import vSphere storage policies",
+ responseObject = VsphereStoragePoliciesResponse.class,
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+ authorized = {RoleType.Admin})
Review comment:
```suggestion
@APICommand(name = ImportVsphereStoragePoliciesCmd.APINAME, description = "Import vSphere storage policies",
responseObject = VsphereStoragePoliciesResponse.class,
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
since = "4.15", authorized = {RoleType.Admin})
```
##########
File path: utils/src/test/java/com/cloud/utils/compression/CompressionUtilTest.java
##########
@@ -0,0 +1,128 @@
+//
+// 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 com.cloud.utils.compression;
Review comment:
new classes should go in `org.apache.cloudstack`
##########
File path: utils/src/main/java/com/cloud/utils/compression/CompressionUtil.java
##########
@@ -0,0 +1,58 @@
+//
+// 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 com.cloud.utils.compression;
Review comment:
new classes should go in `org.apache.cloudstack`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org