OpenStack Compute (nova)

Merge lp://qastaging/~vishvananda/nova/lp824433 into lp://qastaging/~hudson-openstack/nova/trunk

lp824433
Merge into trunk

Proposed by Vish Ishaya on 2011-09-09

Status:

Merged

Approved by:

Brian Lamar on 2011-09-19

Approved revision:

1530

Merged at revision:

1590

Proposed branch:

lp://qastaging/~vishvananda/nova/lp824433

Merge into:

lp://qastaging/~hudson-openstack/nova/trunk

Diff against target:

173 lines (+84/-44)

2 files modified

nova/tests/test_libvirt.py (+71/-33)
nova/virt/libvirt/connection.py (+13/-11)

To merge this branch:

bzr merge lp://qastaging/~vishvananda/nova/lp824433

Medium

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Brian Lamar (community)	2011-09-09	Approve on 2011-09-19
Mark McLoughlin (community)		Approve on 2011-09-19
Todd Willey (community)		Approve on 2011-09-16
Review via email: mp+74838@code.qastaging.launchpad.net

Description of the change

Fixes the handling of snapshotting in libvirt driver to actually use the proper image type instead of using raw for everything. Also cleans up an unneeded flag. Based on doude's initial work.

lp://qastaging/~vishvananda/nova/lp824433 updated on 2011-09-09

1529. By Vish Ishaya on 2011-09-09: remove extra line for pep8

Revision history for this message

Brian Lamar (blamar) wrote on 2011-09-12:

Not sure why, but this seems to fail for me:

======================================================================
FAIL: test_snapshot_in_raw_format (nova.tests.test_libvirt.LibvirtConnTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/brian.lamar/Projects/openstack/nova/lp824433/nova/tests/test_libvirt.py", line 357, in test_snapshot_in_raw_format
self.assertEquals(snapshot['disk_format'], FLAGS.snapshot_image_format)
AssertionError: 'qcow2' != None

----------------------------------------------------------------------
Ran 1730 tests in 478.018s

FAILED (SKIP=4, failures=1)

review: Needs Fixing

Revision history for this message

Vish Ishaya (vishvananda) wrote on 2011-09-14:

sigh. got bitten by libvirt tests not running on mac bug. Seems like we should use the skip decorator for those. Fixing.

Revision history for this message

Vish Ishaya (vishvananda) wrote on 2011-09-14:

fixed.

lp://qastaging/~vishvananda/nova/lp824433 updated on 2011-09-14

1530. By Vish Ishaya on 2011-09-14: fixed tests

Revision history for this message

Todd Willey (xtoddx) wrote on 2011-09-16:

lgtm

review: Approve

Revision history for this message

Mark McLoughlin (markmc) wrote on 2011-09-19:

lgtm too

Some cleanup ideas:

- I was pretty confused by this:

    + source_format = base.get('disk_format') or 'raw'
    + image_format = FLAGS.snapshot_image_format or source_format
    + if FLAGS.use_cow_images:
    + source_format = 'qcow2'
    + metadata['disk_format'] = image_format

would this be more clear?

    + image_format = source_format = base.get('disk_format') or 'raw'
    + if FLAGS.use_cow_images:
    + source_format = 'qcow2'
    + if FLAGS.snapshot_image_format:
    + image_format = FLAGS.snapshot_image_format
    + metadata['disk_format'] = image_format

- It's a pity to see the copy-and-paste in the tests, if it was re-factored to:

def do_test_snapshot(self, disk_format, use_cow_images, snapshot_image_format, result_format):
....

(i.e. also allow setting the source disk format and the use_cow_images flag too)

then we could have e.g.

    def test_snapshot(self):
        do_test_snapshot(self, None, False, None, 'raw')
        do_test_snapshot(self, None, False, 'raw', 'raw')
        do_test_snapshot(self, None, False, 'qcow2', 'qcow2')
        do_test_snapshot(self, None, True, None, 'qcow2')
        do_test_snapshot(self, 'raw', False, None, 'raw')
        do_test_snapshot(self, 'raw', True, None, 'qcow2')
        do_test_snapshot(self, 'qcow2', False, None, 'qcow2')
        do_test_snapshot(self, 'qcow2', True, None, 'qcow2')