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

David Lehman dlehman at redhat.com
Wed Nov 28 23:40:33 UTC 2012


On Tue, 2012-11-27 at 13:30 +0100, Vratislav Podzimek wrote:
> 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?

Fixed locally.

> 
> >  
> >          # 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?

It could get 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.

I've used model.get_value a fair amount in custom.py -- especially in
DisksDialog, which has an unfortunate amount of code in common with
VolumeGroupDialog.

> 
> > +            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.

Good idea. Fixed locally.

> 
> > +        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.

Also fixed locally.

> 
> > +
> > +            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")
> 




More information about the anaconda-patches mailing list