incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hugo Trippaers <HTrippa...@schubergphilis.com>
Subject RE: Nicira integration pre-review
Date Tue, 19 Jun 2012 06:55:46 GMT


-----Original Message-----
From: Chiradeep Vittal [mailto:Chiradeep.Vittal@citrix.com] 
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) 
>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 ?

HT: That is done by the NiciraNvpNetworkGuru during implement and shutdown. The Guru will
make sure the network (logical switch) is created and destroyed 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 
>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.

Ok :-)

>
> - 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 '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 technology 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 tell which isolation
methods it supports and the NetworkManager will check this before asking a guru to design
a network.

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

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 
>><HTrippaers@schubergphilis.com> wrote:
>>> Actually attaching the patch should help I think.
>


Mime
View raw message