Fixes regarding VOLUME_DELETE events resulting from account deletion. by ProjectMoon · Pull Request #1491 · apache/cloudstack

ProjectMoon · 2016-04-13T13:13:21Z

New version of #1373, but updated for the 4.7 branch with another fix that allows it to properly find expunged root volumes. This is a bug fix, which is why we target the 4.7 branch.

Original pull request:
Fixes regarding usage event emission.

UsageEventUtils was previously not checking deleted accounts, which meant that if an account was deleted that had some resources running on it, those resources would get destroyed without emitting any events.

Furthermore, the VOLUME_DELETE event of ROOT volumes is the responsibility of the UserVmManager, which gets circumvented when expunging resources following the account deletion. Added a check to the AccountManager which catches the ROOT volumes that need to be deleted and emits events for them.

To test this: Create a new user. As that user, create and destroy an instance. This should cause the VM_CREATE, VM_START, VM_STOP, VM_DESTROY, VOLUME_CREATE, and VOLUME_DELETE events to be emitted.
Create a new instance as the same user. Log in as admin, and delete the user. The same set of events should be emitted, and there should be no duplicate DELETE events for the ROOT volume of the previous instance.

pdube · 2016-04-13T23:36:44Z

 s_logger.error("Unable to expunge vm: " + vm.getId());
 accountCleanupNeeded = true;
 }
