Fixes regarding VOLUME_DELETE events resulting from account deletion #1624

ProjectMoon · 2016-08-02T14:40:50Z

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

yadvr · 2016-08-05T21:11:04Z

@blueorangutan kick

blueorangutan · 2016-08-05T21:12:03Z

A Trillian-Jenkins job has been kicked to build packages and start testing. I'll keep you posted as I make progress.

blueorangutan · 2016-08-05T22:18:35Z

Packaging result: ✔centos6 ✔centos7 ✔debian repo: http://packages.shapeblue.com/cloudstack/pr/1624

jburwell · 2016-08-10T19:29:00Z

server/test/com/cloud/user/AccountManagerImplTest.java

+                f.set(utils, this.getClass().getDeclaredField(usageUtilsFields.get(fieldName)).get(this));
+            } catch (IllegalArgumentException | IllegalAccessException
+                    | NoSuchFieldException | SecurityException e) {
+                // TODO Auto-generated catch block


Is there a TODO to be resolved? Is there a TODO to be resolved? Seems like the test should fail if one of these exceptions occurs.

yadvr · 2016-09-01T09:38:40Z

LGTM. Thanks @ProjectMoon as soon as #1654 is merged, we'll get this merged as well. Do we have any regression tests results, especially around the changed code?

yadvr · 2016-09-01T11:59:41Z

@ProjectMoon please rebase now, PR 1654 has been merged now. DB upgrades should not cause errors.

ProjectMoon · 2016-09-01T12:45:53Z

The pull request has been rebased.

ProjectMoon · 2016-09-01T16:44:11Z

Looks like it's ready for merging now.

jburwell · 2016-09-01T19:53:23Z

@ProjectMoon we need a test LGTM. I inquired yesterday -- do you have someone lined up to test it?

ProjectMoon · 2016-09-02T09:34:56Z

We can have someone test it. Are you looking for results of a manual test, reproduction steps, or something automated? A manual test is probably the best from our side because we don't really have the time to write any unit/integration tests at the moment beyond what already exists in this PR.

yadvr · 2016-09-02T10:30:07Z

server/src/com/cloud/user/AccountManagerImpl.java

+        SearchCriteria<VolumeVO> c = sb.create();
+        c.setParameters("instanceId", instanceId);
+        c.setParameters("vType", Volume.Type.ROOT);
+        return _volumeDao.customSearchIncludingRemoved(c, null);


This method does not return expunged volume, but root volume of a VM. Could there be a case where a VM can have more than one root volume? If not, let's rename the method to say getInstanceRootVolume and return just the VolumeVO instead a list.

nnesic · 2016-09-16T09:38:03Z

@blueorangutan kick

jburwell · 2016-09-18T15:31:22Z

@ProjectMoon I want to get this PR into 4.8.2.0, 4.9.1.0, and 4.10.0.0 for which we will be cutting RCs on 25 Sept 2016. Tests should be the entire Marvin smoke test suite against VMware, KVM, and XenServer, as well as, the test_account component test.

Also, is it possible to automate the test procedure described in the description? Given the amount of change in the project, we want to keep manual testing to an absolute minimum.

Finally, is there a JIRA ticket for this bug?

karuturi · 2016-09-21T09:39:08Z

I started BVT run(xenserver, advanced zone) on this PR. @cloudmonger will post the results here in approx. 4 hrs.

ProjectMoon · 2016-10-13T13:44:10Z

This branch has been rebased to latest 4.8.

karuturi · 2016-10-13T16:32:27Z

Iso tests failed as the iso url wasnt accessible from our test environment. All is well here.
@ProjectMoon were there any code changes in your latest rebase? (meaning, do we have to run smoke tests again?)

ProjectMoon · 2016-10-13T21:55:03Z

No code changes, just updating the commits under it.

yadvr · 2016-10-21T05:16:54Z

@ProjectMoon can you squash your changes, and rebase against latest base branch?

…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. The account manager now attempts to propperly destroy the vm before expunging it. This way the destroy logic is respected, including the event emission.

ProjectMoon · 2016-10-21T09:33:53Z

The commits have been squashed, commit message updated to make sense (I think; let me know if it's not right), and rebased against latest 4.8.

karuturi · 2016-10-21T11:26:02Z

I started another test run on this. If it goes through, I will merge this tonight.

cloudmonger · 2016-10-21T13:23:34Z

ACS CI BVT Run

Sumarry:
Build Number 116
Hypervisor xenserver
NetworkType Advanced
Passed=103
Failed=2
Skipped=5

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

Failed tests:

test_deploy_vm_iso.py
- test_deploy_vm_from_iso Failing since 4 runs
test_vm_life_cycle.py
- test_10_attachAndDetach_iso Failing since 4 runs

Skipped tests:
test_01_test_vm_volume_snapshot
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
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_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_non_contigiousvlan.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_ssvm.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_routers_network_ops.py
test_disk_offerings.py

jburwell · 2016-10-22T03:37:08Z

@blueorangutan package

blueorangutan · 2016-10-22T03:38:03Z

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

blueorangutan · 2016-10-22T04:05:06Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-88

karuturi · 2016-10-22T14:09:05Z

Iso failures are due to URL access issues. All good here. merging this now.

…-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 <[email protected]>

karuturi · 2016-10-22T14:18:38Z

This is pushed to 4.8 but not merged fwd.
looks like 4.8 is not fwd merged for some commits and there are conflicts now. Need to investigate further. Will continue on monday.

karuturi · 2016-10-24T06:02:30Z

It is now fwd merged to all the branches. Thanks

jburwell reviewed Aug 10, 2016
View reviewed changes

yadvr reviewed Sep 2, 2016
View reviewed changes

ProjectMoon closed this Sep 17, 2016

ProjectMoon reopened this Sep 17, 2016

asfgit merged commit d989c5d into apache:4.8 Oct 22, 2016

ProjectMoon deleted the pr-volume-usage-events-fixes-4.8 branch October 24, 2016 09:11

Fixes regarding VOLUME_DELETE events resulting from account deletion #1624

Fixes regarding VOLUME_DELETE events resulting from account deletion #1624

Uh oh!

Conversation

ProjectMoon commented Aug 2, 2016

Uh oh!

yadvr commented Aug 5, 2016

Uh oh!

blueorangutan commented Aug 5, 2016

Uh oh!

blueorangutan commented Aug 5, 2016

Uh oh!

jburwell Aug 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 1, 2016

Uh oh!

yadvr commented Sep 1, 2016

Uh oh!

ProjectMoon commented Sep 1, 2016

Uh oh!

ProjectMoon commented Sep 1, 2016

Uh oh!

jburwell commented Sep 1, 2016

Uh oh!

ProjectMoon commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr Sep 2, 2016

Choose a reason for hiding this comment

Uh oh!

nnesic commented Sep 16, 2016

Uh oh!

jburwell commented Sep 18, 2016

Uh oh!

karuturi commented Sep 21, 2016

Uh oh!

ProjectMoon commented Oct 13, 2016

Uh oh!

karuturi commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProjectMoon commented Oct 13, 2016

Uh oh!

yadvr commented Oct 21, 2016

Uh oh!

ProjectMoon commented Oct 21, 2016

Uh oh!

karuturi commented Oct 21, 2016

Uh oh!

cloudmonger commented Oct 21, 2016

ACS CI BVT Run

Uh oh!

jburwell commented Oct 22, 2016

Uh oh!

blueorangutan commented Oct 22, 2016

Uh oh!

blueorangutan commented Oct 22, 2016

Uh oh!

karuturi commented Oct 22, 2016

Uh oh!

karuturi commented Oct 22, 2016

Uh oh!

karuturi commented Oct 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jburwell Aug 10, 2016 •

edited

Loading

ProjectMoon commented Sep 2, 2016 •

edited

Loading

karuturi commented Oct 13, 2016 •

edited

Loading