[PATCH 2/4] Add support for changing a new LV's VG.

Vratislav Podzimek vpodzime at redhat.com
Tue Nov 27 12:30:18 UTC 2012


See inline comments.

On Wed, 2012-11-21 at 13:44 -0600, David Lehman wrote:
> You can choose from any of the defined VGs or you can create a new VG.
> 
> Also provided is some basic editing for new VGs: name and disk set.
> ---
>  pyanaconda/storage/__init__.py        |   46 +++++-
>  pyanaconda/ui/gui/spokes/custom.glade |  270 ++++++++++++++++++++++++++++++++-
>  pyanaconda/ui/gui/spokes/custom.py    |  256 ++++++++++++++++++++++++++++++-
>  3 files changed, 560 insertions(+), 12 deletions(-)
> 
> diff --git a/pyanaconda/storage/__init__.py b/pyanaconda/storage/__init__.py
> index 4b3ba0f..07f2cce 100644
> --- a/pyanaconda/storage/__init__.py
> +++ b/pyanaconda/storage/__init__.py
> @@ -1832,6 +1832,10 @@ class Storage(object):
>              # short-circuit the logic below for partitions
>              return factory.member_list
>  
> +        if container and container.exists:
> +            # don't try to modify an existing container
> +            return container.parents
> +
>          if factory.container_size_func is None:
>              return []
>  
> @@ -1988,9 +1992,17 @@ class Storage(object):
>                      self, size, [d.name for d in disks], raid_level))
>          return factory_class(self, size, disks, raid_level, encrypted)
>  
> -    def getContainer(self, factory, device=None):
> +    def getContainer(self, factory, device=None, container_name=None):
> +        # XXX would it be useful to implement this as a series of fallbacks
> +        #     instead of mutually exclusive branches?
>          container = None
> -        if device:
> +        if container_name:
> +            container = self.devicetree.getDeviceByName(container_name)
> +            if container and container not in factory.container_list:
> +                log.debug("specified container name %s is wrong type (%s)"
> +                            % (container_name, container.type))
> +                container = None
> +        elif device:
>              if hasattr(device, "vg"):
>                  container = device.vg
>              elif hasattr(device, "volume"):
> @@ -2032,6 +2044,9 @@ class Storage(object):
>  
>                  raid_level          (btrfs/md/lvm only) RAID level (string)
>  
> +                name                name for new device
> +                container_name      name of requested container
> +
>                  device              an already-defined but non-existent device
>                                      to adjust instead of creating a new device
>  
> @@ -2059,11 +2074,19 @@ class Storage(object):
>          disks = kwargs.get("disks")
>          encrypted = kwargs.get("encrypted", self.data.autopart.encrypted)
>  
> +        name = kwargs.get("name")
> +        container_name = kwargs.get("container_name")
> +
>          device = kwargs.get("device")
>  
>          # md, btrfs
>          raid_level = kwargs.get("raid_level")
>  
> +        # we can't do anything with existing devices
> +        if device and device.exists:
> +            log.info("newDevice refusing to change device %s" % device)
> +            return
> +
>          if not fstype:
>              fstype = self.getFSType(mountpoint=mountpoint)
>              if fstype == "swap":
> @@ -2082,7 +2105,10 @@ class Storage(object):
>              raise StorageError("no disks specified for new device")
>  
>          self.size_sets = [] # clear this since there are no growable reqs now
> -        container = self.getContainer(factory, device=device)
> +
> +
> +        container = self.getContainer(factory, device=device,
> +                                      container_name=container_name)
Are two empty lines needed here?

