Bug 1279658 - TUI runs space check even if packaging spoke failed, but this is unsafe
Summary: TUI runs space check even if packaging spoke failed, but this is unsafe
Keywords:
Status: CLOSED DUPLICATE of bug 1279413
Alias: None
Product: Fedora
Classification: Fedora
Component: anaconda
Version: 23
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: ---
Assignee: Anaconda Maintenance Team
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-11-10 00:20 UTC by Adam Williamson
Modified: 2015-11-10 17:03 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2015-11-10 16:59:57 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 1277638 0 unspecified CLOSED anaconda fails to handle kickstart line like "repo --name=updates" 2021-02-22 00:41:40 UTC

Internal Links: 1277638

Description Adam Williamson 2015-11-10 00:20:36 UTC
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():
                print(self._checker.error_message)

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():

        try:
            payload.updateBaseRepo(fallback=fallback, checkmount=checkmount)
            payload.addDriverRepos()
        except (OSError, PayloadError) as e:
            log.error("PayloadError: %s", e)
            self._error = self.ERROR_SETUP
            self._setState(self.STATE_ERROR)
            payload.unsetup()
            return

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 16:59:57 UTC
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 17:03:27 UTC
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.