DaanHoogland commented on code in PR #8142:
URL: https://github.com/apache/cloudstack/pull/8142#discussion_r1380135758
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -294,12 +294,14 @@ public KVMStoragePool getStoragePoolByURI(String uri) {
String uuid = null;
String sourceHost = "";
StoragePoolType protocol = null;
- if (storageUri.getScheme().equalsIgnoreCase("nfs") ||
storageUri.getScheme().equalsIgnoreCase("NetworkFilesystem")) {
+ final String scheme = storageUri.getScheme().toLowerCase();
+ List<String> acceptedSchemes = List.of("nfs", "networkfilesystem",
"filesystem");
+ if (acceptedSchemes.contains(scheme)) {
Review Comment:
the `IgnoreCase` part disappeared here. Could that give issue for for
instance "networkFileSystem"?
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java:
##########
@@ -244,7 +246,7 @@ public TemplateInfo
getReadyBypassedTemplateOnPrimaryStore(long templateId, Long
if (poolId == null) {
//Get ISO from existing pool ref
HostVO host = hostDao.findById(hostId);
- List<StoragePoolVO> pools =
getStoragePoolsFromClusterOrZone(host.getClusterId(), host.getDataCenterId(),
host.getHypervisorType());
+ List<StoragePoolVO> pools =
getStoragePoolsFromClusterOrZoneOrLocal(host.getClusterId(),
host.getDataCenterId(), host.getHypervisorType(), hostId);
Review Comment:
here we can
```suggestion
List<StoragePoolVO> pools =
getStoragePoolsFromClusterOrZoneOrLocal(host.getClusterId(),
host.getDataCenterId(), host.getHypervisorType(), hostId);
if (pools.isEmpty()) {
throw new CloudRuntimeException("No storage pool found
where to download template: " + templateId);
}
```
because we know `null` will be returned otherwise. (and I think that should
be an exception as well.)
##########
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/version/KubernetesVersionManagerImpl.java:
##########
@@ -163,6 +164,9 @@ private VirtualMachineTemplate
registerKubernetesVersionIso(final Long zoneId, f
if (StringUtils.isNotEmpty(isoChecksum)) {
registerIsoCmd.setChecksum(isoChecksum);
}
+ if (directDownload != null) {
+ registerIsoCmd.setDirectDownload(directDownload);
+ }
Review Comment:
default is false. Should we make that explicit here?
##########
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/version/KubernetesVersionManagerImpl.java:
##########
@@ -297,8 +302,14 @@ public KubernetesSupportedVersionResponse
addKubernetesSupportedVersion(final Ad
if (compareSemanticVersions(semanticVersion, MIN_KUBERNETES_VERSION) <
0) {
throw new InvalidParameterValueException(String.format("New
supported Kubernetes version cannot be added as %s is minimum version supported
by Kubernetes Service", MIN_KUBERNETES_VERSION));
}
- if (zoneId != null && dataCenterDao.findById(zoneId) == null) {
- throw new InvalidParameterValueException("Invalid zone specified");
+ if (zoneId != null) {
+ DataCenter zone = dataCenterDao.findById(zoneId);
+ if (zone == null) {
+ throw new InvalidParameterValueException("Invalid zone
specified");
+ }
+ if (DataCenter.Type.Edge.equals(zone.getType())) {
+ directDownload = true;
Review Comment:
aha
##########
ui/src/views/image/AddKubernetesSupportedVersion.vue:
##########
@@ -207,23 +218,30 @@ export default {
}
})
},
+ handleZoneChange (zoneIdx) {
+ const zone = this.zones[zoneIdx]
+ if (zone.type && zone.type === 'Edge') {
+ this.form.directdownload = true
+ this.directDownloadDisabled = true
+ return
+ }
+ this.form.directdownload = false
+ this.directDownloadDisabled = false
+ },
Review Comment:
The value directDownload is set to false , when the prior selection was an
edge zone and the current is not. But what if before it was already selected to
be true, then edge zone was selected and deselected again?
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java:
##########
@@ -217,20 +216,23 @@ private Long
getOneMatchingPoolIdFromRefs(List<VMTemplateStoragePoolVO> existing
}
}
}
- return null;
+ return pools.get(0).getId();
}
/**
- * Retrieve storage pools with scope = cluster or zone matching clusterId
or dataCenterId depending on their scope
+ * Retrieve storage pools with scope = cluster or zone or local matching
clusterId or dataCenterId or hostId depending on their scope
*/
- private List<StoragePoolVO> getStoragePoolsFromClusterOrZone(Long
clusterId, long dataCenterId, Hypervisor.HypervisorType hypervisorType) {
+ private List<StoragePoolVO> getStoragePoolsFromClusterOrZoneOrLocal(Long
clusterId, long dataCenterId, Hypervisor.HypervisorType hypervisorType, long
hostId) {
Review Comment:
naming suggestion
```suggestion
private List<StoragePoolVO> getStoragePoolsForScope(Long clusterId, long
dataCenterId, Hypervisor.HypervisorType hypervisorType, long hostId) {
```
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java:
##########
@@ -204,11 +205,9 @@ public List<TemplateInfo> listTemplateOnCache(long
templateId) {
*/
private Long getOneMatchingPoolIdFromRefs(List<VMTemplateStoragePoolVO>
existingRefs, List<StoragePoolVO> pools) {
if (pools.isEmpty()) {
- throw new CloudRuntimeException("No storage pools found");
+ return null;
Review Comment:
returning null is not a good practice. Can this `if (pools.isEmpty())` be
handled in the calling methods and an exception be thrown here?
--
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]
