review comments addressed

api/src/main/java/com/cloud/offering/DiskOffering.java

@@ -57,8 +57,6 @@ public String toString() {
 
     String getName();
 
-    boolean isSystemUse();
-
     String getDisplayText();
 
     ProvisioningType getProvisioningType();

api/src/main/java/org/apache/cloudstack/api/command/user/volume/ChangeOfferingForVolumeCmd.java

@@ -63,13 +63,13 @@ public class ChangeOfferingForVolumeCmd extends BaseAsyncCmd implements UserCmd
             description = "new disk offering id")
     private Long newDiskOfferingId;
 
-    @Parameter(name = ApiConstants.SIZE, type = CommandType.LONG, required = false, description = "New volume size in GB")
+    @Parameter(name = ApiConstants.SIZE, type = CommandType.LONG, required = false, description = "New volume size in GB for the custom disk offering")
     private Long size;
 
-    @Parameter(name = ApiConstants.MIN_IOPS, type = CommandType.LONG, required = false, description = "New minimum number of IOPS")
+    @Parameter(name = ApiConstants.MIN_IOPS, type = CommandType.LONG, required = false, description = "New minimum number of IOPS for the custom disk offering")
     private Long minIops;
 
-    @Parameter(name = ApiConstants.MAX_IOPS, type = CommandType.LONG, required = false, description = "New maximum number of IOPS")
+    @Parameter(name = ApiConstants.MAX_IOPS, type = CommandType.LONG, required = false, description = "New maximum number of IOPS for the custom disk offering")
     private Long maxIops;
 
     @Parameter(name = ApiConstants.AUTO_MIGRATE, type = CommandType.BOOLEAN, required = false, description = "Flag for automatic migration of the volume " +

api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java

@@ -209,19 +209,21 @@ public class ServiceOfferingResponse extends BaseResponseWithAnnotations {
     private Boolean dynamicScalingEnabled;
 
     @SerializedName(ApiConstants.DISK_OFFERING_STRICTNESS)
-    @Param(description = "True/False to indicate the strictness of the disk offering association with the compute offering. When set to true, override of disk offering is not allowed when VM is deployed and change disk offering is not allowed for the ROOT disk after the VM is deployed", since = "4.16")
+    @Param(description = "True/False to indicate the strictness of the disk offering association with the compute offering. " +
+            "When set to true, override of disk offering is not allowed when VM is deployed and " +
+            "change disk offering is not allowed for the ROOT disk after the VM is deployed", since = "4.16")
     private Boolean diskOfferingStrictness;
 
     @SerializedName(ApiConstants.DISK_OFFERING_ID)
     @Param(description = "the ID of the disk offering to which service offering is linked", since = "4.16")
     private String diskOfferingId;
 
     @SerializedName("diskofferingname")
-    @Param(description = "name of the disk offering")
+    @Param(description = "name of the disk offering", since = "4.16")
     private String diskOfferingName;
 
     @SerializedName("diskofferingdisplaytext")
-    @Param(description = "the display text of the disk offering")
+    @Param(description = "the display text of the disk offering", since = "4.16")
     private String diskOfferingDisplayText;
 
     public ServiceOfferingResponse() {

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java

@@ -156,7 +156,7 @@ public void destroyVolume(String volumeEntity) {
     @Override
     public VirtualMachineEntity createVirtualMachine(String id, String owner, String templateId, String hostName, String displayName, String hypervisor, int cpu,
         int speed, long memory, Long diskSize, List<String> computeTags, List<String> rootDiskTags, Map<String, List<NicProfile>> networkNicMap, DeploymentPlan plan,
-        Long rootDiskSize, Map<String, Map<Integer, String>> extraDhcpOptionMap, Map<Long, DiskOffering> dataDiskTemplateToDiskOfferingMap, Long diskOfferingId, Long rootDiskOfferingId) throws InsufficientCapacityException {
+        Long rootDiskSize, Map<String, Map<Integer, String>> extraDhcpOptionMap, Map<Long, DiskOffering> dataDiskTemplateToDiskOfferingMap, Long dataDiskOfferingId, Long rootDiskOfferingId) throws InsufficientCapacityException {
 
         // VirtualMachineEntityImpl vmEntity = new VirtualMachineEntityImpl(id, owner, hostName, displayName, cpu, speed, memory, computeTags, rootDiskTags, networks,
         // vmEntityManager);
@@ -187,7 +187,7 @@ public VirtualMachineEntity createVirtualMachine(String id, String owner, String
 
         DiskOfferingVO rootDiskOffering = _diskOfferingDao.findById(rootDiskOfferingId);
         if (rootDiskOffering == null) {
-            throw new InvalidParameterValueException("Unable to find disk offering " + rootDiskOfferingId);
+            throw new InvalidParameterValueException("Unable to find root disk offering " + rootDiskOfferingId);
         }
         rootDiskOfferingInfo.setDiskOffering(rootDiskOffering);
         rootDiskOfferingInfo.setSize(rootDiskSize);
@@ -204,10 +204,10 @@ public VirtualMachineEntity createVirtualMachine(String id, String owner, String
             }
         }
 
-        if (diskOfferingId != null) {
-            DiskOfferingVO diskOffering = _diskOfferingDao.findById(diskOfferingId);
+        if (dataDiskOfferingId != null) {
+            DiskOfferingVO diskOffering = _diskOfferingDao.findById(dataDiskOfferingId);
             if (diskOffering == null) {
-                throw new InvalidParameterValueException("Unable to find disk offering " + diskOfferingId);
+                throw new InvalidParameterValueException("Unable to find data disk offering " + dataDiskOfferingId);
             }
             if (!diskOffering.isComputeOnly()) {
                 Long size = null;
@@ -244,7 +244,7 @@ public VirtualMachineEntity createVirtualMachine(String id, String owner, String
             for (Entry<Long, DiskOffering> datadiskTemplateToDiskOffering : dataDiskTemplateToDiskOfferingMap.entrySet()) {
                 DiskOffering dataDiskOffering = datadiskTemplateToDiskOffering.getValue();
                 if (dataDiskOffering == null) {
-                    throw new InvalidParameterValueException("Unable to find disk offering " + diskOfferingId);
+                    throw new InvalidParameterValueException("Unable to find disk offering " + dataDiskOfferingId);
                 }
                 if (dataDiskOffering.getDiskSize() == 0) { // Custom disk offering is not supported for volumes created from datadisk templates
                     throw new InvalidParameterValueException("Disk offering " + dataDiskOffering + " requires size parameter.");

engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java

@@ -219,7 +219,7 @@ 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 diskOfferingVO = new DiskOfferingVO(name, displayText, provisioningType, false, tags, recreatable, false, true);
         diskOfferingVO.setUniqueName(uniqueName);
         diskOfferingVO = diskOfferingDao.persistDefaultDiskOffering(diskOfferingVO);
 
@@ -238,7 +238,7 @@ public List<ServiceOfferingVO> createSystemServiceOfferings(String name, String
             useLocal = false;
         }
 
-        diskOfferingVO = new DiskOfferingVO(name + (useLocal ? " - Local Storage" : ""), displayText, provisioningType, false, tags, recreatable, useLocal, systemUse, true);
+        diskOfferingVO = new DiskOfferingVO(name + (useLocal ? " - Local Storage" : ""), displayText, provisioningType, false, tags, recreatable, useLocal, true);
         diskOfferingVO.setUniqueName(uniqueName + (useLocal ? "-Local" : ""));
         diskOfferingVO = diskOfferingDao.persistDefaultDiskOffering(diskOfferingVO);
 

engine/schema/src/main/java/com/cloud/storage/DiskOfferingVO.java

@@ -75,9 +75,6 @@ public class DiskOfferingVO implements DiskOffering {
     @Column(name = "use_local_storage")
     private boolean useLocalStorage;
 
-    @Column(name = "system_use")
-    private boolean systemUse;
-
     @Column(name = "customized")
     private boolean customized;
 
@@ -193,7 +190,7 @@ public DiskOfferingVO(String name, String displayText, Storage.ProvisioningType
         state = State.Active;
     }
 
-    public DiskOfferingVO(String name, String displayText, Storage.ProvisioningType provisioningType, boolean mirrored, String tags, boolean recreatable, boolean useLocalStorage, boolean systemUse,
+    public DiskOfferingVO(String name, String displayText, Storage.ProvisioningType provisioningType, boolean mirrored, String tags, boolean recreatable, boolean useLocalStorage,
             boolean customized) {
         computeOnly = true;
         this.name = name;
@@ -202,14 +199,13 @@ public DiskOfferingVO(String name, String displayText, Storage.ProvisioningType
         this.tags = tags;
         this.recreatable = recreatable;
         this.useLocalStorage = useLocalStorage;
-        this.systemUse = systemUse;
         this.customized = customized;
         uuid = UUID.randomUUID().toString();
         state = State.Active;
     }
 
     public DiskOfferingVO(long id, String name, String displayText, Storage.ProvisioningType provisioningType, boolean mirrored, String tags, boolean recreatable, boolean useLocalStorage,
-            boolean systemUse, boolean customized, boolean customizedIops, Long minIops, Long maxIops) {
+            boolean customized, boolean customizedIops, Long minIops, Long maxIops) {
         this.id = id;
         computeOnly = true;
         this.name = name;
@@ -218,7 +214,6 @@ public DiskOfferingVO(long id, String name, String displayText, Storage.Provisio
         this.tags = tags;
         this.recreatable = recreatable;
         this.useLocalStorage = useLocalStorage;
-        this.systemUse = systemUse;
         this.customized = customized;
         this.customizedIops = customizedIops;
         uuid = UUID.randomUUID().toString();
@@ -319,15 +314,6 @@ public void setName(String name) {
         this.name = name;
     }
 
-    @Override
-    public boolean isSystemUse() {
-        return systemUse;
-    }
-
-    public void setSystemUse(boolean systemUse) {
-        this.systemUse = systemUse;
-    }
-
     @Override
     public String getDisplayText() {
         return displayText;

engine/schema/src/main/java/com/cloud/storage/dao/DiskOfferingDao.java

@@ -24,10 +24,6 @@
 
 public interface DiskOfferingDao extends GenericDao<DiskOfferingVO, Long> {
 
-    List<DiskOfferingVO> findPrivateDiskOffering();
-
-    List<DiskOfferingVO> findPublicDiskOfferings();
-
     DiskOfferingVO findByUniqueName(String uniqueName);
 
     DiskOfferingVO persistDefaultDiskOffering(DiskOfferingVO offering);

engine/schema/src/main/java/com/cloud/storage/dao/DiskOfferingDaoImpl.java

@@ -45,8 +45,6 @@ public class DiskOfferingDaoImpl extends GenericDaoBase<DiskOfferingVO, Long> im
     @Inject
     protected DiskOfferingDetailsDao detailsDao;
 
-    private final SearchBuilder<DiskOfferingVO> PrivateDiskOfferingSearch;
-    private final SearchBuilder<DiskOfferingVO> PublicDiskOfferingSearch;
     protected final SearchBuilder<DiskOfferingVO> UniqueNameSearch;
     private final String SizeDiskOfferingSearch = "SELECT * FROM disk_offering WHERE " +
             "disk_size = ? AND provisioning_type = ? AND removed IS NULL";
@@ -55,29 +53,13 @@ public class DiskOfferingDaoImpl extends GenericDaoBase<DiskOfferingVO, Long> im
     protected final static long GB_UNIT_BYTES = 1024 * 1024 * 1024;
 
     protected DiskOfferingDaoImpl() {
-        PrivateDiskOfferingSearch = createSearchBuilder();
-        PrivateDiskOfferingSearch.and("diskSize", PrivateDiskOfferingSearch.entity().getDiskSize(), SearchCriteria.Op.EQ);
-        PrivateDiskOfferingSearch.done();
-
-        PublicDiskOfferingSearch = createSearchBuilder();
-        PublicDiskOfferingSearch.and("system", PublicDiskOfferingSearch.entity().isSystemUse(), SearchCriteria.Op.EQ);
-        PublicDiskOfferingSearch.and("removed", PublicDiskOfferingSearch.entity().getRemoved(), SearchCriteria.Op.NULL);
-        PublicDiskOfferingSearch.done();
-
         UniqueNameSearch = createSearchBuilder();
         UniqueNameSearch.and("name", UniqueNameSearch.entity().getUniqueName(), SearchCriteria.Op.EQ);
         UniqueNameSearch.done();
 
         _computeOnlyAttr = _allAttributes.get("computeOnly");
     }
 
-    @Override
-    public List<DiskOfferingVO> findPrivateDiskOffering() {
-        SearchCriteria<DiskOfferingVO> sc = PrivateDiskOfferingSearch.create();
-        sc.setParameters("diskSize", 0);
-        return listBy(sc);
-    }
-
     @Override
     public List<DiskOfferingVO> searchIncludingRemoved(SearchCriteria<DiskOfferingVO> sc, final Filter filter, final Boolean lock, final boolean cache) {
         return super.searchIncludingRemoved(sc, filter, lock, cache);
@@ -101,17 +83,6 @@ protected List<DiskOfferingVO> executeList(final String sql, final Object... par
         return super.executeList(sql, false, params);
     }
 
-    @Override
-    public List<DiskOfferingVO> findPublicDiskOfferings() {
-        SearchCriteria<DiskOfferingVO> sc = PublicDiskOfferingSearch.create();
-        sc.setParameters("system", false);
-        List<DiskOfferingVO> offerings = listBy(sc);
-        if(offerings!=null) {
-            offerings.removeIf(o -> (!detailsDao.findDomainIds(o.getId()).isEmpty()));
-        }
-        return offerings;
-    }
-
     @Override
     public DiskOfferingVO findByUniqueName(String uniqueName) {
         SearchCriteria<DiskOfferingVO> sc = UniqueNameSearch.create();

plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java

@@ -455,7 +455,7 @@ private Long getImportingVMGuestOs(VirtualMachineConfigSummary configSummary) {
     private ServiceOfferingVO createServiceOfferingForVMImporting(Integer cpus, Integer memory, Integer maxCpuUsage) {
         String name = "Imported-" + cpus + "-" + memory;
 
-        DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, name, Storage.ProvisioningType.THIN, false, null, false, false, false, true);
+        DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, name, Storage.ProvisioningType.THIN, false, null, false, false, true);
         diskOfferingVO = diskOfferingDao.persistDefaultDiskOffering(diskOfferingVO);
 
 

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -3022,7 +3022,7 @@ private DiskOfferingVO createDiskOfferingInternal(final long userId, final boole
                                                       Long iopsWriteRate, Long iopsWriteRateMax, Long iopsWriteRateMaxLength,
                                                       final Integer hypervisorSnapshotReserve, String cacheMode, final Long storagePolicyID) {
 
-        DiskOfferingVO diskOffering = new DiskOfferingVO(name, displayText, typedProvisioningType, false, tags, false, localStorageRequired, isSystem, true);
+        DiskOfferingVO diskOffering = new DiskOfferingVO(name, displayText, typedProvisioningType, false, tags, false, localStorageRequired, true);
 
         if (Boolean.TRUE.equals(isCustomizedIops) || isCustomizedIops == null) {
             minIops = null;

server/src/main/java/com/cloud/server/ConfigurationServerImpl.java

@@ -920,16 +920,15 @@ private DiskOfferingVO createDefaultDiskOffering(String name, String description
 
         DiskOfferingVO newDiskOffering = new DiskOfferingVO(name, description, provisioningType, diskSize, tags, isCustomized, null, null, null);
         newDiskOffering.setUniqueName("Cloud.Com-" + name);
-        // leaving the above reference to cloud.com in as it is an identifyer and has no real world relevance
-        newDiskOffering.setSystemUse(isSystemUse);
+        // leaving the above reference to cloud.com in as it is an identifier and has no real world relevance
         newDiskOffering = _diskOfferingDao.persistDefaultDiskOffering(newDiskOffering);
         return newDiskOffering;
     }
 
     private ServiceOfferingVO createServiceOffering(long userId, String name, int cpu, int ramSize, int speed, String displayText,
             ProvisioningType provisioningType, boolean localStorageRequired, boolean offerHA, String tags) {
         tags = cleanupTags(tags);
-        DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, displayText, provisioningType, false, tags, false, false, false, true);
+        DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, displayText, provisioningType, false, tags, false, false, true);
         diskOfferingVO.setUniqueName("Cloud.Com-" + name);
         diskOfferingVO = _diskOfferingDao.persistDefaultDiskOffering(diskOfferingVO);
 

server/src/main/java/com/cloud/test/DatabaseConfig.java

@@ -932,7 +932,7 @@ protected void saveServiceOffering() {
         } else {
             useLocalStorage = false;
         }
-        DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, displayText, provisioningType, false, null, false, false, false, true);
+        DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, displayText, provisioningType, false, null, false, false, true);
 
         ServiceOfferingVO serviceOffering =
             new ServiceOfferingVO(name, cpu, ramSize, speed, null, null, ha, displayText,

server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java

@@ -1138,7 +1138,6 @@ public void isNotPossibleToResizeTestNoRootDiskSize() {
         prepareAndRunTestOfIsNotPossibleToResize(Type.ROOT, 0l, Storage.ImageFormat.QCOW2, false);
     }
 
-    // FR123 update this test case to check service offering strictness
     private void prepareAndRunTestOfIsNotPossibleToResize(Type volumeType, Long rootDisk, Storage.ImageFormat imageFormat, boolean expectedIsNotPossibleToResize) {
         VolumeVO volume = Mockito.mock(VolumeVO.class);
         when(volume.getVolumeType()).thenReturn(volumeType);