We used the same trick on f22-branch and it resolved many issues there. Here it fixes at least two reported bugs and a number of unreported.
Vratislav Podzimek (3): Prevent any changes in the StorageSpoke if just going back Implement the class for storage snapshots Create and use snapshot of on-disk storage with no modifications
pyanaconda/storage_utils.py | 65 +++++++++++++++++++++++++++++ pyanaconda/ui/gui/spokes/storage.py | 82 +++++++++++++++++++++++++++++-------- 2 files changed, 129 insertions(+), 18 deletions(-)
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.
Related: rhbz#1160862 Related: rhbz#1187644
(port of the commit 0c9a8bc1192c320bc1461687e88f4275eff4b213 from f22-branch)
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/ui/gui/spokes/storage.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 64d1aed..eabc96d 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -716,6 +716,14 @@ class StorageSpoke(NormalSpoke, StorageChecker): # user might want to change settings presented in the dialogs shown from # within this method.
+ 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 + # Remove all non-existing devices if autopart was active when we last # refreshed. if self._previous_autopart: @@ -735,15 +743,6 @@ class StorageSpoke(NormalSpoke, StorageChecker): 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 - if arch.isS390(): # check for unformatted DASDs and launch dasdfmt if any discovered dasds = make_unformatted_dasd_list(self.selected_disks)
Related: rhbz#1160862 Related: rhbz#1187644
(cherry picked from commit b10e958a2d1a3a115a0de272c6ecb3175d3f7f4c) Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/storage_utils.py | 61 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)
diff --git a/pyanaconda/storage_utils.py b/pyanaconda/storage_utils.py index ada7692..a5a69c5 100644 --- a/pyanaconda/storage_utils.py +++ b/pyanaconda/storage_utils.py @@ -335,3 +335,64 @@ def verify_LUKS_devices_have_key(storage): not d.format.exists and \ not d.format.hasKey): yield LUKSDeviceWithoutKeyError(_("LUKS device %s has no encryption key") % (dev.name,)) + +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()
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.
Resolves: rhbz#1160862 Resolves: rhbz#1187644
(ported commit 0f267be6966374fd247d705bee003d1f7eca18fe from f22-branch)
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/storage_utils.py | 4 +++ pyanaconda/ui/gui/spokes/storage.py | 71 ++++++++++++++++++++++++++++++------- 2 files changed, 63 insertions(+), 12 deletions(-)
diff --git a/pyanaconda/storage_utils.py b/pyanaconda/storage_utils.py index a5a69c5..c1dbe52 100644 --- a/pyanaconda/storage_utils.py +++ b/pyanaconda/storage_utils.py @@ -396,3 +396,7 @@ class StorageSnapshot(object):
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 eabc96d..4c2bce5 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -53,6 +53,7 @@ from pyanaconda.ui.gui.spokes.lib.dasdfmt import DasdFormatDialog from pyanaconda.ui.categories.system import SystemCategory from pyanaconda.ui.gui.utils import escape_markup, gtk_action_nowait, ignoreEscape from pyanaconda.ui.helpers import StorageChecker +from pyanaconda.storage_utils import on_disk_storage
from pyanaconda.kickstart import doKickstartStorage, refreshAutoSwapSize, resetCustomStorageData from blivet import arch @@ -259,6 +260,7 @@ class StorageSpoke(NormalSpoke, StorageChecker):
self._last_clicked_overview = None self._cur_clicked_overview = None + self._last_selected_disks = None
self._grabObjects()
@@ -453,6 +455,11 @@ class StorageSpoke(NormalSpoke, StorageChecker): # 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] + + # 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: @@ -711,28 +718,65 @@ class StorageSpoke(NormalSpoke, StorageChecker):
return True
+ 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) + + + 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 _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) + 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.
- 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)) + # 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 - 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) + disk_selection_changed = False + if self._last_selected_disks: + disk_selection_changed = (self._last_selected_disks != set(self.selected_disks)) + + # remember the disk selection for future decisions + self._last_selected_disks = set(self.selected_disks) + + autopart = not self._customPart.get_active() + if disk_selection_changed or (not self._previous_autopart and autopart): + # 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. + # The same applies to change from custom part to autopart. + 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()
# hide/unhide disks as requested for disk in self.disks: @@ -767,6 +811,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): # back to the hub. return
+ 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)) + # 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.
On Fri, Jun 26, 2015 at 11:39:07AM +0200, Vratislav Podzimek wrote:
disks = [d for d in self.disks if d.name in self.selected_disks]
I think this ^^^^ could be a generator. :)
disks_size = sum((d.size for d in disks), Size(0))
Ack to all of these.
On Mon, 2015-06-29 at 16:48 -0700, Brian C. Lane wrote:
On Fri, Jun 26, 2015 at 11:39:07AM +0200, Vratislav Podzimek wrote:
disks = [d for d in self.disks if d.name in self.selected_disks]I think this ^^^^ could be a generator. :)
I'm not quite sure about this. 'disks' is used in other places below.
anaconda-patches@lists.fedorahosted.org
-
Brian C. Lane -
Vratislav Podzimek