Bug 1279658 - TUI runs space check even if packaging spoke failed, but this is unsafe
TUI runs space check even if packaging spoke failed, but this is unsafe
Status: CLOSED DUPLICATE of bug 1279413
Product: Fedora
Classification: Fedora
Component: anaconda (Show other bugs)
Unspecified Unspecified
unspecified Severity high
: ---
: ---
Assigned To: Anaconda Maintenance Team
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2015-11-09 19:20 EST by Adam Williamson
Modified: 2015-11-10 12:03 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2015-11-10 11:59:57 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

  None (edit)
Description Adam Williamson 2015-11-09 19:20:36 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 flags.automatedInstall:
            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():

        payload.setup(storage, instClass)

but later on, if it encounters an error, it calls payload.unsetup():

            payload.updateBaseRepo(fallback=fallback, checkmount=checkmount)
        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.
Comment 1 David Shea 2015-11-10 11:59:57 EST
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 ***
Comment 2 Adam Williamson 2015-11-10 12:03:27 EST
but...but this one has a beautiful, hand-crafted adamw explanation! *single tear*

Note You need to log in before you can comment on or make changes to this bug.