cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Darren Shepherd" <>
Subject Re: Review Request 14084: Updates to @ActionEvent to be compatible with Spring AOP
Date Fri, 13 Sep 2013 07:30:17 GMT

> On Sept. 12, 2013, 5:28 p.m., Kelven Yang wrote:
> > Darren, I saw you added the annotation for group of events and annotated in various
places, is there any test done for the business logic validation?

I added an annotation @ActionEvents and its used in only one place and that is on AccountManagerImpl.createUserAccount().
 In general nested ActionEvents won't work with Spring AOP.  So previously createUserAccount
had an @ActionEvent and that internally called createUser which also had an @ActionEvent.
 With Spring AOP the second will not be called.  So the @ActionEvents is to specify that two
events should be recorded for that method invocation.  

In general the whole concept of nesting actionevents is somewhat broken.  The eventDetails
is stored on the CallContext which is a ThreadLocal.  There is only one of those, so as you
nest the child's details override the parent's.  This is how things already were.  I don't
think the move to Spring AOP will be completely ideal for ActionEvents.  It limits some things
you can do.  But I would argue that this whole ActionEvents framework should be reconcidered.
 Instead of all this ad-hoc based annotations and random strings of text the developers choose,
a simple framework in the API can provide more precise and useful information for auditing.
 Billing is not based off of ActionEvents and that information is far more accurate and useful

To further illustrate the complete uselessness (in my opinion) of these events, if you reboot
a VM, for example, you get an VM.REBOOT event that has a description "rebooting user vm: 7"
 What is VM 7?  The user has no visibility to internal VM ids.

I tested all changes to ensure that the same events were fired as before.  There was some
exceptions though.  In some cases related to the firewall rule stuff you would make one change
to a rule that would then fire an event for every single rule you had.  In situations like
that, since nesting is a problem, I ensured the top level event was still properly recorded.

- Darren

This is an automatically generated e-mail. To reply, visit:

On Sept. 11, 2013, 5:07 p.m., Darren Shepherd wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> -----------------------------------------------------------
> (Updated Sept. 11, 2013, 5:07 p.m.)
> Review request for cloudstack, Kelven Yang and Kishan Kavala.
> Repository: cloudstack-git
> Description
> -------
> Two fields were added to CallContext to allow call to dynamically change
> the event type and description.  Additionally a @ActionEvents annotation
> was added to allow a method to specify multiple events
> Spring AOP will not intercept calls to "this" so @ActionEvent needs to be
> put on public methods that are externally invoked
> Annotations that needed to be changed were identified by doing byte code 
> analysis using objectweb asm.  Code for that is at 
> and there
> are instructions to run it there.
> Diffs
> -----
>   api/src/com/cloud/event/ PRE-CREATION 
>   api/src/org/apache/cloudstack/context/ e3c1bf2a7b97573cdeb4f530d0afe74cb7e3e834

>   engine/components-api/src/com/cloud/configuration/ 6e76b6ffb91c200127589831893d9d79970aafdb

>   engine/components-api/src/com/cloud/network/rules/ fa12cd804a67138740f9d9042709938871dc8629

>   server/src/com/cloud/configuration/ fb727a1705487416b7069fc2aca5086fd726e700

>   server/src/com/cloud/event/ ba7e270af90f7bc191b570a7cc131319f446e2f6

>   server/src/com/cloud/event/ 60f5633fc3c53dac960247308de12b60b492de59

>   server/src/com/cloud/network/firewall/ cd83c4e52f85adc9c9d9c7997c28838f2c15b323

>   server/src/com/cloud/server/ a3efd2129ce082023d79e55872e8134d1b6bd85c

>   server/src/com/cloud/user/ 0602514fcf429b09a62edf65f4b0dc0e87d80b94

>   server/src/com/cloud/vm/ c3a718ac55be05f123b062a627c2de042c4321ab

>   server/test/com/cloud/network/ c50459e98737eaf5662bb44c6e9a12fad54b4175

>   server/test/com/cloud/vpc/ 3ec146b953726c9480b1e15848a67e4746dd65a6

> Diff:
> Testing
> -------
> Thanks,
> Darren Shepherd

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message