You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/06/09 22:02:44 UTC

[GitHub] [cloudstack] GabrielBrascher opened a new pull request #5095: Failed to scale from services with same root disk size

GabrielBrascher opened a new pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095


   ### Description
   
   When changing from a Service Offering with the root disk size enforced to another offering with the same root disk size enforced this will lead to the following error message.
   
   ![image](https://user-images.githubusercontent.com/5025148/121435280-fdf41600-c954-11eb-8627-731c902af85e.png)
   
   
   On PR #4341 I covered manual tests **1, 2, & 3** as described on "**How Has This Been Tested?**"; however, none of these covered the issue discovered.
   
   This PR addresses the test case **4** that is reported in "**How Has This Been Tested?**"
   
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [X] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [ ] Major
   - [X] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ ] Major
   - [X] Minor
   - [ ] Trivial
   
   
   ### Screenshots (if appropriate):
   N/A
   
   ### How Has This Been Tested?
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   
   Tests:
   Created the following service offerings (via API and also via UI):
   
       0GB - default behavior with no Root Disk Size configured
       5GB - 5GB Root
       10GB - 10GB Root
       15GB - 15GB Root
   
   Executed following tests
   
   1. Test root volume resize.
   Expected result: not allowed to resize
   Steps:
   a - deploy a VM with any of the root size configured offerings (e.g. 5GB)
   b - Change root volume size
   c - not allowed
   
   2. Test resize via service offering.
   Expected result: allow 5GB -> 10GB, and 10 GB -> 15, fail to "dowsize" from 10GB -> 5GB
   Steps:
   a - deploy VM with offering 5GB
   b - Stop VM
   c - Change Service offering to 10GB
   d - Change offering back to 5GB, fails (as expected)
   e - Change offering to 15GB
   
   3. Test resize via offering to a Service offering with the default root size (0 GB) and then customize the volume
   Expected result: allow 5GB -> 0GB, successfully change the root volume to a custom size
   Steps:
   a - deploy VM with offering 5GB
   b - Stop VM
   c - Change Service offering to 0GB
   d - resize volume to custom size, e.g 50 GB
   
   4. **Test resize via service offering.
   Expected result: allow 10GB -> 10GB  successfully change the offering; note that they are two offerings where the compute resources are changed but root size stays the same**
   Steps:
   a - deploy VM with offering 10GB
   b - Stop VM
   c - Change Service offering to other offering 10GB


-- 
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

[GitHub] [cloudstack] rhtyd merged pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095


   


-- 
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

[GitHub] [cloudstack] rhtyd commented on a change in pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r649127023



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -928,12 +932,6 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 format = template.getFormat();
             }
 
