On Tue, Sep 1, 2015 at 8:06 PM, Dimitris Aragiorgis < [email protected]> wrote:
> * Hrvoje Ribicic <[email protected]> [2015-08-31 19:28:31 +0200]: > > > On Tue, Aug 18, 2015 at 12:06 PM, Dimitris Aragiorgis < > > [email protected]> wrote: > > > > > Change hotplug-related methods (i.e., HotAddNic() and HotAddDisk()) > > > to use slot 'hvinfo' instead of slot 'pci'. > > > > > > Change HasPCIDevice() to HasDevice(). This will parse the output of > > > the 'query-block' and 'query-pci' QMP commands and try to match the > > > given device ID. > > > > > > Make _VerifyHotplugCommand() depend only on the device ID and not > > > on the whole device object. > > > > > > Signed-off-by: Dimitris Aragiorgis <[email protected]> > > > --- > > > lib/hypervisor/hv_kvm/__init__.py | 14 +++++----- > > > lib/hypervisor/hv_kvm/monitor.py | 52 > > > +++++++++++++++++++++++++------------ > > > 2 files changed, 42 insertions(+), 24 deletions(-) > > > > > > diff --git a/lib/hypervisor/hv_kvm/__init__.py > > > b/lib/hypervisor/hv_kvm/__init__.py > > > index 7d5b0b2..4c13253 100644 > > > --- a/lib/hypervisor/hv_kvm/__init__.py > > > +++ b/lib/hypervisor/hv_kvm/__init__.py > > > @@ -274,8 +274,7 @@ def _GetExistingDeviceInfo(dev_type, device, > runtime): > > > @type runtime: tuple (cmd, nics, hvparams, disks) > > > @param runtime: the runtime data to search for the device > > > @raise errors.HotplugError: in case the requested device does not > > > - exist (e.g. device has been added without --hotplug option) or > > > - device info has not pci slot (e.g. old devices in the cluster) > > > + exist (e.g. device has been added without --hotplug option) > > > > > > """ > > > index = _DEVICE_RUNTIME_INDEX[dev_type] > > > @@ -2021,18 +2020,17 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > > return bus_slots > > > > > > @_with_qmp > > > - def _VerifyHotplugCommand(self, _instance, > > > - device, kvm_devid, should_exist): > > > + def _VerifyHotplugCommand(self, _instance, kvm_devid, should_exist): > > > """Checks if a previous hotplug command has succeeded. > > > > > > Depending on the should_exist value, verifies that an entry > > > identified by > > > - the PCI slot and device ID is present or not. > > > + device ID is present or not. > > > > > > @raise errors.HypervisorError: if result is not the expected one > > > > > > """ > > > for i in range(5): > > > - found = self.qmp.HasPCIDevice(device, kvm_devid) > > > + found = self.qmp.HasDevice(kvm_devid) > > > logging.info("Verifying hotplug command (retry %s): %s", i, > found) > > > if found and should_exist: > > > break > > > @@ -2077,7 +2075,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > > self.qmp.HotAddNic(device, kvm_devid, tapfds, vhostfds, > features) > > > utils.WriteFile(self._InstanceNICFile(instance.name, seq), > > > data=tap) > > > > > > - self._VerifyHotplugCommand(instance, device, kvm_devid, True) > > > + self._VerifyHotplugCommand(instance, kvm_devid, True) > > > # update relevant entries in runtime file > > > index = _DEVICE_RUNTIME_INDEX[dev_type] > > > entry = _RUNTIME_ENTRY[dev_type](device, extra) > > > @@ -2104,7 +2102,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > > elif dev_type == constants.HOTPLUG_TARGET_NIC: > > > self.qmp.HotDelNic(kvm_devid) > > > utils.RemoveFile(self._InstanceNICFile(instance.name, seq)) > > > - self._VerifyHotplugCommand(instance, kvm_device, kvm_devid, False) > > > + self._VerifyHotplugCommand(instance, kvm_devid, False) > > > index = _DEVICE_RUNTIME_INDEX[dev_type] > > > runtime[index].remove(entry) > > > self._SaveKVMRuntime(instance, runtime) > > > diff --git a/lib/hypervisor/hv_kvm/monitor.py > > > b/lib/hypervisor/hv_kvm/monitor.py > > > index f2b7d02..96dcea8 100644 > > > --- a/lib/hypervisor/hv_kvm/monitor.py > > > +++ b/lib/hypervisor/hv_kvm/monitor.py > > > @@ -255,7 +255,7 @@ class QmpConnection(MonitorSocket): > > > _CAPABILITIES_COMMAND = "qmp_capabilities" > > > _QUERY_COMMANDS = "query-commands" > > > _MESSAGE_END_TOKEN = "\r\n" > > > - _QEMU_PCI_SLOTS = 32 # The number of PCI slots QEMU exposes by > default > > > + _QEMU_PCI_SLOTS = 16 # The number of PCI slots QEMU exposes by > default > > > > > > def __init__(self, monitor_filename): > > > super(QmpConnection, self).__init__(monitor_filename) > > > @@ -501,13 +501,10 @@ class QmpConnection(MonitorSocket): > > > self.Execute("netdev_add", arguments) > > > > > > arguments = { > > > - "driver": "virtio-net-pci", > > > - "id": devid, > > > - "bus": "pci.0", > > > - "addr": hex(nic.pci), > > > "netdev": devid, > > > "mac": nic.mac, > > > } > > > + arguments.update(nic.hvinfo) > > > > > > > I feel it's a bit dangerous to so closely tie these two together. Can you > > at least add a test that checks that a created hvinfo object has only > these > > four parameters, and nothing else in it? > > > > I guess you mean a unittest for _GenerateDeviceHVInfo(). Will do. > > Yep - also make sure to add a comment stating that should the format change, this function should be changed as well. Thanks! > > > > > if enable_mq: > > > arguments.update({ > > > "mq": "on", > > > @@ -559,12 +556,9 @@ class QmpConnection(MonitorSocket): > > > self._RemoveFdset(fdset) > > > > > > arguments = { > > > - "driver": "virtio-blk-pci", > > > - "id": devid, > > > - "bus": "pci.0", > > > - "addr": hex(disk.pci), > > > "drive": devid, > > > } > > > + arguments.update(disk.hvinfo) > > > self.Execute("device_add", arguments) > > > > > > @_ensure_connection > > > @@ -589,21 +583,47 @@ class QmpConnection(MonitorSocket): > > > devices = bus["devices"] > > > return devices > > > > > > - @_ensure_connection > > > - def HasPCIDevice(self, device, devid): > > > - """Check if a specific device exists or not on a running instance. > > > - > > > - It will match the PCI slot of the device and the id currently > > > - obtained by _GenerateDeviceKVMId(). > > > + def _HasPCIDevice(self, devid): > > > + """Check if a specific device ID exists on the PCI bus. > > > > > > """ > > > for d in self._GetPCIDevices(): > > > - if d["qdev_id"] == devid and d["slot"] == device.pci: > > > + if d["qdev_id"] == devid: > > > + return True > > > + > > > + return False > > > + > > > + def _GetBlockDevices(self): > > > + """Get the block devices of a running instance. > > > + > > > > > > > Please document the return value here - it would be very useful to know > > exactly what is expected from QEMU. > > > > I guess a sample output will do. > > I meant epydoc, but this is better :) > > > > > + """ > > > + self._check_connection() > > > + devices = self.Execute("query-block") > > > + return devices > > > + > > > + def _HasBlockDevice(self, devid): > > > + """Check if a specific device ID exists among block devices. > > > + > > > + """ > > > + for d in self._GetBlockDevices(): > > > + if d["device"] == devid: > > > return True > > > > > > return False > > > > > > @_ensure_connection > > > + def HasDevice(self, devid): > > > + """Check if a specific device exists or not on a running instance > > > + > > > + It first checks PCI devices and the block devices > > > > > > > Very minor nit: apply same punctuation to all docstrings. > > > > Minor but correct :) > > > > > > + > > > + """ > > > + if (self._HasPCIDevice(devid) or self._HasBlockDevice(devid)): > > > + return True > > > + > > > + return False > > > + > > > + @_ensure_connection > > > def GetFreePCISlot(self): > > > """Get the first available PCI slot of a running instance. > > > > > > -- > > > 1.7.10.4 > > > > > > > > Hrvoje Ribicic > > Ganeti Engineering > > Google Germany GmbH > > Dienerstr. 12, 80331, München > > > > Geschäftsführer: Graham Law, Christine Elizabeth Flores > > Registergericht und -nummer: Hamburg, HRB 86891 > > Sitz der Gesellschaft: Hamburg > Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
