On 8/3/12 7:40 PM, "Tomoe Sugihara" <to...@midokura.com> wrote:
> > >> On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: >> > >>plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBrid >>geVifDriver.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/LibvirtBrid >>geVifDriver.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? Good point. That logic should be moved in to the LibVirtVifDriver as well. > > >> On Aug. 3, 2012, 10:31 p.m., Chiradeep Vittal wrote: >> > >>plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBrid >>geVifDriver.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/LibvirtBrid >>geVifDriver.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/LibvirtComp >>utingResource.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/LibvirtComp >>utingResource.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/LibvirtBrid >>geVifDriver.java PRE-CREATION >> >>plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComp >>utingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4 >> >>plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVifD >>river.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 >> >> >
