This is the port of the early disk snapshot stuff we implemented for the Fedora 22 on the *f22-branch*. Turns out a better and reliable solution for a number of bugs these changes fixed and prevented won't be available any time soon so I'm porting this to the master branch to prevent serious regressions.
There have been some conflicts, basically all caused by the fixes for kickstart autopart missing passphrase and by changes done for the s390 stuff moved from blivet to libblockdev.
From: Vratislav Podzimek vpodzime@redhat.com
This method has grown into something very long, complicated and hard to understand as a whole. It happened many times in the past that when doing changes we forgot to cover one of the many branches its code code could go through.
In order to mitigate the risk of such errors in the future this patch moves some parts of the code into separate methods, names some contants and deduplicates some checks previously done multiple times with zero risk of changes happening in the meantime. Also it unifies the response codes of the dialogs by changing the ResizeDialog's "Reclaim" button's response ID to 1 (meaning OK).
Conflicts: pyanaconda/ui/gui/spokes/storage.py
(cherry-picked from commit 4e49e26b0fe9b9b3922a3bf0faff6860685624c1) --- pyanaconda/ui/gui/spokes/lib/resize.glade | 2 +- pyanaconda/ui/gui/spokes/storage.py | 297 +++++++++++++++--------------- 2 files changed, 151 insertions(+), 148 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/lib/resize.glade b/pyanaconda/ui/gui/spokes/lib/resize.glade index 223416c..4bb8f0e 100644 --- a/pyanaconda/ui/gui/spokes/lib/resize.glade +++ b/pyanaconda/ui/gui/spokes/lib/resize.glade @@ -360,7 +360,7 @@ </child> <action-widgets> <action-widget response="0">cancelButton</action-widget> - <action-widget response="2">resizeButton</action-widget> + <action-widget response="1">resizeButton</action-widget> </action-widgets> <child internal-child="accessible"> <object class="AtkObject" id="resizeDialog-atkobject"> diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 8e5f367..552d70a 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -87,9 +87,13 @@
# Response ID codes for all the various buttons on all the dialogs. RESPONSE_CANCEL = 0 +RESPONSE_OK = 1 RESPONSE_MODIFY_SW = 2 RESPONSE_RECLAIM = 3 RESPONSE_QUIT = 4 +DASD_FORMAT_NO_CHANGE = -1 +DASD_FORMAT_REFRESH = 1 +DASD_FORMAT_RETURN_TO_HUB = 2
class InstallOptionsDialogBase(GUIObject): uiFile = "spokes/storage.glade" @@ -736,12 +740,7 @@ def run_lightbox_dialog(self, dialog):
return rc
- def _check_encrypted(self): - # even if they're not doing autopart, setting autopart.encrypted - # establishes a default of encrypting new devices - if not self.encrypted: - return True - + def _setup_passphrase(self): dialog = PassphraseDialog(self.data) rc = self.run_lightbox_dialog(dialog) if rc != 1: @@ -756,44 +755,14 @@ def _check_encrypted(self):
return True
- def on_back_clicked(self, button): - # We can't exit early if it looks like nothing has changed because the - # user might want to change settings presented in the dialogs shown from - # within this method. + def _remove_nonexistant_partitions(self): + for partition in self.storage.partitions[:]: + # check if it's been removed in a previous iteration + if not partition.exists and \ + partition in self.storage.partitions: + self.storage.recursiveRemove(partition)
- if self.autopart_missing_passphrase: - self._check_encrypted() - NormalSpoke.on_back_clicked(self, button) - return - - # Do not enter this method multiple times if user clicking multiple times - # on back button - if self._back_clicked: - return - else: - self._back_clicked = True - - # Remove all non-existing devices if autopart was active when we last - # refreshed. - if self._previous_autopart: - self._previous_autopart = False - for partition in self.storage.partitions[:]: - # check if it's been removed in a previous iteration - if not partition.exists and \ - partition in self.storage.partitions: - self.storage.recursiveRemove(partition) - - # make sure no containers were split up by the user's disk selection - self.clear_info() - self.errors = checkDiskSelection(self.storage, self.selected_disks) - if self.errors: - # The disk selection has to make sense before we can proceed. - self.set_error(_("There was a problem with your disk selection. " - "Click here for details.")) - self._back_clicked = False - return - - # hide/unhide disks as requested + def _hide_unhide_disks(self): for disk in self.disks: if disk.name not in self.selected_disks and \ disk in self.storage.devices: @@ -802,40 +771,20 @@ def on_back_clicked(self, button): disk not in self.storage.devices: self.storage.devicetree.unhide(disk)
- # show the installation options dialog - disks = [d for d in self.disks if d.name in self.selected_disks] - disks_size = sum((d.size for d in disks), Size(0)) - - # No disks selected? The user wants to back out of the storage spoke. - if not disks: - NormalSpoke.on_back_clicked(self, button) - return + def _check_dasd_formats(self): + rc = DASD_FORMAT_NO_CHANGE + dasds = self.storage.devicetree.make_unformatted_dasd_list(self.selected_disks) + if len(dasds) > 0: + # We want to apply current selection before running dasdfmt to + # prevent this information from being lost afterward + applyDiskSelection(self.storage, self.data, self.selected_disks) + dialog = DasdFormatDialog(self.data, self.storage, dasds) + ignoreEscape(dialog.window) + rc = self.run_lightbox_dialog(dialog)
- if arch.isS390(): - # check for unformatted DASDs and launch dasdfmt if any discovered - dasds = self.storage.devicetree.make_unformatted_dasd_list(disks) - if len(dasds) > 0: - # We want to apply current selection before running dasdfmt to - # prevent this information from being lost afterward - applyDiskSelection(self.storage, self.data, self.selected_disks) - dialog = DasdFormatDialog(self.data, self.storage, dasds) - ignoreEscape(dialog.window) - rc = self.run_lightbox_dialog(dialog) - if rc == 1: - # User hit OK on the dialog - self.refresh() - elif rc == 2: - # User clicked uri to return to hub. - NormalSpoke.on_back_clicked(self, button) - return - elif rc != 2: - # User either hit cancel on the dialog or closed it via escape, - # there was no formatting done. - # NOTE: rc == 2 means the user clicked on the link that takes t - # back to the hub. - self._back_clicked = False - return + return rc
+ def _check_space_and_get_dialog(self, disks): # Figure out if the existing disk labels will work on this platform # you need to have at least one of the platform's labels in order for # any of the free space to be useful. @@ -853,6 +802,7 @@ def on_back_clicked(self, button): disk_free = sum(f[0] for f in free_space.values()) fs_free = sum(f[1] for f in free_space.values())
+ disks_size = sum((d.size for d in disks), Size(0)) required_space = self.payload.spaceRequired auto_swap = sum((r.size for r in self.storage.autoPartitionRequests if r.fstype == "swap"), Size(0)) @@ -867,98 +817,151 @@ def on_back_clicked(self, button): if disk_free >= required_space + auto_swap: dialog = None elif disks_size >= required_space: - if self._customPart.get_active() or self._reclaim.get_active(): - dialog = None - else: - dialog = NeedSpaceDialog(self.data, payload=self.payload) - dialog.refresh(required_space, auto_swap, disk_free, fs_free) - rc = self.run_lightbox_dialog(dialog) + dialog = NeedSpaceDialog(self.data, payload=self.payload) + dialog.refresh(required_space, auto_swap, disk_free, fs_free) else: dialog = NoSpaceDialog(self.data, payload=self.payload) dialog.refresh(required_space, auto_swap, disk_free, fs_free) - rc = self.run_lightbox_dialog(dialog)
- if not dialog: - # Plenty of room - there's no need to pop up a dialog, so just send - # the user to wherever they asked to go. That's either the custom - # spoke or the hub. - # - OR - - # Not enough room, but the user checked the reclaim button. + # the 'dialog' variable is always set by the if statement above + return dialog
- self.encrypted = self._encrypted.get_active() + def _run_dialogs(self, disks, start_with): + rc = self.run_lightbox_dialog(start_with) + if rc == RESPONSE_RECLAIM: + # we need to run another dialog
- if self._customPart.get_active(): - self.autopart = False - self.skipTo = "CustomPartitioningSpoke" - else: - self.autopart = True - - # We might first need to ask about an encryption passphrase. - if not self._check_encrypted(): - self._back_clicked = False - return - - # Oh and then we might also want to go to the reclaim dialog. - if self._reclaim.get_active(): - self.apply() - if not self._show_resize_dialog(disks): - # User pressed cancel on the reclaim dialog, so don't leave - # the storage spoke. - self._back_clicked = False - return - elif rc == RESPONSE_CANCEL: - # A cancel button was clicked on one of the dialogs. Stay on this - # spoke. Generally, this is because the user wants to add more disks. + # respect disk selection and other choices in the ReclaimDialog + self.apply() + resize_dialog = ResizeDialog(self.data, self.storage, self.payload) + resize_dialog.refresh(disks) + + return self._run_dialogs(disks, start_with=resize_dialog) + else: + # we are done + return rc + + def on_back_clicked(self, button): + # We can't exit early if it looks like nothing has changed because the + # user might want to change settings presented in the dialogs shown from + # within this method. + + # Do not enter this method multiple times if user clicking multiple times + # on back button + if self._back_clicked: + return + else: + self._back_clicked = True + + if self.autopart_missing_passphrase: + self._setup_passphrase() + NormalSpoke.on_back_clicked(self, button) + return + + # Remove all non-existing devices if autopart was active when we last + # refreshed. + if self._previous_autopart: + self._previous_autopart = False + self._remove_nonexistant_partitions() + + # hide/unhide disks as requested + self._hide_unhide_disks() + + disks = [d for d in self.disks if d.name in self.selected_disks] + # No disks selected? The user wants to back out of the storage spoke. + if not disks: + NormalSpoke.on_back_clicked(self, button) + return + + # make sure no containers were split up by the user's disk selection + self.clear_info() + self.errors = checkDiskSelection(self.storage, self.selected_disks) + if self.errors: + # The disk selection has to make sense before we can proceed. + self.set_error(_("There was a problem with your disk selection. " + "Click here for details.")) self._back_clicked = False return - elif rc == RESPONSE_MODIFY_SW: - # The "Fedora software selection" link was clicked on one of the - # dialogs. Send the user to the software spoke. - self.skipTo = "SoftwareSelectionSpoke" - elif rc == RESPONSE_RECLAIM: - # Not enough space, but the user can make enough if they do some - # work and free up space. - self.encrypted = self._encrypted.get_active() - - if not self._check_encrypted(): - return
- self.apply() - if not self._show_resize_dialog(disks): - # User pressed cancel on the reclaim dialog, so don't leave - # the storage spoke. + if arch.isS390(): + # check for unformatted DASDs and launch dasdfmt if any discovered + rc = self._check_dasd_formats() + if rc == DASD_FORMAT_NO_CHANGE: + pass + elif rc == DASD_FORMAT_REFRESH: + # User hit OK on the dialog + self.refresh() + elif rc == DASD_FORMAT_RETURN_TO_HUB: + # User clicked uri to return to hub. + NormalSpoke.on_back_clicked(self, button) + return + else: + # User either hit cancel on the dialog or closed it via escape, + # there was no formatting done. self._back_clicked = False return
- # And then go to the custom partitioning spoke if they chose to - # do so. - if self._customPart.get_active(): - self.autopart = False - self.skipTo = "CustomPartitioningSpoke" - else: - self.autopart = True - elif rc == RESPONSE_QUIT: - # Not enough space, and the user can't do anything about it so - # they chose to quit. - raise SystemExit("user-selected exit") - else: - # I don't know how we'd get here, but might as well have a - # catch-all. Just stay on this spoke. + # even if they're not doing autopart, setting autopart.encrypted + # establishes a default of encrypting new devices + self.encrypted = self._encrypted.get_active() + + # We might first need to ask about an encryption passphrase. + if self.encrypted and not self._setup_passphrase(): self._back_clicked = False return
+ # At this point there are three possible states: + # 1) user chose custom part => just send them to the CustomPart spoke + # 2) user wants to reclaim some more space => run the ResizeDialog + # 3) we are just asked to do autopart => check free space and see if we need + # user to do anything more + self.autopart = not self._customPart.get_active() + dialog = None + if not self.autopart: + self.skipTo = "CustomPartitioningSpoke" + elif self._reclaim.get_active(): + # HINT: change the logic of this 'if' statement if we are asked to + # support "reclaim before custom partitioning" + + # respect disk selection and other choices in the ReclaimDialog + self.apply() + dialog = ResizeDialog(self.data, self.storage, self.payload) + dialog.refresh(disks) + else: + dialog = self._check_space_and_get_dialog(disks) + + if dialog: + # more dialogs may need to be run based on user choices, but we are + # only interested in the final result + rc = self._run_dialogs(disks, start_with=dialog) + + if rc == RESPONSE_OK: + # nothing special needed + pass + elif rc == RESPONSE_CANCEL: + # A cancel button was clicked on one of the dialogs. Stay on this + # spoke. Generally, this is because the user wants to add more disks. + self._back_clicked = False + return + elif rc == RESPONSE_MODIFY_SW: + # The "Fedora software selection" link was clicked on one of the + # dialogs. Send the user to the software spoke. + self.skipTo = "SoftwareSelectionSpoke" + elif rc == RESPONSE_QUIT: + # Not enough space, and the user can't do anything about it so + # they chose to quit. + raise SystemExit("user-selected exit") + else: + # I don't know how we'd get here, but might as well have a + # catch-all. Just stay on this spoke. + self._back_clicked = False + return + if self.autopart: refreshAutoSwapSize(self.storage) self.applyOnSkip = True NormalSpoke.on_back_clicked(self, button)
- def _show_resize_dialog(self, disks): - resizeDialog = ResizeDialog(self.data, self.storage, self.payload) - resizeDialog.refresh(disks) - - rc = self.run_lightbox_dialog(resizeDialog) - return rc - def on_custom_toggled(self, button): # The custom button won't be active until after this handler is run, # so we have to negate everything here.
From: Vratislav Podzimek vpodzime@redhat.com
If user deselects all disks it means they just want to go back from the disk selection screen. If that's the case, we shouldn't do any changes to storage configuration.
Conflicts: pyanaconda/ui/gui/spokes/storage.py
(cherry-picked from commit 0c9a8bc1192c320bc1461687e88f4275eff4b213) --- pyanaconda/ui/gui/spokes/storage.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 552d70a..b651d79 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -858,6 +858,12 @@ def on_back_clicked(self, button): NormalSpoke.on_back_clicked(self, button) return
+ disks = [d for d in self.disks if d.name in self.selected_disks] + # No disks selected? The user wants to back out of the storage spoke. + if not disks: + NormalSpoke.on_back_clicked(self, button) + return + # Remove all non-existing devices if autopart was active when we last # refreshed. if self._previous_autopart: @@ -867,12 +873,6 @@ def on_back_clicked(self, button): # hide/unhide disks as requested self._hide_unhide_disks()
- disks = [d for d in self.disks if d.name in self.selected_disks] - # No disks selected? The user wants to back out of the storage spoke. - if not disks: - NormalSpoke.on_back_clicked(self, button) - return - # make sure no containers were split up by the user's disk selection self.clear_info() self.errors = checkDiskSelection(self.storage, self.selected_disks)
From: Vratislav Podzimek vpodzime@redhat.com
Related: rhbz#1166598 (cherry picked from commit b10e958a2d1a3a115a0de272c6ecb3175d3f7f4c) --- pyanaconda/storage_utils.py | 62 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
diff --git a/pyanaconda/storage_utils.py b/pyanaconda/storage_utils.py index 342dfb5..7289bc5 100644 --- a/pyanaconda/storage_utils.py +++ b/pyanaconda/storage_utils.py @@ -382,3 +382,65 @@ def bound_size(size, device, old_size): size = min_size
return size + +class StorageSnapshot(object): + """R/W snapshot of storage (i.e. a :class:`blivet.Blivet` instance)""" + + def __init__(self, storage=None): + """ + Create new instance of the class + + :param storage: if given, its snapshot is created + :type storage: :class:`blivet.Blivet` + """ + if storage: + self._storage_snap = storage.copy() + else: + self._storage_snap = None + + @property + def storage(self): + return self._storage_snap + + @property + def created(self): + return bool(self._storage_snap) + + def create_snapshot(self, storage): + """Create (and save) snapshot of storage""" + + self._storage_snap = storage.copy() + + def dispose_snapshot(self): + """ + Dispose (unref) the snapshot + + .. note:: + + In order to free the memory taken by the snapshot, all references + returned by :property:`self.storage` have to be unrefed too. + """ + self._storage_snap = None + + def reset_to_snapshot(self, storage, dispose=False): + """ + Reset storage to snapshot (**modifies :param:`storage` in place**) + + :param storage: :class:`blivet.Blivet` instance to reset to the created snapshot + :param bool dispose: whether to dispose the snapshot after reset or not + :raises ValueError: if no snapshot is available (was not created before) + """ + if not self.created: + raise ValueError("No snapshot created, cannot reset") + + # we need to create a new copy from the snapshot first -- simple + # assignment from the snapshot would result in snapshot being modified + # by further changes of 'storage' + new_copy = self._storage_snap.copy() + storage.devicetree = new_copy.devicetree + storage.roots = new_copy.roots + storage.fsset = new_copy.fsset + + if dispose: + self.dispose_snapshot() +
From: Vratislav Podzimek vpodzime@redhat.com
Reverting changes in storage is really complicated and trying to do so lead us to many hard bugs like the one referenced above. So instead of playing this tricky game, let's just create a snapshot of unmodified on-disk storage and revert everything to it whenever we need to start over.
Big THANKS! goes to Vojtech Trefny vtrefny@redhat.com for his help with this patch.
(cherry picked from commit 0f267be6966374fd247d705bee003d1f7eca18fe)
Conflicts: pyanaconda/ui/gui/spokes/storage.py --- pyanaconda/storage_utils.py | 3 ++ pyanaconda/ui/gui/spokes/storage.py | 62 +++++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 17 deletions(-)
diff --git a/pyanaconda/storage_utils.py b/pyanaconda/storage_utils.py index 7289bc5..b3774ec 100644 --- a/pyanaconda/storage_utils.py +++ b/pyanaconda/storage_utils.py @@ -444,3 +444,6 @@ def reset_to_snapshot(self, storage, dispose=False): if dispose: self.dispose_snapshot()
+# a snapshot of early storage as we got it from scanning disks without doing any +# changes +on_disk_storage = StorageSnapshot() diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index b651d79..476c8a9 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -74,6 +74,7 @@ from pyanaconda.i18n import _, C_, CN_, P_ from pyanaconda import constants, iutil, isys from pyanaconda.bootloader import BootLoaderError +from pyanaconda.storage_utils import on_disk_storage
from pykickstart.constants import CLEARPART_TYPE_NONE, AUTOPART_TYPE_LVM from pykickstart.errors import KickstartValueError @@ -264,6 +265,7 @@ def __init__(self, *args, **kwargs): self.encrypted = False self.passphrase = "" self.selected_disks = self.data.ignoredisk.onlyuse[:] + self._last_selected_disks = None self._back_clicked = False self.autopart_missing_passphrase = False
@@ -484,15 +486,19 @@ def _on_disk_focus_in(self, overview, event): self._cur_clicked_overview = overview
def refresh(self): - self.disks = getDisks(self.storage.devicetree) - self._back_clicked = False
+ self.disks = getDisks(self.storage.devicetree) + # synchronize our local data store with the global ksdata disk_names = [d.name for d in self.disks] - # don't put disks with hidden formats in selected_disks self.selected_disks = [d for d in self.data.ignoredisk.onlyuse - if d in disk_names] + if d in disk_names] + + # unhide previously hidden disks so that they don't look like being + # empty (because of all child devices hidden) + self._unhide_disks() + self.autopart = self.data.autopart.autopart self.autoPartType = self.data.autopart.type if self.autoPartType is None: @@ -762,14 +768,18 @@ def _remove_nonexistant_partitions(self): partition in self.storage.partitions: self.storage.recursiveRemove(partition)
- def _hide_unhide_disks(self): + def _hide_disks(self): for disk in self.disks: if disk.name not in self.selected_disks and \ disk in self.storage.devices: self.storage.devicetree.hide(disk) - elif disk.name in self.selected_disks and \ - disk not in self.storage.devices: - self.storage.devicetree.unhide(disk) + + def _unhide_disks(self): + if self._last_selected_disks: + for disk in self.disks: + if disk.name not in self.selected_disks and \ + disk.name not in self._last_selected_disks: + self.storage.devicetree.unhide(disk)
def _check_dasd_formats(self): rc = DASD_FORMAT_NO_CHANGE @@ -853,25 +863,42 @@ def on_back_clicked(self, button): else: self._back_clicked = True
+ # make sure the snapshot of unmodified on-disk-storage model is created + if not on_disk_storage.created: + on_disk_storage.create_snapshot(self.storage) + if self.autopart_missing_passphrase: self._setup_passphrase() NormalSpoke.on_back_clicked(self, button) return
- disks = [d for d in self.disks if d.name in self.selected_disks] # No disks selected? The user wants to back out of the storage spoke. - if not disks: + if not self.selected_disks: NormalSpoke.on_back_clicked(self, button) return
- # Remove all non-existing devices if autopart was active when we last - # refreshed. - if self._previous_autopart: - self._previous_autopart = False - self._remove_nonexistant_partitions() + disk_selection_changed = False + if self._last_selected_disks: + disk_selection_changed = (self._last_selected_disks != set(self.selected_disks))
- # hide/unhide disks as requested - self._hide_unhide_disks() + # remember the disk selection for future decisions + self._last_selected_disks = set(self.selected_disks) + + if disk_selection_changed: + # Changing disk selection is really, really complicated and has + # always been causing numerous hard bugs. Let's not play the hero + # game and just revert everything and start over again. + on_disk_storage.reset_to_snapshot(self.storage) + self.disks = getDisks(self.storage.devicetree) + else: + # Remove all non-existing devices if autopart was active when we last + # refreshed. + if self._previous_autopart: + self._previous_autopart = False + self._remove_nonexistant_partitions() + + # hide disks as requested + self._hide_disks()
# make sure no containers were split up by the user's disk selection self.clear_info() @@ -916,6 +943,7 @@ def on_back_clicked(self, button): # 3) we are just asked to do autopart => check free space and see if we need # user to do anything more self.autopart = not self._customPart.get_active() + disks = [d for d in self.disks if d.name in self.selected_disks] dialog = None if not self.autopart: self.skipTo = "CustomPartitioningSpoke"
A new version with a block of code that has been dropped (disk selection checking) in the port added back. Thanks jenkins+pylint for catching this by an unused import!
Added label: ACK.
Looks good to me.
Just adding the info that this fixes bug [rhbz#1250875] (https://bugzilla.redhat.com/show_bug.cgi?id=1250875)
Looks ok to me as well.
Pushed.
Closed.
Added label: f23-branch.
Pushing this to the f23-branch too as it fixes a reported F23 bug and it will hopefully prevent many F22 bugs from reappearing.
anaconda-patches@lists.fedorahosted.org
-
dashea