You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Tomoe Sugihara <to...@midokura.com> on 2012/08/03 01:18:03 UTC
Re: Review Request: Introduce pluggable vif driver support on KVM
-----------------------------------------------------------
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.
Changes
-------
fix typo in commit log
Summary (updated)
-----------------
Introduce pluggable vif driver support on KVM
Description (updated)
-------
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 (updated)
-----
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
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Chiradeep Vittal <Ch...@citrix.com>.
On 8/3/12 7:29 PM, "Tomoe Sugihara" <to...@midokura.com> wrote:
>
>
>> On Aug. 3, 2012, 10:03 p.m., Chiradeep Vittal wrote:
>> >
>>plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVifD
>>river.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
>
>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?
Hopefully you can make it more generic than that. It seems that the main
problem is the plug method. If not, that is OK
>
>
>- 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 <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
>>
>>
>
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
> 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
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?
- 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 <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
>
>
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
> 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 <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
>
>
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Chiradeep Vittal <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review9837
-----------------------------------------------------------
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVifDriver.java
<https://reviews.apache.org/r/6285/#comment20852>
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
- Chiradeep Vittal
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
>
>
Re: Review Request: Introduce Vif Driver in KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review10008
-----------------------------------------------------------
Any other comments or issues that I can address?
I'd like get this in for 4.0. Thanks!
- Tomoe Sugihara
On Aug. 6, 2012, 1:39 a.m., Tomoe Sugihara wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6285/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2012, 1:39 a.m.)
>
>
> Review request for cloudstack, edison su and Chiradeep Vittal.
>
>
> Description
> -------
>
> Add BridgeVifDriver and move current vif implementation to it.
>
> - remove dependency on VirtualRoutingResource.
> - factor out some of the networking code in LibvirtComputingResource
> to BridgeVifDriver.
>
> Add base class for KVM VifDriver.
>
>
> Add VifDriver Interface for KVM.
>
>
> Diffs
> -----
>
> core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 33e297187fd7deaa6f6f975a115ff3c4c80f6794
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriverBase.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
>
>
Re: Review Request: Introduce Vif Driver in KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
> On Aug. 6, 2012, 9:34 a.m., Wido den Hollander wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java, line 124
> > <https://reviews.apache.org/r/6285/diff/4/?file=134275#file134275line124>
> >
> > The general rule on Linux is still to limit interface names to 12 characters.
> >
> > CloudVirBr is already 10 characters, so with 4 characters for the VLAN number you get to 14.
> >
> > Maybe change to cVirBr<id> ?
Hi Wido,
Thanks for the comment:)
Actually this is from legacy code and I think IF name can go up to 15 chars:
http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=include/linux/if.h;h=f995c663c493baf4d369bcf05d3bed20d38dcd1f;hb=HEAD#l26
- Tomoe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review9882
-----------------------------------------------------------
On Aug. 6, 2012, 1:39 a.m., Tomoe Sugihara wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6285/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2012, 1:39 a.m.)
>
>
> Review request for cloudstack, edison su and Chiradeep Vittal.
>
>
> Description
> -------
>
> Add BridgeVifDriver and move current vif implementation to it.
>
> - remove dependency on VirtualRoutingResource.
> - factor out some of the networking code in LibvirtComputingResource
> to BridgeVifDriver.
>
> Add base class for KVM VifDriver.
>
>
> Add VifDriver Interface for KVM.
>
>
> Diffs
> -----
>
> core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 33e297187fd7deaa6f6f975a115ff3c4c80f6794
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriverBase.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
>
>
Re: Review Request: Introduce Vif Driver in KVM
Posted by Wido den Hollander <wi...@widodh.nl>.
> On Aug. 6, 2012, 9:34 a.m., Wido den Hollander wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java, line 124
> > <https://reviews.apache.org/r/6285/diff/4/?file=134275#file134275line124>
> >
> > The general rule on Linux is still to limit interface names to 12 characters.
> >
> > CloudVirBr is already 10 characters, so with 4 characters for the VLAN number you get to 14.
> >
> > Maybe change to cVirBr<id> ?
>
> Tomoe Sugihara wrote:
> Hi Wido,
>
> Thanks for the comment:)
> Actually this is from legacy code and I think IF name can go up to 15 chars:
>
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=include/linux/if.h;h=f995c663c493baf4d369bcf05d3bed20d38dcd1f;hb=HEAD#l26
Ah, that is indeed legacy. This should be fine then since VLAN numbers can't exceed 4 digits.
- Wido
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review9882
-----------------------------------------------------------
On Aug. 6, 2012, 1:39 a.m., Tomoe Sugihara wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6285/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2012, 1:39 a.m.)
>
>
> Review request for cloudstack, edison su and Chiradeep Vittal.
>
>
> Description
> -------
>
> Add BridgeVifDriver and move current vif implementation to it.
>
> - remove dependency on VirtualRoutingResource.
> - factor out some of the networking code in LibvirtComputingResource
> to BridgeVifDriver.
>
> Add base class for KVM VifDriver.
>
>
> Add VifDriver Interface for KVM.
>
>
> Diffs
> -----
>
> core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 33e297187fd7deaa6f6f975a115ff3c4c80f6794
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriverBase.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
>
>
Re: Review Request: Introduce Vif Driver in KVM
Posted by Wido den Hollander <wi...@widodh.nl>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review9882
-----------------------------------------------------------
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java
<https://reviews.apache.org/r/6285/#comment21008>
The general rule on Linux is still to limit interface names to 12 characters.
CloudVirBr is already 10 characters, so with 4 characters for the VLAN number you get to 14.
Maybe change to cVirBr<id> ?
- Wido den Hollander
On Aug. 6, 2012, 1:39 a.m., Tomoe Sugihara wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6285/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2012, 1:39 a.m.)
>
>
> Review request for cloudstack, edison su and Chiradeep Vittal.
>
>
> Description
> -------
>
> Add BridgeVifDriver and move current vif implementation to it.
>
> - remove dependency on VirtualRoutingResource.
> - factor out some of the networking code in LibvirtComputingResource
> to BridgeVifDriver.
>
> Add base class for KVM VifDriver.
>
>
> Add VifDriver Interface for KVM.
>
>
> Diffs
> -----
>
> core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 33e297187fd7deaa6f6f975a115ff3c4c80f6794
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriverBase.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
>
>
Re: Review Request: Introduce Vif Driver in KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
> On Aug. 10, 2012, 7:06 p.m., edison su wrote:
> > Ship It!
Thanks for the review. I'll close this review.
- Tomoe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review10141
-----------------------------------------------------------
On Aug. 6, 2012, 1:39 a.m., Tomoe Sugihara wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6285/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2012, 1:39 a.m.)
>
>
> Review request for cloudstack, edison su and Chiradeep Vittal.
>
>
> Description
> -------
>
> Add BridgeVifDriver and move current vif implementation to it.
>
> - remove dependency on VirtualRoutingResource.
> - factor out some of the networking code in LibvirtComputingResource
> to BridgeVifDriver.
>
> Add base class for KVM VifDriver.
>
>
> Add VifDriver Interface for KVM.
>
>
> Diffs
> -----
>
> core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 33e297187fd7deaa6f6f975a115ff3c4c80f6794
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriverBase.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
>
>
Re: Review Request: Introduce Vif Driver in KVM
Posted by edison su <ed...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review10141
-----------------------------------------------------------
Ship it!
Ship It!
- edison su
On Aug. 6, 2012, 1:39 a.m., Tomoe Sugihara wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6285/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2012, 1:39 a.m.)
>
>
> Review request for cloudstack, edison su and Chiradeep Vittal.
>
>
> Description
> -------
>
> Add BridgeVifDriver and move current vif implementation to it.
>
> - remove dependency on VirtualRoutingResource.
> - factor out some of the networking code in LibvirtComputingResource
> to BridgeVifDriver.
>
> Add base class for KVM VifDriver.
>
>
> Add VifDriver Interface for KVM.
>
>
> Diffs
> -----
>
> core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 33e297187fd7deaa6f6f975a115ff3c4c80f6794
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java PRE-CREATION
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriverBase.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
>
>
Re: Review Request: Introduce Vif Driver in KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/
-----------------------------------------------------------
(Updated Aug. 6, 2012, 1:39 a.m.)
Review request for cloudstack, edison su and Chiradeep Vittal.
Changes
-------
Incorporate Chiradeep's comments.
Summary (updated)
-----------------
Introduce Vif Driver in KVM
Description (updated)
-------
Add BridgeVifDriver and move current vif implementation to it.
- remove dependency on VirtualRoutingResource.
- factor out some of the networking code in LibvirtComputingResource
to BridgeVifDriver.
Add base class for KVM VifDriver.
Add VifDriver Interface for KVM.
Diffs (updated)
-----
core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 33e297187fd7deaa6f6f975a115ff3c4c80f6794
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java PRE-CREATION
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java PRE-CREATION
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriverBase.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
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Chiradeep Vittal <Ch...@citrix.com>.
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
>>
>>
>
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
> 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
>
>
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Tomoe Sugihara <to...@midokura.com>.
> 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
>
> Tomoe Sugihara wrote:
> 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?
>
<Chiradeep>
Good point. That logic should be moved in to the LibVirtVifDriver as well.
</Chiradeep>
Ok. I have moved getGuestNicModel to VifDriverBase class with protected visibility and BridgeVifDriver inherits that.
- 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
>
>
Re: Review Request: Introduce pluggable vif driver support on KVM
Posted by Chiradeep Vittal <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6285/#review9838
-----------------------------------------------------------
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java
<https://reviews.apache.org/r/6285/#comment20855>
This should not be a concern of the vif driver. Leave it in LibVirtComputingResource
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java
<https://reviews.apache.org/r/6285/#comment20854>
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
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java
<https://reviews.apache.org/r/6285/#comment20857>
Is it "nothing needed" or TODO? Please explain either way
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java
<https://reviews.apache.org/r/6285/#comment20858>
Not sure why the virtRouterResource gets involved here. Perhaps the method can be moved from there to here?
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
<https://reviews.apache.org/r/6285/#comment20856>
This is where I would call configure() on the vifDriver
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
<https://reviews.apache.org/r/6285/#comment20853>
Should not printStackTrace, use logger.error() instead
- Chiradeep Vittal
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
>
>