Merge lp:~nttdata/nova/850602 into lp:~hudson-openstack/nova/trunk

[Kei Masumoto]

block migration needs to copy backing_file.
This branch enables existing nova handling backing_file.
Changes has been done to 2 methods in nova.virt.libvirt.connection.py.
 - pre_block_migration
 - get_instance_disk_info

Unit-test is also changed following this changes.

[Jason Kölker (jason-koelker)]

Functionality looks good. Just a few fixes in the tests:

15 + #network_info = _fake_network_info(self.stubs, 1)
Instead of leaving the other network_info lines comented out, they should be deleted.

56 + open('/tmp/aaa', 'w+').write(str(info))
Do you mean to leave this file around?

Other than that, looks good.

review: Needs Fixing

[Jason Kölker (jason-koelker)]

There are also some pep8 issues that should be fixed before it hits trunk:

$ ./run_tests.sh -N -x -p
Running pep8 ...
nova/tests/test_libvirt.py:586:63: W291 trailing whitespace
        # as _fake_network_info calls utils.import_class() and
                                                              ^
    JCR: Trailing whitespace is superfluous.
    FBM: Except when it occurs as part of a blank line (i.e. the line is
         nothing but whitespace). According to Python docs[1] a line with only
         whitespace is considered a blank line, and is to be ignored. However,
         matching a blank line to its indentation level avoids mistakenly
         terminating a multi-line statement (e.g. class declaration) when
         pasting code into the standard Python interpreter.

         [1] http://docs.python.org/reference/lexical_analysis.html#blank-lines

    The warning returned varies on whether the line itself is blank, for easier
    filtering for those who want to indent their blank lines.

    Okay: spam(1)
    W291: spam(1)\s
    W293: class Foo(object):\n \n bang = 12
nova/tests/test_libvirt.py:771:63: W291 trailing whitespace
        # as _fake_network_info calls utils.import_class() and
                                                              ^
    JCR: Trailing whitespace is superfluous.
    FBM: Except when it occurs as part of a blank line (i.e. the line is
         nothing but whitespace). According to Python docs[1] a line with only
         whitespace is considered a blank line, and is to be ignored. However,
         matching a blank line to its indentation level avoids mistakenly
         terminating a multi-line statement (e.g. class declaration) when
         pasting code into the standard Python interpreter.

         [1] http://docs.python.org/reference/lexical_analysis.html#blank-lines

    The warning returned varies on whether the line itself is blank, for easier
    filtering for those who want to indent their blank lines.

    Okay: spam(1)
    W291: spam(1)\s
    W293: class Foo(object):\n \n bang = 12
nova/virt/libvirt/connection.py:1748:1: W293 blank line contains whitespace

^
    JCR: Trailing whitespace is superfluous.
    FBM: Except when it occurs as part of a blank line (i.e. the line is
         nothing but whitespace). According to Python docs[1] a line with only
         whitespace is considered a blank line, and is to be ignored. However,
         matching a blank line to its indentation level avoids mistakenly
         terminating a multi-line statement (e.g. class declaration) when
         pasting code into the standard Python interpreter.

         [1] http://docs.python.org/reference/lexical_analysis.html#blank-lines

    The warning returned varies on whether the line itself is blank, for easier
    filtering for those who want to indent their blank lines.

    Okay: spam(1)
    W291: spam(1)\s
    W293: class Foo(object):\n \n bang = 12

[Brian Waldon (bcwaldon)]

15, 85: I would prefer you just delete these lines rather than leave them commented out

115: This will definitely cause someone a headache down the road. Can you check local_gb instead of the name?

review: Needs Fixing

[Kei Masumoto (masumotok)]

Hello Jason and Brian,
Thanks for review this path, and sorry late reply.
I fixed based on your comment. can you check this?

[Brian Waldon (bcwaldon)]

Thanks, Kei. Looks good!

review: Approve

[Jason Kölker (jason-koelker)]

Is Bueno!

review: Approve

