[GitHub] [cloudstack] ravening commented on a change in pull request #4341: Allow to configure root disk size via Service Offering (diskoffering of type Service).

[GitBox <gi...@apache.org>]

ravening commented on a change in pull request #4341:
URL: https://github.com/apache/cloudstack/pull/4341#discussion_r509971665



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2518,6 +2520,12 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
             }
         }
 
+        if (rootDiskSizeInGiB != null && rootDiskSizeInGiB <= 0L) {
+            throw new InvalidParameterValueException(String.format("The Root disk size is of %s GB but it must be greater than 0.", rootDiskSizeInGiB));
+        } else if (rootDiskSizeInGiB != null) {
+            long rootDiskSizeInBytes = rootDiskSizeInGiB * GiB_TO_BYTES;
+            offering.setDiskSize(rootDiskSizeInBytes);

Review comment:
       An optimization suggestion here. You can write below one liner since `rootDiskSizeInBytes` is not used anywhere else
   ```
   offering.setDiskSize(rootDiskSizeInGiB * GiB_TO_BYTES)
   ```

##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1139,6 +1143,13 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    private void checkIfVolumeIsRootAndVmIsRunning(Long newSize, VolumeVO volume, VMInstanceVO vmInstanceVO) {
+        if (!volume.getSize().equals(newSize) && volume.getVolumeType().equals(Volume.Type.ROOT) && !State.Stopped.equals(vmInstanceVO.getState())) {

Review comment:
       Any specific reason why we are not allowing resizing when vm is running?

##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java
##########
@@ -126,6 +126,9 @@
     @Parameter(name = ApiConstants.SERVICE_OFFERING_DETAILS, type = CommandType.MAP, description = "details for planner, used to store specific parameters")
     private Map details;
 
+    @Parameter(name = ApiConstants.ROOT_DISK_SIZE, type = CommandType.LONG, required = false, description = "the Root disk size in GB.")

Review comment:
       Do we need `since` parameter 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org