>  
>          # TODO: striping, mirroring, &c
>          # TODO: non-partition members (pv-on-md)
> @@ -2129,8 +2155,12 @@ class Storage(object):
>          # set up container
>          if not container and factory.new_container_attr:
>              log.debug("creating new container")
> +            if container_name:
> +                kwa = {"name": container_name}
> +            else:
> +                kwa = {}
>              try:
> -                container = factory.new_container(parents=parents)
> +                container = factory.new_container(parents=parents, **kwa)
>              except StorageError as e:
>                  log.error("failed to create new device: %s" % e)
>                  # Clean up by destroying the newly created member devices.
> @@ -2213,12 +2243,18 @@ class Storage(object):
>                  else:
>                      self.__cleanUpMemberDevices(parents)
>  
> +            if name:
> +                kwa = {"name": name}
> +            else:
> +                kwa = {}
> +
>              try:
>                  device = factory.new_device(parents=parents,
>                                              size=size,
>                                              fmt_type=fstype,
>                                              mountpoint=mountpoint,
> -                                            fmt_args=fmt_args)
> +                                            fmt_args=fmt_args,
> +                                            **kwa)
>              except StorageError as e:
>                  log.error("device instance creation failed: %s" % e)
>                  _container_post_error()
> diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py
> index fe2d9b0..21501af 100644
> --- a/pyanaconda/ui/gui/spokes/custom.py
> +++ b/pyanaconda/ui/gui/spokes/custom.py
> @@ -21,13 +21,17 @@
>  #
>  
>  # TODO:
> -# - empty size when adding a mountpoint is no longer using all available space
> +# - Add a way for users to specify the names of md, lv, subvol devices.
> +# - We should either remove boot disk selection from the cart we show from here
> +#   or re-partition when it gets changed to make a best-effort at keeping up.
> +# - Add new LVs to existing VG if creating a new VG fails outright.
>  # - Deleting an LV is not reflected in available space in the bottom left.
>  #   - this is only true for preexisting LVs
>  # - Device descriptions, suggested sizes, etc. should be moved out into a support file.
>  # - Tabbing behavior in the accordion is weird.
>  # - Update feature space costs when size spinner changes.
> -# - Either disable stripe/mirror for LVM or implement it for device creation.
> +# - Implement striping and mirroring for LVM.
> +# - Implement container management for btrfs.
>  # - If you click to add a mountpoint while editing a device the lightbox
>  #   screenshot is taken prior to the ui update so the background shows the old
>  #   size and free space while you're deciding on a size for the new device.
> @@ -92,6 +96,7 @@ NOTEBOOK_LUKS_PAGE = 2
>  NOTEBOOK_UNEDITABLE_PAGE = 3
>  
>  new_install_name = N_("New %s %s Installation") % (productName, productVersion)
> +new_vg_text = N_("Create a new volume group ...")
>  unrecoverable_error_msg = N_("Storage configuration reset due to unrecoverable "
>                               "error. Click for details.")
>  device_configuration_error_msg = N_("Device reconfiguration failed. Click for "
> @@ -340,6 +345,69 @@ class DisksDialog(GUIObject):
>      def run(self):
>          return self.window.run()
>  
> +class VolumeGroupDialog(GUIObject):
> +    builderObjects = ["vg_dialog", "disk_store", "vg_disk_view"]
> +    mainWidgetName = "vg_dialog"
> +    uiFile = "spokes/custom.glade"
> +
> +    def __init__(self, *args, **kwargs):
> +        self._disks = kwargs.pop("disks")
> +        free = kwargs.pop("free")
> +        self.name = kwargs.pop("name") or ""
I believe you want kwargs.pop("name", "") here. Or could kwargs contain
"name": None?

> +        self.selected = kwargs.pop("selected")[:]
> +        GUIObject.__init__(self, *args, **kwargs)
> +
> +        self.builder.get_object("vg_name_entry").set_text(self.name)
> +
> +        self._store = self.builder.get_object("disk_store")
> +        # populate the store
> +        for disk in self._disks:
> +            self._store.append([disk.description,
> +                                str(Size(spec="%dMB" % disk.size)),
> +                                str(free[disk.name][0]),
> +                                disk.serial,
> +                                disk.id])
> +
> +        treeview = self.builder.get_object("vg_disk_view")
> +        model = treeview.get_model()
> +        itr = model.get_iter_first()
> +
> +        selected_ids = [d.id for d in self.selected]
> +        log.debug("selected: %s" % [d.name for d in self.selected])
> +        log.debug("selected: %s" % selected_ids)
> +        selection = treeview.get_selection()
> +        while itr:
> +            disk_id = model.get_value(itr, 4)
> +            log.debug("store: %d" % disk_id)
> +            if disk_id in selected_ids:
> +                selection.select_iter(itr)
> +
> +            itr = model.iter_next(itr)
> +
> +    def on_cancel_clicked(self, button):
> +        self.window.destroy()
> +
> +    def _get_disk_by_id(self, disk_id):
> +        for disk in self._disks:
> +            if disk.id == disk_id:
> +                return disk
> +
> +    def on_save_clicked(self, button):
> +        treeview = self.builder.get_object("vg_disk_view")
> +        model, paths = treeview.get_selection().get_selected_rows()
> +        self.selected = []
> +        for path in paths:
> +            itr = model.get_iter(path)
> +            disk_id = model.get_value(itr, 4)
We use model[itr][4] elsewhere.

> +            self.selected.append(self._get_disk_by_id(disk_id))
> +
> +        self.name = self.builder.get_object("vg_name_entry").get_text()
> +
> +        self.window.destroy()
> +
> +    def run(self):
> +        return self.window.run()
> +
>  class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>      builderObjects = ["customStorageWindow", "sizeAdjustment",
>                        "partitionStore",
> @@ -360,7 +428,9 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          self._fs_types = []             # list of supported fstypes
>          self._unused_devices = None     # None indicates uninitialized
>          self._free_space = Size(bytes=0)
> +
>          self._device_disks = []
> +        self._device_container_name = None
>  
>          self._initialized = False
>  
> @@ -826,7 +896,6 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          prev_encrypted = device.encrypted
>          log.debug("old encryption setting: %s" % prev_encrypted)
>          encryption_checkbox = self.builder.get_object("encryptCheckbox")
> -        encrypted = None
>          encrypted = encryption_checkbox.get_active()
>  
>          changed_encryption = (prev_encrypted != encrypted)
> @@ -893,6 +962,13 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>              self._populate_right_side(selector)
>              return
>  
> +        # XXX Now we have to do something squirrely with the encryption setting.
> +        # We've done the checks for things that cannot be on an encrypted
> +        # device. From here on out, it means "encrypt new devices we create",
> +        # which we don't want to do in the event that encryption is already
> +        # present below the surface of an existing device stack.
> +        encrypted = encrypted and encryption_checkbox.get_sensitive()
> +
>          with ui_storage_logger():
>              # create a new factory using the appropriate size and type
>              factory = self.__storage.getDeviceFactory(device_type, size,
> @@ -919,9 +995,22 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          old_disk_set = device.disks
>          if hasattr(device, "req_disks") and not device.exists:
>              old_disk_set = device.req_disks
> +
>          changed_disk_set = (set(old_disk_set) != set(self._device_disks))
>  
> -        if changed_device_type or changed_raid_level:
> +        changed_container = False
> +        old_container_name = None
> +        container = self.__storage.getContainer(factory)
> +        if not changed_device_type and device_type == DEVICE_TYPE_LVM:
> +            old_container = self.__storage.getContainer(factory,
> +                                                        device=device)
> +            container = self.__storage.getContainer(factory,
> +                                                    container_name=self._device_container_name)
> +            if self._device_container_name != old_container.name:
> +                old_container_name = old_container.name
> +                changed_container = True
> +
> +        if changed_device_type or changed_raid_level or changed_container:
>              if changed_device_type:
>                  log.info("changing device type from %s to %s"
>                              % (current_device_type, device_type))
> @@ -935,6 +1024,10 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>                              % ([d.name for d in device.disks],
>                                 [d.name for d in self._device_disks]))
>  
> +            if changed_container:
> +                log.info("changing container from %s to %s"
> +                            % (old_container.name, self._device_container_name))
> +
>              # remove the current device
>              self.clear_errors()
>              root = self._current_selector._root
> @@ -945,7 +1038,6 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>  
>              with ui_storage_logger():
>                  disks = self._device_disks[:]
> -                container = self.__storage.getContainer(factory)
>                  if container and changed_device_type:
>                      log.debug("overriding disk set with container's")
>                      disks = container.disks[:]
> @@ -955,6 +1047,7 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>                                           disks=disks, mountpoint=mountpoint,
>                                           label=label, raid_level=raid_level,
>                                           encrypted=encrypted,
> +                                         container_name=self._device_container_name,
>                                           selector=selector)
>                  except ErrorRecoveryFailure as e:
>                      self._error = e
> @@ -979,6 +1072,7 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>                                               label=old_label,
>                                               raid_level=old_raid_level,
>                                               encrypted=prev_encrypted,
> +                                             container_name=old_container_name,
>                                               selector=selector)
>                      except StorageError as e:
>                          # failed to recover.
> @@ -1043,6 +1137,7 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>                                                   device=device,
>                                                   disks=self._device_disks[:],
>                                                   encrypted=encrypted,
> +                                                 container_name=self._device_container_name,
>                                                   raid_level=raid_level)
>                      except ErrorRecoveryFailure as e:
>                          self._error = e
> @@ -1368,6 +1463,13 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>  
>          log.debug("updated device_disks to %s" % [d.name for d in self._device_disks])
>  
> +        if hasattr(use_dev, "vg"):
> +            self._device_container_name = use_dev.vg.name
> +        else:
> +            self._device_container_name = None
> +
> +        log.debug("updated device_vg_name to %s" % self._device_container_name)
> +
>          selectedDeviceLabel.set_text(selector.props.name)
>          selectedDeviceDescLabel.set_text(self._description(selector.props.name))
>  
> @@ -1522,9 +1624,10 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          typeCombo.set_active(type_index_map[device_type])
>  
>          # you can't change the type of an existing device
> -        typeCombo.set_sensitive(not device.exists)
> +        typeCombo.set_sensitive(not use_dev.exists)
>  
>          self._populate_raid(raid_level, device.size)
> +        self._populate_lvm(device=use_dev)
>          self.builder.get_object("optionsNotebook").set_sensitive(not device.exists)
>  
>      ###
> @@ -1807,6 +1910,94 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>  
>          self._device_disks = disks
>  
> +    def run_vg_editor(self, vg=None, name=None):
> +        if vg:
> +            vg_name = vg.name
> +        elif name:
> +            vg_name = name
> +
> +        dialog = VolumeGroupDialog(self.data,
> +                                   name=vg_name,
> +                                   disks=self._clearpartDevices,
> +                                   free=self._currentFreeInfo,
> +                                   selected=self._device_disks)
> +
> +        with enlightbox(self.window, dialog.window):
> +            rc = dialog.run()
> +
> +        if rc == 0:
> +            return
> +
> +        disks = dialog.selected
> +        name = dialog.name
> +        log.debug("new disks for %s: %s" % (name, [d.name for d in disks]))
> +        if not disks:
> +            self._error = "No disks selected. Not saving changes."
> +            self.window.set_info(Gtk.MessageType.INFO,
> +                                 self._error)
> +            self.window.show_all()
> +            return
> +
> +        log.debug("new VG name: %s" % name)
> +        if name != vg_name and name in self.__storage.names:
> +            self._error = _("Volume Group name %s is already in use. Not "
> +                            "saving changes.")
> +            self.window.set_info(Gtk.MessageType.INFO,
> +                                 self._error)
> +            self.window.show_all()
> +            return
> +
> +        self._device_disks = disks
> +        self._device_container_name = name
> +
> +    def on_modify_vg_clicked(self, button):
> +        vg_name = self.builder.get_object("volumeGroupCombo").get_active_text()
> +
> +        vg = self.__storage.devicetree.getDeviceByName(vg_name)
> +
> +        # pass the name along with any found vg since we could be modifying a
> +        # vg that hasn't been instantiated yet
> +        self.run_vg_editor(vg=vg, name=vg_name)
> +
> +        log.debug("%s -> %s" % (vg_name, self._device_container_name))
> +        if vg_name == self._device_container_name:
> +            return
> +
> +        log.debug("renaming VG %s to %s" % (vg_name, self._device_container_name))
> +        if vg:
> +            self.__storage.devicetree.names.remove(vg.name)
> +            self.__storage.devicetree.names.append(self._device_container_name)
> +            vg.name = self._device_container_name
> +
> +        vg_combo = self.builder.get_object("volumeGroupCombo")
> +        for idx, data in enumerate(vg_combo.get_model()):
> +            # we're looking for the original vg name
> +            if data[0] == vg_name:
> +                vg_combo.remove(idx)
> +                vg_combo.insert_text(idx, self._device_container_name)
> +                vg_combo.set_active(idx)
> +                break
> +
> +    def on_vg_changed(self, combo):
> +        vg_name = combo.get_active_text()
> +        log.debug("new vg selection: %s" % vg_name)
> +        if vg_name == new_vg_text:
> +            # run the vg editor dialog with a default name and disk set
> +            hostname = self.data.network.hostname
> +            name = self.__storage.suggestContainerName(hostname=hostname)
> +            self.run_vg_editor(name=name)
> +            for idx, data in enumerate(combo.get_model()):
> +                if data[0] == new_vg_text:
> +                    combo.insert_text(idx, self._device_container_name)
> +                    combo.set_active(idx)
> +                    break
> +        else:
> +            self._device_container_name = vg_name
> +
> +        vg = self.__storage.devicetree.getDeviceByName(self._device_container_name)
> +        vg_exists = getattr(vg, "exists", False)    # might not be in the tree
> +        self.builder.get_object("modifyVGButton").set_sensitive(not vg_exists)
> +
>      def on_selector_clicked(self, selector):
>          if not self._initialized:
>              return
> @@ -1971,6 +2162,55 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>                                   hasattr(fmt, "label"))
>          mountPointEntry.set_sensitive(fmt.mountable)
>  
> +    def _populate_lvm(self, device=None):
> +        #
> +        # set up the volume group widgets for lvm or hide them for other types
> +        #
This could be a docstring.

