DaanHoogland commented on code in PR #7692:
URL: https://github.com/apache/cloudstack/pull/7692#discussion_r1281850965
##########
engine/schema/src/main/java/com/cloud/storage/GuestOSHypervisorVO.java:
##########
@@ -72,7 +73,9 @@ public String getHypervisorVersion() {
@Override
public String getHypervisorType() {
- return hypervisorType;
+ return hypervisorType.equalsIgnoreCase("Custom") ?
+ HypervisorGuru.HypervisorCustomDisplayName.value() :
+ hypervisorType;
Review Comment:
should be part of `Hypervisor.HypervisorType`
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -1422,6 +1424,16 @@ public StoragePoolResponse
createStoragePoolForMigrationResponse(StoragePool poo
return listPools.get(0);
}
+ /**
+ * Returns the display name of a hypervisor type in case the custom
hypervisor is used,
+ * using the 'hypervisor.custom.display.name' setting. Otherwise, returns
hypervisor name
+ */
+ public static String
getDisplayHypervisorTypeString(Hypervisor.HypervisorType hypervisorType) {
+ return hypervisorType != Hypervisor.HypervisorType.Custom ?
+ hypervisorType.toString() :
+ HypervisorGuru.HypervisorCustomDisplayName.value();
+ }
+
Review Comment:
should be part of `Hypervisor.HypervisorType`
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -4470,7 +4474,9 @@ public List<String> getHypervisors(final Long zoneId) {
} else {
final List<ClusterVO> clustersForZone =
_clusterDao.listByZoneId(zoneId);
for (final ClusterVO cluster : clustersForZone) {
- result.add(cluster.getHypervisorType().toString());
+ result.add(cluster.getHypervisorType() !=
HypervisorType.Custom ?
+ cluster.getHypervisorType().toString() :
+
HypervisorGuru.HypervisorCustomDisplayName.value());
Review Comment:
again
##########
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java:
##########
@@ -561,12 +561,13 @@ private void
prepareAndRunConfigureCustomRootDiskSizeTest(Map<String, String> cu
public void verifyIfHypervisorSupportRootdiskSizeOverrideTest() {
Hypervisor.HypervisorType[] hypervisorTypeArray =
Hypervisor.HypervisorType.values();
int exceptionCounter = 0;
- int expectedExceptionCounter = hypervisorTypeArray.length - 4;
+ int expectedExceptionCounter = hypervisorTypeArray.length - 5;
for(int i = 0; i < hypervisorTypeArray.length; i++) {
if (Hypervisor.HypervisorType.KVM == hypervisorTypeArray[i]
|| Hypervisor.HypervisorType.XenServer ==
hypervisorTypeArray[i]
|| Hypervisor.HypervisorType.VMware ==
hypervisorTypeArray[i]
+ || Hypervisor.HypervisorType.Custom ==
hypervisorTypeArray[i]
|| Hypervisor.HypervisorType.Simulator ==
hypervisorTypeArray[i]) {
Review Comment:
```suggestion
if
(Hypervisor.HypervisorType.isSupported(hypervisorTypeArray[i]) {
```
or somthing like that.
##########
server/src/main/java/com/cloud/api/query/dao/VolumeJoinDaoImpl.java:
##########
@@ -147,8 +148,10 @@ public VolumeResponse newVolumeResponse(ResponseView view,
VolumeJoinVO volume)
volResponse.setSize(volume.getVolumeStoreSize());
volResponse.setCreated(volume.getCreatedOnStore());
- if (view == ResponseView.Full)
-
volResponse.setHypervisor(ApiDBUtils.getHypervisorTypeFromFormat(volume.getDataCenterId(),
volume.getFormat()).toString());
+ if (view == ResponseView.Full) {
+ Hypervisor.HypervisorType hypervisorTypeFromFormat =
ApiDBUtils.getHypervisorTypeFromFormat(volume.getDataCenterId(),
volume.getFormat());
+
volResponse.setHypervisor(ApiResponseHelper.getDisplayHypervisorTypeString(hypervisorTypeFromFormat));
Review Comment:
can be a single call to `Hypervisor.HypervisorType`
##########
server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java:
##########
@@ -125,7 +127,11 @@ public HostResponse newHostResponse(HostJoinVO host,
EnumSet<HostDetails> detail
hostResponse.setCpuNumber(host.getCpus());
hostResponse.setZoneId(host.getZoneUuid());
hostResponse.setDisconnectedOn(host.getDisconnectedOn());
- hostResponse.setHypervisor(host.getHypervisorType());
+ if (host.getHypervisorType() != null) {
+ String hypervisorType = host.getHypervisorType() !=
Hypervisor.HypervisorType.Custom ?
+ host.getHypervisorType().toString() :
HypervisorGuru.HypervisorCustomDisplayName.value();
Review Comment:
this should be a method of `Hypervisor.hypervisorType`
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -4327,7 +4327,8 @@ protected long configureCustomRootDiskSize(Map<String,
String> customParameters,
* @throws InvalidParameterValueException if the hypervisor does not
support rootdisksize override
*/
protected void
verifyIfHypervisorSupportsRootdiskSizeOverride(HypervisorType hypervisorType) {
- if (!(hypervisorType == HypervisorType.KVM || hypervisorType ==
HypervisorType.XenServer || hypervisorType == HypervisorType.VMware ||
hypervisorType == HypervisorType.Simulator)) {
+ if (!(hypervisorType == HypervisorType.KVM || hypervisorType ==
HypervisorType.XenServer ||hypervisorType == HypervisorType.VMware ||
+ hypervisorType == HypervisorType.Simulator || hypervisorType
== HypervisorType.Custom)) {
Review Comment:
how about `Arrays.asList(HypervisorType.KVM, HypervisorType.XenServer,
HypervisorType.VMware, HypervisorType.Simulator,
HypervisorType.Custom).contains(hypervisorType)` and even better, mover the
array to `Hypervisor.HypervisorType`
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -5052,6 +5056,12 @@ public boolean finalizeStart(VirtualMachineProfile
profile, long hostId, Command
}
}
+ if (returnedVncPassword != null &&
!originalVncPassword.equals(returnedVncPassword)) {
+ UserVmVO userVm = _vmDao.findById(profile.getId());
+ userVm.setVncPassword(returnedVncPassword);
+ _vmDao.update(userVm.getId(), userVm);
+ }
Review Comment:
separate method?
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -853,6 +855,15 @@ public String updateConfiguration(final long userId, final
String name, final St
} catch (final Throwable e) {
throw new CloudRuntimeException("Failed to clean up download
URLs in template_store_ref or volume_store_ref due to exception ", e);
}
+ } else if
(HypervisorGuru.HypervisorCustomDisplayName.key().equals(name)) {
+ String hypervisorListConfigName = Config.HypervisorList.key();
+ String hypervisors = _configDao.getValue(hypervisorListConfigName);
+ if (Arrays.asList(hypervisors.split(",")).contains(previousValue))
{
+ hypervisors = hypervisors.replace(previousValue, value);
+ s_logger.info(String.format("Updating the hypervisor list
configuration '%s' " +
+ "to match the new custom hypervisor display name",
hypervisorListConfigName));
+ _configDao.update(hypervisorListConfigName, hypervisors);
+ }
Review Comment:
can you extract to a separate method?
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -652,7 +653,9 @@ public List<? extends Host> discoverHosts(final AddHostCmd
cmd) throws IllegalAr
}
}
- return discoverHostsFull(dcId, podId, clusterId, clusterName, url,
username, password, cmd.getHypervisor(), hostTags, cmd.getFullUrlParams(),
false);
+ String hypervisorType =
cmd.getHypervisor().equalsIgnoreCase(HypervisorGuru.HypervisorCustomDisplayName.value())
?
+ "Custom" : cmd.getHypervisor();
Review Comment:
can there be only one type of custom hypervisor and hence only one
`HypervisorCustomDisplayName`?
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -1264,7 +1265,10 @@ public Pair<List<? extends Cluster>, Integer>
searchForClusters(final ListCluste
}
if (hypervisorType != null) {
- sc.setParameters("hypervisorType", hypervisorType);
+ String hypervisorStr = (String) hypervisorType;
+ String hypervisorSearch =
HypervisorGuru.HypervisorCustomDisplayName.value()
+ .equals(hypervisorStr) ? "Custom" : hypervisorStr;
Review Comment:
this logic is too scattered all over the system. should be in
`Hypervisor.HypervisorType`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