-            if (volume.getVolumeType().equals(Volume.Type.ROOT) && diskOffering.getDiskSize() > 0 && format != null && format != ImageFormat.ISO) {

Review comment:
       @Pearl1594 can also regression test for multi-disk ova and deploy-as-is (with/without)




-- 
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

[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r648912592



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1172,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, ImageFormat format, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       Yes @GabrielBrascher , the ISO check seems unnecessary. Looks good. Thanks




-- 
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

[GitHub] [cloudstack] blueorangutan commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858633030


   <b>Trillian test result (tid-899)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 32462 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5095-t899-kvm-centos7.zip
   Smoke tests completed. 87 look OK, 0 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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

[GitHub] [cloudstack] GabrielBrascher commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858767067


   @shwstppr @Pearl1594 @rhtyd I've just added a commit that removes the disk format validation as it seems not necessary.
   
   To give some context, I moved the validation that was added at PR https://github.com/apache/cloudstack/pull/4341 to be executed only if the new size is not null; otherwise, it would not be necessary as it could be a simple change on offerings without changing the volume size.
   
   I think that by moving the if conditional it also does not require the ISO format validation and narrows down such flow to only when resizing a volume.


-- 
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

[GitHub] [cloudstack] Pearl1594 commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858256555


   @blueorangutan package


-- 
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

[GitHub] [cloudstack] Pearl1594 commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-859688487


   @GabrielBrascher Seems like the NPE still persists - when attempting to resize the volume of a VM deployed using an ISO:
   ```
   021-06-11 14:57:03,029 ERROR [c.c.a.ApiAsyncJobDispatcher] (API-Job-Executor-1:ctx-7cd19915 job-76) (logid:66182097) Unexpected exception while executing org.apache.cloudstack.api.command.admin.volume.ResizeVolumeCmdByAdmin
   java.lang.NullPointerException
   	at com.cloud.storage.VolumeApiServiceImpl.isNotPossibleToResize(VolumeApiServiceImpl.java:1184)
   	at com.cloud.storage.VolumeApiServiceImpl.resizeVolume(VolumeApiServiceImpl.java:938)
   	at com.cloud.storage.VolumeApiServiceImpl.resizeVolume(VolumeApiServiceImpl.java:194)
   	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
   
   ```
   However, resizing after scaling works fine.
   There needs to be a null check for the serviceOfferingView Object, or something around the lines of:
   `return (isNotIso && serviceOfferingView.getRootDiskSize() > 0 && isRoot);`
   


-- 
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

[GitHub] [cloudstack] GabrielBrascher commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-859635011






-- 
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

[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r648912592



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1172,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, ImageFormat format, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       Yes @GabrielBrascher , the ISO check seems unnecessary. In that case the section to fetch Image format can be removed altogether. Looks good. Thanks




-- 
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

[GitHub] [cloudstack] Pearl1594 commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858284272


   @blueorangutan test


-- 
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

[GitHub] [cloudstack] blueorangutan commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858284484


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5095: Failed to scale from services with same root disk size

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r648716671



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1172,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, ImageFormat format, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       @Pearl1594 I changed this if-conditional when compared to PR #4829. 
   
   I removed the ISO format comparison (`format != ImageFormat.ISO`) as the firs validation checks for `serviceOfferingView.getRootDiskSize()`. I could not see a situation of an ISO getting into this flow.
   
   Please let me know if I am missing anything.
   Thanks in advance!




-- 
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

[GitHub] [cloudstack] rhtyd commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-860329875


   @shwstppr @Pearl1594 are we lgtm on it, was it (manually) test for cases? 


-- 
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

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r649930628



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1165,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       @shwstppr thanks for testing it, I really appreciate it. I will get the ISO format validation back then.
   
   Regarding the serviceOfferingView, it is used as that table contains the root disk size which allows us to diff offerings that enforce the root disk size of those that allow resizing the volume.

##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1165,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       @shwstppr thanks for testing it, I really appreciate it. I will get the ISO format validation back then.
   
   Regarding the serviceOfferingView, it is used as that "table/view" contains the root disk size which allows us to diff offerings that enforce the root disk size of those that allow resizing the volume.

##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1165,27 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering)
+     */
+    protected boolean isNotPossibleToResize(VolumeVO volume, DiskOfferingVO diskOffering) {
+        Long templateId = volume.getTemplateId();
+        ImageFormat format = null;
+        if (templateId != null) {
+            VMTemplateVO template = _templateDao.findById(templateId);

Review comment:
       @shwstppr sounds good; I've added a commit for it.




-- 
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

[GitHub] [cloudstack] blueorangutan commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-859260033






-- 
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

[GitHub] [cloudstack] GabrielBrascher commented on pull request #5095: Failed to scale from services with same root disk size

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858135234


   @rhtyd I just reproduced this issue on 4.15.1.0; thus, I took the liberty of placing this PR milestone as **4.15.1.0**.
   
   I don't want to block the release due to this raised PR, you have the "authority" (as the RM) here to manage goals to **4.15.1.0**; no problem if you think this should go to **4.15.2.0**.
   Thanks!


-- 
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

[GitHub] [cloudstack] GabrielBrascher commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-860655020


   Thanks for the tests & review @shwstppr @Pearl1594!


-- 
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

[GitHub] [cloudstack] blueorangutan commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858283673


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 194


-- 
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

[GitHub] [cloudstack] shwstppr commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-859259678






-- 
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

[GitHub] [cloudstack] rhtyd commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-859626996


   @blueorangutan package


-- 
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

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r649344607



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1172,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, ImageFormat format, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       @shwstppr thanks for the ping. It seems like the case indeed. I added a commit that completely removes the format as well as some of the code used previously to retrieve the format.




-- 
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

[GitHub] [cloudstack] shwstppr commented on a change in pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
shwstppr commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r649782157



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1165,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       @GabrielBrascher this gives NPE while resizing volumes of VM deployed using ISO,
   ```
   2021-06-11 07:45:54,492 ERROR [c.c.a.ApiAsyncJobDispatcher] (API-Job-Executor-15:ctx-d6c38778 job-85) (logid:c6ed7fa6) Unexpected exception while executing org.apache.cloudstack.api.command.admin.volume.ResizeVolumeCmdByAdmin
   java.lang.NullPointerException
   	at com.cloud.storage.VolumeApiServiceImpl.isNotPossibleToResize(VolumeApiServiceImpl.java:1175)
   	at com.cloud.storage.VolumeApiServiceImpl.resizeVolume(VolumeApiServiceImpl.java:938)
   	at com.cloud.storage.VolumeApiServiceImpl.resizeVolume(VolumeApiServiceImpl.java:194)
   	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
   	at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:344)
   	at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198)
   	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163)
   	at org.apache.cloudstack.network.contrail.management.EventUtils$EventInterceptor.invoke(EventUtils.java:107)
   	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:175)
   	at com.cloud.event.ActionEventInterceptor.invoke(ActionEventInterceptor.java:51)
   	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:175)
   	at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:97)
   	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
   	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:215)
   	at com.sun.proxy.$Proxy217.resizeVolume(Unknown Source)
   ```
   Do we need to check serviceOfferingView and not diskOffering itself?

##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1165,27 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering)
+     */
+    protected boolean isNotPossibleToResize(VolumeVO volume, DiskOfferingVO diskOffering) {
+        Long templateId = volume.getTemplateId();
+        ImageFormat format = null;
+        if (templateId != null) {
+            VMTemplateVO template = _templateDao.findById(templateId);

Review comment:
       @GabrielBrascher is it possible to use findByIdIncludingRemoved else will reopen and rebase #5099 




-- 
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

[GitHub] [cloudstack] blueorangutan commented on pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#issuecomment-858256845


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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

[GitHub] [cloudstack] shwstppr commented on a change in pull request #5095: Failed to scale between Service Offerings with the same root disk size

Posted by GitBox <gi...@apache.org>.
shwstppr commented on a change in pull request #5095:
URL: https://github.com/apache/cloudstack/pull/5095#discussion_r649199637



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1167,6 +1172,16 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep
                 shrinkOk);
     }
 
+    /**
+     * A volume should not be resized if it covers ALL the following scenarios: <br>
+     * 1 - Root volume <br>
+     * 2 - && Current Disk Offering enforces a root disk size (in this case one can resize only by changing the Service Offering) <br>
+     */
+    private boolean isNotPossibleToResize(VolumeVO volume, ImageFormat format, DiskOfferingVO diskOffering) {
+        ServiceOfferingJoinVO serviceOfferingView = serviceOfferingJoinDao.findById(diskOffering.getId());
+        return serviceOfferingView.getRootDiskSize() > 0 && volume.getVolumeType().equals(Volume.Type.ROOT);

Review comment:
       Missed the discussion here.
   @GabrielBrascher if you decide to remove ImageFormat bits, #5099 can be closed
   cc @Pearl1594 




-- 
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