These patches come from my attempts to fix bug 1085201. It turned out there are more issues that need fixing to make disk selection changes work, but these are steps forward. The rest depends on the changes dlehman is working on for cancelling actions in blivet.
Vratislav Podzimek (3): Do not directly remove devices from previous autopart Apply changes in disk selection even if not reclaiming space Short-circuit Storage spoke leaving when no changes were done
pyanaconda/ui/gui/spokes/storage.py | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)
These devices where created when creation actions were registered and should be removed when (if) those actions are cancelled (e.g. as part of hiding/unhiding devices).
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/ui/gui/spokes/storage.py | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index b951aa9..220bb14 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -718,15 +718,6 @@ class StorageSpoke(NormalSpoke, StorageChecker): # user might want to change settings presented in the dialogs shown from # within this method.
- # 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)
# hide/unhide disks as requested for disk in self.disks:
If we don't run any dialog and user didn't requested reclaiming space we should still apply disk selection in the Storage spoke.
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/ui/gui/spokes/storage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 220bb14..2a48fca 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -823,9 +823,10 @@ class StorageSpoke(NormalSpoke, StorageChecker): if not self._check_encrypted(): return
+ self.apply() + # 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.
On 09/11/2014 09:05 AM, Vratislav Podzimek wrote:
If we don't run any dialog and user didn't requested reclaiming space we should still apply disk selection in the Storage spoke.
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/ui/gui/spokes/storage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 220bb14..2a48fca 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -823,9 +823,10 @@ class StorageSpoke(NormalSpoke, StorageChecker): if not self._check_encrypted(): return
self.apply()
# 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.
This is likely to expose a pile of bugs related to how we handle custom storage and kickstart, some of which I started fixing last week. Since this set overlaps with what I'm working on I'll see how these fit with what I'm doing by applying them and seeing how much of my progress it undoes. I'll report back once I have more information. I like patches 2 and 3, but I'm unsure about patch 1 as far as handling multiple trips into the storage spoke when autopart is selected.
David
On Mon, 2014-09-15 at 13:05 -0500, David Lehman wrote:
On 09/11/2014 09:05 AM, Vratislav Podzimek wrote:
If we don't run any dialog and user didn't requested reclaiming space we should still apply disk selection in the Storage spoke.
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/ui/gui/spokes/storage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 220bb14..2a48fca 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -823,9 +823,10 @@ class StorageSpoke(NormalSpoke, StorageChecker): if not self._check_encrypted(): return
self.apply()
# 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.
This is likely to expose a pile of bugs related to how we handle custom storage and kickstart, some of which I started fixing last week. Since this set overlaps with what I'm working on I'll see how these fit with what I'm doing by applying them and seeing how much of my progress it undoes. I'll report back once I have more information. I like patches 2 and 3, but I'm unsure about patch 1 as far as handling multiple trips into the storage spoke when autopart is selected.
That's what I tested and tried to fix. As we discussed on IRC Anaconda should not directly remove devices outside of the "actions management" because those devices were created together with their CreateDevice actions and should be removed as part of their actions' cancellation.
If user changes nothing in the Storage spoke and hits "Done" we should consider that a request to just leave the spoke with no changes.
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/ui/gui/spokes/storage.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 2a48fca..ac90281 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -270,6 +270,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): target=self.run_dasdfmt))
self._previous_autopart = False + self._previous_selected = None + self._previous_encrypted = None + self._previous_reclaim = None
self._last_clicked_overview = None self._cur_clicked_overview = None @@ -310,6 +313,9 @@ class StorageSpoke(NormalSpoke, StorageChecker):
@gtk_action_nowait def execute(self): + if not self._any_change: + return + # Spawn storage execution as a separate thread so there's no big delay # going back from this spoke to the hub while StorageChecker.run runs. # Yes, this means there's a thread spawning another thread. Sorry. @@ -400,6 +406,13 @@ class StorageSpoke(NormalSpoke, StorageChecker): return filter(lambda child: isinstance(child, AnacondaWidgets.DiskOverview), self.specialized_disks_box.get_children())
+ @property + def _any_change(self): + return not(self._previous_autopart is self.autopart and + self._previous_encrypted is self.encrypted and + self._previous_reclaim is self._reclaim.get_active() and + set(self._previous_selected) == set(self.selected_disks)) + def _on_disk_clicked(self, overview, event): # This handler only runs for these two kinds of events, and only for # activate-type keys (space, enter) in the latter event's case. @@ -474,6 +487,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): self.passphrase = self.data.autopart.passphrase
self._previous_autopart = self.autopart + self._previous_encrypted = self.encrypted + self._previous_selected = self.selected_disks[:] + self._previous_reclaim = self._reclaim.get_active()
# First, remove all non-button children. for child in self.localOverviews + self.advancedOverviews: @@ -718,6 +734,11 @@ class StorageSpoke(NormalSpoke, StorageChecker): # user might want to change settings presented in the dialogs shown from # within this method.
+ # check if something has changed preventing issues with None == False + if not self._any_change: + # nothing changed, nothing to do + NormalSpoke.on_back_clicked(self, button) + return
# hide/unhide disks as requested for disk in self.disks:
On 09/11/2014 09:05 AM, Vratislav Podzimek wrote:
If user changes nothing in the Storage spoke and hits "Done" we should consider that a request to just leave the spoke with no changes.
One comment at the end...
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/ui/gui/spokes/storage.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 2a48fca..ac90281 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -270,6 +270,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): target=self.run_dasdfmt))
self._previous_autopart = False
self._previous_selected = None
self._previous_encrypted = None
self._previous_reclaim = None self._last_clicked_overview = None self._cur_clicked_overview = None
@@ -310,6 +313,9 @@ class StorageSpoke(NormalSpoke, StorageChecker):
@gtk_action_nowait def execute(self):
if not self._any_change:
return
# Spawn storage execution as a separate thread so there's no big delay # going back from this spoke to the hub while StorageChecker.run runs. # Yes, this means there's a thread spawning another thread. Sorry.
@@ -400,6 +406,13 @@ class StorageSpoke(NormalSpoke, StorageChecker): return filter(lambda child: isinstance(child, AnacondaWidgets.DiskOverview), self.specialized_disks_box.get_children())
- @property
- def _any_change(self):
return not(self._previous_autopart is self.autopart and
self._previous_encrypted is self.encrypted and
self._previous_reclaim is self._reclaim.get_active() and
set(self._previous_selected) == set(self.selected_disks))
def _on_disk_clicked(self, overview, event): # This handler only runs for these two kinds of events, and only for # activate-type keys (space, enter) in the latter event's case.
@@ -474,6 +487,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): self.passphrase = self.data.autopart.passphrase
self._previous_autopart = self.autopart
self._previous_encrypted = self.encrypted
self._previous_selected = self.selected_disks[:]
self._previous_reclaim = self._reclaim.get_active() # First, remove all non-button children. for child in self.localOverviews + self.advancedOverviews:
@@ -718,6 +734,11 @@ class StorageSpoke(NormalSpoke, StorageChecker): # user might want to change settings presented in the dialogs shown from # within this method.
# check if something has changed preventing issues with None == False
if not self._any_change:
# nothing changed, nothing to do
NormalSpoke.on_back_clicked(self, button)
return
This doesn't take into account the possibility that the user wants to redo custom storage without changing any of the settings on the main storage spoke. Otherwise I like this one.
David
# hide/unhide disks as requested for disk in self.disks:
On Mon, 2014-09-15 at 20:47 -0500, David Lehman wrote:
On 09/11/2014 09:05 AM, Vratislav Podzimek wrote:
If user changes nothing in the Storage spoke and hits "Done" we should consider that a request to just leave the spoke with no changes.
One comment at the end...
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/ui/gui/spokes/storage.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index 2a48fca..ac90281 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -270,6 +270,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): target=self.run_dasdfmt))
self._previous_autopart = False
self._previous_selected = None
self._previous_encrypted = None
self._previous_reclaim = None self._last_clicked_overview = None self._cur_clicked_overview = None
@@ -310,6 +313,9 @@ class StorageSpoke(NormalSpoke, StorageChecker):
@gtk_action_nowait def execute(self):
if not self._any_change:
return
# Spawn storage execution as a separate thread so there's no big delay # going back from this spoke to the hub while StorageChecker.run runs. # Yes, this means there's a thread spawning another thread. Sorry.
@@ -400,6 +406,13 @@ class StorageSpoke(NormalSpoke, StorageChecker): return filter(lambda child: isinstance(child, AnacondaWidgets.DiskOverview), self.specialized_disks_box.get_children())
- @property
- def _any_change(self):
return not(self._previous_autopart is self.autopart and
self._previous_encrypted is self.encrypted and
self._previous_reclaim is self._reclaim.get_active() and
set(self._previous_selected) == set(self.selected_disks))
def _on_disk_clicked(self, overview, event): # This handler only runs for these two kinds of events, and only for # activate-type keys (space, enter) in the latter event's case.
@@ -474,6 +487,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): self.passphrase = self.data.autopart.passphrase
self._previous_autopart = self.autopart
self._previous_encrypted = self.encrypted
self._previous_selected = self.selected_disks[:]
self._previous_reclaim = self._reclaim.get_active() # First, remove all non-button children. for child in self.localOverviews + self.advancedOverviews:
@@ -718,6 +734,11 @@ class StorageSpoke(NormalSpoke, StorageChecker): # user might want to change settings presented in the dialogs shown from # within this method.
# check if something has changed preventing issues with None == False
if not self._any_change:
# nothing changed, nothing to do
NormalSpoke.on_back_clicked(self, button)
return
This doesn't take into account the possibility that the user wants to redo custom storage without changing any of the settings on the main storage spoke. Otherwise I like this one.
Good catch, I forgot to add the 'and not self._customPart.get_active()' part for this condition. Fixing locally.
This doesn't take into account the possibility that the user wants to redo custom storage without changing any of the settings on the main storage spoke. Otherwise I like this one.
And in fact, there is a comment to this effect in storage.py:
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.
- Chris
On Tue, 2014-09-16 at 11:48 -0400, Chris Lumens wrote:
This doesn't take into account the possibility that the user wants to redo custom storage without changing any of the settings on the main storage spoke. Otherwise I like this one.
And in fact, there is a comment to this effect in storage.py:
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.
The only use case that comes to my mind is if user wants to reclaim some more space, but okay, dropping this patch.
On 09/16/2014 01:18 PM, Vratislav Podzimek wrote:
On Tue, 2014-09-16 at 11:48 -0400, Chris Lumens wrote:
This doesn't take into account the possibility that the user wants to redo custom storage without changing any of the settings on the main storage spoke. Otherwise I like this one.
And in fact, there is a comment to this effect in storage.py:
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.
The only use case that comes to my mind is if user wants to reclaim some more space, but okay, dropping this patch.
Maybe you can just add 'or self._reclaim.get_active()' to the same if statement you added 'or self._customPart.get_active()' to instead of dropping the patch completely.
David
On Tue, 2014-09-16 at 14:52 -0500, David Lehman wrote:
On 09/16/2014 01:18 PM, Vratislav Podzimek wrote:
On Tue, 2014-09-16 at 11:48 -0400, Chris Lumens wrote:
This doesn't take into account the possibility that the user wants to redo custom storage without changing any of the settings on the main storage spoke. Otherwise I like this one.
And in fact, there is a comment to this effect in storage.py:
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.
The only use case that comes to my mind is if user wants to reclaim some more space, but okay, dropping this patch.
Maybe you can just add 'or self._reclaim.get_active()' to the same if statement you added 'or self._customPart.get_active()' to instead of dropping the patch completely.
Oh, you're right, that would still help at least a bit. It would at least short-circuit the easiest case where user enters the spoke again with the same disk selection, no reclaim requests and autopart selected. Tweaking, keeping and pushing. Thanks, guys!
Vratislav Podzimek (2): Properly revert changes when cancelling Partition destruction Inform parents when their child is added back to the devicetree
blivet/deviceaction.py | 15 +++++++++++++++ blivet/devicetree.py | 4 +++- 2 files changed, 18 insertions(+), 1 deletion(-)
When a partition is scheduled to be destroyed, the action removes it's device from the devicetree and from its parent's format. While the former change is reverted in the devicetree's action cancelling, the latter one is ignored and needs this fix.
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- blivet/deviceaction.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py index db4c7a7..fb11f8e 100644 --- a/blivet/deviceaction.py +++ b/blivet/deviceaction.py @@ -336,6 +336,14 @@ class ActionDestroyDevice(DeviceAction): def __init__(self, device): # XXX should we insist that device.fs be None? DeviceAction.__init__(self, device) + self._origPartition = None + self._origDiskFormat = None + + if isinstance(device, PartitionDevice) and device.disk is not None: + # removing partition, save it and its format in case we have to + # revert changes when canceling this action + self._origPartition = device.partedPartition + self._origDiskFormat = device.disk.format
def execute(self): super(ActionDestroyDevice, self).execute() @@ -349,6 +357,13 @@ class ActionDestroyDevice(DeviceAction): except Exception: # pylint: disable=broad-except log_exception_info(fmt_str="failed to remove info for device %s from libparted cache", fmt_args=[self.device])
+ def cancel(self): + super(ActionDestroyDevice, self).cancel() + + # canceling partition removal, add the partition back to its disk format + if self._origDiskFormat and self._origPartition: + self._origDiskFormat.addPartition(self._origPartition) + def requires(self, action): """ Return True if self requires action.
On 09/11/2014 09:09 AM, Vratislav Podzimek wrote:
When a partition is scheduled to be destroyed, the action removes it's device from the devicetree and from its parent's format. While the former change is reverted in the devicetree's action cancelling, the latter one is ignored and needs this fix.
I think I fixed the same problem here with my patch adding add/remove hooks to device classes. I also changed ActionDestroyFormat and ActionCreateFormat to save a deep copy of the format instead of just saving a reference to the same instance. I'm not totally certain, but I think my patches will provide a more general solution to this problem.
David
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/deviceaction.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py index db4c7a7..fb11f8e 100644 --- a/blivet/deviceaction.py +++ b/blivet/deviceaction.py @@ -336,6 +336,14 @@ class ActionDestroyDevice(DeviceAction): def __init__(self, device): # XXX should we insist that device.fs be None? DeviceAction.__init__(self, device)
self._origPartition = None
self._origDiskFormat = None
if isinstance(device, PartitionDevice) and device.disk is not None:
# removing partition, save it and its format in case we have to
# revert changes when canceling this action
self._origPartition = device.partedPartition
self._origDiskFormat = device.disk.format def execute(self): super(ActionDestroyDevice, self).execute()
@@ -349,6 +357,13 @@ class ActionDestroyDevice(DeviceAction): except Exception: # pylint: disable=broad-except log_exception_info(fmt_str="failed to remove info for device %s from libparted cache", fmt_args=[self.device])
- def cancel(self):
super(ActionDestroyDevice, self).cancel()
# canceling partition removal, add the partition back to its disk format
if self._origDiskFormat and self._origPartition:
self._origDiskFormat.addPartition(self._origPartition)
def requires(self, action): """ Return True if self requires action.
On Mon, 2014-09-15 at 13:02 -0500, David Lehman wrote:
On 09/11/2014 09:09 AM, Vratislav Podzimek wrote:
When a partition is scheduled to be destroyed, the action removes it's device from the devicetree and from its parent's format. While the former change is reverted in the devicetree's action cancelling, the latter one is ignored and needs this fix.
I think I fixed the same problem here with my patch adding add/remove hooks to device classes. I also changed ActionDestroyFormat and ActionCreateFormat to save a deep copy of the format instead of just saving a reference to the same instance. I'm not totally certain, but I think my patches will provide a more general solution to this problem.
Great, thanks! Can you please add the bugs referenced by these two patches to your patches as Resolves? I'm willing to take care of any additional fixes that could be needed on top of them to fix the bugs.
Otherwise their self.kids counter contains and invalid value.
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- blivet/devicetree.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index c4cb1b7..021ad45 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -520,8 +520,10 @@ class DeviceTree(object): # remove the device from the tree self._removeDevice(action.device) elif action.isDestroy and action.isDevice: - # add the device back into the tree + # add the device back into the tree and inform parents about it self._addDevice(action.device) + for parent in action.device.parents: + parent.addChild()
action.cancel() self._actions.remove(action)
On 09/11/2014 09:09 AM, Vratislav Podzimek wrote:
Otherwise their self.kids counter contains and invalid value.
Related: rhbz#1085201 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/devicetree.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index c4cb1b7..021ad45 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -520,8 +520,10 @@ class DeviceTree(object): # remove the device from the tree self._removeDevice(action.device) elif action.isDestroy and action.isDevice:
# add the device back into the tree
# add the device back into the tree and inform parents about it self._addDevice(action.device)
for parent in action.device.parents:
parent.addChild() action.cancel() self._actions.remove(action)
FWIW this is also covered by the set I'm testing now.
David
On Thu, 2014-09-11 at 16:09 +0200, Vratislav Podzimek wrote:
Vratislav Podzimek (2): Properly revert changes when cancelling Partition destruction Inform parents when their child is added back to the devicetree
blivet/deviceaction.py | 15 +++++++++++++++ blivet/devicetree.py | 4 +++- 2 files changed, 18 insertions(+), 1 deletion(-)
Turns out these two also fix https://bugzilla.redhat.com/show_bug.cgi?id=1090657
anaconda-patches@lists.fedorahosted.org