You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Hugo Trippaers <HT...@schubergphilis.com> on 2012/06/13 11:51:51 UTC
Nicira integration pre-review
Hey All,
As some of you know Somik Behera and me have been working on adding Nicira support to CloudStack. We have identified a few phases in the project, where phase one is L2 isolated networking. Attached to this email is a patch that will add Nicira phase 1 integration to CloudStack. We have done internal testing using this patch and now we would like to invite you to have a look as well before we submit this patch for review.
We are looking for feedback on the integration when the existing modules, we already discussed a few things in a recent meeting with Chiradeep, but real code makes it easier to see how we implemented this.
Based on the feedback and some pending thoughts about name conventions we will modify the patch and submit it for review.
Looking forward to your feedback.
The patch can also be found on GitHub at https://github.com/schubergphilis/CloudStack/commit/35e11044faf83a074987848368a0a9399e97ab28
Kind regards,
Hugo Trippaers
Re: Nicira integration pre-review
Posted by David Nalley <da...@gnsa.us>.
On Wed, Jun 13, 2012 at 8:27 AM, Chip Childers
<ch...@sungard.com> wrote:
> Attachments are apparently being stripped out by the mailer daemon.
>
> -chip
I think it's based on size.
RE: Nicira integration pre-review
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
-----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" <HT...@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" <HT...@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
>><HT...@schubergphilis.com> wrote:
>>> Actually attaching the patch should help I think.
>
Re: Nicira integration pre-review
Posted by Chiradeep Vittal <Ch...@citrix.com>.
On 6/15/12 4:27 AM, "Hugo Trippaers" <HT...@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 ?
>
> - 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
>'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: 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" <HT...@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
>><HT...@schubergphilis.com> wrote:
>>> Actually attaching the patch should help I think.
>
RE: Nicira integration pre-review
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
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_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" <HT...@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
><HT...@schubergphilis.com> wrote:
>> Actually attaching the patch should help I think.
Re: Nicira integration pre-review
Posted by Chiradeep Vittal <Ch...@citrix.com>.
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.
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.
- 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
- 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.
- 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
- 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.
On 6/13/12 6:00 AM, "Hugo Trippaers" <HT...@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
><HT...@schubergphilis.com> wrote:
>> Actually attaching the patch should help I think.
RE: Nicira integration pre-review
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
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 <HT...@schubergphilis.com> wrote:
> Actually attaching the patch should help I think.
Re: Nicira integration pre-review
Posted by Chip Childers <ch...@sungard.com>.
Attachments are apparently being stripped out by the mailer daemon.
-chip
On Wed, Jun 13, 2012 at 6:00 AM, Hugo Trippaers
<HT...@schubergphilis.com> wrote:
> Actually attaching the patch should help I think.
RE: Nicira integration pre-review
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
Actually attaching the patch should help I think.
Cheers,
Hugo
From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
Sent: Wednesday, June 13, 2012 11:52 AM
To: cloudstack-dev@incubator.apache.org
Cc: Somik Behera (somik@nicira.com)
Subject: Nicira integration pre-review
Hey All,
As some of you know Somik Behera and me have been working on adding Nicira support to CloudStack. We have identified a few phases in the project, where phase one is L2 isolated networking. Attached to this email is a patch that will add Nicira phase 1 integration to CloudStack. We have done internal testing using this patch and now we would like to invite you to have a look as well before we submit this patch for review.
We are looking for feedback on the integration when the existing modules, we already discussed a few things in a recent meeting with Chiradeep, but real code makes it easier to see how we implemented this.
Based on the feedback and some pending thoughts about name conventions we will modify the patch and submit it for review.
Looking forward to your feedback.
The patch can also be found on GitHub at https://github.com/schubergphilis/CloudStack/commit/35e11044faf83a074987848368a0a9399e97ab28
Kind regards,
Hugo Trippaers
RE: Nicira integration pre-review
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
Hey David,
Thanks for the feedback, I've modified all license headers in my bits of the code.
The phases we have currently identified together with Nicira are:
1) First integration, L2 networking only and single NVP setup for the entire cloud.
This means a static configuration for the NVP setup and the possibility to create guest networks in CloudStack using the NVP solution (create logical switch and logical port attachments). Source should be ready for production use and pushed upstream to the opensource version of CloudStack. VIrtualRouter is used for SNAT and all other features.
2) More in depth integration, Use the pluggable resource system to add a Nicira NVP device to a zone and allow different NVP setups per zone. Use some L3 features like SNAT directly from the Nicira NVP instead of the VirtualRouter.
3) Everything we don't get to do in the other phases ;-)
So currently we have allocated resources to do phase 1, which is the current patch. My goal is to finish this patch and get it upstreamed into the CloudStack master branch. When that is done we'll move to the next phase.
Cheers,
Hugo
-----Original Message-----
From: David Nalley [mailto:david@gnsa.us]
Sent: Friday, June 15, 2012 7:34 PM
To: cloudstack-dev@incubator.apache.org
Cc: Somik Behera (somik@nicira.com)
Subject: Re: Nicira integration pre-review
On Wed, Jun 13, 2012 at 5:51 AM, Hugo Trippaers <HT...@schubergphilis.com> wrote:
> Hey All,
>
>
>
> As some of you know Somik Behera and me have been working on adding
> Nicira support to CloudStack. We have identified a few phases in the
> project, where phase one is L2 isolated networking. Attached to this
> email is a patch that will add Nicira phase 1 integration to
> CloudStack. We have done internal testing using this patch and now we
> would like to invite you to have a look as well before we submit this patch for review.
>
>
>
> We are looking for feedback on the integration when the existing
> modules, we already discussed a few things in a recent meeting with
> Chiradeep, but real code makes it easier to see how we implemented this.
>
>
>
> Based on the feedback and some pending thoughts about name conventions
> we will modify the patch and submit it for review.
>
>
>
> Looking forward to your feedback.
>
>
>
> The patch can also be found on GitHub at
> https://github.com/schubergphilis/CloudStack/commit/35e11044faf83a0749
> 87848368a0a9399e97ab28
>
Hugo,
This is awesome. Thanks for letting us have a peak.
A couple of just housekeeping items:
You have license headers that don't match the ASF requirements, you may want to review this:
http://www.apache.org/legal/src-headers.html
I am also curious as to what you see the remaining phases of support to be?
--David
Re: Nicira integration pre-review
Posted by David Nalley <da...@gnsa.us>.
On Wed, Jun 13, 2012 at 5:51 AM, Hugo Trippaers
<HT...@schubergphilis.com> wrote:
> Hey All,
>
>
>
> As some of you know Somik Behera and me have been working on adding Nicira
> support to CloudStack. We have identified a few phases in the project, where
> phase one is L2 isolated networking. Attached to this email is a patch that
> will add Nicira phase 1 integration to CloudStack. We have done internal
> testing using this patch and now we would like to invite you to have a look
> as well before we submit this patch for review.
>
>
>
> We are looking for feedback on the integration when the existing modules, we
> already discussed a few things in a recent meeting with Chiradeep, but real
> code makes it easier to see how we implemented this.
>
>
>
> Based on the feedback and some pending thoughts about name conventions we
> will modify the patch and submit it for review.
>
>
>
> Looking forward to your feedback.
>
>
>
> The patch can also be found on GitHub at
> https://github.com/schubergphilis/CloudStack/commit/35e11044faf83a074987848368a0a9399e97ab28
>
Hugo,
This is awesome. Thanks for letting us have a peak.
A couple of just housekeeping items:
You have license headers that don't match the ASF requirements, you
may want to review this:
http://www.apache.org/legal/src-headers.html
I am also curious as to what you see the remaining phases of support to be?
--David