Uh oh!
There was an error while loading. Please reload this page.
Usage event fixes for deleted accounts - #924
Conversation
nnesic
commented
Oct 12, 2015
Hi, could you please re-trigger the build, since it seems to have crashed? |
remibergsma
commented
Oct 12, 2015
Hi @nnesic, you can do that by force pushing your commits again to the PR branch. The tests will then run again. |
kishankavala
commented
Oct 14, 2015
@remibergsma We not have any tests that will check for correct events on account delete. |
nnesic
commented
Oct 14, 2015
I added two unit tests to check that the account manager adheres to its new responsibility of emitting the usage events for root volumes of instances which were not in destroyed state prior to the account deletion. |
remibergsma
commented
Oct 24, 2015
Pinging @borisroman@michaelandersen to review this PR. |
nnesic
commented
Nov 19, 2015
Hello, how is the review on this going? |
DaanHoogland
commented
Nov 19, 2015
code reviewed LGTM, @remibergsma kan you start a bubble with this? |
remibergsma
commented
Nov 20, 2015
I tried rebasing against current master but I run into build issues. Tried again but same thing. @nnesic Could you try rebasing the PR against master and run a build? |
This was causing problems when deleting accounts which had running resources. The resources are stopped and destroyed, but we never get a usage event indicating so.
…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.
To allow distinguishing between pre- and post-stateTransition events
ProjectMoon
commented
Nov 23, 2015
The PR has been fixed by the application of two more commits. Maybe squashing is in order before a merge. The abuse of reflection required to make the unit test play nice is an indicator that maybe the UsageEventUtils class should be made non-static though. |
ProjectMoon
commented
Nov 27, 2015
What's the status of this pull request? Do we need to redo this PR on 4.6 instead? Also what are the thoughts about squashing some of these commits down? |
yadvr
commented
Jan 27, 2016
@ProjectMoon can you rebase against latest master, and squash changes to a single commit |
ProjectMoon
commented
Jan 27, 2016
Squashing and rebasing is definitely needed. But isn't this a bug fix though? And thus should go on to 4.6 and then be forward merged? |
yadvr
commented
Jan 27, 2016
@ProjectMoon yes please squash them and maybe open a new PR against 4.7 (or 4.6) closing this one as github won't allow you to edit/change the source/dest branches. |
ProjectMoon
commented
Jan 27, 2016
New request is located at #1372. |
ProjectMoon
commented
Jan 27, 2016
I don't have the close button, so this one needs to be closed again. |
yadvr
commented
Jan 27, 2016
@nnesic since you're the author you can close this PR @ProjectMoon thanks for the new PR |
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.