Re-enable IP address usage hiding #4327

olivierlemasle · 2020-09-14T16:11:14Z

Description

With this commit, listUsageRecords filters out usages made on IP addresses on networks with "Hide IP Address Usage". This was previously introduced in #3235.

This requires a DB schema change, to introduce a flag is_hidden in table cloud_usage.cloud_usage.

Cf. #4193

Types of changes

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)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

How Has This Been Tested?

Multiple IP addresses have been created.

Those on shared networks without "Hide IP Address Usage" generate usages normally;
Those on shared networks with "Hide IP Address Usage" generate usages with flag is_hidden, and are not returned by the API.

Pearl1594 · 2020-09-15T04:50:32Z

@blueorangutan package

blueorangutan · 2020-09-15T04:51:16Z

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

blueorangutan · 2020-09-15T05:35:00Z

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

DaanHoogland · 2020-09-21T07:27:21Z

@blueorangutan test

blueorangutan · 2020-09-21T07:28:06Z

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

olivierlemasle · 2020-09-24T16:02:25Z

Kindly ping @rhtyd

DaanHoogland · 2020-09-25T07:47:50Z

@blueorangutan package

blueorangutan · 2020-09-25T07:48:23Z

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

blueorangutan · 2020-09-25T08:19:12Z

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

DaanHoogland · 2020-09-25T08:37:08Z

@olivierlemasle Do you have a smoke or integration test for this? it would help guard the feature in the future.
@blueorangutan test

blueorangutan · 2020-09-25T08:38:23Z

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

olivierlemasle · 2020-09-28T16:06:43Z

@DaanHoogland I must say I do not know CloudStack smoke tests well; are there any smoke tests on usages data in CloudStack currently, that I could use as a basis?

DaanHoogland · 2020-09-28T16:40:21Z

look in the test/integration/ subdirectory. I imagine there are marvin tests for related functionality, if you want to go that way. there is https://github.com/apache/cloudstack/blob/master/test/integration/smoke/test_usage.py and I hav e to admit I don't know how relevant it is for your case.

svenvogel · 2020-10-20T09:32:48Z

@olivierlemasle which milestone version should it merged? we can set it to 4.16?

DaanHoogland · 2020-10-20T09:44:50Z

@svenvogel as it is marked a bug I wouldimagine 4.15 or before to be appropriate. unless we decide it is too risky and not major..?

olivierlemasle · 2020-10-21T14:52:39Z

👍 for 4.15! Without it, IP addresses will generate usages on networks flagged "Hide IP address usages"...

DaanHoogland · 2020-10-21T17:12:47Z

@blueorangutan package

blueorangutan · 2020-10-21T17:14:01Z

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

blueorangutan · 2020-10-21T17:55:57Z

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

DaanHoogland · 2020-10-22T09:36:48Z

@blueorangutan test

olivierlemasle · 2020-10-30T10:54:50Z

@DaanHoogland @svenvogel I just rebased on master to fix the conflict in schema-41400to41500.sql.

DaanHoogland · 2020-10-30T11:02:17Z

@blueorangutan package

blueorangutan · 2020-10-30T11:03:44Z

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

blueorangutan · 2020-10-30T11:36:08Z

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

DaanHoogland · 2020-10-30T12:59:33Z

@blueorangutan test

blueorangutan · 2020-10-30T13:00:39Z

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

Pearl1594

Code LGTM

DaanHoogland · 2020-10-30T13:09:20Z

@andrijapanicsb what do you think about this?

andrijapanicsb · 2020-10-30T13:13:14Z

what is being different with this vs the original PR?

olivierlemasle · 2020-10-30T14:32:05Z

@andrijapanicsb
In #3235, usages were filtered out on the fly by the ApiResponseHelper, each time a listUsageRecords result was returned. It means that that filter ran only for the requested page of usages.

However, like all listXxx requests, listUsageRecords's response has a count field which is equal to the total number of usages (all pages included). It then caused the regession #4191, which was fixed in #4193 by reverting the filtering.

To filter the "hidden" IP address usages with a correct count, it is necessary to know how many usages were filtered out in the other pages. This PR moves the filter from the API layer to the usage server, to get the filter right without damaging performances.

NB: Since today's rebase, a timeout in smoke tests cause Travis build to fail.

blueorangutan · 2020-10-30T22:38:40Z

Trillian test result (tid-3110)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32514 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4327-t3110-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland

one little question, i don't like null returns. otherwise good.

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

With this commit, listUsageRecords filters out usages made on IP addresses on networks with "Hide IP Address Usage". This was previously introduced in apache#3235. This requires a DB schema change, to introduce a flag is_hidden in table cloud_usage.cloud_usage.

olivierlemasle · 2020-11-06T10:47:45Z

