cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hugo Trippaers <>
Subject RE: Nicira integration pre-review
Date Fri, 15 Jun 2012 11:27:20 GMT
Hello Chiradeep,

Thanks a lot for the review, comments inline.



-----Original Message-----
From: Chiradeep Vittal [] 
Sent: Friday, June 15, 2012 8:56 AM
To: CloudStack DeveloperList
Cc: Somik Behera (
Subject: Re: Nicira integration pre-review

That's a big patch set. You should split it up, e.g., Patch Set #1: Nicira API glue Patch
Set #2: Guru and Element Patch Set #3: etc.
HT: No problem, will do

Since this is a pre-review, I will give general comments:
 - the coding guide is here:
   Note variable conventions, class and interface javadoc etc.

HT: Will take care of this before submitting for review

 - NiciraElement is incomplete it appears, so at least a //TODO:(Hugo) do blah here would
be nice. Also, NiciraNvpElement would be better since they probably will have other products

HT: Renamed all classes to NiciraNvp already after feedback from Somik. What is missing from
NiciraENvpElement? For the current phase (L2 connectivity for Nicira based networks) it is
complete I think.

 - It is a convention that actual API calls to external elements are made from within a Resource.
The NiciraElement would be expected to call an API on the NiciraManager. The NiciraManager
would construct relevant messages, for example: CreateAndAttachLogicalPortCommand and dispatch
it to the NiciraNvpResource. The convention ensures that the decision to run these components
as standalone processes can be a late binding decision.

HT: Is this something you want in before accepting this patch, or could we leave this for
the next release?

 - Not sure that you need to modify any base classes at all. For example NetworkGuruBase --
the modifications can be in the subclass. Not sure I understand the purpose of 'isolationMethods'

HT: The isolation methods was already in the code, I just extended it with the STT isolation
method. I had to modify base classes because a lot of the networking code assumed VLANs were
being used to isolate traffic. In our case vlans are not used, but we use the Nicira NVP solution
to create logical networks and ports. I assumed the isolation method was there to allow gurus
to check if they should be repsonsibe for creation of the guest network. For example the ExternalGuestNetwork
guru takes care of flat of VLAN networks (IsolationMethod empty and VLAN). The Ovs guru deals
with the GRE isolation method and the NiciraNvp Guru takes care of STT based physical networks.
The current networkManager will keep asking gurus even though a previous guru has already
decided to design the network, this could result in multiple guest networks if more than one
guru can design a certain network. In the current code it is solved by checking if the tunnelmanager
is enabled, depending on that either Ovs  guru or external guest guru will design a network.
My idea was to extend this to checking for the physical isolation type selected when creating
the zone. The other option would be to use components.xml to decide what gurus are enabled,
but I think this is a more clean solution that works 'out-of-the-box'.

HT: We also needed to change some TO classes because we need the uuids at some point and the
XenResource to store those uuids in the vif configuration.

 - It is unfortunate that you had to modify enums. We should probably move to a static list
initialized at startup (like Network.Service) that others can extend in their own class.

HT: I agree, a sort of framework where network connectivity and/or isolation method providers
could be pluggable would be nice.

On 6/13/12 6:00 AM, "Hugo Trippaers" <> wrote:

>Thanks for the heads-up, didn't notice.
>Here is a link to the file
>-----Original Message-----
>From: Chip Childers []
>Sent: Wednesday, June 13, 2012 2:27 PM
>Cc: Somik Behera (
>Subject: Re: Nicira integration pre-review
>Attachments are apparently being stripped out by the mailer daemon.
>On Wed, Jun 13, 2012 at 6:00 AM, Hugo Trippaers 
><> wrote:
>> Actually attaching the patch should help I think.

View raw message