[Rafi Khardalian (rkhardalian)]

Prior to having this patch I was having my root disks corrupted with block migrations using the latest Diablo release packages. I just applied the patch to my compute nodes and re-tested and it resolved the issue.

[Brian Waldon (bcwaldon)]

Kei: can you migrate this MP over to review.openstack.org?

[lp:~nttdata/nova/850602]

1575. By Kei Masumoto on 2011-09-28

delete unnecessary os.remove(disk) in pre_block_migration. block_migration fails if disk is removed.

[Kei Masumoto (masumotok)]

Brian, thanks for notification! I migrated this MP, but unfortunately, "approbe" is not migrated. So could you check it again?

<https://review.openstack.org/#change,716>

Kei
-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Brian Waldon
Sent: Tuesday, September 27, 2011 2:18 AM
To: <email address hidden>
Subject: Re: [Merge] lp:~nttdata/nova/850602 into lp:nova/diablo

Kei: can you migrate this MP over to review.openstack.org?
--
https://code.launchpad.net/~nttdata/nova/850602/+merge/75480
Your team NTT DATA is subscribed to branch lp:~nttdata/nova/850602.

[Brian Waldon (bcwaldon)]

moved to gerrit

Unmerged revisions

1575. By Kei Masumoto on 2011-09-28

delete unnecessary os.remove(disk) in pre_block_migration. block_migration fails if disk is removed.

1574. By Kei Masumoto on 2011-09-22

using instance['local_gb'] on creating backing file

1573. By Kei Masumoto on 2011-09-22

pep8 error and unnecessary debug statement fixed

1572. By Kei Masumoto on 2011-09-15

fix bugs

=== modified file 'nova/tests/test_libvirt.py'
--- nova/tests/test_libvirt.py	2011-09-19 14:22:34 +0000
+++ nova/tests/test_libvirt.py	2011-09-28 13:07:25 +0000
@@ -629,9 +629,12 @@
 
         fake_timer = FakeTime()
 
+        # _fake_network_info must be called before create_fake_libvirt_mock(),
+        # as _fake_network_info calls utils.import_class() and
+        # create_fake_libvirt_mock() mocks utils.import_class().
+        network_info = _fake_network_info(self.stubs, 1)
         self.create_fake_libvirt_mock()
         instance_ref = db.instance_create(self.context, self.test_instance)
-        network_info = _fake_network_info(self.stubs, 1)
 
         # Start test
         self.mox.ReplayAll()
@@ -684,10 +687,7 @@
                 return vdmock
 
         self.create_fake_libvirt_mock(lookupByName=fake_lookup)
-#        self.mox.StubOutWithMock(self.compute, "recover_live_migration")
         self.mox.StubOutWithMock(self.compute, "rollback_live_migration")
-#        self.compute.recover_live_migration(self.context, instance_ref,
-#                                             dest='dest')
         self.compute.rollback_live_migration(self.context, instance_ref,
                                             'dest', False)
 
@@ -722,7 +722,8 @@
 
         # Test data
         instance_ref = db.instance_create(self.context, self.test_instance)
-        dummyjson = '[{"path": "%s/disk", "local_gb": "10G", "type": "raw"}]'
+        dummyjson = ('[{"path": "%s/disk", "local_gb": "10G",'
+                     ' "type": "raw", "backing_file": ""}]')
 
         # Preparing mocks
         # qemu-img should be mockd since test environment might not have
@@ -763,7 +764,10 @@
                     "</devices></domain>")
 
         ret = ("image: /test/disk\nfile format: raw\n"
-               "virtual size: 20G (21474836480 bytes)\ndisk size: 3.1G\n")
+               "virtual size: 20G (21474836480 bytes)\ndisk size: 3.1G\n"
+               "disk size: 102M\n"
+               "cluster_size: 2097152\n"
+               "backing file: /test/dummy (actual path: /backing/file)\n")
 
         # Preparing mocks
         vdmock = self.mox.CreateMock(libvirt.virDomain)
