Return-Path: 
 <cloudstack-dev-return-3101-apmail-incubator-cloudstack-dev-archive=incubator.apache.org@incubator.apache.org>
X-Original-To: apmail-incubator-cloudstack-dev-archive@minotaur.apache.org
Delivered-To: apmail-incubator-cloudstack-dev-archive@minotaur.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id CBCF7D60A
	for <apmail-incubator-cloudstack-dev-archive@minotaur.apache.org>;
 Mon,  6 Aug 2012 01:36:56 +0000 (UTC)
Received: (qmail 21889 invoked by uid 500); 6 Aug 2012 01:36:56 -0000
Delivered-To: apmail-incubator-cloudstack-dev-archive@incubator.apache.org
Received: (qmail 21856 invoked by uid 500); 6 Aug 2012 01:36:56 -0000
Mailing-List: contact cloudstack-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:cloudstack-dev-help@incubator.apache.org>
List-Unsubscribe: <mailto:cloudstack-dev-unsubscribe@incubator.apache.org>
List-Post: <mailto:cloudstack-dev@incubator.apache.org>
List-Id: <cloudstack-dev.incubator.apache.org>
Reply-To: cloudstack-dev@incubator.apache.org
Delivered-To: mailing list cloudstack-dev@incubator.apache.org
Received: (qmail 21842 invoked by uid 99); 6 Aug 2012 01:36:56 -0000
Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40)
    by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 06 Aug 2012 01:36:56 +0000
Received: from reviews.apache.org (localhost [127.0.0.1])
	by reviews.apache.org (Postfix) with ESMTP id 6C9501C1863;
	Mon,  6 Aug 2012 01:36:55 +0000 (UTC)
Content-Type: multipart/alternative;
 boundary="===============6582241733680068566=="
MIME-Version: 1.0
Subject: Re: Review Request:  Introduce pluggable vif driver support on KVM
From: "Tomoe Sugihara" <tomoe@midokura.com>
To: "edison su" <edison.su@citrix.com>,
 "Chiradeep Vittal" <chiradeepv@gmail.com>
Cc: "cloudstack" <cloudstack-dev@incubator.apache.org>,
 "Tomoe Sugihara" <tomoe@midokura.com>
Date: Mon, 06 Aug 2012 01:36:55 -0000
Message-ID: <20120806013655.5557.971@reviews.apache.org>
X-ReviewBoard-URL: https://reviews.apache.org
Auto-Submitted: auto-generated
Sender: "Tomoe Sugihara" <noreply@reviews.apache.org>
X-ReviewGroup: cloudstack
X-ReviewRequest-URL: https://reviews.apache.org/r/6285/
X-Sender: "Tomoe Sugihara" <noreply@reviews.apache.org>
References: <20120803220323.22302.81167@reviews.apache.org>
In-Reply-To: <20120803220323.22302.81167@reviews.apache.org>
Reply-To: "Tomoe Sugihara" <tomoe@midokura.com>

--===============6582241733680068566==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable



> On Aug. 3, 2012, 10:03 p.m., Chiradeep Vittal wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVi=
fDriver.java, line 35
> > <https://reviews.apache.org/r/6285/diff/3/?file=3D133630#file133630line=
35>
> >
> >     This should be an Interface rather than an abstract class
> >     The interface should belong in com.cloud.resource in core and shoul=
d be called VifDriver. You can have a subclass that is abstract (VifDriverB=
ase) as well. The constructor should not throw exceptions, instead you shou=
ld 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 fo=
r 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 hyperv=
isors, 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/LibvirtBr=
idgeVifDriver.java PRE-CREATION =

>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtCo=
mputingResource.java 73101a9896c4863bc75502ff8b0b6daceff52dc4 =

>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVi=
fDriver.java PRE-CREATION =

> =

> Diff: https://reviews.apache.org/r/6285/diff/
> =

> =

> Testing
> -------
> =

> Manually tested launching VM on all-in-one KVM box with Advanced networki=
ng.
> =

> =

> Thanks,
> =

> Tomoe Sugihara
> =

>


--===============6582241733680068566==--