Hi Aline, I have sent V2 incorporated your comments. Thanks, Archana Singh On 09/01/2016 01:59 AM, Aline Manera wrote:

Hi Archana,

The overall patch looks good to me.

I just made a suggestion below to simplify the code.

On 08/31/2016 01:16 PM, archus@linux.vnet.ibm.com wrote:

...

From: Archana Singh <archus@linux.vnet.ibm.com>

1) Code ensures that additional type macvtap, ovs are only supported on s390x/s390 architecture. On other arch it throws Error for these two type. 2) Code ensures that source is provided for type macvtap, ovs and only applicable for s390x/s390 architecture. 3) Code to create/get VM ifaces for source of type macvtap or ovs on s390x/s390 architecture.

Signed-off-by: Archana Singh <archus@linux.vnet.ibm.com> --- API.json | 16 +++++++++++--- i18n.py | 9 +++++++- model/vmifaces.py | 60 +++++++++++++++++++++++++++++++++++++++++++++------ xmlutils/interface.py | 8 +++++++ 4 files changed, 82 insertions(+), 11 deletions(-)

diff --git a/API.json b/API.json index 8899dd2..adaa2f7 100644 --- a/API.json +++ b/API.json @@ -511,18 +511,28 @@ "error": "KCHVMIF0007E", "properties": { "type": { - "description": "The type of VM network interface that libvirt supports", + "description": "The type of VM network interface that libvirt supports. Type 'macvtap' for host network interface (Ethernet, Bond, VLAN) to be connected as direct MacVTap or 'ovs' for openvswitch host network interface to be connected as virtual switch to a VM or 'network' for libvirt virtual network to be connected to VM. ", "type": "string", - "pattern": "^network$", + "pattern": "^network|macvtap|ovs$", "required": true, "error": "KCHVMIF0004E" }, "network": { "description": "the name of one available network", - "minLength": 1, "type": "string", "error": "KCHVMIF0005E" }, + "source": { + "description": "The host network interface name. It should be name of host network interface(Ethernet, Bond, VLAN) for type 'macvtap' and name of host openvswitch bridge interface for type ovs", + "type": "string", + "error": "KCHVMIF0016E" + }, + "mode": { + "description": "Only applicable for macvtap ifaces type. That indicates whether packets will be delivered directly to target device (bridge) or to the external bridge (vepa-capable bridge)", + "type": "string", + "pattern": "^bridge|vepa$", + "error": "KCHVMIF0017E" + }, "model": { "description": "model of emulated network interface card", "type": "string", diff --git a/i18n.py b/i18n.py index 90dc67d..6731601 100644 --- a/i18n.py +++ b/i18n.py @@ -151,7 +151,7 @@ messages = {

"KCHVMIF0001E": _("Interface %(iface)s does not exist in virtual machine %(name)s"), "KCHVMIF0002E": _("Network %(network)s specified for virtual machine %(name)s does not exist"), - "KCHVMIF0004E": _("Supported virtual machine interfaces type is only network"), + "KCHVMIF0004E": _("Supported virtual machine interfaces type are network, ovs and macvtap.Type ovs and macvtap are only supported for s390x/s390 architecture."), "KCHVMIF0005E": _("Network name for virtual machine interface must be a string"), "KCHVMIF0006E": _("Invalid network model card specified for virtual machine interface"), "KCHVMIF0007E": _("Specify type and network to add a new virtual machine interface"), @@ -159,6 +159,13 @@ messages = { "KCHVMIF0009E": _("MAC Address %(mac)s already exists in virtual machine %(name)s"), "KCHVMIF0010E": _("Invalid MAC Address"), "KCHVMIF0011E": _("Cannot change MAC address of a running virtual machine"), + "KCHVMIF0012E": _("Type macvtap and ovs are only supported on s390x/s390 architecture."), + "KCHVMIF0013E": _("Source attribute is only supported on s390x/s390 architecture."), + "KCHVMIF0014E": _("If source is provided, only type supported are macvtap and ovs."), + "KCHVMIF0015E": _("For type macvtap and ovs, source has to be provided"), + "KCHVMIF0016E": _("Source name for virtual machine interface must be string"), + "KCHVMIF0017E": _("Invalid source mode. Valid options are: bridge or vepa."), +

"KCHTMPL0001E": _("Template %(name)s already exists"), "KCHTMPL0002E": _("Source media %(path)s not found"), diff --git a/model/vmifaces.py b/model/vmifaces.py index 0ec89b6..d2939ba 100644 --- a/model/vmifaces.py +++ b/model/vmifaces.py @@ -18,6 +18,7 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

import libvirt +import os import random from lxml import etree, objectify

@@ -42,8 +43,6 @@ class VMIfacesModel(object):

def create(self, vm, params): conn = self.conn.get() - networks = conn.listNetworks() + conn.listDefinedNetworks() - networks = map(lambda x: x.decode('utf-8'), networks)

if params['type'] == 'network': network = params.get("network") @@ -51,10 +50,43 @@ class VMIfacesModel(object): if network is None: raise MissingParameter('KCHVMIF0007E')

+ networks = conn.listNetworks() + conn.listDefinedNetworks() + networks = map(lambda x: x.decode('utf-8'), networks) + if network not in networks: raise InvalidParameter('KCHVMIF0002E', {'name': vm, 'network': network})

+ # For architecture other than s390x/s390 type ovs/macvtap + # and source interface are not supported. + if os.uname()[4] not in ['s390x', 's390']:

...

+ if params['type'] == 'ovs' or params['type'] == 'macvtap':

Tip:

if params['type'] in ['ovs', 'macvtap']

...

+ raise InvalidParameter('KCHVMIF0012E') + if params.get('source'): + raise InvalidParameter('KCHVMIF0013E') + + # For s390x/s390 architecture + if os.uname()[4] in ['s390x', 's390']: + params['name'] = params.get("source", None) +

...

+ # For type ovs and mavtap, source interface has to be provided. + if params['name'] is None and (params['type'] == 'ovs' or + params['type'] == 'macvtap'):

Same suggestion here.

...

+ raise InvalidParameter('KCHVMIF0015E') + # If source interface provided, only type supported are ovs + # and mavtap.

...

+ if params['name'] is not None and params['type'] != 'ovs' and \ + params['type'] != 'macvtap': + raise InvalidParameter('KCHVMIF0014E')

And here.

...

+ + # FIXME: Validation if source interface exists. + if params['type'] == 'macvtap': + params['type'] = 'direct' + params['mode'] = params.get('mode', None) + elif params['type'] == 'ovs': + params['type'] = 'bridge' + params['virtualport_type'] = 'openvswitch' + macs = (iface.mac.get('address') for iface in self.get_vmifaces(vm, self.conn))

@@ -83,7 +115,6 @@ class VMIfacesModel(object): flags |= libvirt.VIR_DOMAIN_AFFECT_CONFIG if DOM_STATE_MAP[dom.info()[0]] != "shutoff": flags |= libvirt.VIR_DOMAIN_AFFECT_LIVE - dom.attachDeviceFlags(xml, flags)

return params['mac'] @@ -126,13 +157,28 @@ class VMIfaceModel(object):

info['type'] = iface.attrib['type'] info['mac'] = iface.mac.get('address') - info['network'] = iface.source.get('network') + + if iface.attrib.get('virtualport'): + info['virtualport'] = iface.virtualport.get('type') + + if info['type'] == 'direct': + info['source'] = iface.source.get('dev') + info['mode'] = iface.source.get('mode') + info['type'] = 'macvtap' + elif (info['type'] == 'bridge' and + info.get('virtualport') == 'openvswitch'): + info['source'] = iface.source.get('bridge') + info['type'] = 'ovs' + else: + info['network'] = iface.source.get('network') + if iface.find("model") is not None: info['model'] = iface.model.get('type') - if info['type'] == 'bridge': + if info['type'] == 'bridge' and \ + info.get('virtualport') != 'openvswitch': info['bridge'] = iface.source.get('bridge') - info['ips'] = self._get_ips(vm, info['mac'], info['network']) - + if info.get('network'): + info['ips'] = self._get_ips(vm, info['mac'], info['network']) return info

def _get_ips(self, vm, mac, network): diff --git a/xmlutils/interface.py b/xmlutils/interface.py index 4b9fe10..3b8ad91 100644 --- a/xmlutils/interface.py +++ b/xmlutils/interface.py @@ -98,6 +98,10 @@ def get_iface_macvtap_xml(params, arch=None, os_distro=None, os_version=None):

interface.append(E.model(type=model))

+ mac = params.get('mac', None) + if mac is not None: + interface.append(E.mac(address=mac)) + return ET.tostring(interface, encoding='utf-8', pretty_print=True)

@@ -124,4 +128,8 @@ def get_iface_ovs_xml(params, arch=None, os_distro=None, os_version=None):

interface.append(E.model(type=model))

+ mac = params.get('mac', None) + if mac is not None: + interface.append(E.mac(address=mac)) + return ET.tostring(interface, encoding='utf-8', pretty_print=True)