On 08/04/2015 09:34, Crístian Deives wrote:
> Currently, network interfaces can only be added to shutoff VMs. However,
> the use might want to attach an interface to a running VM as well.
>
> Allow network interfaces to be attached also while the VM is running.
> The related test cases have been updated to perform the same operations
> on both a shutoff and a running VM.
>
> Fix issue #548 (Kimchi does not support NIC hot plug) - backend only.
>
> Signed-off-by: Crístian Deives <cristiandeives@gmail.com>
> ---
>   src/kimchi/i18n.py           |  1 -
>   src/kimchi/model/vmifaces.py | 45 ++++++++++++++---------
>   tests/test_model.py          | 86 ++++++++++++++++++++++----------------------
>   3 files changed, 73 insertions(+), 59 deletions(-)
>
> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
> index 46990a5..d4c1cc6 100644
> --- a/src/kimchi/i18n.py
> +++ b/src/kimchi/i18n.py
> @@ -118,7 +118,6 @@ 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"),
> -    "KCHVMIF0003E": _("Do not support guest interface hot plug attachment"),
>       "KCHVMIF0004E": _("Supported virtual machine interfaces type is only network"),
>       "KCHVMIF0005E": _("Network name for virtual machine interface must be a string"),
>       "KCHVMIF0006E": _("Invalid network model card specified for virtual machine interface"),
> diff --git a/src/kimchi/model/vmifaces.py b/src/kimchi/model/vmifaces.py
> index 0c10fa9..a64315d 100644
> --- a/src/kimchi/model/vmifaces.py
> +++ b/src/kimchi/model/vmifaces.py
> @@ -22,7 +22,7 @@ import random
>   import libvirt
>   from lxml import etree, objectify
>
> -from kimchi.exception import InvalidOperation, InvalidParameter, NotFoundError
> +from kimchi.exception import InvalidParameter, NotFoundError
>   from kimchi.model.config import CapabilitiesModel
>   from kimchi.model.vms import DOM_STATE_MAP, VMModel
>   from kimchi.xmlutils.interface import get_iface_xml
> @@ -48,10 +48,6 @@ class VMIfacesModel(object):
>               raise InvalidParameter("KCHVMIF0002E",
>                                      {'name': vm, 'network': params["network"]})
>
> -        dom = VMModel.get_vm(vm, self.conn)
> -        if DOM_STATE_MAP[dom.info()[0]] != "shutoff":
> -            raise InvalidOperation("KCHVMIF0003E")
> -
>           macs = (iface.mac.get('address')
>                   for iface in self.get_vmifaces(vm, self.conn))
>
> @@ -60,10 +56,19 @@ class VMIfacesModel(object):
>               if params['mac'] not in macs:
>                   break
>
> +        dom = VMModel.get_vm(vm, self.conn)
> +

We need to restrict the hot plug operation to the drivers that supports it.