> +        device_type = self._get_current_device_type()
> +        if device is None:
> +            if self._current_selector is None:
> +                return
> +
> +            device = self._current_selector._device
> +
> +        vg_combo = self.builder.get_object("volumeGroupCombo")
> +        vg_button = self.builder.get_object("modifyVGButton")
> +        vg_label = self.builder.get_object("volumeGroupLabel")
> +        if device_type == DEVICE_TYPE_LVM:
> +            # set up the vg widgets and then bail out
> +            if self._device_container_name:
> +                default_vg = self._device_container_name
> +            else:
> +                with ui_storage_logger():
> +                    factory = self.__storage.getDeviceFactory(DEVICE_TYPE_LVM,
> +                                                              0)
> +                    container = self.__storage.getContainer(factory)
> +                    default_vg = getattr(container, "name", None)
> +
> +            vg_combo.remove_all()
> +            vgs = self.__storage.vgs
> +            for vg in vgs:
> +                vg_combo.append_text(vg.name)
> +                if default_vg and vg.name == default_vg:
> +                    vg_combo.set_active(vgs.index(vg))
> +
> +            vg_combo.append_text(new_vg_text)
> +            if default_vg is None:
> +                vg_combo.set_active(len(vg_combo.get_model()))
> +                #vg_combo.set_active(-1)
This could probably go away.

> +
> +            vg_combo.show()
> +            vg_button.show()
> +            vg_label.show()
> +
> +            # make the combo and button insensitive for existing LVs
> +            can_change_vg = (device is not None and not device.exists)
> +            vg_combo.set_sensitive(can_change_vg)
> +        else:
> +            vg_label.hide()
> +            vg_combo.hide()
> +            vg_button.hide()
> +
>      def on_device_type_changed(self, combo):
>          if not self._initialized:
>              return
> @@ -2011,8 +2251,12 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          elif new_type == DEVICE_TYPE_MD:
>              raid_level = "raid0"
>  
> +        # lvm uses the RHS to set disk set. no foolish minds here.
> +        self._configButton.set_sensitive(new_type != DEVICE_TYPE_LVM)
> +
>          size = self.builder.get_object("sizeSpinner").get_value()
>          self._populate_raid(raid_level, size)
> +        self._populate_lvm()
>  
>          # begin btrfs magic
>          fsCombo = self.builder.get_object("fileSystemTypeCombo")

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list