UTAH

Merge lp://staging/~nuclearbob/utah/bug1178241 into lp://staging/utah

bug1178241
Merge into dev

Proposed by Max Brustkern on 2013-05-09

Status:

Merged

Approved by:

Javier Collado on 2013-05-09

Approved revision:

900

Merged at revision:

901

Proposed branch:

lp://staging/~nuclearbob/utah/bug1178241

Merge into:

lp://staging/utah

Diff against target:

142 lines (+94/-14)

3 files modified

debian/changelog (+5/-1)
tests/test_vm.py (+67/-0)
utah/provisioning/vm.py (+22/-13)

To merge this branch:

bzr merge lp://staging/~nuclearbob/utah/bug1178241

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Javier Collado (community)		2013-05-09	Approve on 2013-05-09
Review via email: mp+163180@code.staging.launchpad.net

Description of the change

This branch cleans up the vm using a finally clause if an exception occurs during installation. It also adds destroy to the regular cleanup, since running undefine without destroy first seems to be unreliable. I test it using this as a config file:
{
    "install_steps": [
        {
            "booted": true,
            "message": "This message will always time out",
            "pattern": "antidisestablishmentarianism should never match",
            "timeout": 1
        }
    ]
}
I'm working on an actual test case, which I'll push once it's ready, but I'd like to get the bugfix reviewed while I'm working on that.

Revision history for this message

Andy Doan (doanac) wrote on 2013-05-09:

so far so good

lp://staging/~nuclearbob/utah/bug1178241 updated on 2013-05-09

898. By Max Brustkern on 2013-05-09: Made cleanup more robust in case the vm is already destroyed
899. By Max Brustkern on 2013-05-09: Added test for vm removal after installation failure

Revision history for this message

Max Brustkern (nuclearbob) wrote on 2013-05-09:

I've updated the initial change to make it more robust.

I've also added a test case, but there are currently two problems with it. One of them is really a VM problem: if we create a vm, we end up with an install.log file owned by root. Without escalating privileges, there's no way to change that ownership, so subsequent runs with the same name will fail. This doesn't show up right now since we're always incrementing vm names. I could use a random or incremented name to work around this, but it might be worth actually fixing.

Additionally, I don't know how to throw good messages with assertRaises or assertRaisesRegexp. We need to assert that the machine doesn't exist already before the test starts and after it finishes, but right now if that fails, we just see that libvirtError wasn't raised.

lp://staging/~nuclearbob/utah/bug1178241 updated on 2013-05-09