You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by nvazquez <gi...@git.apache.org> on 2016/06/15 20:26:28 UTC
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
GitHub user nvazquez opened a pull request:
https://github.com/apache/cloudstack/pull/1593
CLOUDSTACK-9417: Usage module refactoring
### Introduction
Usage sanity check file was not been updated on sanity check.
It is proposed:
* New usage folder `/var/cache/cloudstack/usage,` creation on `cloudstack-usage` package built.
* New sanity check file location in new folder `/var/cache/cloudstack/usage`
* Timestamp included in `usage.log` file
* Include `updateMaxId()` on sanity check as it wasn't being updated
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nvazquez/cloudstack bothvmmapandpr1584
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1593.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1593
----
commit 388125d6a2454ee2bf79be62075a8e64c6496661
Author: nvazquez <ni...@gmail.com>
Date: 2016-06-15T20:22:11Z
CLOUDSTACK-9417: Add timestamp on usage.log file
commit 759889d344473d75dc1ff90e788c5c7e7106ec08
Author: nvazquez <ni...@gmail.com>
Date: 2016-06-15T20:23:03Z
CLOUDSTACK-9417: Usage module refactor
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r74807418
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -72,7 +72,9 @@ protected boolean checkItemCountByPstmt(CheckCase checkCase) throws SQLException
try (PreparedStatement pstmt = conn.prepareStatement(checkCase.sqlTemplate)) {
if(checkCase.checkId) {
pstmt.setInt(1, lastId);
- pstmt.setInt(2, maxId);
+ if (maxId > 0) {
+ pstmt.setInt(2, maxId);
--- End diff --
Hi @rafaelweingartner sorry I haven't seen you comment earlier, thanks for reviewing!
Actually the first parameter on line 74 belongs to the one set on line 216, and the one inside the `if` is only set if it was set on the query, on line 194.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@serg38 giving your explanations I am ok with your proposal. Just one addendum, I think we should document it as much as possible.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1593
Packaging result: \u2714centos6 \u2714centos7 \u2714debian repo: http://packages.shapeblue.com/cloudstack/pr/1593
Job ID #65
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
LGTM with smoke testing. RHEL 6 management servers, advanced networking, Vmware 5.5 .and 6 hypervisors
[root@ussarlabcsmgt41 smoke]# cat /tmp//MarvinLogs/test_volumes_340FH1/results.txt|grep -v ok
test DeployVM in anti-affinity groups for project ... === TestName: test_DeployVmAntiAffinityGroup_in_project | Status : SUCCESS ===
test DeployVM in anti-affinity groups ... === TestName: test_DeployVmAntiAffinityGroup | Status : SUCCESS ===
Test Deploy Virtual Machine ... SKIP: Skipping test because suitable hypervisor/host not present
Test Deploy Virtual Machine from ISO ... === TestName: test_deploy_vm_from_iso | Status : SUCCESS ===
Test deploy virtual machine with root resize ... === TestName: test_00_deploy_vm_root_resize | Status : SUCCESS ===
Test proper failure to deploy virtual machine with rootdisksize of 0 ... === TestName: test_01_deploy_vm_root_resize | Status : SUCCESS ===
Test proper failure to deploy virtual machine with rootdisksize less than template size ... === TestName: test_02_deploy_vm_root_resize | Status : SUCCESS ===
Test to deploy vm with a first fit offering ... === TestName: test_deployvm_firstfit | Status : SUCCESS ===
Test deploy VMs using user concentrated planner ... === TestName: test_deployvm_userconcentrated | Status : SUCCESS ===
Test deploy VMs using user dispersion planner ... === TestName: test_deployvm_userdispersing | Status : SUCCESS ===
Test userdata as GET, size > 2k ... === TestName: test_deployvm_userdata | Status : SUCCESS ===
Test userdata as POST, size > 2k ... === TestName: test_deployvm_userdata_post | Status : SUCCESS ===
Test to create disk offering ... === TestName: test_01_create_disk_offering | Status : SUCCESS ===
Test to create a sparse type disk offering ... === TestName: test_02_create_sparse_type_disk_offering | Status : SUCCESS ===
Test to create a sparse type disk offering ... === TestName: test_04_create_fat_type_disk_offering | Status : SUCCESS ===
Test to update existing disk offering ... === TestName: test_02_edit_disk_offering | Status : SUCCESS ===
Test to delete disk offering ... === TestName: test_03_delete_disk_offering | Status : SUCCESS ===
Test to ensure 4 default roles cannot be deleted ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Test to check role, role permissions and account life cycles ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Test for role-rule enforcement in case of multiple mgmt servers ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Test to ensure role in use cannot be deleted ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests normal lifecycle operations for roles ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests role update ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests that default four roles exist ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests role update ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests role update when role is in use by an account ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests concurrent order updation of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests creation of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests deletion of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests listing of default role's permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests order updation of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
test update configuration setting at zone level scope ... === TestName: test_UpdateConfigParamWithScope | Status : SUCCESS ===
Test guest vlan range dedication ... === TestName: test_dedicateGuestVlanRange | Status : SUCCESS ===
Test create public & private ISO ... === TestName: test_01_create_iso | Status : SUCCESS ===
Test Edit ISO ... === TestName: test_02_edit_iso | Status : SUCCESS ===
Test delete ISO ... === TestName: test_03_delete_iso | Status : SUCCESS ===
Test for extract ISO ... === TestName: test_04_extract_Iso | Status : SUCCESS ===
Update & Test for ISO permissions ... === TestName: test_05_iso_permissions | Status : SUCCESS ===
Test for copy ISO from one zone to another ... SKIP: Not enough zones available to perform copy template
Test delete ISO ... === TestName: test_07_list_default_iso | Status : SUCCESS ===
Test listing Volumes using 'ids' parameter ... === TestName: test_01_list_volumes | Status : SUCCESS ===
Test listing Templates using 'ids' parameter ... === TestName: test_02_list_templates | Status : SUCCESS ===
Test listing Snapshots using 'ids' parameter ... === TestName: test_03_list_snapshots | Status : SUCCESS ===
Test to create Load balancing rule with source NAT ... === TestName: test_01_create_lb_rule_src_nat | Status : SUCCESS ===
Test to create Load balancing rule with non source NAT ... === TestName: test_02_create_lb_rule_non_nat | Status : SUCCESS ===
Test for assign & removing load balancing rule ... === TestName: test_assign_and_removal_lb | Status : SUCCESS ===
Tests that SAML users are not allowed CloudStack local log in ... === TestName: login_test_saml_user | Status : SUCCESS ===
Test network ACL lists and items in VPC ... === TestName: test_network_acl | Status : SUCCESS ===
# 1. Register a template for VMware with nicAdapter vmxnet3 ... SKIP: VCenter API Integration Remaining
Test to add and update added nic to a virtual machine ... === TestName: test_01_nic | Status : SUCCESS ===
Test to update a physical network and extend its vlan ... === TestName: test_extendPhysicalNetworkVlan | Status : SUCCESS ===
test update configuration setting at storage scope ... === TestName: test_UpdateStorageOverProvisioningFactor | Status : SUCCESS ===
Test for create region ... === TestName: test_createRegion | Status : SUCCESS ===
Test reset virtual machine on reboot ... === TestName: test_01_reset_vm_on_reboot | Status : SUCCESS ===
Test volume detail ... === TestName: test_01_updatevolumedetail | Status : SUCCESS ===
Test scale virtual machine ... SKIP: Skipping scale VM operation because VMware tools are not installed on the VM
Test to create service offering ... === TestName: test_01_create_service_offering | Status : SUCCESS ===
Test to update existing service offering ... === TestName: test_02_edit_service_offering | Status : SUCCESS ===
Test to delete service offering ... === TestName: test_03_delete_service_offering | Status : SUCCESS ===
Test to change service to a small capacity ... === TestName: test_04_change_offering_small | Status : SUCCESS ===
Test List secondary storage VMs ... === TestName: test_01_list_sec_storage_vm | Status : SUCCESS ===
Test List console proxy VMs ... === TestName: test_02_list_cpvm_vm | Status : SUCCESS ===
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : SUCCESS ===
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : SUCCESS ===
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS ===
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS ===
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS ===
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS ===
Tests allowed APIs for common account types ... === TestName: test_static_role_account_acls | Status : SUCCESS ===
Test create public & private template ... === TestName: test_01_create_template | Status : SUCCESS ===
Test when createTemplate is used to create templates having the same name all of them get ... === TestName: test_CreateTemplateWithDuplicateName | Status : SUCCESS ===
Test Edit template ... === TestName: test_02_edit_template | Status : SUCCESS ===
Test delete template ... === TestName: test_03_delete_template | Status : SUCCESS ===
Test for extract template ... === TestName: test_04_extract_template | Status : SUCCESS ===
Update & Test for template permissions ... === TestName: test_05_template_permissions | Status : SUCCESS ===
Test for copy template from one zone to another ... SKIP: Not enough zones available to perform copy template
Test only public templates are visible to normal user ... === TestName: test_07_list_public_templates | Status : SUCCESS ===
Test System templates are not visible to normal user ... === TestName: test_08_list_system_templates | Status : SUCCESS ===
Check events in usage_events table when VM creation fails ... === TestName: test_01_positive_tests_usage | Status : SUCCESS ===
Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
Tests for basic zone virtual router ... === TestName: test_basicZoneVirtualRouter | Status : SUCCESS ===
Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
Test for attach and detach ISO to virtual machine ... === TestName: test_10_attachAndDetach_iso | Status : SUCCESS ===
Test Volume creation for all Disk Offerings (incl. custom) ... === TestName: test_01_create_volume | Status : SUCCESS ===
Attach a created Volume to a Running VM ... === TestName: test_02_attach_volume | Status : SUCCESS ===
Download a Volume attached to a VM ... === TestName: test_03_download_attached_volume | Status : SUCCESS ===
Delete a Volume attached to a VM ... === TestName: test_04_delete_attached_volume | Status : SUCCESS ===
Detach a Volume attached to a VM ... === TestName: test_05_detach_volume | Status : SUCCESS ===
Download a Volume unattached to an VM ... === TestName: test_06_download_detached_volume | Status : SUCCESS ===
Test resize (negative) non-existent volume ... SKIP: Resize Volume is unsupported on VmWare and Hyper-V
Test resize a volume ... SKIP: Resize Volume is unsupported on VmWare and Hyper-V
Delete a Volume unattached to an VM ... === TestName: test_09_delete_detached_volume | Status : SUCCESS ===
----------------------------------------------------------------------
## **Ran 104 tests in 12377.260s**
## **OK (SKIP=21)**
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@serg38 since we don't have a general key/value store table, we may need to create a new table in `cloud_usage` db.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1593
Thanks @jburwell for your review!
About your first question, as @serg38 said file will be created on sanity check run if it didn't exist, and then it would just update it.
About your second question, it would be nice that it can be included on 4.9 but I think it will be deferred to 4.10. I just targeted it to 4.9 as it was current version without realizing about 4.9 freeze. Actually I tried updating JIRA ticket to 4.10 but it doesn't exist. Should I leave version fields blank?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r76731031
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -43,9 +43,9 @@
protected static final int DEFAULT_AGGREGATION_RANGE = 1440;
protected StringBuilder errors;
protected List<CheckCase> checkCases;
- protected String lastCheckFile = "/usr/local/libexec/sanity-check-last-id";
+ protected String lastCheckFile = "/var/cache/cloudstack/usage/sanity-check-last-id";
--- End diff --
Just a thought, why we store this in a file (on one of the mgmt/usage server hosts) and not in database?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rafaelweingartner @rhtyd Do we make any decision on this? The scope of this PR was very limited initially. It would be much easier if we don't expand it too far.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@serg38 sorry the `test` keyword is restricted to RMs and few other people for now. There are failure that causes initial setup to fail. Can you confirm if this (mgmt server and usage server) work in your env with the above packages?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rhtyd a Trillian-Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r67563826
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -72,7 +72,9 @@ protected boolean checkItemCountByPstmt(CheckCase checkCase) throws SQLException
try (PreparedStatement pstmt = conn.prepareStatement(checkCase.sqlTemplate)) {
if(checkCase.checkId) {
pstmt.setInt(1, lastId);
- pstmt.setInt(2, maxId);
+ if (maxId > 0) {
+ pstmt.setInt(2, maxId);
+ }
--- End diff --
This ``if`` creates a circumstance where the second parameter of the ``PreparedStatement`` will not be set. In this circumstance, an exception will be thrown. Therefore, this code either always set the value or throw an ``IllegalStateException`` explaining the condition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r74966728
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -72,7 +72,9 @@ protected boolean checkItemCountByPstmt(CheckCase checkCase) throws SQLException
try (PreparedStatement pstmt = conn.prepareStatement(checkCase.sqlTemplate)) {
if(checkCase.checkId) {
pstmt.setInt(1, lastId);
- pstmt.setInt(2, maxId);
+ if (maxId > 0) {
+ pstmt.setInt(2, maxId);
--- End diff --
Thanks @rafaelweingartner!
Absolutely, now I notice there was also a similar [comment](https://github.com/apache/cloudstack/pull/1593/files#r67563826) by @jburwell about this, I should have explained it a little further on this PR description as it can be confusing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@jburwell LGTM for the test part. This PR was extensively tested on
Environment: RHEL 6 management servers, Vmware ESX5.5 and 6.0 with advanced networking
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r67938119
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -72,7 +72,9 @@ protected boolean checkItemCountByPstmt(CheckCase checkCase) throws SQLException
try (PreparedStatement pstmt = conn.prepareStatement(checkCase.sqlTemplate)) {
if(checkCase.checkId) {
pstmt.setInt(1, lastId);
- pstmt.setInt(2, maxId);
+ if (maxId > 0) {
+ pstmt.setInt(2, maxId);
+ }
--- End diff --
Hi @jburwell, actually when `maxId = -1` parameter is not set to prepared statement, it is only added when it is found in `readMaxId()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r67565090
--- Diff: usage/conf/log4j-cloud_usage.xml.in ---
@@ -48,7 +48,7 @@ under the License.
</rollingPolicy>
<layout class="org.apache.log4j.EnhancedPatternLayout">
- <param name="ConversionPattern" value="%-5p [%c{3}] (%t:%x) (logid:%X{logcontextid}) %m%n"/>
+ <param name="ConversionPattern" value="%d{ISO8601} %-5p [%c{3}] (%t:%x) (logid:%X{logcontextid}) %m%n"/>
--- End diff --
Why is this change to the logging format being made? Existing users may have log parsing scripts that may be impacted.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@serg38 I completely agree that it should be in the database, but it's not a configuration setting. Furthermore, it requires a different transactional behavior to operate properly. Therefore, it should be placed in a separate tracking table -- either a new table or as a column on an existing table.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1593
Thanks @wido! I refactored debian packaging and added fedora21's `cloud.spec` file
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r76730939
--- Diff: debian/cloudstack-usage.postinst ---
@@ -43,6 +43,10 @@ case "$1" in
rm -rf /etc/cloudstack/usage/key
ln -s /etc/cloudstack/management/key /etc/cloudstack/usage/key
fi
+
+ # Usage cache folder permissions
+ chmod 0770 /var/cache/cloudstack/usage
+ chgrp cloud /var/cache/cloudstack/usage
--- End diff --
Maybe we can use `debian/cloudstack-usage.dirs` for declaring/making directories? /cc @wido
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1593
Packaging result: \u2716centos6 \u2714centos7 \u2714debian repo: http://packages.shapeblue.com/cloudstack/pr/1593
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@nvazquez when users upgrade existing usage servers, will the location change of the ```sanity-check-last-id``` file impact their operation?
/cc @abhinandanprateek
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rhtyd Confirming. No issues on usage and management servers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1593
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-211
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@nvazquez could you please squash your commits?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/1593
### ACS CI BVT Run
**Sumarry:**
Build Number 412
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=1
Skipped=7
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_non_contigiousvlan.py
* test_extendPhysicalNetworkVlan Failed
**Skipped tests:**
test_01_test_vm_volume_snapshot
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_login.py
test_deploy_vm_iso.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_routers_network_ops.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@jburwell @rhtyd What are your ideas on where in DB to store it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@jburwell Creating a new table to hold one setting seems to be excessive. How about using sequence table? In this regard last_Id used by usage sanity checker is a sequence.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1593
I understand why you want to use something that already exists, instead of creating a new table/DAO/service class.
Isn\u2019t this table (event_details) used for anything else?
If it is something that is already out there and is not being used, I have nothing against using it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1593
Packaging changes seem good to me. Java changes as well.
Based on the code this is LGTM to me
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@blueorangutan test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rhtyd @jburwell Can this be merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rhtyd @jburwell @rafaelweingartner I agree with @rhtyd to move sanity checker last_id to the DB. No reason to keep in in file system. How about using
"usage.sanity.check.lastid in configuration table with the following attributes
Category: Hidden
component : management-server
default value: 0
isdynamic: 0
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@abhinandanprateek do you have time to test this PR when upgrading a a clustered usage server environment from 4.9 to master?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r74468365
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -72,7 +72,9 @@ protected boolean checkItemCountByPstmt(CheckCase checkCase) throws SQLException
try (PreparedStatement pstmt = conn.prepareStatement(checkCase.sqlTemplate)) {
if(checkCase.checkId) {
pstmt.setInt(1, lastId);
- pstmt.setInt(2, maxId);
+ if (maxId > 0) {
+ pstmt.setInt(2, maxId);
--- End diff --
Is this set the setting of the parameter for the SQL excerpt at line 216?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rafaelweingartner In usage_evant_details there will be never details with event_id=0 so there will be no conflict ever if we do it this way. Based on the code there are some details e.g. CPU speed that might be saved to details table at some point but they are always tied to respective event_id which is greater then 0 for normal events. Event id=0 can be considered system and generated by Sanity job.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@jburwell Sure, they are squashed now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@serg38 I have this PR on a list to re-review. In my view, using a global setting for this value is whole inappropriate. I have been busy with other items, and haven't had a chance to get back around to it. I plan to get it to it in the next day or two.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@karuturi a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests against packages at http://packages.shapeblue.com/cloudstack/pr/1593
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@blueorangutan test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@karuturi @jburwell There are 2 LGTM in this PR. Packaging, Travis and Jenkins all passed. Can this be merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@serg38 we require Marvin test results and, as well as, the results of any manual tests to be posted into the PR. This information is a pre-requisite for merging PRs. Please see other closed PRs for examples of test result reporting.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@swill I noticed that the associated JIRA ticket was opened after the 4.9.0 freeze, but is targeted against 4.9.0. Should it be included in 4.9 or deferred to 4.10?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r75795653
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -72,7 +72,9 @@ protected boolean checkItemCountByPstmt(CheckCase checkCase) throws SQLException
try (PreparedStatement pstmt = conn.prepareStatement(checkCase.sqlTemplate)) {
if(checkCase.checkId) {
pstmt.setInt(1, lastId);
- pstmt.setInt(2, maxId);
+ if (maxId > 0) {
+ pstmt.setInt(2, maxId);
+ }
--- End diff --
@nvazquez I understand now. When ``maxId <= 0`` a different prepared statement will be used which has only one placeholder. A bit convoluted, but you are just working with the existing complexity.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@serg38 I only see code review LGTMs. We need at least one test LGTM before this PR. In this case, those tests must be run against real hardware and hypervisors. Also, we need an LGTM from all reviewers that raised an issue. I haven't had a chance tore-review as I am on leave this week. (All PRs require at least one code review and at least test LGTM to be merged).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@jburwell If no sanity-check-last-id is present it will run Sanity on the whole cloud_usage data set and then create a new sanity-check-last-id file. The whole part of sanity checking related to sanity-check-last-id has been broken for a while since the part to update sanity-check-last-id wasn't in the code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@nvazquez @jburwell @serg38 @rafaelweingartner ping
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r76730973
--- Diff: usage/conf/log4j-cloud_usage.xml.in ---
@@ -48,7 +48,7 @@ under the License.
</rollingPolicy>
<layout class="org.apache.log4j.EnhancedPatternLayout">
- <param name="ConversionPattern" value="%-5p [%c{3}] (%t:%x) (logid:%X{logcontextid}) %m%n"/>
+ <param name="ConversionPattern" value="%d{ISO8601} %-5p [%c{3}] (%t:%x) (logid:%X{logcontextid}) %m%n"/>
--- End diff --
Much needed, usage logs never had timestamps.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r74957023
--- Diff: usage/src/com/cloud/usage/UsageSanityChecker.java ---
@@ -72,7 +72,9 @@ protected boolean checkItemCountByPstmt(CheckCase checkCase) throws SQLException
try (PreparedStatement pstmt = conn.prepareStatement(checkCase.sqlTemplate)) {
if(checkCase.checkId) {
pstmt.setInt(1, lastId);
- pstmt.setInt(2, maxId);
+ if (maxId > 0) {
+ pstmt.setInt(2, maxId);
--- End diff --
@nvazquez thanks for the explanation. Now I understood it a little bit more. These SLQs that are built using if(s)/else(s) and use few methods are a bit hard to follow sometimes without debugging it in runtime.
This was my only doubt. So, LGTM here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1593#discussion_r67618240
--- Diff: usage/conf/log4j-cloud_usage.xml.in ---
@@ -48,7 +48,7 @@ under the License.
</rollingPolicy>
<layout class="org.apache.log4j.EnhancedPatternLayout">
- <param name="ConversionPattern" value="%-5p [%c{3}] (%t:%x) (logid:%X{logcontextid}) %m%n"/>
+ <param name="ConversionPattern" value="%d{ISO8601} %-5p [%c{3}] (%t:%x) (logid:%X{logcontextid}) %m%n"/>
--- End diff --
@jburwell. RPM Conflict resolution will create .rpmnew log4j config (at least in case of RHEL/Centos). Existing installations will not be affected and if needed the change has to be manually merged to active log4j config.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@rhtyd @jburwell @rafaelweingartner Can we use event_details table which is not used at the moment? if we consider sanity job to generate event_id=0 then max_id can be easily represented there as ("id","0","max_id","value")
Alternatively a new table would require new DAO isn't it? Seems to be huge scope creep for such a minor fix as permission issue. Can we just merge it as it is and open another PR for the extended scope?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1593: CLOUDSTACK-9417: Usage module refactoring
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1593
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---