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 Mon, 06 Aug 2012 01:36:55 GMT


> On Aug. 3, 2012, 10:03 p.m., Chiradeep Vittal wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVifDriver.java,
line 35
> > <https://reviews.apache.org/r/6285/diff/3/?file=133630#file133630line35>
> >
> >     This should be an Interface rather than an abstract class
> >     The interface should belong in com.cloud.resource in core and should be called
VifDriver. You can have a subclass that is abstract (VifDriverBase) as well. The constructor
should not throw exceptions, instead you should have an abstract configure() method that throws
a ConfigurationException
> 
> Tomoe Sugihara wrote:
>     Thanks for the review. Sure, I can do that. 
>     Note that VifDriver will be hypervisor specific (signature differes), so maybe I
should create a interface com.cloud.hypervisor.kvm.VifDriver for KVM, com.cloud.hypervisor.xen.VifDriver
for Xen. What do you think?

<chiradeep>
Hopefully you can make it more generic than that. It seems that the main
problem is the plug method. If not, that is OK
</chiradeep>

I agree, hopefully we can make it more generic.

For now, I'll create a interface at /plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java.
On KVM, different vif implementation creates different Libvirt xml, so the simplest way is
that plug() method return LibvirtVMDef.InterfaceDef, which isn't/shouldn't be in core module.
And this is not the case on other hypervisors, meaning VifDrive is hypervisor specific.


- Tomoe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review9837
-----------------------------------------------------------


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