shwstppr commented on code in PR #7712:
URL: https://github.com/apache/cloudstack/pull/7712#discussion_r1373046987
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/vm/ImportUnmanagedInstanceCmd.java:
##########
@@ -84,9 +84,16 @@ public class ImportUnmanagedInstanceCmd extends BaseAsyncCmd
{
@Parameter(name = ApiConstants.NAME,
type = CommandType.STRING,
required = true,
- description = "the hypervisor name of the instance")
+ description = "the name of the instance as it is known to the
hypervisor")
private String name;
+ @Parameter(name = ApiConstants.SERVICE_OFFERING_ID,
+ type = CommandType.UUID,
+ entityType = ServiceOfferingResponse.class,
+ required = true,
+ description = "the ID of the service offering for the virtual
machine")
+ private Long serviceOfferingId;
+
Review Comment:
Is this refactoring needed?
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -512,11 +519,21 @@ private Pair<UnmanagedInstanceTO.Disk,
List<UnmanagedInstanceTO.Disk>> getRootAn
rootDisk = disk;
} else {
dataDisks.add(disk);
+ DiskOffering diskOffering =
diskOfferingDao.findById(dataDiskOfferingMap.getOrDefault(disk.getDiskId(),
null));
+ if ((disk.getCapacity() == null || disk.getCapacity() <= 0) &&
diskOffering != null) {
Review Comment:
Why are we not getting capacity from the hypervisor?
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -512,11 +519,21 @@ private Pair<UnmanagedInstanceTO.Disk,
List<UnmanagedInstanceTO.Disk>> getRootAn
rootDisk = disk;
} else {
dataDisks.add(disk);
+ DiskOffering diskOffering =
diskOfferingDao.findById(dataDiskOfferingMap.getOrDefault(disk.getDiskId(),
null));
+ if ((disk.getCapacity() == null || disk.getCapacity() <= 0) &&
diskOffering != null) {
+ disk.setCapacity(diskOffering.getDiskSize());
+ }
}
}
- if (diskIdsWithoutOffering.size() > 1) {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
String.format("VM has total %d disks, disk offering mapping not provided for %d
disks. Disk IDs that may need a disk offering - %s", disks.size(),
diskIdsWithoutOffering.size()-1, String.join(", ", diskIdsWithoutOffering)));
+ if (diskIdsWithoutOffering.size() > 1 || rootDisk == null) {
+ throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
String.format("VM has total %d disks, disk offering mapping not provided for %d
disks. Disk IDs that may need a disk offering - %s", disks.size(),
diskIdsWithoutOffering.size() - 1, String.join(", ", diskIdsWithoutOffering)));
Review Comment:
In which case rootDisk is null at this point when we have already checked
`callerDiskIds.size() != disks.size() - 1`?
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -955,14 +978,20 @@ private UserVm importVirtualMachineInternal(final
UnmanagedInstanceTO unmanagedI
}
// Check NICs and supplied networks
Map<String, Network.IpAddresses> nicIpAddressMap =
getNicIpAddresses(unmanagedInstance.getNics(), callerNicIpAddressMap);
- Map<String, Long> allNicNetworkMap =
getUnmanagedNicNetworkMap(unmanagedInstance.getName(),
unmanagedInstance.getNics(), nicNetworkMap, nicIpAddressMap, zone, hostName,
owner);
+ Map<String, Long> allNicNetworkMap =
getUnmanagedNicNetworkMap(unmanagedInstance.getName(),
unmanagedInstance.getNics(), nicNetworkMap, nicIpAddressMap, zone, hostName,
owner, host.getHypervisorType());
if (!CollectionUtils.isEmpty(unmanagedInstance.getNics())) {
allDetails.put(VmDetailConstants.NIC_ADAPTER,
unmanagedInstance.getNics().get(0).getAdapterType());
}
+
+ if (StringUtils.isNotEmpty(unmanagedInstance.getVncPassword())) {
Review Comment:
should there be a hypervisor check?
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1354,6 +1393,6 @@ public String getConfigComponentName() {
@Override
public ConfigKey<?>[] getConfigKeys() {
- return new ConfigKey<?>[] { UnmanageVMPreserveNic };
+ return new ConfigKey<?>[]{UnmanageVMPreserveNic};
Review Comment:
is this needed?
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -574,17 +591,22 @@ private void checkUnmanagedNicAndNetworkForImport(String
instanceName, Unmanaged
if (!autoAssign &&
network.getGuestType().equals(Network.GuestType.Isolated)) {
return;
}
+ checksOnlyNeededForVmware(nic, network, hypervisorType);
+ }
- String networkBroadcastUri = network.getBroadcastUri() == null ? null
: network.getBroadcastUri().toString();
- if (nic.getVlan() != null && nic.getVlan() != 0 && nic.getPvlan() ==
null &&
- (StringUtils.isEmpty(networkBroadcastUri) ||
- !networkBroadcastUri.equals(String.format("vlan://%d",
nic.getVlan())))) {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
String.format("VLAN of network(ID: %s) %s is found different from the VLAN of
nic(ID: %s) vlan://%d during VM import", network.getUuid(),
networkBroadcastUri, nic.getNicId(), nic.getVlan()));
- }
- String pvLanType = nic.getPvlanType() == null ? "" :
nic.getPvlanType().toLowerCase().substring(0, 1);
- if (nic.getVlan() != null && nic.getVlan() != 0 && nic.getPvlan() !=
null && nic.getPvlan() != 0 &&
- (StringUtils.isEmpty(networkBroadcastUri) ||
!String.format("pvlan://%d-%s%d", nic.getVlan(), pvLanType,
nic.getPvlan()).equals(networkBroadcastUri))) {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
String.format("PVLAN of network(ID: %s) %s is found different from the VLAN of
nic(ID: %s) pvlan://%d-%s%d during VM import", network.getUuid(),
networkBroadcastUri, nic.getNicId(), nic.getVlan(), pvLanType, nic.getPvlan()));
+ private void checksOnlyNeededForVmware(UnmanagedInstanceTO.Nic nic,
Network network, final Hypervisor.HypervisorType hypervisorType) {
+ if (hypervisorType == Hypervisor.HypervisorType.VMware) {
Review Comment:
Why not return early if it is not VMware
if (!Hypervisor.HypervisorType.VMware.equals(hypervisorType)) {
return;
//continue with rest
Question - why VLAN checks are not needed for KVM?
##########
server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java:
##########
@@ -278,7 +282,6 @@ public void setUp() throws Exception {
nullable(String.class),
nullable(Hypervisor.HypervisorType.class), nullable(Map.class),
nullable(VirtualMachine.PowerState.class))).thenReturn(userVm);
NetworkVO networkVO = Mockito.mock(NetworkVO.class);
when(networkVO.getGuestType()).thenReturn(Network.GuestType.L2);
-
when(networkVO.getBroadcastUri()).thenReturn(URI.create(String.format("vlan://%d",
instanceNic.getVlan())));
Review Comment:
why? Not used anymore/
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1194,12 +1225,18 @@ public UserVmResponse
importUnmanagedInstance(ImportUnmanagedInstanceCmd cmd) {
if (unmanagedInstance == null) {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
String.format("Unable to retrieve details for unmanaged VM: %s", name));
}
+
+ if (template.getName().equals(VM_IMPORT_DEFAULT_TEMPLATE_NAME)
&& cluster.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) {
Review Comment:
Question - why import with default template is not allowed for KVM?
##########
ui/src/views/compute/wizard/MultiNetworkSelection.vue:
##########
@@ -49,7 +49,8 @@
v-for="network in validNetworks[record.id]"
:key="network.id"
:label="network.displaytext + (network.broadcasturi ? ' (' +
network.broadcasturi + ')' : '')">
- {{ network.displaytext + (network.broadcasturi ? ' (' +
network.broadcasturi + ')' : '') }}
+ <div v-if="this.hypervisor === 'KVM'">{{ network.displaytext +
' - ' + (network.id.slice(0,8)) }}</div>
Review Comment:
no idea about these constants. Would be better to make a new method with
some doc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
