cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Huang <Alex.Hu...@citrix.com>
Subject FW: [Refactor AccountManagerImpl][DISCUSS]
Date Wed, 06 Feb 2013 15:09:56 GMT


From: Alex Huang
Sent: Tuesday, February 05, 2013 8:57 PM
To: 'Joe San'
Subject: RE: [Refactor AccountManagerImpl][DISCUSS]

Joe,

I was commenting on the fact that AccountManagerImpl is not done in this style, which causes
all the different dependencies including dao stuff.  To refactor AccountManagerImpl, I think
the following needs to be done.  You may know this well already but just for completeness,
I include everything I can think so.


-          Make sure all states for account/users are properly enumerated.  It looks like
it should be enumerated on Account.java

-          Add dependency on something like google guava's event bus

-          Move the different parts of AccountManagerImpl into the respective managers.  i.e.
vm cleanup to userVmManagerImpl.cleanupOnAccountDeletion, volumes back to userVmManagerImpl.cleanupOnAccountDeletion.

-          Add the appropriate annotation from event bus to the methods.

-          Register with the event bus in the configure() method of the managers.

-          In AccontManagerImpl, implement a processing loop that just manages the state of
the account and publish the new state.  For example on account deletion:

o   Set the state to delete to prevent the account from logging in.

o   Signal to the event bus the new state along with the Account object.

o   If any of the listeners throws an error, mark the account as cleanup required and signal
error to the administrator.

o   If the administrator figures out what happened, they should correct the problem and then
delete the account again.

o   If the account cleanup is successful, remove any cleanup required attribute and any unique
keys in the account row and then mark the row as removed.

AccountManager as it is today should only control account and users so it having access to
account dao and user dao is fine but it shouldn't access other daos.

After this, you should be able to write a unit tests that does the following:
- inject a mocked listener, mocked daos, and mocked alertmanager
- run account actions (create/delete/update etc)
- invoke error inside the mocked listener and then check if the account has the appropriate
states and the appropriate alerts have fired.
- run account actions again and this time have the mocked listener succeed and see that the
mocked dao was invoked to remove the account at the end.

--Alex




From: Joe San [mailto:codeintheopen@gmail.com]
Sent: Monday, February 04, 2013 2:04 PM
To: Alex Huang
Subject: Re: [Refactor AccountManagerImpl][DISCUSS]

Alex,

Thanks for the email.

I understand the listener model of doing things. But the AccountManager class has methods
other than delete as well. As I could see from the source code that the AccountManager#deleteAccount
has most of the dao calls in the cleanupAccount, I guess we need to refactor that. I could
try that.

>From your email below:

- The thread actually will go to all of the listeners and processes delete so code to handle
vms, volumes, rules, ip addresses etc all reside in their own managers.

Could you let me know which listeners are you talking about?

Regards,
Jothi
On Sat, Feb 2, 2013 at 3:25 PM, Alex Huang <Alex.Huang@citrix.com<mailto:Alex.Huang@citrix.com>>
wrote:
This is a very good topic.  Here's my $.02 on this.

The reason why AccountMgr has so many dependencies is because of the following:

"Oh deleting an account?  I have to delete all the vms, I have to delete the volumes, I have
to remove all firewall rules, I have to release ip address, etc"  So the developers naturally
add it to AccountMgr since they're all part of an account deletion.  Imagine that for all
of accountmanager's functions.  Then that tightly couples the internals of CloudStack.

What it really should do is follow the pattern set in AgentManager.  AgentManager says I take
care of agent connections but if you're interested in doing processing due to agent connection
loss/connect, then register a listener with me and I will publish the agent connection events.
 That way all other managers are decoupled from AgentManager.

Now, this is different from a pure publisher/subscriber model because in this we don't want
the processing to be completely independent of each other since the account manager does want
to know if some resources have not been cleaned up.  We just want the code to be decoupled.

In this model, it would work like this.

On delete:
- AccountManager sets the state of the account to 'Deleted'.  This causes the account to not
be able to login.
- AccountManager fires a DELETE event.
- The thread actually will go to all of the listeners and processes delete so code to handle
vms, volumes, rules, ip addresses etc all reside in their own managers.
- This event may cause ripple effects throughout the Managers as each Manager should have
their own listener structure.
- Once all of that is done, then AccountManager finally removes the row from the table.

On error:
- AccountManager appropriately notifies the caller.
- The caller then fixes the problem and calls delete again.
- Account Manager then again goes through the procedure above.

All of CloudStack's managers should behave in this pattern.  I would encourage all of the
developers to study this and contact me with any questions.

--Alex


> -----Original Message-----
> From: Joe San [mailto:codeintheopen@gmail.com<mailto:codeintheopen@gmail.com>]
> Sent: Saturday, February 02, 2013 5:57 AM
> To: cloudstack-dev@incubator.apache.org<mailto:cloudstack-dev@incubator.apache.org>
> Subject: [Refactor AccountManagerImpl][DISCUSS]
>
> Cloudstack dev team,
>
> I would like to propose to refactor the AccountManagerImpl.java. The idea
> would be to make the implementation without any instance fields. At
> present
> there are a lot of Dao's being injected. This brings a lot of cross
> references to other dao's as well. Ideally AccountManagerImpl only needs
> the AccountDAO and should only contain methods pertaining to Account
> related activities. Any other DAO calls like NetworkDAO should be handled
> through the NetworkManagerImpl (needs to be created if one does not
> exist).
>
> I would love to see AccountManagerImpl free of any injections.
>
> Regards,
> Joe


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