NiciraNvpElement

[Chiradeep Vittal <Ch...@citrix.com>]

Hi Hugo,

I was refactoring some code and noticed the following:

 1.  The NiciraNvpElement is present in the checked-in components.xml.in. This is odd since it is a plugin and should be enabled only if the installation is using NiciraNvp
 2.  In the implement method, a lock is taken against the network. First, this is unnecessary since the outer NetworkManagerImpl.implement() already takes this lock. Second, the Network table belongs to the Network Manager/orchestrator and it is expected that the plugin elements keep their hands off of these internal tables.
 3.  If the implement chain throws an exception or even at a later point in the workflow, then the shutdown() method is /may be called. Therefore it is expected that the shutdown method is usually an undo of the implement. However this rule is not fast — if it makes sense to maintain some state in implement(), it may be OK to not roll back that state.

Check it out and let me know if you have any questions.
--
Chiradeep

Re: orchestration and plugins (was NiciraNvpElement)

[Murali Reddy <Mu...@citrix.com>]

>
>Unfortunately the VirtualNetworkApplianceManager behaves both as a plugin
>and an orchestrator making it a bad example of how to develop a network
>plugin. 

Chiradeep,

If time permits I was planning to partially clean-up
VirtualNetworkApplianceManager as part of the CLOUDSTACK-655 [1] which is
prerequisite for ELB parity with AWS [2]. Idea is to pull the
orchestration part in to a appliance provisioning framework, which will
enable specific plug-in's for VR, NetScaler VPX or Vyatta can be hooked in
to VirtualNetworkApplianceManager. Could you please put your
observation/thoughts in CLOUDSTACK-655 on how responsibilities of
orchestration and plug-in should be separated out, that will help working
on the bug.

[1] https://issues.apache.org/jira/browse/CLOUDSTACK-655
[2] https://issues.apache.org/jira/browse/CLOUDSTACK-654

orchestration and plugins (was NiciraNvpElement)

[Chiradeep Vittal <Ch...@citrix.com>]

Bringing this back after my refactoring efforts.
Let me explain this a little bit better.
The core orchestration of network elements belongs to the NetworkManager.
It interacts with the orchestration database in a transactional manner. It
also 'drives' the network plugins during various state transitions. The
network plugins are not expected to make changes to the orchestration
tables -- they only make changes to the resources they own and any data
they own.

In short, the plugins are not expected to make mutating calls to the
NetworkManager or directly modify the core orchestration tables.
I've moved the data model queries to the NetworkModel class to make this
clearer.

If you follow the implement() method in NetworkManagerImpl, you can see
that it
(a) ensures that a source nat ip is allocated if the source nat service is
enabled
(b) locks the network row in the database
(c) calls the plugin's implement()

So, there is no need for plugins to perform tasks (a) and (b)

Unfortunately the VirtualNetworkApplianceManager behaves both as a plugin
and an orchestrator making it a bad example of how to develop a network
plugin. 

HTH
--
Chiradeep

On 11/19/12 7:09 PM, "Chiradeep Vittal" <Ch...@citrix.com>
wrote:

>Hi Hugo,
>
>I was refactoring some code and noticed the following:
>
> 1.  The NiciraNvpElement is present in the checked-in components.xml.in.
>This is odd since it is a plugin and should be enabled only if the
>installation is using NiciraNvp
> 2.  In the implement method, a lock is taken against the network. First,
>this is unnecessary since the outer NetworkManagerImpl.implement()
>already takes this lock. Second, the Network table belongs to the Network
>Manager/orchestrator and it is expected that the plugin elements keep
>their hands off of these internal tables.
> 3.  If the implement chain throws an exception or even at a later point
>in the workflow, then the shutdown() method is /may be called. Therefore
>it is expected that the shutdown method is usually an undo of the
>implement. However this rule is not fast ‹ if it makes sense to maintain
>some state in implement(), it may be OK to not roll back that state.
>
>Check it out and let me know if you have any questions.
>--
>Chiradeep