New: [rhinstaller/anaconda/pulls/74 f22-branch] Fix issues with disk selection changes (#1166598)
**Please ignore the first two commits, those have already been ACKed on the mailing list and just were not pushed to f22-branch yet (I'm waiting for Beta to be declared GO)**
The first of the latter four commits make the ``on_back_clicked`` method of the *StorageSpoke* less complicated because it has been bloating with if's and duplicated chunks of code for quite some time into the shape that was really hard to understand not even modify. To review this commit I highly recommend to check the result instead of/before going through the changes. The second commit is just a minor change simplifying the logic a bit.
The last two commits implement and use a snapshot of on-disk storage as we get it from scanning disks before we do any modifications so that we can revert everything to it in case user changes their mind about disk selection. The previous "limbo" with hiding/unhiding disks and relying on the result of cancelling actions on the way being the original state of everything was brave, but not working. However, it still is the right thing to do and that's why this PR is only for the f22-branch with quite a bit of hope that action cancelling will have been fixed in blivet by the time of F23.
From: Vratislav Podzimek vpodzime@redhat.com
It's not supposed to be read from the outside.
Related: rhbz#1210003 --- pyanaconda/ui/gui/spokes/storage.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 372c2e6..a84e13e 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -250,7 +250,7 @@ def __init__(self, *args, **kwargs): self.encrypted = False self.passphrase = "" self.selected_disks = self.data.ignoredisk.onlyuse[:] - self.back_clicked = False + self._back_clicked = False
# This list contains all possible disks that can be included in the install. # All types of advanced disks should be set up for us ahead of time, so @@ -462,7 +462,7 @@ def _on_disk_focus_in(self, overview, event): def refresh(self): self.disks = getDisks(self.storage.devicetree)
- self.back_clicked = False + self._back_clicked = False
# synchronize our local data store with the global ksdata disk_names = [d.name for d in self.disks] @@ -739,10 +739,10 @@ def on_back_clicked(self, button):
# Do not enter this method multiple times if user clicking multiple times # on back button - if self.back_clicked: + if self._back_clicked: return else: - self.back_clicked = True + self._back_clicked = True
# Remove all non-existing devices if autopart was active when we last # refreshed.
From: Vratislav Podzimek vpodzime@redhat.com
Otherwise later click on the Done button would look like if the user clicked the Done button twice in a row without our handler being finished and would thus be ignored. --- pyanaconda/ui/gui/spokes/storage.py | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index a84e13e..3cba326 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -794,6 +794,7 @@ def on_back_clicked(self, button): # 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
# Figure out if the existing disk labels will work on this platform @@ -855,6 +856,7 @@ def on_back_clicked(self, button):
# 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. @@ -863,10 +865,12 @@ def on_back_clicked(self, button): 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. + self._back_clicked = False return elif rc == RESPONSE_MODIFY_SW: # The "Fedora software selection" link was clicked on one of the @@ -884,6 +888,7 @@ def on_back_clicked(self, button): 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
# And then go to the custom partitioning spoke if they chose to @@ -900,6 +905,7 @@ def on_back_clicked(self, button): 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:
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). --- pyanaconda/ui/gui/spokes/lib/resize.glade | 2 +- pyanaconda/ui/gui/spokes/storage.py | 267 +++++++++++++++--------------- 2 files changed, 136 insertions(+), 133 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 3cba326..7f12235 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -81,9 +81,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" @@ -712,12 +716,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: @@ -732,29 +731,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. - - # 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) + def _remove_unexisting_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)
- # 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: @@ -763,40 +747,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 = 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 = 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 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. @@ -814,6 +778,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)) @@ -828,98 +793,136 @@ 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 + + def _run_dialogs(self, disks, start_with): + rc = self.run_lightbox_dialog(start_with) + if rc == RESPONSE_RECLAIM: + # we need to run another dialog + + # 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
- self.encrypted = self._encrypted.get_active() + # Remove all non-existing devices if autopart was active when we last + # refreshed. + if self._previous_autopart: + self._previous_autopart = False + self._remove_unexisting_partitions() + + # hide/unhide disks as requested + self._hide_unhide_disks()
- if self._customPart.get_active(): - self.autopart = False - self.skipTo = "CustomPartitioningSpoke" + 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 + + 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: - 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. + # User either hit cancel on the dialog or closed it via escape, + # there was no formatting done. + self._back_clicked = False + return + + # 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 - 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
+ # 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() - if not self._show_resize_dialog(disks): - # User pressed cancel on the reclaim dialog, so don't leave - # the storage spoke. + 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 - - # 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" + 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: - 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. - self._back_clicked = False - return + # 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.
In reply to line 734 of pyanaconda/ui/gui/spokes/storage.py:
_remove_nonexistant_partitions sounds better.
In reply to line 734 of pyanaconda/ui/gui/spokes/storage.py:
changed in my local copy
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. --- 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 7f12235..d71ccce 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -829,6 +829,12 @@ def on_back_clicked(self, button): else: self._back_clicked = True
+ 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: @@ -838,12 +844,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 - if arch.isS390(): # check for unformatted DASDs and launch dasdfmt if any discovered rc = self._check_dasd_formats()
From: Vratislav Podzimek vpodzime@redhat.com
Related: rhbz#1166598 --- pyanaconda/storage_utils.py | 68 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)
diff --git a/pyanaconda/storage_utils.py b/pyanaconda/storage_utils.py index d0906d3..d66ab0f 100644 --- a/pyanaconda/storage_utils.py +++ b/pyanaconda/storage_utils.py @@ -380,3 +380,71 @@ 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() +
I think you should drop the extra whitespace after the docstring.
I like to separate the function/method header from its code, but I don't insist on that. :) Let's see what others' opinions on this are.
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. --- 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 d66ab0f..6ea7677 100644 --- a/pyanaconda/storage_utils.py +++ b/pyanaconda/storage_utils.py @@ -448,3 +448,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 d71ccce..0807e5b 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -68,6 +68,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 @@ -254,6 +255,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
# This list contains all possible disks that can be included in the install. @@ -464,15 +466,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: @@ -738,14 +744,18 @@ def _remove_unexisting_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 @@ -829,20 +839,37 @@ def on_back_clicked(self, button): else: self._back_clicked = True
- disks = [d for d in self.disks if d.name in self.selected_disks] + # 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) + # 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_unexisting_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_unexisting_partitions() + + # hide disks as requested + self._hide_disks()
if arch.isS390(): # check for unformatted DASDs and launch dasdfmt if any discovered @@ -877,6 +904,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"
In reply to line 453 of pyanaconda/storage_utils.py:
I think this should be called explicitly someplace, doing this at the module level won't do much anyway since nothing was passed to it.
In reply to line 453 of pyanaconda/storage_utils.py:
So you think this should be changed to ``on_disk_storage = None`` with ``StorageSnapshot(self.storage)`` used instead of ``on_disk_storage.create_snapshot(self.storage)`` below, right? I like it both the same so I can definitely change it that way.
In reply to line 453 of pyanaconda/storage_utils.py:
Well, I'd rather there was no global at all, but that is an improvement.
In reply to line 453 of pyanaconda/storage_utils.py:
In future, I'd like to move this class to *blivet* and extend the ``Blivet`` class to allow doing snapshots, but I'd like to test the approach in F22 anaconda first. In blivet, it would be a kind of commitment to a potentially problematic API/functionality.
In reply to line 453 of pyanaconda/storage_utils.py:
Now that I think about it, I'm afraid the change would break things once we wanted to use the on_disk_storage from multiple places -- we need to have the object created on import (just like e.g. threadMgr). If we want to have the snapshot available only for the ``StorageSpoke`` we can create it there instead.
Added label: f22-branch.
Some more info -- the original intention was to use the new snapshot also for custom partitioning and it's 'Reset' button instead of it having its own copy of storage as ``self._storage_playground`` , but that would require making the ``StorageSnapshot`` class much more complicated and would result in potential issues with custom partitioning doing changes to storage representation other spokes also have work with. A copy of storage on a common setup (a VM with 2 disks and LVM on top of it) takes ~5 MiB RAM which is not worth doing dangerous changes, I think. Machines with more complicated storage are going to have more RAM so we don't have to worry about this, I think.
**Please ignore the first two commits, those have already been ACKed on the mailing list and just were not pushed to f22-branch yet (I'm waiting for Beta to be declared GO)**
Remember to also grab b2fabda9 from master when you push the first two to f22-branch. There was one reference that got missed.
- Chris
On Thu, 2015-04-16 at 09:20 -0400, Chris Lumens wrote:
**Please ignore the first two commits, those have already been ACKed on the mailing list and just were not pushed to f22-branch yet (I'm waiting for Beta to be declared GO)**
Remember to also grab b2fabda9 from master when you push the first two to f22-branch. There was one reference that got missed.
Thanks for the heads up! Turns out that that appearance of self.back_clicked doesn't exist on f22-branch though. :)
Am I correct that the premise for this is as follows?
1. hiding disks is pretty easy/reliable 2. un-hiding disks is problematic 3. therefore, we use a work flow that does not involve un-hiding disks
Am I correct that the premise for this is as follows?
hiding disks is pretty easy/reliable un-hiding disks is problematic therefore, we use a work flow that does not involve un-hiding disks
Exactly. We still do disk un-hide, but only to get correct free space information on spoke revisit which is not problematic, because free space is reported correctly and we don't use such un-hidden disks futher -- we revert to snapshot and start over disks as they were in the very beginning.
I realized I forgot to add "THANKS to @vojtechtrefny for the help with last commit!" to the commit message, so I'm adding it here and I'll update the commit message when it gets applied to master.
This all looks pretty good to me, other than those minor comments. I'd like to give it a try though, which I'll do tomorrow.
Added label: ACK.
Looks okay to me, too.
Pushed to f22-branch.
Closed.
anaconda-patches@lists.fedorahosted.org
-
Chris Lumens -
dashea -
Vratislav Podzimek