Fixes regarding VOLUME_DELETE events resulting from account deletion. #1373
Conversation
|
Added the license header to the MockUsageEventDao class to fix rat report error. |
|
Hi @ProjectMoon , |
|
I'm not the original author of this work, but I think it was just because a class for testing was needed. Looking at it now I'm not really sure why it can't just be the regular Edit: reason I got back is that she was following the pattern of other tests (AccountManager tests specifically). |
|
@rafaelweingartner I have removed the MockUsageEventDao and replaced it with a normal Mockito mock of the UsageEventDao interface. |
There was a problem hiding this comment.
I do not think you need this kind of comment.
|
@ProjectMoon that is great, |
The logic about volume deletion was 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. 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. 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. This commits adds 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. It also adds the volume status to the event emitted on the event bus, for distinguishing between pre- and post- state transitions.
|
I added the suggested changes. Also, now that #1382 has been merged to 4.6, the checks should succeed. |
|
Congratulations, the code is much better now. |
|
1373.network.results.txt the errors in the network tests are due to moved tests en not related to volumes. I am satisfied with these results: LGTM |
|
Who gets to be the authority on merging this? |
|
@ProjectMoon Currently waiting for a new Release Manager for version 4.9 to pick this up |
|
Even for forward-merged bug fixes? |
ACS CI BVT RunSumarry: The follwing tests have known issues Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests:
Skipped tests: Passed test suits: |
|
We have found that this fix actually does not work on 4.7+. We are going to submit a new pull request based on 4.7 with a working fix. |
…-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]>
6 participants
New version of #924, but on the right branch with the commits squashed.
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.