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 2020/10/16 06:50:47 UTC

[GitHub] [cloudstack] GabrielBrascher opened a new pull request #4409: Enhance UpdateDiskOfferingCmd

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


   This PR adds the ability to update the following disk offering fields
   - cache-mode,
   - IOPS (Read/Write) Rate / IOPS (Read/Write) Rate Max / IOPS (Read/Write) Rate Max Length
     Bytes (Read/Write) Rate /
   - Bytes (Read/Write) Rate Max / Bytes (Read/Write) Rate Max Length
   
   ## Description
   <!--- Describe your changes in detail -->
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [x] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ## 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. -->
   
   1. Execute multiple API requests updating different combinations with the included (13) parameters.
   2. Assert that all parameters got updated on DB accordingly.
   3. Ensure that IOPS/Bytes validations are properly done, e.g. expected Exceptions such as `Bytes Write rate (5000) cannot be greater than Bytes Write maximum rate (3000)`.
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
   


----------------------------------------------------------------
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 #4409: Enhance UpdateDiskOfferingCmd

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


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2263


----------------------------------------------------------------
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 #4409: Enhance UpdateDiskOfferingCmd

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


   @DaanHoogland 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] blueorangutan commented on pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @GabrielBrascher 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] DaanHoogland commented on pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @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 pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @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 pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @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] DaanHoogland commented on pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @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] GabrielBrascher commented on a change in pull request #4409: Enhance UpdateDiskOfferingCmd

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



##########
File path: server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java
##########
@@ -946,17 +946,32 @@ public void validateMaximumIopsAndBytesLengthTestDefaultLengthConfigs() {
 
     @Test
     public void shouldUpdateDiskOfferingTests(){
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), Mockito.anyString(), Mockito.anyInt(), Mockito.anyBoolean(), Mockito.anyString()));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), Mockito.anyString(), nullable(Integer.class), nullable(Boolean.class), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), Mockito.anyInt(), nullable(Boolean.class), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), Mockito.anyBoolean(), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), nullable(Boolean.class), Mockito.anyString()));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), Mockito.anyString(), Mockito.anyInt(), Mockito.anyBoolean(), Mockito.anyString(), Mockito.anyString()));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), Mockito.anyString(), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), Mockito.anyInt(), nullable(Boolean.class), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), Mockito.anyBoolean(), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), nullable(Boolean.class), Mockito.anyString(), Mockito.anyString()));
     }
 
     @Test
     public void shouldUpdateDiskOfferingTestFalse(){
-        Assert.assertFalse(configurationMgr.shouldUpdateDiskOffering(null, null, null, null, null));
+        Assert.assertFalse(configurationMgr.shouldUpdateDiskOffering(null, null, null, null, null, null));
+    }
+
+    @Test
+    public void shouldUpdateIopsRateParametersTestFalse() {
+        configurationMgr.shouldUpdateIopsRateParameters(null, null, null, null, null, null);

Review comment:
       Thanks, updated!




----------------------------------------------------------------
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] DaanHoogland commented on pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @GabrielBrascher conflict arose, can you check?


----------------------------------------------------------------
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 #4409: Enhance UpdateDiskOfferingCmd

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


   @GabrielBrascher 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] DaanHoogland commented on pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @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 #4409: Enhance UpdateDiskOfferingCmd

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


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2199


----------------------------------------------------------------
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 #4409: Enhance UpdateDiskOfferingCmd

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


   <b>Trillian test result (tid-3048)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37795 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4409-t3048-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 84 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 314.38 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 300.74 | test_vpc_redundant.py
   


----------------------------------------------------------------
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 #4409: Enhance UpdateDiskOfferingCmd

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


   @DaanHoogland 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] DaanHoogland removed a comment on pull request #4409: Enhance UpdateDiskOfferingCmd

Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4409:
URL: https://github.com/apache/cloudstack/pull/4409#issuecomment-711728924


   @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 removed a comment on pull request #4409: Enhance UpdateDiskOfferingCmd

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4409:
URL: https://github.com/apache/cloudstack/pull/4409#issuecomment-711731440


   @DaanHoogland 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] DaanHoogland commented on pull request #4409: Enhance UpdateDiskOfferingCmd

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


   @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] GabrielBrascher commented on pull request #4409: Enhance UpdateDiskOfferingCmd

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


   Fixed conflicts, going to re-run a few manual tests before moving back to ready to review.
   
   @RodrigoDLopez can you please take a look at this PR? The conflicts were caused by a PR of yours; to fix the conflicts I did change slightly `shouldUpdateDiskOffering` adding cache mode. Nothing that would impact on tags updat process; however, I would appreciate some good eyes to check if all is good to go ;)


----------------------------------------------------------------
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] DaanHoogland merged pull request #4409: Enhance UpdateDiskOfferingCmd

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


   


----------------------------------------------------------------
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 #4409: Enhance UpdateDiskOfferingCmd

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


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2194


----------------------------------------------------------------
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 #4409: Enhance UpdateDiskOfferingCmd

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


   @DaanHoogland 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] RodrigoDLopez commented on a change in pull request #4409: Enhance UpdateDiskOfferingCmd

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



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3273,8 +3280,17 @@ protected void updateDiskOfferingTagsIfIsNotNull(String tags, DiskOfferingVO dis
      * Check if it needs to update any parameter when updateDiskoffering is called
      * Verify if name or displayText are not blank, tags is not null, sortkey and displayDiskOffering is not null
      */
-    protected boolean shouldUpdateDiskOffering(String name, String displayText, Integer sortKey, Boolean displayDiskOffering, String tags) {
-        return StringUtils.isNotBlank(name) || StringUtils.isNotBlank(displayText) || tags != null || sortKey != null || displayDiskOffering != null;
+    protected boolean shouldUpdateDiskOffering(String name, String displayText, Integer sortKey, Boolean displayDiskOffering, String tags, String cacheMode) {

Review comment:
       those change will not impact into edit diskoffering tags

##########
File path: server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java
##########
@@ -946,17 +946,32 @@ public void validateMaximumIopsAndBytesLengthTestDefaultLengthConfigs() {
 
     @Test
     public void shouldUpdateDiskOfferingTests(){
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), Mockito.anyString(), Mockito.anyInt(), Mockito.anyBoolean(), Mockito.anyString()));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), Mockito.anyString(), nullable(Integer.class), nullable(Boolean.class), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), Mockito.anyInt(), nullable(Boolean.class), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), Mockito.anyBoolean(), nullable(String.class)));
-        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), nullable(Boolean.class), Mockito.anyString()));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), Mockito.anyString(), Mockito.anyInt(), Mockito.anyBoolean(), Mockito.anyString(), Mockito.anyString()));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), Mockito.anyString(), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), Mockito.anyInt(), nullable(Boolean.class), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), Mockito.anyBoolean(), nullable(String.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), nullable(Boolean.class), Mockito.anyString(), Mockito.anyString()));
     }
 
     @Test
     public void shouldUpdateDiskOfferingTestFalse(){
-        Assert.assertFalse(configurationMgr.shouldUpdateDiskOffering(null, null, null, null, null));
+        Assert.assertFalse(configurationMgr.shouldUpdateDiskOffering(null, null, null, null, null, null));
+    }
+
+    @Test
+    public void shouldUpdateIopsRateParametersTestFalse() {
+        configurationMgr.shouldUpdateIopsRateParameters(null, null, null, null, null, null);

Review comment:
       I believe your test is incorrect.
   You need an assertion 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