[virt-tools-list] [virt-manager PATCH 7/7] scsi: Add controller index option and virtio-scsi bus option
Cole Robinson
crobinso at redhat.com
Wed Nov 22 22:23:45 UTC 2017
On 11/06/2017 07:52 AM, Lin Ma wrote:
> When we change disk bus to 'scsi' or add scsi disks, virt-manager relies
> on libvirt's rules to create addresses. Sometimes it works, sometimes not,
> Espectially in multiple scsi controllers case, we can not exactly control
> which virtual disks attach to which scsi controller, sometimes libvirt
> creates unnecessary scsi controllers for us, attach virtual disks to the
> scsi controllers which we don't want. It has confused users for a while.
>
> This patch added two new option to solve the above issue:
> 1. add virtio-scsi bus option. users can decide which kind of scsi bus
> they want to use, instead of guessing through libvirt.
> 2. add controller index option. when users configure disk bus, it allows
> users to exactly choice which existed scsi controller they want to attach
> or create a new one.
>
> The patch changed some mechanism about adding/configuring scsi bus, and
> generating device addrStr instead of libvirt did, it avoids some annoying
> issues.
>
There's a lot going on here, this needs to be split into multiple
patches. At the very least: addhardware controller change, details
controller change, details virtio-scsi change, but probably some smaller
patches too
Can you describe more what use case you are trying to facilitate better
here with the controller indexing changes? Better handling 8+ scsi
devices? Or just a simple 'I want to use virtio-scsi' use case. Before
adding more UI complexity I want to be sure this isn't something we can
mostly fix by having virt-manager be smarter by default
One thing this makes me think: not sure how much sense it makes to allow
changing a disk's bus value for an existing VM (it makes sense in the
'customize' wizard before install time). I'm struggling to think of a
usecase outside of 'virt testing' or 'trying to just get this vm to boot'
Some comments inline
> Signed-off-by: Lin Ma <lma at suse.com>
> ---
> ui/addhardware.ui | 40 +++++++++++++++
> ui/details.ui | 40 +++++++++++++++
> virtManager/addhardware.py | 125 +++++++++++++++++++++++++++++++++++----------
> virtManager/details.py | 80 ++++++++++++++++++++++++++++-
> virtManager/domain.py | 7 ++-
> virtinst/device.py | 15 +++++-
> virtinst/devicedisk.py | 2 +
> 7 files changed, 278 insertions(+), 31 deletions(-)
>
> diff --git a/ui/addhardware.ui b/ui/addhardware.ui
The UI works okay but I think I'd rather have the controller field pop
up _below_ the bus field.
> diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
> index d661602d..9205725f 100644
> --- a/virtManager/addhardware.py
> +++ b/virtManager/addhardware.py
> @@ -97,6 +97,7 @@ class vmmAddHardware(vmmGObjectUI):
> "on_create_finish_clicked": self._finish,
> "on_hw_list_changed": self._hw_selected,
>
> + "on_storage_bustype_changed": self._change_storage_bustype,
> "on_storage_devtype_changed": self._change_storage_devtype,
>
> "on_mac_address_clicked": self._change_macaddr_use,
> @@ -196,6 +197,10 @@ class vmmAddHardware(vmmGObjectUI):
> netmodel_list = self.widget("net-model")
> self.build_network_model_combo(self.vm, netmodel_list)
>
> + # Scsi index combo
> + self.build_controller_index_combo(self.vm,
> + self.widget("storage-scsi-index"))
> +
> # Disk bus type
> self.build_disk_bus_combo(self.vm,
> self.widget("storage-bustype"))
> @@ -678,6 +683,41 @@ class vmmAddHardware(vmmGObjectUI):
> model.append([None, _("Hypervisor default")])
> combo.set_active(0)
>
> + @staticmethod
> + def build_controller_index_combo(vm, combo):
> + ignore = vm
> + model = Gtk.ListStore(str, str)
> + combo.set_model(model)
> + uiutil.init_combo_text_column(combo, 1)
> + model.set_sort_column_id(1, Gtk.SortType.ASCENDING)
> + combo.set_active(-1)
> +
> + @staticmethod
> + def has_available_scsi_unit(vm, controller_index):
> + used_unit = 0
> + for disk in vm.get_disk_devices(inactive=True):
> + if disk.bus == "scsi":
> + if disk.address.controller == controller_index:
> + used_unit += 1
> + if used_unit < 7:
> + return True
> + else:
> + return False
> +
> + @staticmethod
> + def find_available_scsi_unit(vm, controller_index):
> + max_unit = 7
> + avail_unit = 0
> + used_list = []
> + for disk in vm.get_disk_devices(inactive=True):
> + if disk.bus == "scsi":
> + if str(disk.address.controller) == controller_index:
> + used_list.append(disk.address.unit)
> + while avail_unit < max_unit:
> + if avail_unit not in used_list:
> + return avail_unit
> + avail_unit += 1
> +
These two could be static methods in devicecontroller.py. You'd need to
pass in vm.get_xmlobj() output rather than a disk list though. Then it
would be easier to unit test
> @staticmethod
> def build_disk_bus_combo(vm, combo):
> ignore = vm
> @@ -718,6 +758,9 @@ class vmmAddHardware(vmmGObjectUI):
> if vm.conn.is_xen() or vm.conn.is_test():
> disk_bus_types.append("xen")
>
> + if vm.get_hv_type() in ["qemu", "kvm", "test"]:
> + disk_bus_types.append("virtio-scsi")
> +
> rows = []
> for bus in disk_bus_types:
> rows.append([bus, virtinst.VirtualDisk.pretty_disk_bus(bus)])
> @@ -725,10 +768,11 @@ class vmmAddHardware(vmmGObjectUI):
> model.clear()
>
> bus_map = {
> - "disk": ["ide", "sata", "scsi", "sd", "usb", "virtio", "xen"],
> + "disk": ["ide", "sata", "scsi", "sd", "usb", "virtio", "xen",
> + "virtio-scsi"],
> "floppy": ["fdc"],
> - "cdrom": ["ide", "sata", "scsi"],
> - "lun": ["scsi"],
> + "cdrom": ["ide", "sata", "scsi", "virtio-scsi"],
> + "lun": ["scsi", "virtio-scsi"],
> }
> for row in rows:
> if row[0] in bus_map[devtype]:
> @@ -1132,6 +1176,30 @@ class vmmAddHardware(vmmGObjectUI):
> # Device page listeners #
> #########################
>
> + def _change_storage_bustype(self, ignore):
> + bustype = uiutil.get_list_selection(
> + self.widget("storage-bustype"))
> +
> + controller_model = bustype
> + if controller_model in ["scsi", "virtio-scsi"]:
> + model = self.widget("storage-scsi-index").get_model()
> + model.clear()
> + if controller_model == "scsi":
> + controller_model = None
> + for controller in self.vm.get_controller_devices():
> + if (controller.type == "scsi" and
> + controller.model == controller_model):
> + if not vmmAddHardware.has_available_scsi_unit(
> + self.vm, controller.index):
> + continue
> + model.append([str(controller.index),
> + str(controller.index)])
> + model.append(["new", "New"])
> + self.widget("storage-scsi-index").set_active(0)
> + self.widget("storage-scsi-idx").set_visible(True)
> + else:
> + self.widget("storage-scsi-idx").set_visible(False)
> +
> def _change_storage_devtype(self, ignore):
> devtype = uiutil.get_list_selection(
> self.widget("storage-devtype"))
> @@ -1429,10 +1497,9 @@ class vmmAddHardware(vmmGObjectUI):
> self._dev.validate()
> return ret
>
> - def _set_disk_controller(self, disk, controller_model, used_disks):
> - # Add a SCSI controller with model virtio-scsi if needed
> + def _set_disk_controller(self, disk, controller_model):
> disk.vmm_controller = None
> - if controller_model != "virtio-scsi":
> + if disk.bus != "scsi":
> return None
>
> # Get SCSI controllers
> @@ -1440,7 +1507,7 @@ class vmmAddHardware(vmmGObjectUI):
> ctrls_scsi = [x for x in controllers if
> (x.type == VirtualController.TYPE_SCSI)]
>
> - # Create possible new controller
> + # Create new controller
> controller = VirtualController(self.conn.get_backend())
> controller.type = "scsi"
> controller.model = controller_model
> @@ -1450,23 +1517,7 @@ class vmmAddHardware(vmmGObjectUI):
> if ctrls_scsi:
> controller.index = max([x.index for x in ctrls_scsi]) + 1
>
> - # Take only virtio-scsi ones
> - ctrls_scsi = [x for x in ctrls_scsi
> - if x.model == controller_model]
> -
> - # Save occupied places per controller
> - occupied = collections.defaultdict(int)
> - for d in used_disks:
> - if (d.get_target_prefix() == disk.get_target_prefix() and
> - d.bus == "scsi"):
> - num = virtinst.VirtualDisk.target_to_num(d.target)
> - occupied[num / 7] += 1
> - for c in ctrls_scsi:
> - if occupied[c.index] < 7:
> - controller = c
> - break
> - else:
> - disk.vmm_controller = controller
> + disk.vmm_controller = controller
>
> return controller.index
>
> @@ -1479,9 +1530,12 @@ class vmmAddHardware(vmmGObjectUI):
> self.widget("storage-cache"))
>
> controller_model = None
> - if (bus == "scsi" and
> + controller_index = None
> + scsi_unit = None
> + if (bus == "virtio-scsi" and
> self.vm.get_hv_type() in ["qemu", "kvm", "test"] and
> not self.vm.xmlobj.os.is_pseries()):
> + bus = "scsi"
> controller_model = "virtio-scsi"
>
> collidelist = [d.path for d in self.vm.get_disk_devices()]
> @@ -1507,10 +1561,25 @@ class vmmAddHardware(vmmGObjectUI):
> if d.target not in used:
> used.append(d.target)
>
> - prefer_ctrl = self._set_disk_controller(
> - disk, controller_model, self.vm.get_disk_devices(inactive=True))
> + new_ctrl_idx = self._set_disk_controller(disk, controller_model)
> + disk.generate_target(used, new_ctrl_idx)
> +
> + if bus == "scsi":
> + controller_index = uiutil.get_list_selection(
> + self.widget("storage-scsi-index"))
> + if controller_index != "new":
> + scsi_unit = vmmAddHardware.find_available_scsi_unit(
> + self.vm, controller_index)
> + disk.address.type = "drive"
> + disk.address.controller = controller_index
> + disk.address.bus = "0"
> + disk.address.target = "0"
> + disk.address.unit = scsi_unit
> + disk.vmm_controller = None
> + else:
> + disk.address.type = "drive"
> + disk.address.controller = new_ctrl_idx
>
> - disk.generate_target(used, prefer_ctrl)
> except Exception as e:
> return self.err.val_err(_("Storage parameter error."), e)
>
> diff --git a/virtManager/details.py b/virtManager/details.py
> index a9e0cb79..3c116787 100644
> --- a/virtManager/details.py
> +++ b/virtManager/details.py
> @@ -30,6 +30,7 @@ import libvirt
> import virtinst
> from virtinst import util
> from virtinst import VirtualRNGDevice
> +from virtinst import VirtualController
>
> from . import vmmenu
> from . import uiutil
> @@ -538,7 +539,8 @@ class vmmDetails(vmmGObjectUI):
> "on_disk_removable_changed": lambda *x: self.enable_apply(x, EDIT_DISK_REMOVABLE),
> "on_disk_cache_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_CACHE),
> "on_disk_io_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_IO),
> - "on_disk_bus_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_BUS),
> + "on_controller_index_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_BUS),
> + "on_disk_bus_combo_changed": self.disk_bus_combo_changed,
> "on_disk_format_changed": self.disk_format_changed,
> "on_disk_serial_changed": lambda *x: self.enable_apply(x, EDIT_DISK_SERIAL),
> "on_disk_sgio_entry_changed": lambda *x: self.enable_apply(x, EDIT_DISK_SGIO),
> @@ -1006,6 +1008,10 @@ class vmmDetails(vmmGObjectUI):
> disk_bus = self.widget("disk-bus")
> vmmAddHardware.build_disk_bus_combo(self.vm, disk_bus)
>
> + # Controller index combo
> + controller_index = self.widget("controller-index")
> + vmmAddHardware.build_controller_index_combo(self.vm, controller_index)
> +
> # Network model
> net_model = self.widget("network-model")
> vmmAddHardware.build_network_model_combo(self.vm, net_model)
> @@ -1822,6 +1828,41 @@ class vmmDetails(vmmGObjectUI):
> boot_list.get_selection().emit("changed")
> self.enable_apply(EDIT_BOOTORDER)
>
> + def disk_bus_combo_changed(self, ignore):
> + controller_model = uiutil.get_list_selection(self.widget("disk-bus"))
> +
> + if controller_model in ["scsi", "virtio-scsi"]:
> + disk = self.get_hw_selection(HW_LIST_COL_DEVICE)
> + model = self.widget("controller-index").get_model()
> + model.clear()
> + current_controller = -1
> + i = -1
> + if controller_model == "scsi":
> + controller_model = None
> + for controller in self.vm.get_controller_devices():
> + if (controller.type == "scsi" and
> + controller.model == controller_model):
> + i += 1
> + if (disk.bus == "scsi" and
> + disk.address.controller == controller.index):
> + current_controller = i
> + if not vmmAddHardware.has_available_scsi_unit(
> + self.vm, controller.index):
> + if current_controller != i:
> + continue
> + model.append([str(controller.index),
> + str(controller.index)])
> + model.append(["new", "New"])
> + if current_controller >= 0:
> + self.widget("controller-index").set_active(current_controller)
> + else:
> + self.widget("controller-index").set_active(0)
> + self.widget("controller-idx").set_visible(True)
> + else:
> + self.widget("controller-idx").set_visible(False)
> +
> + self.enable_apply(EDIT_DISK_BUS)
> +
> def disk_format_changed(self, ignore):
> self.widget("disk-format-warn").show()
> self.enable_apply(EDIT_DISK_FORMAT)
> @@ -2127,6 +2168,35 @@ class vmmDetails(vmmGObjectUI):
> if self.edited(EDIT_DISK_BUS):
> bus = uiutil.get_list_selection(self.widget("disk-bus"))
> addr = None
> + controller_model = None
> + if bus in ["scsi", "virtio-scsi"]:
> + controller_index = uiutil.get_list_selection(
> + self.widget("controller-index"))
> + if bus == "virtio-scsi":
> + bus = "scsi"
> + controller_model = "virtio-scsi"
> + if controller_index != "new":
> + scsi_bus = "0"
> + scsi_target = "0"
> + scsi_unit = vmmAddHardware.find_available_scsi_unit(
> + self.vm, controller_index)
> + addr = (controller_index + "/" + scsi_bus + "/" +
> + scsi_target + "/" + str(scsi_unit))
> + else:
> + controllers = self.vm.get_controller_devices()
> + ctrls_scsi = [x for x in controllers if (x.type == "scsi")]
> +
> + new_ctrl = VirtualController(self.conn.get_backend())
> + new_ctrl.type = "scsi"
> + new_ctrl.model = controller_model
> + new_ctrl.index = 0
> +
> + if ctrls_scsi:
> + new_ctrl.index = max([x.index for x in ctrls_scsi]) + 1
> +
> + self.vm.add_device(new_ctrl)
> +
> + addr = str(new_ctrl.index) + "/"
>
This is too much open coded logic, especially since it seems to match
much of what we are doing in addhardware already... we need to find a
way to share this stuff more
> kwargs["bus"] = bus
> kwargs["addrstr"] = addr
> @@ -2681,6 +2751,14 @@ class vmmDetails(vmmGObjectUI):
> vmmAddHardware.populate_disk_bus_combo(self.vm, devtype,
> self.widget("disk-bus").get_model())
> uiutil.set_list_selection(self.widget("disk-bus"), bus)
> + if bus == virtinst.VirtualController.TYPE_SCSI:
> + for controller in self.vm.get_controller_devices():
> + if (controller.type == "scsi" and
> + controller.index == disk.address.controller):
> + if controller.model == "virtio-scsi":
> + uiutil.set_list_selection(self.widget("disk-bus"),
> + "virtio-scsi")
> +
> self.widget("disk-serial").set_text(serial or "")
>
> button = self.widget("disk-cdrom-connect")
> diff --git a/virtManager/domain.py b/virtManager/domain.py
> index a1f59e38..fb658d59 100644
> --- a/virtManager/domain.py
> +++ b/virtManager/domain.py
> @@ -791,7 +791,12 @@ class vmmDomain(vmmLibvirtObject):
> editdev.bus = bus
>
> if oldbus == bus:
> - return
> + if oldbus == "scsi":
> + controller_index = addrstr.split("/")[0]
> + if editdev.address.controller == controller_index:
> + return
> + else:
> + return
>
> editdev.address.clear()
> editdev.address.set_addrstr(addrstr)
> diff --git a/virtinst/device.py b/virtinst/device.py
> index 4f9dcb2c..a8436177 100644
> --- a/virtinst/device.py
> +++ b/virtinst/device.py
> @@ -36,7 +36,7 @@ class VirtualDeviceAddress(XMLBuilder):
> """
> Examples:
> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> - <address type='drive' controller='0' bus='0' unit='0'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> <address type='ccid' controller='0' slot='0'/>
> <address type='virtio-serial' controller='1' bus='0' port='4'/>
> """
> @@ -68,6 +68,19 @@ class VirtualDeviceAddress(XMLBuilder):
> self.domain, self.bus = addrstr.split(":", 1)
> elif addrstr == "spapr-vio":
> self.type = self.ADDRESS_TYPE_SPAPR_VIO
> + elif addrstr.count("/") in [1, 2, 3]:
> + self.type = self.ADDRESS_TYPE_DRIVE
> + self.controller = addrstr.split("/")[0]
> + if addrstr.count("/") == 1:
> + if addrstr.split("/")[1] != "":
> + self.bus = addrstr.split("/")[1]
> + elif addrstr.count("/") == 2:
> + self.bus = addrstr.split("/")[1]
> + self.target = addrstr.split("/")[2]
> + elif addrstr.count("/") == 3:
> + self.bus = addrstr.split("/")[1]
> + self.target = addrstr.split("/")[2]
> + self.unit = addrstr.split("/")[3]
> else:
> raise ValueError(_("Could not determine or unsupported "
> "format of '%s'") % addrstr)
It's hard for me to follow in the code exactly where this address
parsing comes into play and the domain.py It should probably be a
separate though with unit testing
- Cole
More information about the virt-tools-list
mailing list