@@ -793,7 +797,9 @@
                         info[0]['path'] == '/test/disk' and
                         info[1]['path'] == '/test/disk.local' and
                         info[0]['local_gb'] == '10G' and
-                        info[1]['local_gb'] == '20G')
+                        info[1]['local_gb'] == '20G' and
+                        info[0]['backing_file'] == "" and
+                        info[1]['backing_file'] == "file")
 
         db.instance_destroy(self.context, instance_ref['id'])
 
@@ -806,6 +812,10 @@
         def fake_none(self, instance):
             return
 
+        # _fake_network_info must be called before create_fake_libvirt_mock(),
+        # as _fake_network_info calls utils.import_class() and
+        # create_fake_libvirt_mock() mocks utils.import_class().
+        network_info = _fake_network_info(self.stubs, 1)
         self.create_fake_libvirt_mock()
         instance = db.instance_create(self.context, self.test_instance)
 
@@ -815,8 +825,6 @@
         conn.firewall_driver.setattr('setup_basic_filtering', fake_none)
         conn.firewall_driver.setattr('prepare_instance_filter', fake_none)
 
-        network_info = _fake_network_info(self.stubs, 1)
-
         try:
             conn.spawn(self.context, instance, network_info)
         except Exception, e:
 
=== modified file 'nova/virt/libvirt/connection.py'
--- nova/virt/libvirt/connection.py	2011-09-20 10:12:01 +0000
+++ nova/virt/libvirt/connection.py	2011-09-28 13:07:25 +0000
@@ -1726,9 +1726,31 @@
 
         for info in disk_info:
             base = os.path.basename(info['path'])
-            # Get image type and create empty disk image.
+            # Get image type and create empty disk image, and
+            # create backing file in case of qcow2.
             instance_disk = os.path.join(instance_dir, base)
-            utils.execute('qemu-img', 'create', '-f', info['type'],
+            if not info['backing_file']:
+                utils.execute('qemu-img', 'create', '-f', info['type'],
+                              instance_disk, info['local_gb'])
+
+            else:
+                # Creating backing file follows same way as spawning instances.
+                backing_file = os.path.join(FLAGS.instances_path,
+                                            '_base', info['backing_file'])
+
+                if not os.path.exists(backing_file):
+                    self._cache_image(fn=self._fetch_image,
+                        context=ctxt,
+                        target=info['path'],
+                        fname=info['backing_file'],
+                        cow=FLAGS.use_cow_images,
+                        image_id=instance_ref['image_ref'],
+                        user_id=instance_ref['user_id'],
+                        project_id=instance_ref['project_id'],
+                        size=instance_ref['local_gb'])
+
+                utils.execute('qemu-img', 'create', '-f', info['type'],
+                          '-o', 'backing_file=%s' % backing_file,
                           instance_disk, info['local_gb'])
 
         # if image has kernel and ramdisk, just download
@@ -1819,12 +1841,17 @@
                 driver_nodes[cnt].get_properties().get_next().getContent()
             if disk_type == 'raw':
                 size = int(os.path.getsize(path))
+                backing_file = ""
             else:
                 out, err = utils.execute('qemu-img', 'info', path)
                 size = [i.split('(')[1].split()[0] for i in out.split('\n')
                     if i.strip().find('virtual size') >= 0]
                 size = int(size[0])
 
+                backing_file = [i.split('actual path:')[1].strip()[:-1]
+                    for i in out.split('\n') if 0 <= i.find('backing file')]
+                backing_file = os.path.basename(backing_file[0])
+
             # block migration needs same/larger size of empty image on the
             # destination host. since qemu-img creates bit smaller size image
             # depending on original image size, fixed value is necessary.
@@ -1840,7 +1867,7 @@
                 break
 
             disk_info.append({'type': disk_type, 'path': path,
-                              'local_gb': size})
+                              'local_gb': size, 'backing_file': backing_file})
 
         return utils.dumps(disk_info)