LGTM, thanks.
On Fri, Mar 28, 2014 at 5:31 PM, Apollon Oikonomopoulos <[email protected]>wrote: > On 18:07 Fri 28 Mar , Apollon Oikonomopoulos wrote: > > Commit 0fe22ad2 moved the call to _GenerateKVMBlockDevicesOptions() from > > _GenerateKVMRuntime() to _ExecuteKvmRuntime. However, while in > > _GenerateKVMRuntime() there is only one set of HVPs, those of the > > instance's configuration, in _ExecuteKVMRuntime there are two sets of > > HVPs: > > > > - The instance's configured HVPs > > - The HVPs the instance used when it was created > ^ s/created/started/ > > > > Currently, _GenerateKVMBlockDevicesOptions() uses the first set of HVPs > > only, meaning that it will always read the configured HVPs. Since > > _ExecuteKVMRuntime() is also called at migration time, it should use the > > instance's running HVPs, otherwise migration while fail. > > > > The following scenario will lead to a crash on migration: > > > > 1. Start the instance (assuming 'paravirtual' disk_type) > > 2. gnt-instance modify -H disk_type=scsi <instance_name> > > 3. gnt-instance migrate <instance_name> > > > > Step 3 will start the instance on the remote node with all disks > > configured as 'scsi' and qemu will crash post-migration. > > > > We fix this by making sure _GenerateKVMBlockDevicesOptions gets the > > running HVPs as well. > > > > Signed-off-by: Apollon Oikonomopoulos <[email protected]> > > --- > > lib/hypervisor/hv_kvm.py | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/lib/hypervisor/hv_kvm.py b/lib/hypervisor/hv_kvm.py > > index b6eda5b..7c29c84 100644 > > --- a/lib/hypervisor/hv_kvm.py > > +++ b/lib/hypervisor/hv_kvm.py > > @@ -1236,12 +1236,14 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > data.append(info) > > return data > > > > - def _GenerateKVMBlockDevicesOptions(self, instance, kvm_disks, > > + def _GenerateKVMBlockDevicesOptions(self, instance, up_hvp, kvm_disks, > > kvmhelp, devlist): > > """Generate KVM options regarding instance's block devices. > > > > @type instance: L{objects.Instance} > > @param instance: the instance object > > + @type up_hvp: dict > > + @param up_hvp: the instance's runtime hypervisor parameters > > @type kvm_disks: list of tuples > > @param kvm_disks: list of tuples [(disk, link_name, uri)..] > > @type kvmhelp: string > > @@ -1252,12 +1254,11 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > @return: list of command line options eventually used by kvm > executable > > > > """ > > - hvp = instance.hvparams > > - kernel_path = hvp[constants.HV_KERNEL_PATH] > > + kernel_path = up_hvp[constants.HV_KERNEL_PATH] > > if kernel_path: > > boot_disk = False > > else: > > - boot_disk = hvp[constants.HV_BOOT_ORDER] == constants.HT_BO_DISK > > + boot_disk = up_hvp[constants.HV_BOOT_ORDER] == > constants.HT_BO_DISK > > > > # whether this is an older KVM version that uses the boot=on flag > > # on devices > > @@ -1265,7 +1266,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > > > dev_opts = [] > > device_driver = None > > - disk_type = hvp[constants.HV_DISK_TYPE] > > + disk_type = up_hvp[constants.HV_DISK_TYPE] > > if disk_type == constants.HT_DISK_PARAVIRTUAL: > > if_val = ",if=%s" % self._VIRTIO > > try: > > @@ -1278,7 +1279,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > else: > > if_val = ",if=%s" % disk_type > > # Cache mode > > - disk_cache = hvp[constants.HV_DISK_CACHE] > > + disk_cache = up_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 > > @@ -1910,6 +1911,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > > self._ConfigureNIC(instance, nic_seq, nic, taps[nic_seq]) > > > > bdev_opts = self._GenerateKVMBlockDevicesOptions(instance, > > + up_hvp, > > kvm_disks, > > kvmhelp, > > devlist) > > -- > > 1.9.0 > > > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