>           os_data = VMModel.vm_get_os_metadata(dom, self.caps.metadata_support)
>           os_distro, os_version = os_data
>           xml = get_iface_xml(params, conn.getInfo()[0], os_distro, os_version)
> -        dom.attachDeviceFlags(xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT)
> +
> +        flags = 0
> +        if dom.isPersistent():
> +            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']
>
> @@ -118,14 +123,16 @@ class VMIfaceModel(object):
>           dom = VMModel.get_vm(vm, self.conn)
>           iface = self._get_vmiface(vm, mac)
>
> -        if DOM_STATE_MAP[dom.info()[0]] != "shutoff":
> -            raise InvalidOperation("KCHVMIF0003E")
> -
>           if iface is None:
>               raise NotFoundError("KCHVMIF0001E", {'name': vm, 'iface': mac})
>
> -        dom.detachDeviceFlags(etree.tostring(iface),
> -                              libvirt.VIR_DOMAIN_AFFECT_CURRENT)
> +        flags = 0
> +        if dom.isPersistent():
> +            flags |= libvirt.VIR_DOMAIN_AFFECT_CONFIG
> +        if DOM_STATE_MAP[dom.info()[0]] != "shutoff":
> +            flags |= libvirt.VIR_DOMAIN_AFFECT_LIVE
> +
> +        dom.detachDeviceFlags(etree.tostring(iface), flags)
>
>       def update(self, vm, mac, params):
>           dom = VMModel.get_vm(vm, self.conn)
> @@ -134,16 +141,22 @@ class VMIfaceModel(object):
>           if iface is None:
>               raise NotFoundError("KCHVMIF0001E", {'name': vm, 'iface': mac})
>
> -        # FIXME we will support to change the live VM configuration later.
> +        flags = 0
> +        if dom.isPersistent():
> +            flags |= libvirt.VIR_DOMAIN_AFFECT_CONFIG
> +        if DOM_STATE_MAP[dom.info()[0]] != "shutoff":
> +            flags |= libvirt.VIR_DOMAIN_AFFECT_LIVE
> +
>           if iface.attrib['type'] == 'network' and 'network' in params:
>               iface.source.attrib['network'] = params['network']
>               xml = etree.tostring(iface)
> -            dom.updateDeviceFlags(xml, flags=libvirt.VIR_DOMAIN_AFFECT_CONFIG)
>
> -        # change on the persisted VM configuration only.
> -        if 'model' in params and dom.isPersistent():
> +            dom.updateDeviceFlags(xml, flags=flags)
> +
> +        if 'model' in params:
>               iface.model.attrib["type"] = params['model']
>               xml = etree.tostring(iface)
> -            dom.updateDeviceFlags(xml, flags=libvirt.VIR_DOMAIN_AFFECT_CONFIG)
> +
> +            dom.updateDeviceFlags(xml, flags=flags)
>
>           return mac
> diff --git a/tests/test_model.py b/tests/test_model.py
> index 63bd721..39e4fe6 100644
> --- a/tests/test_model.py
> +++ b/tests/test_model.py
> @@ -297,9 +297,6 @@ class ModelTests(unittest.TestCase):
>               params = {'name': 'test', 'disks': [], 'cdrom': UBUNTU_ISO}
>               inst.templates_create(params)
>               rollback.prependDefer(inst.template_delete, 'test')
> -            params = {'name': 'kimchi-ifaces', 'template': '/templates/test'}
> -            inst.vms_create(params)
> -            rollback.prependDefer(inst.vm_delete, 'kimchi-ifaces')
>
>               # Create a network
>               net_name = 'test-network'
> @@ -311,45 +308,50 @@ class ModelTests(unittest.TestCase):
>               inst.network_activate(net_name)
>               rollback.prependDefer(inst.network_deactivate, net_name)
>
> -            ifaces = inst.vmifaces_get_list('kimchi-ifaces')
> -            self.assertEquals(1, len(ifaces))
> -
> -            iface = inst.vmiface_lookup('kimchi-ifaces', ifaces[0])
> -            self.assertEquals(17, len(iface['mac']))
> -            self.assertEquals("default", iface['network'])
> -            self.assertIn("model", iface)
> -
> -            # attach network interface to vm
> -            iface_args = {"type": "network",
> -                          "network": "test-network",
> -                          "model": "virtio"}
> -            mac = inst.vmifaces_create('kimchi-ifaces', iface_args)
> -            # detach network interface from vm
> -            rollback.prependDefer(inst.vmiface_delete, 'kimchi-ifaces', mac)
> -            self.assertEquals(17, len(mac))
> -
> -            iface = inst.vmiface_lookup('kimchi-ifaces', mac)
> -            self.assertEquals("network", iface["type"])
> -            self.assertEquals("test-network", iface['network'])
> -            self.assertEquals("virtio", iface["model"])
> -
> -            # attach network interface to vm without providing model
> -            iface_args = {"type": "network",
> -                          "network": "test-network"}
> -            mac = inst.vmifaces_create('kimchi-ifaces', iface_args)
> -            rollback.prependDefer(inst.vmiface_delete, 'kimchi-ifaces', mac)
> -
> -            iface = inst.vmiface_lookup('kimchi-ifaces', mac)
> -            self.assertEquals("network", iface["type"])
> -            self.assertEquals("test-network", iface['network'])
> -
> -            # update vm interface
> -            iface_args = {"network": "default",
> -                          "model": "e1000"}
> -            inst.vmiface_update('kimchi-ifaces', mac, iface_args)
> -            iface = inst.vmiface_lookup('kimchi-ifaces', mac)
> -            self.assertEquals("default", iface['network'])
> -            self.assertEquals("e1000", iface["model"])
> +            for vm_name in ['kimchi-ifaces', 'kimchi-ifaces-running']:
> +                params = {'name': vm_name, 'template': '/templates/test'}
> +                inst.vms_create(params)
> +                rollback.prependDefer(inst.vm_delete, vm_name)
> +
> +                ifaces = inst.vmifaces_get_list(vm_name)
> +                self.assertEquals(1, len(ifaces))
> +
> +                iface = inst.vmiface_lookup(vm_name, ifaces[0])
> +                self.assertEquals(17, len(iface['mac']))
> +                self.assertEquals("default", iface['network'])
> +                self.assertIn("model", iface)
> +
> +                # attach network interface to vm
> +                iface_args = {"type": "network",
> +                              "network": "test-network",
> +                              "model": "virtio"}
> +                mac = inst.vmifaces_create(vm_name, iface_args)
> +                # detach network interface from vm
> +                rollback.prependDefer(inst.vmiface_delete, vm_name, mac)
> +                self.assertEquals(17, len(mac))
> +
> +                iface = inst.vmiface_lookup(vm_name, mac)
> +                self.assertEquals("network", iface["type"])
> +                self.assertEquals("test-network", iface['network'])
> +                self.assertEquals("virtio", iface["model"])
> +
> +                # attach network interface to vm without providing model
> +                iface_args = {"type": "network",
> +                              "network": "test-network"}
> +                mac = inst.vmifaces_create(vm_name, iface_args)
> +                rollback.prependDefer(inst.vmiface_delete, vm_name, mac)
> +
> +                iface = inst.vmiface_lookup(vm_name, mac)
> +                self.assertEquals("network", iface["type"])
> +                self.assertEquals("test-network", iface['network'])
> +
> +                # update vm interface
> +                iface_args = {"network": "default",
> +                              "model": "e1000"}
> +                inst.vmiface_update(vm_name, mac, iface_args)
> +                iface = inst.vmiface_lookup(vm_name, mac)
> +                self.assertEquals("default", iface['network'])
> +                self.assertEquals("e1000", iface["model"])
>
>       @unittest.skipUnless(utils.running_as_root(), 'Must be run as root')
>       def test_vm_disk(self):