cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Darren Shepherd" <darren.s.sheph...@gmail.com>
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
too.  

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:
https://reviews.apache.org/r/14084/#review26056
-----------------------------------------------------------


On Sept. 11, 2013, 5:07 p.m., Darren Shepherd wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14084/
> -----------------------------------------------------------
> 
> (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 
> https://github.com/ibuildthecloud/cloudstack-findbadactionevents and there
> are instructions to run it there.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/event/ActionEvents.java PRE-CREATION 
>   api/src/org/apache/cloudstack/context/CallContext.java e3c1bf2a7b97573cdeb4f530d0afe74cb7e3e834

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

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

>   server/src/com/cloud/configuration/ConfigurationManagerImpl.java fb727a1705487416b7069fc2aca5086fd726e700

>   server/src/com/cloud/event/ActionEventInterceptor.java ba7e270af90f7bc191b570a7cc131319f446e2f6

>   server/src/com/cloud/event/ActionEventUtils.java 60f5633fc3c53dac960247308de12b60b492de59

>   server/src/com/cloud/network/firewall/FirewallManagerImpl.java cd83c4e52f85adc9c9d9c7997c28838f2c15b323

>   server/src/com/cloud/server/ManagementServerImpl.java a3efd2129ce082023d79e55872e8134d1b6bd85c

>   server/src/com/cloud/user/AccountManagerImpl.java 0602514fcf429b09a62edf65f4b0dc0e87d80b94

>   server/src/com/cloud/vm/UserVmManagerImpl.java c3a718ac55be05f123b062a627c2de042c4321ab

>   server/test/com/cloud/network/MockFirewallManagerImpl.java c50459e98737eaf5662bb44c6e9a12fad54b4175

>   server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 3ec146b953726c9480b1e15848a67e4746dd65a6

> 
> Diff: https://reviews.apache.org/r/14084/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Darren Shepherd
> 
>


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