Can we use [MERGE][ACS41] instead of this format? I don't know how others setup their mail filters but mine are on tags within the '[]' and it doesn't catch this email.
Thanks. --Alex > -----Original Message----- > From: Hugo Trippaers [mailto:htrippa...@schubergphilis.com] > Sent: Monday, January 21, 2013 3:37 AM > To: cloudstack-dev@incubator.apache.org > Subject: RE: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch > > 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:w...@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 > > >
