Re: [Kimchi-devel] [PATCH V1] [Kimchi 1/2] Updated code to support VM ifaces on s390x/s390 architecture.

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(a)linux.vnet.ibm.com wrote: > From: Archana Singh <archus(a)linux.vnet.ibm.com&gt; > > 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(a)linux.vnet.ibm.com&gt; > --- > 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)