@DaanHoogland I've just updated the PR after your comment.

DaanHoogland · 2020-11-06T10:59:19Z

@blueorangutan package

blueorangutan · 2020-11-06T11:00:10Z

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

Pearl1594 · 2020-11-06T11:01:49Z

Verified fix:

Created a VM in a shared n/w with hiding IP Usage enabled, and one in a shared network without hiding IP usage - 3 records were created for both networks, While in all there are 33 records, listusagerecords filters out listing ip usage for the n/w with hide ip usage enabled.

select count(*) from cloud_usage\G
*************************** 1. row ***************************
count(*): 33
1 row in set (0.00 sec)

list usagerecords startdate="2020-10-26 00:00:00" enddate="2020-11-02 00:00:00" 
{
  "count": 30,
  "usagerecord": [
    {....
    }
  ]
}

blueorangutan · 2020-11-06T11:58:06Z

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

DaanHoogland · 2020-11-06T15:14:33Z

@blueorangutan test

blueorangutan · 2020-11-06T15:15:16Z

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

blueorangutan · 2020-11-07T01:14:48Z

Trillian test result (tid-3133)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33882 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4327-t3133-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

olivierlemasle mentioned this pull request Sep 14, 2020

Fix usage record count #4193

Merged

5 tasks

olivierlemasle force-pushed the hide_ip_address_usages branch from 5874150 to a28dad5 Compare September 24, 2020 14:05

svenvogel added component:database type:bug labels Sep 28, 2020

DaanHoogland added this to the 4.15.0.0 milestone Oct 22, 2020

DaanHoogland requested review from GabrielBrascher and Pearl1594 October 22, 2020 09:36

olivierlemasle force-pushed the hide_ip_address_usages branch from a28dad5 to dfa2184 Compare October 30, 2020 10:52

Pearl1594 approved these changes Oct 30, 2020

View reviewed changes

DaanHoogland reviewed Nov 6, 2020

View reviewed changes

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java Outdated Show resolved Hide resolved

olivierlemasle force-pushed the hide_ip_address_usages branch from dfa2184 to db65fbd Compare November 6, 2020 10:45

DaanHoogland added the component:usage-server label Nov 7, 2020

DaanHoogland approved these changes Nov 7, 2020

View reviewed changes

DaanHoogland merged commit 5f8289f into apache:master Nov 7, 2020

olivierlemasle deleted the hide_ip_address_usages branch November 7, 2020 10:21

DaanHoogland added the Severity:Major label Nov 10, 2020

Re-enable IP address usage hiding #4327

Re-enable IP address usage hiding #4327

Uh oh!

Conversation

olivierlemasle commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

How Has This Been Tested?

Uh oh!

Pearl1594 commented Sep 15, 2020

Uh oh!

blueorangutan commented Sep 15, 2020

Uh oh!

blueorangutan commented Sep 15, 2020

Uh oh!

DaanHoogland commented Sep 21, 2020

Uh oh!

blueorangutan commented Sep 21, 2020

Uh oh!

olivierlemasle commented Sep 24, 2020

Uh oh!

DaanHoogland commented Sep 25, 2020

Uh oh!

blueorangutan commented Sep 25, 2020

Uh oh!

blueorangutan commented Sep 25, 2020

Uh oh!

DaanHoogland commented Sep 25, 2020

Uh oh!

blueorangutan commented Sep 25, 2020

Uh oh!

olivierlemasle commented Sep 28, 2020

Uh oh!

DaanHoogland commented Sep 28, 2020

Uh oh!

svenvogel commented Oct 20, 2020

Uh oh!

DaanHoogland commented Oct 20, 2020

Uh oh!

olivierlemasle commented Oct 21, 2020

Uh oh!

DaanHoogland commented Oct 21, 2020

Uh oh!

blueorangutan commented Oct 21, 2020

Uh oh!

blueorangutan commented Oct 21, 2020

Uh oh!

DaanHoogland commented Oct 22, 2020

Uh oh!

olivierlemasle commented Oct 30, 2020

Uh oh!

DaanHoogland commented Oct 30, 2020

Uh oh!

blueorangutan commented Oct 30, 2020

Uh oh!

blueorangutan commented Oct 30, 2020

Uh oh!

DaanHoogland commented Oct 30, 2020

Uh oh!

blueorangutan commented Oct 30, 2020

Uh oh!

Pearl1594 left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Oct 30, 2020

Uh oh!

andrijapanicsb commented Oct 30, 2020

Uh oh!

olivierlemasle commented Oct 30, 2020

Uh oh!

blueorangutan commented Oct 30, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olivierlemasle commented Nov 6, 2020

Uh oh!

DaanHoogland commented Nov 6, 2020

olivierlemasle commented Sep 14, 2020 •

edited

Loading