unmount: disabled causes installation failure to exit silently

Bug #1764210 reported by Michael Hudson-Doyle on 2018-04-16
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
curtin
High
Unassigned
curtin (Ubuntu)
Medium
Unassigned

Bug Description

from cmd_install:

    finally:
        log_target_path = instcfg.get('save_install_log', SAVE_INSTALL_LOG)
        if log_target_path:
            copy_install_log(logfile, workingd.target, log_target_path)

        if instcfg.get('unmount', "") == "disabled":
            LOG.info('Skipping unmount: config disabled target unmounting')
            return

        # unmount everything (including iscsi disks)
        util.do_umount(workingd.target, recursive=True)

that 'return' means the function returns normally even in an error case. I think this needs to be changed into an if/else.

Related branches

Changed in curtin:
status: New → In Progress
Ryan Harper (raharper) wrote :

My reading of the python3 docs suggests that this analysis is correct. If we issue a return, then the saved exception is discarded.

If finally is present, it specifies a ‘cleanup’ handler. The try clause is executed, including any except and else clauses. If an exception occurs in any of the clauses and is not handled, the exception is temporarily saved. The finally clause is executed. If there is a saved exception it is re-raised at the end of the finally clause. If the finally clause raises another exception, the saved exception is set as the context of the new exception. If the finally clause executes a return or break statement, the saved exception is discarded:

>>>
>>> def f():
... try:
... 1/0
... finally:
... return 42
...
>>> f()
42

https://docs.python.org/3/reference/compound_stmts.html#try

Ryan Harper (raharper) on 2018-04-16
Changed in curtin:
importance: Undecided → High
Scott Moser (smoser) wrote :

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/curtin/commit/?id=1d4d17ce

Changed in curtin:
status: In Progress → Fix Committed
Scott Moser (smoser) on 2018-04-18
Changed in curtin (Ubuntu):
status: New → Confirmed
importance: Undecided → Medium
status: Confirmed → In Progress
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package curtin - 18.1-5-g572ae5d6-0ubuntu1

---------------
curtin (18.1-5-g572ae5d6-0ubuntu1) bionic; urgency=medium

  * New upstream snapshot.
    - clear-holders: fix lvm name use when shutting down (LP: #1764602)
    - install: prevent unmount: disabled from swallowing installation failures
      (LP: #1764210)
    - vmtest: bionic images no longer use the vlan package
    - pycodestyle: Fix invalid escape sequences in string literals.

 -- Ryan Harper <email address hidden> Wed, 18 Apr 2018 10:15:46 -0500

Changed in curtin (Ubuntu):
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers