incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tomoe Sugihara" <to...@midokura.com>
Subject Re: Review Request: Introduce pluggable vif driver support on KVM
Date Sat, 04 Aug 2012 02:40:20 GMT


> 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 <tomoe@midokura.com>
> 
> Add unplug() hook
> 
> Signed-off-by: Tomoe Sugihara <tomoe@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 <tomoe@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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message