Hello Chiradeep, Thanks a lot for the review, comments inline.
Cheers, Hugo -----Original Message----- From: Chiradeep Vittal [mailto:[email protected]] Sent: Friday, June 15, 2012 8:56 AM To: CloudStack DeveloperList Cc: Somik Behera ([email protected]) 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_Conv 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. - 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' 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'. 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" <[email protected]> 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:[email protected]] >Sent: Wednesday, June 13, 2012 2:27 PM >To: [email protected] >Cc: Somik Behera ([email protected]) >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 ><[email protected]> wrote: >> Actually attaching the patch should help I think.
