> On March 1, 2013, 7:36 p.m., Hugo Trippaers wrote: > > Dave, looks good. > > > > Couple of questions though, there is some bridge specific code in > > LibvirtComputingResource that checks existence of interfaces and such. > > Shouldn't that code also be modified to support multiple drivers per nic? > > > > Also currently there is only the native bridge and openvswitch in here. The > > actual recommendation is to use only one of them as far as i know. What is > > the compelling reason to allow for this difference? In most cases where > > openvswitch is used this means configuring vswitch either via the native > > interface or via the brcompat layer. While possible to combine the two it > > could lead to disastrous results when interfaces start overlapping. E.g. > > both openvswitch and bridge create an interface called br0 (i've been there > > ;-) ) > > > > > > Hugo Trippaers wrote: > Scratch the first question, don't know what i was thinking... :-)
Hi Hugo, Thanks for the review and comments! The Motivation section of the FS gives some background on why we want to allow mixing VIF drivers: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Allow+specification+of+different+VIF+drivers+per+traffic+type+in+KVM <pasted> "The MidoNet plugin intends to handle Guest traffic for Guest and System VMs, and Public traffic for System VMs (Public traffic). However, we wish to let Management / Control traffic be handled by the underlying physical network using the default BridgeVifDriver. Currently, this is not possible without duplicating BridgeVifDriver code into the MidonetVifDriver, as LibVirtComputingResource assumes there is only one VIF driver." Control traffic is probably the clearest example. This traffic is all local to the host, so sending it through a virtual network is overkill. We could copy the control traffic logic (setting up link local network etc) into the MidoNet VIF driver, but what if that logic changes later? The duplication seems very undesirable to me. Since the MidoNet plugin is not checked in, I can see how the use case is less obvious. However, I figured this is a fully backwards compatible change, and I wanted to propose and commit it first, so that the plugin commit is self-contained. Hope that makes sense - let me know if you have any follow-ups. Also, I'm going to reply to Edison's comment below, but I'll need your input too. Thanks, Dave. - Dave ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9604/#review17256 ----------------------------------------------------------- On March 1, 2013, 7:46 a.m., Dave Cahill wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9604/ > ----------------------------------------------------------- > > (Updated March 1, 2013, 7:46 a.m.) > > > Review request for cloudstack, Hugo Trippaers, Wido den Hollander, and Marcus > Sorensen. > > > Description > ------- > > Feature spec: > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Allow+specification+of+different+VIF+drivers+per+traffic+type+in+KVM > > Jira ticket: > https://issues.apache.org/jira/browse/CLOUDSTACK-1311 > > Adds the ability to specify different VIF drivers per traffic type in KVM. > > > This addresses bug CLOUDSTACK-1311. > > > Diffs > ----- > > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java > 99b8723 > > plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtVifDriverTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/9604/diff/ > > > Testing > ------- > > Added unit tests to test all variations of this new configuration (no > configuration, defaults, override some traffic etc). > > Built and deployed, spun up Advanced Isolated network with two VMs, verified > internal and external connectivity. > > > Thanks, > > Dave Cahill > >
