Mailing-List: contact cloudstack-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cloudstack-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: local policy)
From: Hugo Trippaers <HTrippaers@schubergphilis.com>
To: CloudStack DeveloperList <cloudstack-dev@incubator.apache.org>
CC: "Somik Behera (somik@nicira.com)" <somik@nicira.com>
Subject: RE: Nicira integration pre-review
Thread-Topic: Nicira integration pre-review
Thread-Index: 
 Ac1JSfIORkFZTT1QTIOwgGHivzxiSgAAVJEAAAD3fYAABVWCMABTqtEAAAvcYXAAowRfAAAeQ42w
Date: Tue, 19 Jun 2012 06:55:46 +0000
Message-ID: <6DE00C9FDF08A34683DF71786C70EBF0295D1BF0@SBPOMB402.sbp.lan>
References: <6DE00C9FDF08A34683DF71786C70EBF0295583A2@SBPOMB402.sbp.lan>
 <CC04BEF4.24481%chiradeep.vittal@citrix.com>
In-Reply-To: <CC04BEF4.24481%chiradeep.vittal@citrix.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0


-----Original Message-----
From: Chiradeep Vittal [mailto:Chiradeep.Vittal@citrix.com]=20
Sent: Monday, June 18, 2012 8:23 PM
To: CloudStack DeveloperList
Cc: Somik Behera (somik@nicira.com)
Subject: Re: Nicira integration pre-review


On 6/15/12 4:27 AM, "Hugo Trippaers" <HTrippaers@schubergphilis.com> wrote:

>Hello Chiradeep,
>
>Thanks a lot for the review, comments inline.
>
>Cheers,
>
>Hugo
>
>-----Original Message-----
>From: Chiradeep Vittal [mailto:Chiradeep.Vittal@citrix.com]
>Sent: Friday, June 15, 2012 8:56 AM
>To: CloudStack DeveloperList
>Cc: Somik Behera (somik@nicira.com)
>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:
>http://docs.cloud.com/CloudStack_Documentation/Design_Documents/Coding_
>Con
>v
>entions
>   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)=20
>do blah here would be nice. Also, NiciraNvpElement would be better=20
>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=20
>connectivity for Nicira based networks) it is complete I think.

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

HT: That is done by the NiciraNvpNetworkGuru during implement and shutdown.=
 The Guru will make sure the network (logical switch) is created and destro=
yed and the Element will take care of creating logical ports and  attaching=
 Vifs.
>
> - It is a convention that actual API calls to external elements are=20
>made from within a Resource. The NiciraElement would be expected to=20
>call an API on the NiciraManager. The NiciraManager would construct=20
>relevant messages, for example: CreateAndAttachLogicalPortCommand and=20
>dispatch it to the NiciraNvpResource. The convention ensures that the=20
>decision to run these components as standalone processes can be a late bin=
ding decision.
>
>HT: Is this something you want in before accepting this patch, or could=20
>we leave this for the next release?

Yes, I would want this in.

Ok :-)

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

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 t=
echnology and the isolation technology defined on the physical network." so=
 that the NetworkGuru does not need to know about other isolation methods.

HT: Ok, I think I get the idea now. Basically the Guru should be able to te=
ll which isolation methods it supports and the NetworkManager will check th=
is before asking a guru to design a network.

>
>HT: We also needed to change some TO classes because we need the uuids=20
>at some point and the XenResource to store those uuids in the vif=20
>configuration.
>
> - It is unfortunate that you had to modify enums. We should probably=20
>move to a static list initialized at startup (like Network.Service)=20
>that others can extend in their own class.
>
>HT: I agree, a sort of framework where network connectivity and/or=20
>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" <HTrippaers@schubergphilis.com>
>wrote:
>
>>Thanks for the heads-up, didn't notice.
>>
>>Here is a link to the file
>>
>>http://dl.dropbox.com/u/70226362/nicira-phase1.patch
>>
>>Cheers,
>>
>>Hugo
>>
>>-----Original Message-----
>>From: Chip Childers [mailto:chip.childers@sungard.com]
>>Sent: Wednesday, June 13, 2012 2:27 PM
>>To: cloudstack-dev@incubator.apache.org
>>Cc: Somik Behera (somik@nicira.com)
>>Subject: Re: Nicira integration pre-review
>>
>>Attachments are apparently being stripped out by the mailer daemon.
>>
>>-chip
>>
>>On Wed, Jun 13, 2012 at 6:00 AM, Hugo Trippaers=20
>><HTrippaers@schubergphilis.com> wrote:
>>> Actually attaching the patch should help I think.
>