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/03/04 14:35:05 UTC

[GitHub] [cloudstack] GabrielBrascher opened a new pull request #3932: Prevent overflow and null pointer on StatsCollector.java

GabrielBrascher opened a new pull request #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932
 
 
   ## Description
   <!--- Describe your changes in detail -->
   Did a quick check and found some code issues that can result in overflow or null pointer exceptions. Additionally, I took the liberty to do a couple of code enhancements.
   
   ## 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)
   - [X] 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)

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


With regards,
Apache Git Services

[GitHub] [cloudstack] DaanHoogland commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596489281
 
 
   ok, can you rebase @GabrielBrascher ?

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


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-598865670
 
 
   2 x LGTMs, all tests passing (those privategtw tests are "OK", fixed in newer ACS code - this PR is 150+ commits behind master)
   
   Merging.

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


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596990921
 
 
   Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1037

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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#discussion_r387720059
 
 

 ##########
 File path: server/src/main/java/com/cloud/server/StatsCollector.java
 ##########
 @@ -1533,7 +1536,7 @@ protected void writeBatches(InfluxDB influxDbConnection, String dbName, List<Poi
      * The considered disk stats are the following: bytes read, bytes write, IO read,  and IO write.
      */
     protected boolean isCurrentVmDiskStatsDifferentFromPrevious(VmDiskStatisticsVO previousVmDiskStats, VmDiskStatisticsVO currentVmDiskStats) {
-        if (previousVmDiskStats != null) {
 
 Review comment:
   currentVmDiskStats might be null as well.

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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596485689
 
 
   @DaanHoogland I see no problem in applying this at 4.13.1.

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


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596991370
 
 
   @borisstoyanov 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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596504281
 
 
   @DaanHoogland  rebased it to 4.13 :slightly_smiling_face: 

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


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596982525
 
 
   @borisstoyanov 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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-594624173
 
 
   @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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596484976
 
 
   @borisstoyanov the overflow is not something that will happen easily; however, the code is there and it still something that might happen one day depending on the value stored at `_usageAggregationRange`.
   
   The operation that I extracted can result in an overflow as it is an **integer** operation stored in an **integer** _memory_ (and then after storing it as an integer it is cast into a long). I extracted it into a `long = int * Long`, which prevents a overflow as it is not an integer operation anymore but converted into a long operation.

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


With regards,
Apache Git Services

[GitHub] [cloudstack] borisstoyanov commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

borisstoyanov commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-596982269
 
 
   @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


With regards,
Apache Git Services

[GitHub] [cloudstack] ustcweizhou commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

ustcweizhou commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#discussion_r388762987
 
 

 ##########
 File path: server/src/main/java/com/cloud/server/StatsCollector.java
 ##########
 @@ -1533,7 +1536,7 @@ protected void writeBatches(InfluxDB influxDbConnection, String dbName, List<Poi
      * The considered disk stats are the following: bytes read, bytes write, IO read,  and IO write.
      */
     protected boolean isCurrentVmDiskStatsDifferentFromPrevious(VmDiskStatisticsVO previousVmDiskStats, VmDiskStatisticsVO currentVmDiskStats) {
-        if (previousVmDiskStats != null) {
 
 Review comment:
   @GabrielBrascher it looks currentVmDiskStats is not null
   
   
   https://github.com/apache/cloudstack/blob/9a1486fdebc3d4cfd17d581ec1b66fbd9c3a4bcd/server/src/main/java/com/cloud/server/StatsCollector.java#L735-L746

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


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-595277100
 
 
   <b>Trillian test result (tid-1191)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 28797 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3932-t1191-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
   Smoke tests completed. 81 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


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-594624627
 
 
   @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


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-597270185
 
 
   <b>Trillian test result (tid-1225)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 33837 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3932-t1225-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Smoke tests completed. 76 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_02_vpc_privategw_static_routes | `Failure` | 219.74 | test_privategw_acl.py
   test_03_vpc_privategw_restart_vpc_cleanup | `Failure` | 210.20 | test_privategw_acl.py
   test_04_rvpc_privategw_static_routes | `Failure` | 942.54 | test_privategw_acl.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


With regards,
Apache Git Services

[GitHub] [cloudstack] rhtyd commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

rhtyd commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-595055726
 
 
   @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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#discussion_r387705945
 
 

 ##########
 File path: server/src/main/java/com/cloud/server/StatsCollector.java
 ##########
 @@ -454,8 +456,9 @@ protected void init(Map<String, String> configs) {
             s_logger.warn("Usage stats job aggregation range is to small, using the minimum value of " + UsageUtils.USAGE_AGGREGATION_RANGE_MIN);
             _usageAggregationRange = UsageUtils.USAGE_AGGREGATION_RANGE_MIN;
         }
-        _diskStatsUpdateExecutor.scheduleAtFixedRate(new VmDiskStatsUpdaterTask(), (endDate - System.currentTimeMillis()), (_usageAggregationRange * 60 * 1000),
 
 Review comment:
   Potential overflow in 2 Values before it is converted to long by use in an invocation context.

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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on a change in pull request #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#discussion_r387706608
 
 

 ##########
 File path: server/src/main/java/com/cloud/server/StatsCollector.java
 ##########
 @@ -1209,7 +1212,7 @@ private String getAutoscaleAction(HashMap<Long, Double> avgCounter, long groupId
             for (AutoScaleVmGroupPolicyMapVO asVmgPmap : listMap) {
                 AutoScalePolicyVO policyVO = _asPolicyDao.findById(asVmgPmap.getPolicyId());
                 if (policyVO != null) {
-                    Integer quitetime = policyVO.getQuietTime();
 
 Review comment:
   The variable 'quitetime' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Integer'.

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


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-594642192
 
 
   Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1000

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


With regards,
Apache Git Services

[GitHub] [cloudstack] GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

GabrielBrascher commented on issue #3932: Prevent overflow on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932#issuecomment-595767768
 
 
   Thanks for all the reviews!
   
   Just to mention. I did a few manual tests to ensure that no overflow will happen. Setting ONE_MINUTE_IN_MILLISCONDS to Long (or long) will indeed solve overflow issues.

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


With regards,
Apache Git Services

[GitHub] [cloudstack] DaanHoogland closed pull request #3932: Prevent overflow and null pointer on StatsCollector.java

Posted by GitBox <gi...@apache.org>.

DaanHoogland closed pull request #3932: Prevent overflow and null pointer on StatsCollector.java
URL: https://github.com/apache/cloudstack/pull/3932
 
 
   

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


With regards,
Apache Git Services