GutoVeronezi commented on a change in pull request #5008:
URL: https://github.com/apache/cloudstack/pull/5008#discussion_r633608983
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java
##########
@@ -185,25 +185,25 @@ public VirtualMachineEntity createVirtualMachine(String
id, String owner, String
ServiceOfferingVO computeOffering =
_serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
- rootDiskOfferingInfo.setDiskOffering(computeOffering);
- rootDiskOfferingInfo.setSize(rootDiskSize);
-
- if (computeOffering.isCustomizedIops() != null &&
computeOffering.isCustomizedIops()) {
- Map<String, String> userVmDetails =
_userVmDetailsDao.listDetailsKeyPairs(vm.getId());
+ Long diskOfferingId = computeOffering.getDiskOfferingId();
+ if (diskOfferingId != null) {
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(diskOfferingId);
+ if (diskOffering == null) {
+ throw new InvalidParameterValueException("Unable to find disk
offering " + diskOfferingId);
+ }
+ rootDiskOfferingInfo.setDiskOffering(diskOffering);
+ rootDiskOfferingInfo.setSize(rootDiskSize);
- if (userVmDetails != null) {
- String minIops = userVmDetails.get("minIops");
- String maxIops = userVmDetails.get("maxIops");
+ if (diskOffering.isCustomizedIops() != null &&
diskOffering.isCustomizedIops()) {
+ Map<String, String> userVmDetails =
_userVmDetailsDao.listDetailsKeyPairs(vm.getId());
- rootDiskOfferingInfo.setMinIops(minIops != null &&
minIops.trim().length() > 0 ? Long.parseLong(minIops) : null);
- rootDiskOfferingInfo.setMaxIops(maxIops != null &&
maxIops.trim().length() > 0 ? Long.parseLong(maxIops) : null);
- }
- }
+ if (userVmDetails != null) {
+ String minIops = userVmDetails.get("minIops");
+ String maxIops = userVmDetails.get("maxIops");
- if (vm.getDiskOfferingId() != null) {
- DiskOfferingVO diskOffering =
_diskOfferingDao.findById(vm.getDiskOfferingId());
- if (diskOffering == null) {
- throw new InvalidParameterValueException("Unable to find disk
offering " + vm.getDiskOfferingId());
+ rootDiskOfferingInfo.setMinIops(minIops != null &&
minIops.trim().length() > 0 ? Long.parseLong(minIops) : null);
Review comment:
We can use `StringUtils` here:
```java
import org.apache.commons.lang3.StringUtils;
```
```java
rootDiskOfferingInfo.setMinIops(StringUtils.isNotBlank(minIops) ?
Long.parseLong(minIops) : null);
```
Or, to simplify the logic:
```java
rootDiskOfferingInfo.setMinIops(StringUtils.isBlank(minIops) ? null :
Long.parseLong(minIops));
```
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java
##########
@@ -185,25 +185,25 @@ public VirtualMachineEntity createVirtualMachine(String
id, String owner, String
ServiceOfferingVO computeOffering =
_serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
- rootDiskOfferingInfo.setDiskOffering(computeOffering);
- rootDiskOfferingInfo.setSize(rootDiskSize);
-
- if (computeOffering.isCustomizedIops() != null &&
computeOffering.isCustomizedIops()) {
- Map<String, String> userVmDetails =
_userVmDetailsDao.listDetailsKeyPairs(vm.getId());
+ Long diskOfferingId = computeOffering.getDiskOfferingId();
+ if (diskOfferingId != null) {
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(diskOfferingId);
+ if (diskOffering == null) {
+ throw new InvalidParameterValueException("Unable to find disk
offering " + diskOfferingId);
+ }
+ rootDiskOfferingInfo.setDiskOffering(diskOffering);
+ rootDiskOfferingInfo.setSize(rootDiskSize);
- if (userVmDetails != null) {
- String minIops = userVmDetails.get("minIops");
- String maxIops = userVmDetails.get("maxIops");
+ if (diskOffering.isCustomizedIops() != null &&
diskOffering.isCustomizedIops()) {
+ Map<String, String> userVmDetails =
_userVmDetailsDao.listDetailsKeyPairs(vm.getId());
- rootDiskOfferingInfo.setMinIops(minIops != null &&
minIops.trim().length() > 0 ? Long.parseLong(minIops) : null);
- rootDiskOfferingInfo.setMaxIops(maxIops != null &&
maxIops.trim().length() > 0 ? Long.parseLong(maxIops) : null);
- }
- }
+ if (userVmDetails != null) {
+ String minIops = userVmDetails.get("minIops");
+ String maxIops = userVmDetails.get("maxIops");
- if (vm.getDiskOfferingId() != null) {
- DiskOfferingVO diskOffering =
_diskOfferingDao.findById(vm.getDiskOfferingId());
- if (diskOffering == null) {
- throw new InvalidParameterValueException("Unable to find disk
offering " + vm.getDiskOfferingId());
+ rootDiskOfferingInfo.setMinIops(minIops != null &&
minIops.trim().length() > 0 ? Long.parseLong(minIops) : null);
+ rootDiskOfferingInfo.setMaxIops(maxIops != null &&
maxIops.trim().length() > 0 ? Long.parseLong(maxIops) : null);
Review comment:
We can use `StringUtils` here:
```java
import org.apache.commons.lang3.StringUtils;
```
```java
rootDiskOfferingInfo.setMaxIops(StringUtils.isNotBlank(maxIops) ?
Long.parseLong(maxIops) : null);
```
Or, to simplify the logic:
```java
rootDiskOfferingInfo.setMaxIops(StringUtils.isBlank(maxIops) ? null :
Long.parseLong(maxIops));
```
##########
File path: engine/schema/src/main/java/com/cloud/service/ServiceOfferingVO.java
##########
@@ -103,11 +146,16 @@ public ServiceOfferingVO(String name, Integer cpu,
Integer ramSize, Integer spee
volatileVm = false;
this.defaultUse = defaultUse;
this.vmType = vmType == null ? null : vmType.toString().toLowerCase();
+ uuid = UUID.randomUUID().toString();
+ this.systemUse = systemUse;
+ this.name = name;
+ this.displayText = displayText;
}
- public ServiceOfferingVO(String name, Integer cpu, Integer ramSize,
Integer speed, Integer rateMbps, Integer multicastRateMbps, boolean offerHA,
boolean limitCpuUse,
- boolean volatileVm, String displayText,
ProvisioningType provisioningType, boolean useLocalStorage, boolean
recreatable, String tags, boolean systemUse, VirtualMachine.Type vmType) {
- super(name, displayText, provisioningType, false, tags, recreatable,
useLocalStorage, systemUse, true);
+ public ServiceOfferingVO(String name, Integer cpu, Integer ramSize,
Integer speed, Integer rateMbps, Integer multicastRateMbps, boolean offerHA,
+ boolean limitResourceUse, boolean volatileVm,
String displayText, ProvisioningType provisioningType, boolean useLocalStorage,
boolean recreatable, String tags, boolean systemUse,
+ VirtualMachine.Type vmType, String hostTag,
String deploymentPlanner) {
+ //super(name, displayText, provisioningType, false, tags, recreatable,
useLocalStorage, systemUse, true);
Review comment:
As git will keep the history, we can remove the code.
##########
File path:
engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java
##########
@@ -214,10 +218,16 @@ public ServiceOfferingVO
getComputeOffering(ServiceOfferingVO serviceOffering, M
public List<ServiceOfferingVO> createSystemServiceOfferings(String name,
String uniqueName, int cpuCount, int ramSize, int cpuSpeed,
Integer rateMbps, Integer multicastRateMbps, boolean offerHA,
String displayText, ProvisioningType provisioningType,
boolean recreatable, String tags, boolean systemUse,
VirtualMachine.Type vmType, boolean defaultUse) {
+ DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, displayText,
provisioningType, false, tags, recreatable, false, systemUse, true);
+ diskOfferingVO.setUniqueName(uniqueName);
+ diskOfferingVO =
diskOfferingDao.persistDefaultDiskOffering(diskOfferingVO);
+
List<ServiceOfferingVO> list = new ArrayList<ServiceOfferingVO>();
ServiceOfferingVO offering = new ServiceOfferingVO(name, cpuCount,
ramSize, cpuSpeed, rateMbps, multicastRateMbps, offerHA, displayText,
provisioningType, false, recreatable, tags, systemUse, vmType,
defaultUse);
+ // super(name, displayText, provisioningType, false, tags,
recreatable, useLocalStorage, systemUse, true);
Review comment:
As git will keep the history, we can remove the code.
##########
File path:
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2605,46 +2607,51 @@ protected ServiceOfferingVO createServiceOffering(final
long userId, final boole
// Add in disk offering details
continue;
}
- detailsVO.add(new ServiceOfferingDetailsVO(offering.getId(),
detailEntry.getKey(), detailEntryValue, true));
+ detailsVO.add(new
ServiceOfferingDetailsVO(serviceOffering.getId(), detailEntry.getKey(),
detailEntryValue, true));
}
}
if (storagePolicyID != null) {
- detailsVO.add(new ServiceOfferingDetailsVO(offering.getId(),
ApiConstants.STORAGE_POLICY, String.valueOf(storagePolicyID), false));
+ detailsVO.add(new
ServiceOfferingDetailsVO(serviceOffering.getId(), ApiConstants.STORAGE_POLICY,
String.valueOf(storagePolicyID), false));
}
- if ((offering = _serviceOfferingDao.persist(offering)) != null) {
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(offering.getId(),
ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- if (CollectionUtils.isNotEmpty(zoneIds)) {
- for (Long zoneId : zoneIds) {
- detailsVO.add(new
ServiceOfferingDetailsVO(offering.getId(), ApiConstants.ZONE_ID,
String.valueOf(zoneId), false));
- }
- }
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detail : detailsVO) {
- detail.setResourceId(offering.getId());
- }
- _serviceOfferingDetailsDao.saveDetails(detailsVO);
- }
-
+ if ((diskOffering = _diskOfferingDao.persist(diskOffering)) != null) {
+ serviceOffering.setDiskOfferingId(diskOffering.getId());
if (details != null && !details.isEmpty()) {
List<DiskOfferingDetailVO> diskDetailsVO = new
ArrayList<DiskOfferingDetailVO>();
// Support disk offering details for below parameters
if (details.containsKey(Volume.BANDWIDTH_LIMIT_IN_MBPS)) {
- diskDetailsVO.add(new
DiskOfferingDetailVO(offering.getId(), Volume.BANDWIDTH_LIMIT_IN_MBPS,
details.get(Volume.BANDWIDTH_LIMIT_IN_MBPS), false));
+ diskDetailsVO.add(new
DiskOfferingDetailVO(diskOffering.getId(), Volume.BANDWIDTH_LIMIT_IN_MBPS,
details.get(Volume.BANDWIDTH_LIMIT_IN_MBPS), false));
}
if (details.containsKey(Volume.IOPS_LIMIT)) {
- diskDetailsVO.add(new
DiskOfferingDetailVO(offering.getId(), Volume.IOPS_LIMIT,
details.get(Volume.IOPS_LIMIT), false));
+ diskDetailsVO.add(new
DiskOfferingDetailVO(diskOffering.getId(), Volume.IOPS_LIMIT,
details.get(Volume.IOPS_LIMIT), false));
}
if (!diskDetailsVO.isEmpty()) {
diskOfferingDetailsDao.saveDetails(diskDetailsVO);
}
}
+ } else {
+ return null;
+ }
- CallContext.current().setEventDetails("Service offering id=" +
offering.getId());
- return offering;
+ if ((serviceOffering = _serviceOfferingDao.persist(serviceOffering))
!= null) {
+ for (Long domainId : filteredDomainIds) {
+ detailsVO.add(new
ServiceOfferingDetailsVO(serviceOffering.getId(), ApiConstants.DOMAIN_ID,
String.valueOf(domainId), false));
+ }
+ if (CollectionUtils.isNotEmpty(zoneIds)) {
+ for (Long zoneId : zoneIds) {
+ detailsVO.add(new
ServiceOfferingDetailsVO(serviceOffering.getId(), ApiConstants.ZONE_ID,
String.valueOf(zoneId), false));
+ }
+ }
+ if (!detailsVO.isEmpty()) {
Review comment:
We can use `CollectionUtils.isNotEmpty` here.
##########
File path:
server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java
##########
@@ -1557,9 +1555,10 @@ protected boolean hostCanAccessSPool(Host host,
StoragePool pool) {
DiskOfferingVO diskOffering =
_diskOfferingDao.findById(toBeCreated.getDiskOfferingId());
- if (vmProfile.getTemplate().getFormat() == Storage.ImageFormat.ISO
&& vmProfile.getServiceOffering().getTagsArray().length != 0) {
-
diskOffering.setTagsArray(Arrays.asList(vmProfile.getServiceOffering().getTagsArray()));
- }
+ //FR123 check how is it different for service offering
getTagsArray and disk offering's
+ //if (vmProfile.getTemplate().getFormat() ==
Storage.ImageFormat.ISO && vmProfile.getServiceOffering().getTagsArray().length
!= 0) {
+ //
diskOffering.setTagsArray(Arrays.asList(vmProfile.getServiceOffering().getTagsArray()));
+ //}
Review comment:
As git will keep the history, we can remove the code and move the
comment from code to commit message. If it is really needed in code, we can
turn it into a javadoc.
##########
File path:
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
##########
@@ -835,9 +823,11 @@ private UserVm migrateImportedVM(HostVO sourceHost,
VirtualMachineTemplate templ
if (CollectionUtils.isNotEmpty(storagePools)) {
for (StoragePool pool : storagePools) {
if (diskProfileStoragePool.second().getId() !=
pool.getId() &&
- storagePoolSupportsDiskOffering(pool, dOffering) &&
- (!profile.getType().equals(Volume.Type.ROOT) ||
- profile.getType().equals(Volume.Type.ROOT)
&& storagePoolSupportsServiceOffering(pool, serviceOffering))) {
+ storagePoolSupportsDiskOffering(pool, dOffering)
+ //FR123 get confirmation from Abhishek
+ //(!profile.getType().equals(Volume.Type.ROOT) ||
+
//profile.getType().equals(Volume.Type.ROOT) &&
storagePoolSupportsServiceOffering(pool, serviceOffering))
+ ) {
Review comment:
As git will keep the history, we can remove the code and move the
comment from code to commit message. If it is really needed in code, we can
turn it into a javadoc.
##########
File path: engine/schema/src/main/java/com/cloud/service/ServiceOfferingVO.java
##########
@@ -90,9 +132,10 @@ protected ServiceOfferingVO() {
super();
}
+ //FR123 split the usage of below constructor to create service offering vo
and disk offering vo in different daoImpl.java files. Currently it is only
under serviceofferingdaoimpl.java
public ServiceOfferingVO(String name, Integer cpu, Integer ramSize,
Integer speed, Integer rateMbps, Integer multicastRateMbps, boolean offerHA,
String displayText,
ProvisioningType provisioningType, boolean useLocalStorage,
boolean recreatable, String tags, boolean systemUse, VirtualMachine.Type
vmType, boolean defaultUse) {
- super(name, displayText, provisioningType, false, tags, recreatable,
useLocalStorage, systemUse, true);
+ //super(name, displayText, provisioningType, false, tags, recreatable,
useLocalStorage, systemUse, true);
Review comment:
As git will keep the history, we can remove the code.
##########
File path:
engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java
##########
@@ -58,7 +62,7 @@ public ServiceOfferingDaoImpl() {
UniqueNameSearch = createSearchBuilder();
UniqueNameSearch.and("name",
UniqueNameSearch.entity().getUniqueName(), SearchCriteria.Op.EQ);
- UniqueNameSearch.and("system",
UniqueNameSearch.entity().isSystemUse(), SearchCriteria.Op.EQ);
+ UniqueNameSearch.and("system_use",
UniqueNameSearch.entity().isSystemUse(), SearchCriteria.Op.EQ);
Review comment:
As it will be used more times in this class, could it be extracted to a
variable?
##########
File path:
engine/schema/src/main/java/com/cloud/storage/dao/DiskOfferingDaoImpl.java
##########
@@ -82,13 +80,13 @@ protected DiskOfferingDaoImpl() {
@Override
public List<DiskOfferingVO>
searchIncludingRemoved(SearchCriteria<DiskOfferingVO> sc, final Filter filter,
final Boolean lock, final boolean cache) {
- sc.addAnd(_typeAttr, Op.EQ, Type.Disk);
+ //sc.addAnd(_computeOnlyAttr, Op.EQ, false);
return super.searchIncludingRemoved(sc, filter, lock, cache);
}
@Override
public <K> List<K> customSearchIncludingRemoved(SearchCriteria<K> sc,
final Filter filter) {
- sc.addAnd(_typeAttr, Op.EQ, Type.Disk);
+ //sc.addAnd(_computeOnlyAttr, Op.EQ, false);
Review comment:
As git will keep the history, we can remove the code.
##########
File path:
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
##########
@@ -818,9 +806,9 @@ private UserVm migrateImportedVM(HostVO sourceHost,
VirtualMachineTemplate templ
continue;
}
boolean poolSupportsOfferings =
storagePoolSupportsDiskOffering(diskProfileStoragePool.second(), dOffering);
- if (poolSupportsOfferings && profile.getType() ==
Volume.Type.ROOT) {
- poolSupportsOfferings =
storagePoolSupportsServiceOffering(diskProfileStoragePool.second(),
serviceOffering);
- }
+ //FR123 get confirmation from Abhishek if (poolSupportsOfferings
&& profile.getType() == Volume.Type.ROOT) {
+ // poolSupportsOfferings =
storagePoolSupportsServiceOffering(diskProfileStoragePool.second(),
serviceOffering);
+ //}
Review comment:
As git will keep the history, we can remove the code and move the
comment from code to commit message. If it is really needed in code, we can
turn it into a javadoc.
##########
File path:
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
##########
@@ -848,9 +838,10 @@ private UserVm migrateImportedVM(HostVO sourceHost,
VirtualMachineTemplate templ
storagePools = poolsPair.first();
for (StoragePool pool : storagePools) {
if (diskProfileStoragePool.second().getId() !=
pool.getId() &&
- storagePoolSupportsDiskOffering(pool, dOffering) &&
- (!profile.getType().equals(Volume.Type.ROOT) ||
- profile.getType().equals(Volume.Type.ROOT)
&& storagePoolSupportsServiceOffering(pool, serviceOffering))) {
+ storagePoolSupportsDiskOffering(pool, dOffering)
+ //FR123 get confimration from
Abhishek(!profile.getType().equals(Volume.Type.ROOT) ||
+
//profile.getType().equals(Volume.Type.ROOT) &&
storagePoolSupportsServiceOffering(pool, serviceOffering))
Review comment:
As git will keep the history, we can remove the code and move the
comment from code to commit message. If it is really needed in code, we can
turn it into a javadoc.
--
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:
[email protected]
