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: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
Date Mon, 21 Jan 2013 11:36:42 GMT
https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=a0373fe1ff2001c5a9deee537d6862327f8818cc

Comments inline

> -----Original Message-----
> From: Wido den Hollander [mailto:wido@widodh.nl]
> Sent: Thursday, January 17, 2013 12:47 PM
> To: cloudstack-dev@incubator.apache.org
> Subject: Re: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
> 
> On 01/15/2013 10:32 AM, Hugo Trippaers wrote:
> > Hey all,
> >
> > I would like to merge my branch with master. This branch deals with the
> items reported in tickets CLOUDSTACK-934, CLOUDSTACK-727, CLOUDSTACK-
> 101. Basically the patch introduces support for dealing with bridges and
> interfaces on KVM hypervisors running openvswitch. Support is included for
> both vlan and Nicira NVP based networks.
> >
> > During development some bugs were discovered, following the commits
> > for those fixes
> > - 1ceecc92c88c84b99c99547f5c086657ec26f8d7 Fix logic error in
> > ConfigurationServerImpl.java
> > - 87fe64695305d6a2c505f757ab7607b765c313b7 NPE in
> > KVMStoragePoolManager
> >
> > Testing done
> > - mvn clean test
> > - setup clean CloudStack installation with KVM server using vlan based
> isolation. Deploy systemvms and several instances on isolated networks and
> test connectivity.
> > - reuse existing CloudStack installation with Nicira NVP. Add cluster with
> KVM hosts. Deploy several instances and networks and test connectivity.
> >
> > Testing not done
> > - basic networking based setup, I figured that if advanced networks work,
> basic networks should work as well.
> >
> > Risk
> > - minimal risk, anyone willing to use this feature needs to explicitly enable
> this by making changes to agent.properties for the cloud-agent. By default
> the current bridge drivers will be used.
> >
> > Database changes
> > - none
> >
> > Documentation tracked in a separate ticket: CLOUDSTACK-977
> >
> > Cheers,
> >
> 
> I've been reviewing this, but I haven't been able to test the code since I don't
> have anything for this running.
> 
> I did find some things which I report in random order :) I basically went from
> commit to commit in the branch and checked it.
> 
> Globally I've found some indentation things, like:
> 
>          switch (_bridgeType) {
>          case OPENVSWITCH:
>              getOvsPifs();
>              break;
>          case NATIVE:
>          default:
>              getPifs();
>              break;
>          }
> 
> Isn't the indentation missing there for "case"?
> 
> Further in for example OvsVifDriver there are some blank lines with spaces
> or trailing spaces.

Should all be fixed with commit https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=2d69a1855de5110c57010e248f83d88607c668fe

> 
> You also seem to write if/else-contitions in various ways, like:
> 
> if (....) {
> 
> }
> else {
> 
> }
> 
> I thought our code convention said we should use:
> 
> if (....) {
> 
> } else {
> 
> }
> 
> Not a very big deal, but just to prevent other people from taking over the
> "habbits".

Fixed :-)

> 
> In OvsVifDriver there is a method called: "deleteExitingLinkLocalRoutTable"
> 
> Isn't that a typo? Shouldn't 'Rout' be 'Route'?

Copy paste error from BridgeVifDriver, fixed.

> 
> What I don't get in LibvirtVMDef is this:
> 
> if (_virtualPortType != null) {
>    netBuilder.append("<virtualport type='" + _virtualPortType + "'>\n");
>    if (_virtualPortInterfaceId != null) {
>      netBuilder.append("<parameters interfaceid='" + _virtualPortInterfaceId
> + "'/>\n");
>    }
>    netBuilder.append("</virtualport>\n");
> }
> 
> Why should a virtualport be added if the interfaceId is null? Should the first if
> already check if both the type AND id are set? Or can the ID be set after the
> VM is running? (In OvsVifDriver?)

The virtualport with virtual port type is required for LibVirt to know the port should be
configured using OpenVswitch. Without this the default bridge implementation will be used
(according to the documentation at http://libvirt.org/formatnetwork.html)

The interface id is an optional extension that is required for using openvswitch with NiciraNvp.
This id is used to match the vif on the hypervisor with the port configured on the logical
switch in the nicira system.


> 
> If so, couldn't that be an issue if the VM is migrated and this XML definition
> isn't properly transferred to the other host?
> 
> When setting the VLAN tag in the XML definition you do this check:
> 
> if (_vlanTag != -1) {
>    netBuilder.append("<vlan trunk='no'>\n<tag id='" + _vlanTag +
> "'/>\n</vlan>"); }
> 
> It's my understanding that VLAN ID 0 and 4095 are reserved VLAN IDs, so
> shouldn't it be:
> 
> if (_vlanTag > 0) {
> ...
> }

True, fixed.

> 
> This is what I found while doing my first review. Like I said, I can't test the real
> logic now.
> 
> Wido
> 
> > Hugo
> >

Mime
View raw message