[CLOUDSTACK-9338] ACS is not accounting resources of VMs with custom service offering properly

[rafaelweingartner]

Description

ACS accounts the resources when deploying VMs with custom service offerings. However, there are other methods (such as updateResourceCount) that do not execute the resource accounting properly, and these methods update the resource count for an account in the database. Therefore, if a user deploys VMs with custom service offerings, and later this user calls the “updateResourceCount” method, it (the method) will only account for VMs with normal service offerings, and update this as the number of resources used by the account. This will result in a smaller number of resources to be accounted for the given account than the real used value. The problem becomes worse because if the user starts to delete these VMs, it is possible to reach negative values of resources allocated (breaking all of the resource limiting for accounts). This is a very serious attack vector for public cloud providers!

Steps to reproduce:

1. Create a new account and dedicate some resources to it
2. Instantiate a VM using a custom service offering
3. Check the CPU and memory resource count for the given VM (the count must be correct at this point): select * from resource_count where account_id = ?
4. Go to the details of the account and click on “Update resource count”
5. Execute the SQL again (select * from resource_count where account_id = ?), and check if the resource count is correct. The value of resource count will consider only VMs deployed with normal service offering;
6. Delete all VMs
7. Check the resource count table again. It will reach negative values.

[DaanHoogland]

@rafaelweingartner your code looks good at first sight. Are you writing automation tests for this? I think the PR merrits it

[rafaelweingartner]

I can write one if you think it is worth it. I have never created one though.

[bwsw]

@rafaelweingartner
Thank you commiting to this area of code. Proper accounting is very important, I commited in 4.11 regarding accounting too, but it seems there are a lot of possible bugs happen here.

Nb. Please keep in mind to check that domain resources are accounted correctly too. Also, I strongly believe resources should be updated with increment/decrement merhods, but updateResourceCount is only should be used to do periodic update for top level domains when a/b/c/account is used and we would like to update for b and a...

[DaanHoogland]

As @bwsw (wow a company involved) says, accounting is important and needs a lot of tlc. If we don't make regression tests and write fix over fix, regressions are bound to happen, @rafaelweingartner . So if you have the mental and clock space for it , please do.

[rafaelweingartner]

@DaanHoogland, @bwsw I have been trying to understand how to do this test case using that "test library" that we have in ACS. It feels a lot of copy and paste on those test "file/scripts" is that the way it is supposed to be?

Ins't there a straightforward way? I only need to execute a call to ACS, create a custom service offering, deploy a vm using this service offering, and then calling the resource count method and check its return.

I was expecting something clever, where I could simply configure my cloudstack api calls knowing that there will be a cloudstack setup for me. I only need to create a custom service offering, deploy a vm with it, and then call the resource count method and check its result.

[DaanHoogland]

well, there is a lot of integration test code that could be done with a bunch of mocks and a good unit test of course. I have no idea if that applies in this case, but maybe it's worth a try.

[rafaelweingartner]

Well, here is a case that I have not solved with my self yet. The logic is in the SQL. I know it works this way because I tested manually. I could write down a unit test case to see if the SQL is the one I designed to be, but this does not look much elegant to me.

I got marvin working, I am now developing the test case using it.

[ustcweizhou]

you can use UserVmJoinDao instead of SQL.

[rafaelweingartner]

@DaanHoogland, and @bwsw Finally I managed to understand how to create and use this thing to create the python test cases. It should be easier to learn and write these tests (there is also the possibility that I am not that smart)....

I created a new test case called "test_01_updateResourceCount".

==== Marvin Init Successful ====
===final results are now copied to: /tmp/MarvinLogs/test_updateResourceCount_M9C4VO===
Test update resource count for an account using a custom service offering ... === TestName: test_01_updateResourceCount | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 1 test in 1239.604s
OK

[DaanHoogland]

You are way smarter than me @rafaelweingartner , don't be begging for my sympathy.

[borisstoyanov]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1729

[borisstoyanov]

@blueorangutan test

[blueorangutan]

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

[borisstoyanov]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-2277)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28307 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2443-t2277-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 67 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[rafaelweingartner]

@bwsw are you +1 here?

[borisstoyanov]

LGTM

[yadvr]

@rafaelweingartner I scanned PRs merged on master but not 4.11 and came across this PR, since this is marked 'bug' do you think this would be useful for 4.11? If so, can you help create a backport PR for 4.11?

[rafaelweingartner]

I have not seen complains about this before. That is one of the reason why I target most of these things to master. It seems that we were the only ones having these needs. This one only happens if you use custom service offering.

[yadvr]

@rafaelweingartner yes, but any bug fixes are welcome on LTS branch. The issue such as this may affect users around usage/billing if cloudstack providers are offering custom offering to users.

[rafaelweingartner]

Sure. Would you like me do to the cherry-pick on 4.11 branch?

[yadvr]

@rafaelweingartner cherry-pick might fail, can you submit a PR to 4.11 branch or you can also push on my backport PR: #2621