Thanks for the feedback. I've merged the changes into the master branch.
The associated Jenkins run is here: https://builds.apache.org/job/cloudstack-master-maven/487/ Commits that are part of this merge: a0373fe Summary: Apply feedback from Wido a8fcc4f Summary: Add documentation for the KVM support for OpenVswitch 2d69a18 Summary: Begone pesky tabs 9f00302 Summary: Polish and shine 4267a3f Summary: copy/paste error 87fe646 Summary: small fix causing trouble when shutting down virtual machines 1ceecc9 Summary: Fix logic error e0ecf7b Summary: Add vlan configuration to the network inteface definition 68523e6 Summary: Add initial support for OpenVswitch to the KVM hypervisor 3d570c7 Summary: Add vlan configuration to the network inteface definition a0ade28 Summary: Add initial support for OpenVswitch to the KVM hypervisor 11fd121 Summary: Polish and shine 32c5fed Summary: copy/paste error 8738fee Summary: small fix causing trouble when shutting down virtual machines 4764f98 Summary: Fix logic error c6916ff Summary: Add vlan configuration to the network inteface definition c9c40d0 Summary: Add initial support for OpenVswitch to the KVM hypervisor 9adf277 Summary: Add vlan configuration to the network inteface definition ca57868 Summary: Add initial support for OpenVswitch to the KVM hypervisor Issues resolved with this merge CLOUDSTACK-824 CLOUDSTACK-727 CLOUDSTACK-934 Cheers, Hugo -----Original Message----- From: Alex Huang [mailto:alex.hu...@citrix.com] Sent: maandag 21 januari 2013 17:52 To: cloudstack-dev@incubator.apache.org Subject: RE: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch 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 > > >
