Red Hat Bugzilla – Bug 1279658
TUI runs space check even if packaging spoke failed, but this is unsafe
Last modified: 2015-11-10 12:03:27 EST
This is a follow-up to https://bugzilla.redhat.com/show_bug.cgi?id=1277638 . I looked into why the TUI installer crashed when that bug happened, while the GUI installer didn't. I can now explain why, but I'm not 100% sure of what the best fix is, so I thought I'd file a bug for discussion.
We can start here, in the SummaryHub class's prompt() method, in pyanaconda/ui/tui/hubs/summary.py :
# do a bit of final sanity checking, make sure pkg selection
# size < available fs space
if self._checker and not self._checker.check():
on a typical install, self._checker is FileSystemSpaceChecker, and when we run its check() method, one of the things it does is this:
needed = self.payload.spaceRequired
in the #1277638 case, our payload is a DNFPayload, and its spaceRequired does this:
for (key, val) in self.storage.mountpoints.items():
[DO SOME STUFF]
And what actually happened in the TUI case is it blew up there, because that 'self.storage.mountpoints.items()' call failed, because self.storage was None.
The pertinent question is, *why* is the DNFPayload's 'storage' attribute None at that point? And the answer's fairly simple - it's in pyanaconda/packaging/__init__.py, in the PayloadManager class's _runThread() method. Payload classes have their storage attribute initialized to None, and they have a setup() method which configures it (among other things). They also have an unsetup() method which sets it back to None (again, among other things). _runThread() calls payload.setup():
but later on, if it encounters an error, it calls payload.unsetup():
except (OSError, PayloadError) as e:
log.error("PayloadError: %s", e)
self._error = self.ERROR_SETUP
and that's the path we're hitting here. Because of the actual bug in #1277638 , we hit a PayloadError at that point, and _runThread() calls payload.unsetup() , and at that point, the payload's storage property is set back to None.
However, that space check code in the TUI SummaryHub prompt() method gets run regardless of whether the packaging spoke hit an error during setup or not - so we wind up hitting the payload's spaceRequired() property with storage set to None, and that blows up.
There's one obvious fix here, which I tested and it works: just make the whole 'for (key, val) in self.storage.mountpoints.items():' block in spaceRequired conditional, add a 'if self.storage:' above it. It doesn't seem to be strictly required, and the method should still return something sane, I think.
However, I'm not sure if that's the correct/best fix, or if it might be better to not do the space check in prompt() if the packaging or storage spokes are incomplete - that seems like an equally plausible thing to do, what's the point of running a space check if packaging and storage aren't actually set up?
The GUI does this differently - I don't know exactly how/when the space check is run on the GUI path, but it definitely avoids this pitfall. On the GUI path, when you hit #1277638 , the installer correctly just sits at the hub with the packaging spoke in an error condition, and lets you try to fix it.
Duping this to 1279413 since that one's got traceback and logs and such
*** This bug has been marked as a duplicate of bug 1279413 ***
but...but this one has a beautiful, hand-crafted adamw explanation! *single tear*