+lists * Guido Trotter <[email protected]> [2013-07-31 10:05:52 +0200]:
> On Tue, Jul 30, 2013 at 8:51 PM, Dimitris Aragiorgis <[email protected]> wrote: > > * Guido Trotter <[email protected]> [2013-07-30 13:26:20 +0200]: > > > >> On Thu, Jul 25, 2013 at 1:41 AM, Dimitris Aragiorgis <[email protected]> > >> wrote: > >> > >> This is a very big patch, would it make sense to split it, introducing > >> the new methods, and using them, perhaps? > >> > > > > True. I could split it in three parts actually: > > > > - the one related to _GenerateKVMBlockDevicesOptions() > > - the one that defines new methods related to hotplug > > - the one that implements backend methods (and invokes the previous ones) > > > > Thanks. > > >> > Hotplug will not be supported for: > >> > - KVM < 1.0 > >> > - existing devices in the cluster > >> > - python-fdsend module is not installed (NIC hotplug) > >> > - chroot (Disk hotplug) > >> > - security mode other than None (Disk hotplug) > >> > If no hotplug takes place modifications will take place after reboot. > >> > > >> > New methods: > >> > Introduce new method HotplugDevice() and HotAddNic/HotDelNic, > >> > HotAddDisk/HotDelDisk helper methods that eventually make use of > >> > QEMU monitor interface for hotpluging. > >> > > >> > Device naming: > >> > QEMU monitor expects devices to be uniquely named. Device ids derive > >> > from the following function: > >> > kvm_devid = <device_type>-<part of uuid>-pci-<pci_slot> > >> > Device ids must be reproduce-able when we want to remove them. > >> > For that reason we store the pci slot inside the runtime file and > >> > in case we want to remove a device we obtain its pci slot by > >> > parsing the corresponding runtime enrty and matching the device > >> > by its uuid. > >> > > >> > Finding the PCI slot: > >> > For newly added devices Hypervisor parses existing PCI allocations > >> > (via _AnnotateFreePCISlot() and eventually ``info pci`` monitor > >> > command) and decides the PCI slot to plug in the device. During > >> > instance startup hypervisor invokes _UpdatePCISlots() for every > >> > device of the instance. Initial PCI reservations derive from KVM > >> > default setup, that allocates 4 slots for devices other than disks > >> > and NICs. > >> > > >> > NIC hotplug: > >> > - open a tap and get its file descriptor. > >> > - pass fd with SCM rights (using python-fdsend) via monitor socket > >> > - create netdev and device with id=kvm_devid and proper pci info > >> > > >> > Disk hotplug: > >> > - create drive with id=kvm_devid > >> > - create device with id=kvm_devid and corresponding pci info > >> > > >> > In order to migrate a VM, an identical VM should be booted with > >> > exactly the same pci configuration (and with -incoming option). PCI > >> > info is passed via runtime file. To this end every time a hotplug > >> > takes place runtime file must be updated. > >> > > >> > Introduce _GenerateKVMBlockDevicesOptions(): > >> > The runtime file contains one more field: block_devices. kvm_cmd is > >> > extended > >> > with block device options during _ExecuteKVMRuntime(). > >> > > >> > Handle old style format of runtime files: > >> > In case block_devices are already encapsulated inside kvm_cmd > >> > and runtime files have only 3 entries, set block_devices to []. > >> > This way migration will not fail and hotplug will succeed for > >> > new disks only. > >> > > >> > Signed-off-by: Dimitris Aragiorgis <[email protected]> > >> > --- > >> > lib/hypervisor/hv_kvm.py | 356 > >> > ++++++++++++++++++++++++++++++++++++++++------ > >> > 1 file changed, 312 insertions(+), 44 deletions(-) > >> > > >> > diff --git a/lib/hypervisor/hv_kvm.py b/lib/hypervisor/hv_kvm.py > >> > index 4bd8a9e..5bf4255 100644 > >> > --- a/lib/hypervisor/hv_kvm.py > >> > +++ b/lib/hypervisor/hv_kvm.py > >> > @@ -37,10 +37,16 @@ import shutil > >> > import socket > >> > import stat > >> > import StringIO > >> > +import copy > >> > +from bitarray import bitarray > >> > try: > >> > import affinity # pylint: disable=F0401 > >> > except ImportError: > >> > affinity = None > >> > +try: > >> > + import fdsend # pylint: disable=F0401 > >> > +except ImportError: > >> > + fdsend = None > >> > > >> > from ganeti import utils > >> > from ganeti import constants > >> > @@ -79,6 +85,40 @@ _SPICE_ADDITIONAL_PARAMS = frozenset([ > >> > constants.HV_KVM_SPICE_USE_TLS, > >> > ]) > >> > > >> > +FREE = bitarray("0") > >> > + > >> > >> Could you perhaps give this a better name, and/or add a comment about its > >> use? > >> A global constant called "FREE" as a bitarray doesn't say much per se. > >> > > > > True. This could be AVAILABLE_PCI_SLOT. This is bitarray because the > > bitarray.search() needs a bitarray pattern ("0") to search inside the actual > > pci slots allocations (32 bits) > > > > Is this a module-global constant though? Or does its value change? > Anyway ok with changing the name, and adding a comment, if it's constant. > It is a constant. So I'll rename it and add a comment. > >> > +def _GenerateDeviceKVMId(dev_type, dev): > >> > + > >> > >> Missing docstring. > >> > > > > True. Consider the following: > > """ Helper function to generate a unique device name used by KVM > > > > QEMU monitor commands use names to identify devices. Here we use their pci > > slot > > and a part of their UUID to name them. dev.pci might be None for old devices > > in the cluster. Returning None means that device is not hotplug-able. > > > > """ > > Missing @return and @param and @type. But anyway please resubmit with > the docstrings, let's not discuss them in the conversation. :) > > >> > + if not dev or not dev.pci: > >> > + return None > >> > + > >> > + return "%s-%s-pci-%d" % (dev_type.lower(), dev.uuid.split("-")[0], > >> > dev.pci) > >> > >> does it make more sense to return "None" or an exception here? > >> (just want to avoid uncaught None being around because we haven't checked) > >> > > > > dev.pci will be None for old devices of running instances in the > > cluster. If None is returned hotplug code will not be executed at all. > > Which is a problem. You need to fail (gracefully) and report the > failure. Not just "not execute the code and pretend nothing happened". > Also, what happens if at some point you forget to check if it's None > or not? Better to have an exception, which you can't forget to handle. > > > Here we will arrive if we try to hot-remove a device that was added in > > the past (with no pci slot). We could raise an exception I guess but > > where should we catch it? Or will we let the whole opcode fail? I think > > a warning instead is better. > > Ack on the warning, but we need the rpc to report that hotplug didn't > work, rather than, as the patch does now, pretent everything was > allright and not execute the code. > OK. Hypervisor will raise HotplugError Exception and will make the rpc fail but it will be caught by cmdlib which will give the necessary feedback. > > > >> > + > >> > + > >> > +def _UpdatePCISlots(dev, pci_reservations): > >> > + """Update pci configuration for a stopped instance > >> > + > >> > + If dev has a pci slot the reserve it, else find first available. > >> > + > >> > + """ > >> > >> Please document the types here > >> > > > > ACK. @type, @param... > > > >> > + if dev.pci: > >> > + free = dev.pci > >> > + else: > >> > + [free] = pci_reservations.search(FREE, 1) # pylint: disable=E1103 > >> > + if not free: > >> > + raise errors.HypervisorError("All PCI slots occupied") > >> > + dev.pci = int(free) > >> > + > >> > + pci_reservations[free] = True > >> > + > >> > + > >> > +def _RemoveFromRuntimeEntry(devices, device, fn): > >> > >> Missing docstring > >> > > > > Noted along with the following "missing docstring". > > > >> > + try: > >> > + [rem] = [x for x in fn(devices) if x.uuid == device.uuid] > >> > + devices.remove(rem) > >> > + except (ValueError, IndexError): > >> > + logging.info("No device with uuid %s in runtime file", device.uuid) > >> > + > >> > > >> > def _GetTunFeatures(fd, _ioctl=fcntl.ioctl): > >> > """Retrieves supported TUN features from file descriptor. > >> > @@ -569,6 +609,14 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > _BOOT_RE = re.compile(r"^-drive\s([^-]|(?<!^)-)*,boot=on\|off", re.M > >> > | re.S) > >> > _UUID_RE = re.compile(r"^-uuid\s", re.M) > >> > > >> > + _INFO_PCI_RE = re.compile(r'Bus.*device[ ]*(\d+).*') > >> > + _INFO_PCI_CMD = "info pci" > >> > + _INFO_VERSION_RE = \ > >> > + re.compile(r'^QEMU (\d+)\.(\d+)(\.(\d+))?.*monitor.*', re.M) > >> > + _INFO_VERSION_CMD = "info version" > >> > + > >> > + _DEFAULT_PCI_RESERVATIONS = "11110000000000000000000000000000" > >> > + > >> > ANCILLARY_FILES = [ > >> > _KVM_NETWORK_SCRIPT, > >> > ] > >> > @@ -1021,6 +1069,81 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > data.append(info) > >> > return data > >> > > >> > + def _GetExistingDeviceKVMId(self, instance, dev_type, dev): > >> > >> Missing docstring > >> > >> > + (_, kvm_nics, __, block_devices) = self._LoadKVMRuntime(instance) > >> > + if dev_type == constants.HOTPLUG_NIC: > >> > + found = [n for n in kvm_nics > >> > + if n.uuid == dev.uuid] > >> > + elif dev_type == constants.HOTPLUG_DISK: > >> > + found = [d for d, _ in block_devices > >> > + if d.uuid == dev.uuid] > >> > + dev_info = None > >> > + if found: > >> > + dev_info = found[0] > >> > + return _GenerateDeviceKVMId(dev_type, dev_info) > >> > + > >> > + def _GenerateKVMBlockDevicesOptions(self, instance, kvm_cmd, > >> > block_devices, > >> > + pci_reservations, kvmhelp): > >> > + > >> > >> Missing docstring. > >> > >> > + hvp = instance.hvparams > >> > + boot_disk = hvp[constants.HV_BOOT_ORDER] == constants.HT_BO_DISK > >> > + > >> > + # whether this is an older KVM version that uses the boot=on flag > >> > + # on devices > >> > + needs_boot_flag = self._BOOT_RE.search(kvmhelp) > >> > + > >> > + disk_type = hvp[constants.HV_DISK_TYPE] > >> > + if disk_type == constants.HT_DISK_PARAVIRTUAL: > >> > + #TODO: parse kvm -device ? output > >> > >> Space before "TODO" > >> > >> > + disk_model = "virtio-blk-pci" > >> > + if_val = ",if=virtio" > >> > + else: > >> > + if_val = ",if=%s" % disk_type > >> > + # Cache mode > >> > + disk_cache = hvp[constants.HV_DISK_CACHE] > >> > + if instance.disk_template in constants.DTS_EXT_MIRROR: > >> > + if disk_cache != "none": > >> > + # TODO: make this a hard error, instead of a silent overwrite > >> > + logging.warning("KVM: overriding disk_cache setting '%s' with > >> > 'none'" > >> > + " to prevent shared storage corruption on > >> > migration", > >> > + disk_cache) > >> > + cache_val = ",cache=none" > >> > + elif disk_cache != constants.HT_CACHE_DEFAULT: > >> > + cache_val = ",cache=%s" % disk_cache > >> > + else: > >> > + cache_val = "" > >> > + for cfdev, dev_path in block_devices: > >> > + if cfdev.mode != constants.DISK_RDWR: > >> > + raise errors.HypervisorError("Instance has read-only disks > >> > which" > >> > + " are not supported by KVM") > >> > + # TODO: handle FD_LOOP and FD_BLKTAP (?) > >> > + boot_val = "" > >> > + if boot_disk: > >> > + kvm_cmd.extend(["-boot", "c"]) > >> > + boot_disk = False > >> > + if needs_boot_flag and disk_type != constants.HT_DISK_IDE: > >> > + boot_val = ",boot=on" > >> > + drive_val = "file=%s,format=raw%s%s" % \ > >> > + (dev_path, boot_val, cache_val) > >> > + _UpdatePCISlots(cfdev, pci_reservations) > >> > + kvm_devid = _GenerateDeviceKVMId("DISK", cfdev) > >> > + if kvm_devid: > >> > + #TODO: name id after model > >> > >> Same > >> > >> > + drive_val += (",if=none,id=%s" % kvm_devid) > >> > + drive_val += (",bus=0,unit=%d" % cfdev.pci) > >> > + else: > >> > + drive_val += if_val > >> > + > >> > + kvm_cmd.extend(["-drive", drive_val]) > >> > + > >> > + if kvm_devid: > >> > + dev_val = ("%s,drive=%s,id=%s" % > >> > + (disk_model, kvm_devid, kvm_devid)) > >> > + dev_val += ",bus=pci.0,addr=%s" % hex(cfdev.pci) > >> > + kvm_cmd.extend(["-device", dev_val]) > >> > + > >> > + return kvm_cmd > >> > + > >> > >> Rather than taking kvm_cmd as input and extending it shouldn't you > >> just return the -drive values, and let the caller act on kvm_cmd as > >> appropriate? > >> > > > > OK. I'll do that. > > > >> > def _GenerateKVMRuntime(self, instance, block_devices, startup_paused, > >> > kvmhelp): > >> > """Generate KVM information to start an instance. > >> > @@ -1089,9 +1212,8 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > > >> > kernel_path = hvp[constants.HV_KERNEL_PATH] > >> > if kernel_path: > >> > - boot_disk = boot_cdrom = boot_floppy = boot_network = False > >> > + boot_cdrom = boot_floppy = boot_network = False > >> > else: > >> > - boot_disk = hvp[constants.HV_BOOT_ORDER] == constants.HT_BO_DISK > >> > >> What's this change? Can it be extracted out/explained separately? > >> > > > > This exists now in _GenerateKVMBlockDeviceOptions(). I'll double check it. > > > > Sure, but you're changing it in some way, without explaining why. :) > If there is a reason please document it properly/extract out the > change if it's self standing. If there's none, let's revert these > lines? > > >> > boot_cdrom = hvp[constants.HV_BOOT_ORDER] == constants.HT_BO_CDROM > >> > boot_floppy = hvp[constants.HV_BOOT_ORDER] == > >> > constants.HT_BO_FLOPPY > >> > boot_network = hvp[constants.HV_BOOT_ORDER] == > >> > constants.HT_BO_NETWORK > >> > @@ -1107,38 +1229,6 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > needs_boot_flag = self._BOOT_RE.search(kvmhelp) > >> > > >> > disk_type = hvp[constants.HV_DISK_TYPE] > >> > - if disk_type == constants.HT_DISK_PARAVIRTUAL: > >> > - if_val = ",if=virtio" > >> > - else: > >> > - if_val = ",if=%s" % disk_type > >> > - # Cache mode > >> > - disk_cache = hvp[constants.HV_DISK_CACHE] > >> > - if instance.disk_template in constants.DTS_EXT_MIRROR: > >> > - if disk_cache != "none": > >> > - # TODO: make this a hard error, instead of a silent overwrite > >> > - logging.warning("KVM: overriding disk_cache setting '%s' with > >> > 'none'" > >> > - " to prevent shared storage corruption on > >> > migration", > >> > - disk_cache) > >> > - cache_val = ",cache=none" > >> > - elif disk_cache != constants.HT_CACHE_DEFAULT: > >> > - cache_val = ",cache=%s" % disk_cache > >> > - else: > >> > - cache_val = "" > >> > - for cfdev, dev_path in block_devices: > >> > - if cfdev.mode != constants.DISK_RDWR: > >> > - raise errors.HypervisorError("Instance has read-only disks > >> > which" > >> > - " are not supported by KVM") > >> > - # TODO: handle FD_LOOP and FD_BLKTAP (?) > >> > - boot_val = "" > >> > - if boot_disk: > >> > - kvm_cmd.extend(["-boot", "c"]) > >> > - boot_disk = False > >> > - if needs_boot_flag and disk_type != constants.HT_DISK_IDE: > >> > - boot_val = ",boot=on" > >> > - > >> > - drive_val = "file=%s,format=raw%s%s%s" % (dev_path, if_val, > >> > boot_val, > >> > - cache_val) > >> > - kvm_cmd.extend(["-drive", drive_val]) > >> > > >> > #Now we can specify a different device type for CDROM devices. > >> > cdrom_disk_type = hvp[constants.HV_KVM_CDROM_DISK_TYPE] > >> > >> Please move disk_type = hvp[constants.HV_DISK_TYPE] just above this > >> line, as now it's extracted and used only in this section. > >> (ore remove it completely and use the explicit hvp... below, since > >> it's used only once, your choice, but don't live it as a lonely line > >> above). > >> > > > > OK. > > > >> > @@ -1407,7 +1497,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > kvm_nics = instance.nics > >> > hvparams = hvp > >> > > >> > - return (kvm_cmd, kvm_nics, hvparams) > >> > + return (kvm_cmd, kvm_nics, hvparams, block_devices) > >> > > >> > def _WriteKVMRuntime(self, instance_name, data): > >> > """Write an instance's KVM runtime > >> > @@ -1433,9 +1523,13 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > """Save an instance's KVM runtime > >> > > >> > """ > >> > - kvm_cmd, kvm_nics, hvparams = kvm_runtime > >> > + kvm_cmd, kvm_nics, hvparams, block_devices = kvm_runtime > >> > + > >> > serialized_nics = [nic.ToDict() for nic in kvm_nics] > >> > - serialized_form = serializer.Dump((kvm_cmd, serialized_nics, > >> > hvparams)) > >> > + serialized_blockdevs = [(blk.ToDict(), link) for blk, link in > >> > block_devices] > >> > + serialized_form = serializer.Dump((kvm_cmd, serialized_nics, > >> > hvparams, > >> > + serialized_blockdevs)) > >> > + > >> > self._WriteKVMRuntime(instance.name, serialized_form) > >> > > >> > def _LoadKVMRuntime(self, instance, serialized_runtime=None): > >> > @@ -1444,10 +1538,19 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > """ > >> > if not serialized_runtime: > >> > serialized_runtime = self._ReadKVMRuntime(instance.name) > >> > + > >> > loaded_runtime = serializer.Load(serialized_runtime) > >> > - kvm_cmd, serialized_nics, hvparams = loaded_runtime > >> > + if len(loaded_runtime)==3: > >> > + serialized_blockdevs = [] > >> > + kvm_cmd, serialized_nics, hvparams = loaded_runtime > >> > + else: > >> > + kvm_cmd, serialized_nics, hvparams, serialized_blockdevs = > >> > loaded_runtime > >> > + > >> > >> Just wondering if you shouldn't add a runtime "version", and assume 0 > >> if not present, instead of desuming features from the found fields. > >> > > > > This is the same I guess. If version is not used elsewhere.. > > > > Sure, I was just wondering about future-proofing more, as we extend this. > For now I guess we haven't extended it, or at least not in an > incompatible manner. > > >> > kvm_nics = [objects.NIC.FromDict(snic) for snic in serialized_nics] > >> > - return (kvm_cmd, kvm_nics, hvparams) > >> > + block_devices = [(objects.Disk.FromDict(sdisk), link) > >> > + for sdisk, link in serialized_blockdevs] > >> > + > >> > + return (kvm_cmd, kvm_nics, hvparams, block_devices) > >> > > >> > def _RunKVMCmd(self, name, kvm_cmd, tap_fds=None): > >> > """Run the KVM cmd and check for errors > >> > @@ -1472,6 +1575,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > if not self._InstancePidAlive(name)[2]: > >> > raise errors.HypervisorError("Failed to start instance %s" % name) > >> > > >> > + # pylint: disable=R0914 > >> > >> What are you disabling? And why? Can you add a comment here explaining? > >> > > > > pylint was giving me too-many-arguments warnings. I'll dig into it a bit > > further > > and see if this can be avoided. > > > > It'd be better to avoid it, but if you can't at least add a comment > explaining that you're disabling "too many arguments" so it's clear to > whoever reads/changes the code without digging into pylint. > > >> > def _ExecuteKVMRuntime(self, instance, kvm_runtime, kvmhelp, > >> > incoming=None): > >> > """Execute a KVM cmd, after completing it with some last minute > >> > data. > >> > > >> > @@ -1495,9 +1599,12 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > > >> > temp_files = [] > >> > > >> > - kvm_cmd, kvm_nics, up_hvp = kvm_runtime > >> > + kvm_cmd, kvm_nics, up_hvp, block_devices = kvm_runtime > >> > # the first element of kvm_cmd is always the path to the kvm binary > >> > kvm_path = kvm_cmd[0] > >> > + > >> > + kvm_cmd_runtime = copy.deepcopy(kvm_cmd) > >> > + > >> > >> copy.deepcopy tends to be slower than needed. can we avoid it? > >> sometimes serializing/deserializing in memory is faster. of course it > >> might not matter here, perhaps? > >> > > > > Agreed about the slower part but I don't think the overhead is significant > > here. > > > > Ack, although if a "tojson/fromjson" works and is faster perhaps why not? > OK. I'll give it a try. > >> > up_hvp = objects.FillDict(conf_hvp, up_hvp) > >> > > >> > # We know it's safe to run as a different user upon migration, so > >> > we'll use > >> > @@ -1516,6 +1623,13 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > utils.WriteFile(keymap_path, data="include en-us\ninclude %s\n" % > >> > keymap) > >> > kvm_cmd.extend(["-k", keymap_path]) > >> > > >> > + pci_reservations = bitarray(self._DEFAULT_PCI_RESERVATIONS) > >> > + > >> > + kvm_cmd = self._GenerateKVMBlockDevicesOptions(instance, kvm_cmd, > >> > + block_devices, > >> > + pci_reservations, > >> > + kvmhelp) > >> > + > >> > >> ew! in the function above you both update the passed kvm_cmd, return > >> it, and reassign it here to the same. > >> let's avoid this please, as mentioned!! :) > >> > > > > OK! Just return the block device options and append them here in kvm_cmd. > > > >> > # We have reasons to believe changing something like the nic > >> > driver/type > >> > # upon migration won't exactly fly with the instance kernel, so for > >> > nic > >> > # related parameters we'll use up_hvp > >> > @@ -1556,8 +1670,15 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > tapfds.append(tapfd) > >> > taps.append(tapname) > >> > if kvm_supports_netdev: > >> > - nic_val = "%s,mac=%s,netdev=netdev%s" % (nic_model, nic.mac, > >> > nic_seq) > >> > - tap_val = "type=tap,id=netdev%s,fd=%d%s" % (nic_seq, tapfd, > >> > tap_extra) > >> > + nic_val = "%s,mac=%s" % (nic_model, nic.mac) > >> > + _UpdatePCISlots(nic, pci_reservations) > >> > + kvm_devid = _GenerateDeviceKVMId("NIC", nic) > >> > + netdev = kvm_devid or "netdev%d" % nic_seq > >> > >> kvm_devid or ... ? maybe better explicitly putting these in the if > >> below, and restructure the code so there's one only explicit if... > >> (or try/catch if we move _GenerateDevice... to exceptions). > >> > > > > I used that to be more compact. Of course it must change if > > _GenerateDeviceKVMId() > > raises an Exception but still I prefer the "return None" case. > > > > Yes, but IIRC our style guide doesn't allow inline or expressions like that. > And you have an if on the same condition just below, can't we just refactor? > Sure. > >> > + nic_val += (",netdev=%s" % netdev) > >> > + if kvm_devid: > >> > + nic_val += (",id=%s,bus=pci.0,addr=%s" % (kvm_devid, > >> > hex(nic.pci))) > >> > + tap_val = ("type=tap,id=%s,fd=%d%s" % > >> > + (netdev, tapfd, tap_extra)) > >> > kvm_cmd.extend(["-netdev", tap_val, "-device", nic_val]) > >> > else: > >> > nic_val = "nic,vlan=%s,macaddr=%s,model=%s" % (nic_seq, > >> > @@ -1680,6 +1801,10 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > # explicitly requested resume the vm status. > >> > self._CallMonitorCommand(instance.name, self._CONT_CMD) > >> > > >> > + kvm_runtime_with_pci_info = (kvm_cmd_runtime, kvm_nics, > >> > + up_hvp, block_devices) > >> > + return kvm_runtime_with_pci_info > >> > + > >> > >> And next time we add something here we call it > >> _with_pci_info_and_something_else? > >> Also you're changing _ExecuteKVMRuntime's from not returning to > >> returning without updating the docstring. > >> And perhaps we could avoid it and keep the generate/save separate from > >> the execute part? > >> > > > > Point taken :) kvm_runtime and docstring update. I 'll see if the separation > > is easy to implement and if it makes the code cleaner. > > > > Thanks. > > >> > def StartInstance(self, instance, block_devices, startup_paused): > >> > """Start an instance. > >> > > >> > @@ -1690,7 +1815,9 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > kvm_runtime = self._GenerateKVMRuntime(instance, block_devices, > >> > startup_paused, kvmhelp) > >> > self._SaveKVMRuntime(instance, kvm_runtime) > >> > - self._ExecuteKVMRuntime(instance, kvm_runtime, kvmhelp) > >> > + kvm_runtime_with_pci_info = self._ExecuteKVMRuntime(instance, > >> > kvm_runtime, > >> > + kvmhelp) > >> > + self._SaveKVMRuntime(instance, kvm_runtime_with_pci_info) > >> > > >> > >> Save then execute then save? No.... > >> > > > > I'll remove the first one.. > > > > Nope, try to have the right thing generated to save, save it, then > call execute with that so that execute doesn't need to change the data > that needs to be saved, and save doesn't depend on execute to work. > ACK. Make Genarate return valid runtime entries that do not need to be updated after running Execute. It should be doable. > >> > def _CallMonitorCommand(self, instance_name, command): > >> > """Invoke a command on the instance monitor. > >> > @@ -1716,6 +1843,145 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > > >> > return result > >> > > >> > + def _AnnotateFreePCISlot(self, instance, dev): > >> > + """Get the first available pci slot of a runnung instance. > >> > + > >> > + """ > >> > >> @type? @param? @return? > >> Also _Annotate ... "Get the ..." Can you explain better what are you > >> annotating versus getting? > >> > > > > ACK. > > > >> > + slots = bitarray(32) > >> > + slots.setall(False) # pylint: disable=E1101 > >> > >> What error is that and why it's not a problem here? > >> > > ^ (at least add a comment about it) > > >> > + output = self._CallMonitorCommand(instance.name, self._INFO_PCI_CMD) > >> > + for line in output.stdout.splitlines(): > >> > + match = self._INFO_PCI_RE.search(line) > >> > + if match: > >> > + slot = int(match.group(1)) > >> > + slots[slot] = True > >> > + > >> > >> Could we get them all with a multiline re, and iterate just on the > >> result, perhaps? > >> > > > > Sure, we could. > > > >> > + [free] = slots.search(FREE, 1) # pylint: disable=E1101 > >> > + if not free: > >> > + raise errors.HypervisorError("All PCI slots occupied") > >> > + > >> > + dev.pci = int(free) > >> > + > >> > >> Ah, so you're just finding the slot, not annotating anything... or am I > >> wrong? > >> > > > > Yes. Wrong use of annotate. > > > >> > + def _TryHotplug(self, instance, dev_type): > >> > >> _TryHotplug.... > >> > >> > + """Get QEMU version from the instance's monitor. > >> > >> ... gets the qemu version ?? > >> > >> > + > >> > + Hotplug is supported for running instances and for versions >= 1.0. > >> > >> Empty line here. Also @return, @param, etc. > >> > >> > + """ > >> > + if dev_type == constants.HOTPLUG_DISK: > >> > + hvp = instance.hvparams > >> > + security_model = hvp[constants.HV_SECURITY_MODEL] > >> > + use_chroot = hvp[constants.HV_KVM_USE_CHROOT] > >> > + if use_chroot or security_model != constants.HT_SM_NONE: > >> > + return False > >> > >> and returns False (clearly not a version) > >> > >> > + output = self._CallMonitorCommand(instance.name, > >> > self._INFO_VERSION_CMD) > >> > + #TODO: search for netdev_add, drive_add, device_add..... > >> > + match = self._INFO_VERSION_RE.search(output.stdout) > >> > + if not match: > >> > + return False > >> > + v_major, v_min, _, _ = match.groups() > >> > + return (v_major, v_min) >= (1, 0) > >> > >> Or a tuple. No. Please streamline this code. > >> Also please try to see if you can share code between here and the > >> "normal" version getting. > >> Finally keep separate the "get the version" part from the "decide > >> whether X is supported" part, even if you depend on the version. > >> And, if you can, try to see if something is supported via other help > >> commands rather than the version itself (but if impossible the version > >> is ok). > >> > > > > I had this in mind as TODO. I'll include it when I resend the patch set. > > > > Thanks. And feel free to separate it out in its own patch. :) > (getversion from monitor, etc, as their own functions) > OK. But for the time being it will use the existing _CallMonitorCommand(). The whole 'not use socat' thing will follow. I could prepare it by introducing a new method that connects to the monitor by using sockets (actually extract it from _PassTapFd()). > >> > + > >> > + def _CallMonitorHotplugCommand(self, name, cmd): > >> > + output = self._CallMonitorCommand(name, cmd) > >> > + #TODO: parse output and check if succeeded > >> > >> Indeed. But not in the distant future. :) Also: > >> - missing docstring > >> - this is not a hotplug specific function, it seems to be "call+log, > >> or similar". > >> - maybe it's time to change _CallMonitorCommand not to use socat (in a > >> previous patch), especially as we use it more? > >> > > > > I had it here in case we wanted to do things, parse output, log something, > > raise Exception, etc. > > I got lost with "not to use socat". What should it use? > > > > In your patch itself you implement a separate way to connect to the > monitor, by opening a socket, rather than calling socat. > Perhaps we should have only that way, and remove the socat need inside > this module. This could be a pre-patch to the current one. > ("Substitute 'socat' in kvm monitor command with socket handling") > Maybe a mini Class Monitor should be introduced. With methods like connect, get_version, supports(command), passfd, etc. Do you agree on that? > >> > + for line in output.stdout.splitlines(): > >> > + logging.info("%s", line) > >> > + > >> > + def HotplugDevice(self, instance, action, dev_type, device, extra, > >> > seq): > >> > + """ Generic method to hotplug device > >> > + > >> > + Depending on action and dev_type invoke the coresponding method that > >> > >> corresponding. > >> > >> > + does the actual hotplug (via KVM monitor commands). > >> > + Before and after, do all hotplug related steps (e.g. check if > >> > hotplug > >> > + is possible, annotate pci slot to device and generate/get existing > >> > + KVM device ids, read/write runtime file) > >> > + > >> > >> Is this meant as a Hypervisor base function? If so please add the one > >> to the base class first (in a separate patch), and document there and > >> here properly with the complete docstring. > >> > > > > ACK. And the other Hypervisors will raise NotImplemented? And I won't have > > to check for it in backend.py explicitly, right? > > > > Yes. > > >> > + """ > >> > + if self._TryHotplug(instance, dev_type): > >> > >> else? pretend nothing happened and don't return anything, nor raise an > >> unsupported exception? No. > >> > > > > Well I guess _TryHotplug() must do its checks and raise the > > appropriate Exceptions. > > > > Yes. Also it's not "_Try..." it's "_Check..." :) > > >> > + (kvm_cmd, kvm_nics, hvparams, \ > >> > + block_devices) = self._LoadKVMRuntime(instance) > >> > + if action == constants.HOTPLUG_ADD: > >> > >> This looks like will be standard among all hypervisors. Maybe even if > >> the rpc call is only one you want here to have at least: > >> AddHotplugDevice > >> and > >> DelHotplugDevice > >> and perhaps directly Disk and Nic also, so the switch can happen only > >> in backend. > >> > > > > Got lost here... > > The point is: if all hypervisors that implement hotplug have the same code: > > if action == constants.HOTPLUG_ADD: > call add > elif action == constants.HOTPLUG_DEL: > call del > > Maybe the hypervisor interface should be > > HotplugAddDevice > HotplugDelDevice > > or even > HotplugAddNIC > HotplugAddDisk > HotplugDelNIC > HotplugDelDisk > > (perhaps just the first one, with two calls) and we can have the > common "if..." just once in backend.py, rather than in each > hypervisor. > OK. I got it. I vote for the first one too. > > > >> > + self._AnnotateFreePCISlot(instance, device) > >> > + kvm_devid = _GenerateDeviceKVMId(dev_type, device) > >> > + if dev_type == constants.HOTPLUG_DISK: > >> > + self._HotAddDisk(instance, > >> > + device, extra, seq, kvm_devid, block_devices) > >> > + elif dev_type == constants.HOTPLUG_NIC and fdsend: > >> > >> and fdsend is not None. What if it's None? Again, just moving on and > >> pretending nothing happened is not a solution. > >> > > > > Agree. This is a part of the feedback and the overall policy we want to > > apply. I propose here the following: > > > > - In KVM hypervisor do not raise any Exception. > > - Raise NotImplemented for other hypervisors. > > - In case hotplug is not possible use RPC payload to return any > > info/messages > > to masterd and eventually the user. > > - Continue the execution path as if no hotplug takes place. > > Shouldn't we just raise an explicit exception HotplugError, that can > be handled on the master daemon? > Question: HotplugError is going to be raised by hypervisor code. It will be catched by backend.py which will raise and RPCFail that will add a fail_msg inside the rpc result and then in cmdlib we can raise it with result.Raise(). Correct? Invoking this will break the execution path. Do we want this? I think printing out the fail_msg will be enough for the time being. > > > >> > + self._HotAddNic(instance, device, extra, seq, kvm_devid, > >> > kvm_nics) > >> > + elif action == constants.HOTPLUG_REMOVE: > >> > + kvm_devid = self._GetExistingDeviceKVMId(instance, dev_type, > >> > device) > >> > + if dev_type == constants.HOTPLUG_DISK: > >> > + self._HotDelDisk(instance, > >> > + device, extra, seq, kvm_devid, block_devices) > >> > + elif dev_type == constants.HOTPLUG_NIC and fdsend: > >> > + self._HotDelNic(instance, device, extra, seq, kvm_devid, > >> > kvm_nics) > >> > >> You're actually not using fdsend for removing nics. But well, if you > >> want to keep the dependency specular that's fair too. > >> Please add a comment in that sense. > >> > > > > ACK. > > > >> > + self._SaveKVMRuntime(instance, > >> > + (kvm_cmd, kvm_nics, hvparams, block_devices)) > >> > + time.sleep(2) > >> > >> For what? Just so we hope the operation whose output we leave as a > > > > Here to be honest, in case of nic modification (remove and add) the socat > > returned > > error if sleep was not used. I have to benchmark it more I guess.. > > > > Or just not use socat, and move to using sockets. > And can you find out what caused the error, and exactly what we are > waiting to happen, instead of relying on "meh, in a couple of seconds > it works". :) > I see. So lets implement that Monitor class I was talking about. Agree? > >> TODO to check has more chances to succeed? > >> What are we waiting for? Is there a way to properly way and perhaps report? > >> > >> > + > >> > + def _HotAddDisk(self, instance, disk, dev_path, _, kvm_devid, > >> > block_devices): > >> > + """Hotplug/add new disk to and instance > >> > + > >> > >> Usual docstring complaints. > >> > >> > + """ > >> > + command = ("drive_add dummy file=%s,if=none,id=%s,format=raw" % > >> > + (dev_path, kvm_devid)) > >> > + self._CallMonitorHotplugCommand(instance.name, command) > >> > + command = ("device_add virtio-blk-pci,bus=pci.0,addr=%s," > >> > + "drive=%s,id=%s" > >> > + % (hex(disk.pci), kvm_devid, kvm_devid)) > >> > + self._CallMonitorHotplugCommand(instance.name, command) > >> > + block_devices.append((disk, dev_path)) > >> > >> Can you explain the two commands, either in the docstring or somewhere > >> else? > >> Also perhaps if you change the _CallMonitorCommand not to use socat > >> you can even send the two over the same socket without opening and > >> closing and forking a couple of times. > >> > > > > You are right. I'll change that. > > > >> > + > >> > + def _HotAddNic(self, instance, nic, _, seq, kvm_devid, kvm_nics): > >> > + """Hotplug/add nic to an instance > >> > + > >> > >> Same. > >> > >> > + """ > >> > + (tap, fd) = _OpenTap() > >> > + self._PassTapFd(instance, fd, nic) > >> > + command = ("netdev_add tap,id=%s,fd=%s" % (kvm_devid, kvm_devid)) > >> > + self._CallMonitorHotplugCommand(instance.name, command) > >> > + command = ("device_add virtio-net-pci,bus=pci.0,addr=%s,mac=%s," > >> > + "netdev=%s,id=%s" > >> > + % (hex(nic.pci), nic.mac, kvm_devid, kvm_devid)) > >> > + self._CallMonitorHotplugCommand(instance.name, command) > >> > + self._ConfigureNIC(instance, seq, nic, tap) > >> > + utils.WriteFile(self._InstanceNICFile(instance.name, seq), data=tap) > >> > + kvm_nics.append(nic) > >> > + > >> > >> Same. Also wouldn't it make sense to configure it and then plug it? > >> > > > > ACK. > > > >> > + def _HotDelDisk(self, instance, disk, _, __, kvm_devid, > >> > block_devices): > >> > + """Hotplug/remove disk from an instance > >> > + > >> > + """ > >> > + command = "device_del %s" % kvm_devid > >> > + self._CallMonitorHotplugCommand(instance.name, command) > >> > + #command = "drive_del %s" % uuid > >> > + #self._CallMonitorHotplugCommand(instance.name, command) > >> > >> Mmmm... fishy commented out code with no explanation... ;) > >> > > > > True. For some reason it isn't necessary. I'll dig into it. > > > >> > + _RemoveFromRuntimeEntry(block_devices, disk, lambda x: [d for d, l > >> > in x]) > >> > + > >> > + def _HotDelNic(self, instance, nic, _, __, kvm_devid, kvm_nics): > >> > + """Hotplug/remove existing nic from an instance > >> > + > >> > + """ > >> > + command = "device_del %s" % kvm_devid > >> > + self._CallMonitorHotplugCommand(instance.name, command) > >> > + command = "netdev_del %s" % kvm_devid > >> > + self._CallMonitorHotplugCommand(instance.name, command) > >> > + _RemoveFromRuntimeEntry(kvm_nics, nic, lambda x: x) > >> > + > >> > + def _PassTapFd(self, instance, fd, nic): > >> > + """Pass file descriptor to kvm process via monitor socket using > >> > SCM_RIGHTS > >> > + > >> > + """ > >> > + monsock = utils.ShellQuote(self._InstanceMonitor(instance.name)) > >> > + s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) > >> > + s.connect(monsock) > >> > + kvm_devid = _GenerateDeviceKVMId("NIC", nic) > >> > + command = "getfd %s\n" % kvm_devid > >> > + fds = [fd] > >> > + logging.info("%s", fds) > >> > + fdsend.sendfds(s, command, fds = fds) > >> > + s.close() > >> > + > >> > >> You're not creating a new way to connect the the kvm monitor and then > >> using it just for your own purpose here in a forgotten function > >> ignoring all other cases and a more generic implementation, right? ... > >> :) Of course you wouldn't do that? > >> > >> > > > > Got lost here.. > > > > What I said above: it's good to have code to connect to the monitor > command directly instead of via socat, but you shouldn't have it > embeddec in _PassTapFd, but generic, used by _PassTapFd *and* > _CallMonitorCommand. > Monitor class! > >> > @classmethod > >> > def _ParseKVMVersion(cls, text): > >> > """Parse the KVM version from the --help output. > >> > @@ -1831,7 +2097,9 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> > self._SaveKVMRuntime(instance, kvm_runtime) > >> > kvmpath = instance.hvparams[constants.HV_KVM_PATH] > >> > kvmhelp = self._GetKVMOutput(kvmpath, self._KVMOPT_HELP) > >> > - self._ExecuteKVMRuntime(instance, kvm_runtime, kvmhelp) > >> > + kvm_runtime_with_pci_info = \ > >> > + self._ExecuteKVMRuntime(instance, kvm_runtime, kvmhelp) > >> > + self._SaveKVMRuntime(instance, kvm_runtime_with_pci_info) > >> > > >> > >> Again Save + ... + Save is not nice. > >> > >> Thanks, and sorry for the looong review. > >> > > > > Sorry??? For what? For a fine and in depth review?? With bit more irony > > that expected but OK, point taken :) > > Ah, sorry for the irony then, it's not meant badly or personally, I > hope it's not a problem. > Not at all :) > > > > The good thing is that the most comments refer to style issues and > > implementation > > details and not to the main idea/design. I noted down all suggestions and > > I'll > > try to implement them the next few days. > > > No, the main idea/design is ok, but there are still a few things above > that change the general structure of the code, and also the rest of > the kvm implementation (eg. the socat vs socket) > > > Again thanks for the looooong review. > > You're welcome. :) > > Guido
signature.asc
Description: Digital signature