+ else if (!vm.getState().equals(VirtualMachine.State.Destroyed)) {


In what case would the vm's state be destroyed, when the vm was expunged above?

Also, do you know if the expunge thread propagates the usage event correctly?

The check on line 777 makes sure that the volume wasn't already destroyed before expunging. In that case, the delete event has already been emitted somewhere else.
The expunge thread does emit other usage events correctly (we tested deleting an account having active VMs, datadisks, allocated IP's, templates, and snapshots, and upon deleting the account, "delete" usage events were emitted for each of them). The emission of the ROOT volume's "delete" event is handled on a more case-by-case basis (In UserVmManager for regular VM destroys, in VolumeOrchestrator for failed vm creation, etc), and the case where the root volume gets deleted as part of account deletion was not being handled.

koushik-das · 2016-04-14T08:38:33Z

@ProjectMoon On latest master during VM deployment no VOLUME.CREATE is getting generated for ROOT volume. Also during destroy there is no VOLUME.DELETE event. On the event table I only see events for VM creation/destroy. Is ROOT volume events broken in general?

nnesic · 2016-04-14T12:46:56Z

@koushik-das We are mostly working with 4.7 version, where other volume usage events get emitted correctly. We haven't investigated the behavior on master yet.

cristofolini · 2016-04-18T00:43:21Z

@ProjectMoon May I suggest extracting lines 778-784 in AccountManagerImpl to a separate method? That would allow you to write your comment in a proper javadoc and set up a test case targeted at that change.

alexandrelimassantana · 2016-04-18T03:56:04Z

+ return _volumeDao.customSearchIncludingRemoved(c, null);
+ }
+
 protected boolean cleanupAccount(AccountVO account, long callerUserId, Account caller) {


I think it is missing a simple unit tests as a test if the method expungedInstaceRootVolume is called when the cleanupAccount is invoked and there is a vm in destroyed state. That and the guarantee that the method getExpungedInstanceRootVolume works, added to the tests you made would complete it.

swill · 2016-05-12T18:07:32Z

CI RESULTS

Tests Run: 85
Skipped: 0
Failed: 2
Errors: 0
Duration: 4h 19m 11s

Summary of the problem(s):

FAIL: Test redundant router internals
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs2/cloudstack/test/integration/smoke/test_routers_network_ops.py", line 290, in test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true
"Attempt to retrieve google.com index page should be successful!"
AssertionError: Attempt to retrieve google.com index page should be successful!
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_ANUTAN/results.txt

FAIL: Test redundant router internals
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs2/cloudstack/test/integration/smoke/test_routers_network_ops.py", line 483, in test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false
"Attempt to retrieve google.com index page should be successful once rule is added!"
AssertionError: Attempt to retrieve google.com index page should be successful once rule is added!
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_ANUTAN/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_12_2016_06_10_49_HJZG6Q:

/tmp/MarvinLogs/test_network_ANUTAN:

/tmp/MarvinLogs/test_vpc_routers_ZKQ0ZJ:

Uploads will be available until 2016-07-12 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-12T18:08:08Z

Can I get some code review on this PR? Thx...

DaanHoogland · 2016-05-12T18:41:34Z

so @swill, I may find time to review later. In the meanwhile let's use

tag:needsreview

makes sense?

swill · 2016-05-12T19:18:42Z

yep, sounds good...

swill · 2016-05-26T02:42:40Z

This one needs review as well...

ProjectMoon · 2016-05-26T11:29:35Z

Rebased against latest 4.7.

swill · 2016-05-26T13:40:58Z

@ProjectMoon try to close and reopen this one to try to get it green. thx...

swill · 2016-05-26T16:06:10Z

@ProjectMoon and again. :( sorry...

bvbharatk · 2016-06-08T18:31:37Z

ACS CI BVT Run

Sumarry:
Build Number 162
Hypervisor xenserver
NetworkType Advanced
Passed=73
Failed=0
Skipped=3

Link to logs Folder (search by build_no):https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm

Passed test suits:
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.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_disk_offerings.py

…stance. Currently the logic about volume deletion seems to be that an event should be emitted when the volume delete is requested, not when the deletion completes. The VolumeStateListener specifically ignores destroy events for ROOT volumes, assuming that the ROOT volume only gets deleted when the instance is destroyed and the UserVmManager should take care of it. When deleting an account, all of its resources get destroyed, but the instance expunging circumvents the UserVmManager, and thus we miss the VOLUME_DESTROY usage event. Added a check in the AccountManager to emit the deletion event for ROOT volumes belonging to instances which weren't destroyed prior to the account deletion.

ProjectMoon · 2016-08-02T14:35:06Z

Closing to reopen against 4.8.

…-4.8 Fixes regarding VOLUME_DELETE events resulting from account deletionNew version of #1491. **Original Description** New version of #1373, but updated for the 4.7 branch with another fix that allows it to properly find expunged root volumes. This is a bug fix, which is why we target the 4.7 branch. Original pull request: Fixes regarding usage event emission. UsageEventUtils was previously not checking deleted accounts, which meant that if an account was deleted that had some resources running on it, those resources would get destroyed without emitting any events. Furthermore, the VOLUME_DELETE event of ROOT volumes is the responsibility of the UserVmManager, which gets circumvented when expunging resources following the account deletion. Added a check to the AccountManager which catches the ROOT volumes that need to be deleted and emits events for them. To test this: Create a new user. As that user, create and destroy an instance. This should cause the VM_CREATE, VM_START, VM_STOP, VM_DESTROY, VOLUME_CREATE, and VOLUME_DELETE events to be emitted. Create a new instance as the same user. Log in as admin, and delete the user. The same set of events should be emitted, and there should be no duplicate DELETE events for the ROOT volume of the previous instance. * pr/1624: Emit a VOLUME_DELETE usage event when account deletion destroys an instance. Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>

pdube reviewed Apr 13, 2016
View reviewed changes

alexandrelimassantana reviewed Apr 18, 2016
View reviewed changes

ProjectMoon closed this May 26, 2016

ProjectMoon reopened this May 26, 2016

ProjectMoon closed this May 26, 2016

ProjectMoon reopened this May 26, 2016

ProjectMoon closed this Aug 2, 2016

ProjectMoon deleted the pr-volume-usage-events-fixes-4.7 branch August 2, 2016 14:35

ProjectMoon mentioned this pull request Aug 2, 2016
Fixes regarding VOLUME_DELETE events resulting from account deletion #1624
Merged

Uh oh!

Conversation

ProjectMoon commented Apr 13, 2016

Uh oh!

pdubeApr 13, 2016

Choose a reason for hiding this comment

Uh oh!

pdubeApr 13, 2016

Choose a reason for hiding this comment

Uh oh!

nnesicApr 14, 2016

Choose a reason for hiding this comment

Uh oh!

koushik-das commented Apr 14, 2016

Uh oh!

nnesic commented Apr 14, 2016

Uh oh!

cristofolini commented Apr 18, 2016

Uh oh!

alexandrelimassantanaApr 18, 2016

Choose a reason for hiding this comment

Uh oh!

swill commented May 12, 2016

CI RESULTS

Uh oh!

swill commented May 12, 2016

Uh oh!

DaanHoogland commented May 12, 2016

Uh oh!

swill commented May 12, 2016

Uh oh!

swill commented May 26, 2016

Uh oh!

ProjectMoon commented May 26, 2016

Uh oh!

swill commented May 26, 2016

Uh oh!

swill commented May 26, 2016

Uh oh!

bvbharatk commented Jun 8, 2016

ACS CI BVT Run

Uh oh!

ProjectMoon commented Aug 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants