cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chiradeep Vittal <>
Subject Re: Nicira integration pre-review
Date Mon, 18 Jun 2012 18:23:10 GMT

On 6/15/12 4:27 AM, "Hugo Trippaers" <> wrote:

>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.

The shutdown / destroy should delete the corresponding network in NVP ?

> - 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?

Yes, I would want this in.

> - 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' either
>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

Actually Alex had a different solution in mind (see his email from June 4,
subject "SDN integration"). Mainly "Make changes to the NetworkManager
such that it selects the correct NetworkGuru based on the supported
isolation technology and the isolation technology defined on the physical
network." so that the NetworkGuru does not need to know about other
isolation methods.

>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
> - 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.

Can you raise a enhancement request for this?

>On 6/13/12 6:00 AM, "Hugo Trippaers" <>
>>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