> On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java, > > line 49 > > <https://reviews.apache.org/r/6285/diff/3/?file=133628#file133628line49> > > > > This should not be a concern of the vif driver. Leave it in > > LibVirtComputingResource
The default bridge implementation inside LibvirtComutingResource depends on bunch of utility methods from VirtualRoutingReesource to manage bridges. IMO, this part should be cleaned up a lot. But this is a compromise given the time frame. I can try clean up a little more. > On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java, > > line 77 > > <https://reviews.apache.org/r/6285/diff/3/?file=133628#file133628line77> > > > > Instead of requiring the nicModel in the API definition, you can > > configure this subclass with a reference to LibVirtComputingResource. The > > plug API then looks like > > plug(NicTO nic, String guestOsType). The implementation of plug can ask > > LibVirtComputingResource for the nicModel Why would you want to pass guestOsType to vif driver even though responsibility of determining nicModel is in LibvirtComputingResource? If a different implementation of vif driver could choose a different model for a guestOsType, that would make sense. Which one should have the responsibility? > On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java, > > line 121 > > <https://reviews.apache.org/r/6285/diff/3/?file=133628#file133628line121> > > > > Is it "nothing needed" or TODO? Please explain either way Will do. > On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java, > > line 150 > > <https://reviews.apache.org/r/6285/diff/3/?file=133628#file133628line150> > > > > Not sure why the virtRouterResource gets involved here. Perhaps the > > method can be moved from there to here? Agreed. As I said before, this part should really be cleaned up. I'll think about it more how much I can clean up > On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java, > > line 777 > > <https://reviews.apache.org/r/6285/diff/3/?file=133629#file133629line777> > > > > This is where I would call configure() on the vifDriver Will do. > On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java, > > line 781 > > <https://reviews.apache.org/r/6285/diff/3/?file=133629#file133629line781> > > > > Should not printStackTrace, use logger.error() instead Will fix. - Tomoe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6285/#review9838 ----------------------------------------------------------- On Aug. 2, 2012, 11:18 p.m., Tomoe Sugihara wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6285/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2012, 11:18 p.m.) > > > Review request for cloudstack, edison su and Chiradeep Vittal. > > > Description > ------- > > Remove methods that are no longer needed in LibvirtComputingResource.java > > Signed-off-by: Tomoe Sugihara <to...@midokura.com> > > Add unplug() hook > > Signed-off-by: Tomoe Sugihara <to...@midokura.com> > > Add vif driver support > > - define abstract class named LibvirtVifDriver > - add LibvirtBridgeVifDriver to support current network implementation > - unplug() will be added > > Signed-off-by: Tomoe Sugihara <to...@midokura.com> > > > Diffs > ----- > > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java > PRE-CREATION > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java > 73101a9896c4863bc75502ff8b0b6daceff52dc4 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVifDriver.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/6285/diff/ > > > Testing > ------- > > Manually tested launching VM on all-in-one KVM box with Advanced networking. > > > Thanks, > > Tomoe Sugihara > >
