Merge lp:~cerberus/nova/lp809909 into lp:~hudson-openstack/nova/trunk

[Matt Dietz]

Fixes lp809909

Migrate of instance with no local storage fails with exception

Simply checks to see if the instance has any local storage, and if not, skips over the resize VDI step.

[Johannes Erdfelt (johannes.erdfelt)]

Thinking about this more, what would happen if we switched from m1.large (as an example) to m1.tiny? Would the same error occur because 0 is an invalid value? I'd guess we would need to delete the VDI in that case instead of resizing.

Also, in the hypothetical case of two flavors with the same local_gb, a resize of the VDI isn't necessary, right? Should we just skip the call to VDI.resize_online then?

[Matt Dietz (cerberus)]

To the first point, you can't resize down (currently) so this wouldn't be an issue.

Regarding the second, I think a call to resize online with the same disk size effectively becomes a NOP, but you're right, we shouldn't be calling it at all. So much for taking the easy way out.

[Johannes Erdfelt (johannes.erdfelt)]

If it's a NOP, then that's probably not a big deal.

One other concern, which might be moot, would be going from m1.tiny to a larger instance. I originally thought that m1.tiny didn't create a VDI since it wouldn't be necessary with 0GB of local storage, but my logs show it tried to resize a VDI.

Assuming there is no VDI, then we would need to create one first?

The code is a little confusing and I'm not 100% sure what is happening right now.

[Matt Dietz (cerberus)]

I just thought of the same case, and TBH, I don't know what it expects to be able to do. I would guess we would have to create a disk in that scenario. When I originally built this feature, I was thinking only in terms of Rackspace style instances, which all have attached disk.

I'll go ahead and file a bug that attempts to reconcile these other cases as necessary.

[lp:~cerberus/nova/lp809909]

1288. By Matt Dietz on 2011-07-20

CHanges based on feedback

1289. By Matt Dietz on 2011-07-21

Renamed the virt driver resize methods to migration for marginally more understandable code

[Johannes Erdfelt (johannes.erdfelt)]

You might need to merge with trunk, the preview diff looks like it is showing a conflict.

[lp:~cerberus/nova/lp809909]

1290. By Matt Dietz on 2011-07-21

Resolved conflict with trunk

[Johannes Erdfelt (johannes.erdfelt)]

The code tracks a resize_instance boolean all the way through calling the resize_instance() method, but that method doesn't use the resize_instance boolean at all. Is that intended for future use?

[Matt Dietz (cerberus)]

Johannes: it's used in the xenapi virt driver

Passed from compute/manage
43 + self.driver.finish_migration(instance_ref, disk_info, network_info,
44 + resize_instance)

to xenapi_conn
234 + if resize_instance:
235 + self.resize_instance(instance, vdi_uuid, resize_instance)

[lp:~cerberus/nova/lp809909]

1291. By Matt Dietz on 2011-07-27

Fixed the virt driver base

1292. By Matt Dietz on 2011-07-27

Merge from trunk and conflict resolution

1293. By Matt Dietz on 2011-07-27

Removed superfluous parameter

[Johannes Erdfelt (johannes.erdfelt)]

After your recent changes, now resize_instance() is called with an extra unexpected parameter.

review: Needs Fixing

[Jason Kölker (jason-koelker)]

Since cerberus is living it up pimp style at OSCON, I'm going to -steal his karma- shepard the change lp:~jason-koelker/nova/lp809909

[Jason Kölker (jason-koelker)]

Please review here: https://code.launchpad.net/~jason-koelker/nova/lp809909/+merge/69568

review: Disapprove

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-07-26 19:42:56 +0000
+++ nova/compute/manager.py	2011-07-27 18:22:33 +0000
@@ -747,7 +747,7 @@
                 local_gb=instance_type['local_gb'],
                 instance_type_id=instance_type['id']))
 
-        self.driver.revert_resize(instance_ref)
+        self.driver.revert_migration(instance_ref)
         self.db.migration_update(context, migration_id,
                 {'status': 'reverted'})
         usage_info = utils.usage_from_instance(instance_ref)
@@ -845,20 +845,26 @@
 
         """
         migration_ref = self.db.migration_get(context, migration_id)
+
+        resize_instance = False
         instance_ref = self.db.instance_get_by_uuid(context,
                 migration_ref.instance_uuid)
-        instance_type = self.db.instance_type_get_by_flavor_id(context,
-                migration_ref['new_flavor_id'])
-        self.db.instance_update(context, instance_ref.uuid,
-               dict(instance_type_id=instance_type['id'],
-                    memory_mb=instance_type['memory_mb'],
-                    vcpus=instance_type['vcpus'],
-                    local_gb=instance_type['local_gb']))
+        if migration_ref['old_flavor_id'] != migration_ref['new_flavor_id']:
+            instance_type = self.db.instance_type_get_by_flavor_id(context,
+                    migration_ref['new_flavor_id'])
+            self.db.instance_update(context, instance_ref.uuid,
+                   dict(instance_type_id=instance_type['id'],
+                        memory_mb=instance_type['memory_mb'],
+                        vcpus=instance_type['vcpus'],
+                        local_gb=instance_type['local_gb']))
+            resize_instance = True
 
         instance_ref = self.db.instance_get_by_uuid(context,
                                             instance_ref.uuid)
+
         network_info = self._get_instance_nw_info(context, instance_ref)
-        self.driver.finish_resize(instance_ref, disk_info, network_info)
+        self.driver.finish_migration(instance_ref, disk_info, network_info,
+                                  resize_instance)
 
         self.db.migration_update(context, migration_id,
                 {'status': 'finished', })

=== modified file 'nova/tests/test_compute.py'
--- nova/tests/test_compute.py	2011-07-22 20:11:39 +0000
+++ nova/tests/test_compute.py	2011-07-27 18:22:33 +0000
@@ -420,7 +420,7 @@
         def fake(*args, **kwargs):
             pass
 
-        self.stubs.Set(self.compute.driver, 'finish_resize', fake)
+        self.stubs.Set(self.compute.driver, 'finish_migration', fake)
         self.stubs.Set(self.compute.network_api, 'get_instance_nw_info', fake)
         context = self.context.elevated()
         instance_id = self._create_instance()
@@ -531,8 +531,8 @@
         def fake(*args, **kwargs):
             pass
 
-        self.stubs.Set(self.compute.driver, 'finish_resize', fake)
-        self.stubs.Set(self.compute.driver, 'revert_resize', fake)
+        self.stubs.Set(self.compute.driver, 'finish_migration', fake)
+        self.stubs.Set(self.compute.driver, 'revert_migration', fake)
         self.stubs.Set(self.compute.network_api, 'get_instance_nw_info', fake)
 
         self.compute.run_instance(self.context, instance_id)

=== modified file 'nova/tests/test_xenapi.py'
--- nova/tests/test_xenapi.py	2011-07-27 17:07:52 +0000
+++ nova/tests/test_xenapi.py	2011-07-27 18:22:33 +0000
@@ -783,27 +783,96 @@
         conn = xenapi_conn.get_connection(False)
         conn.migrate_disk_and_power_off(instance, '127.0.0.1')
 
-    def test_finish_resize(self):
-        instance = db.instance_create(self.context, self.values)
-        stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests)
-        stubs.stubout_loopingcall_start(self.stubs)
-        conn = xenapi_conn.get_connection(False)
-        network_info = [({'bridge': 'fa0', 'id': 0, 'injected': False},
-                          {'broadcast': '192.168.0.255',
-                           'dns': ['192.168.0.1'],
-                           'gateway': '192.168.0.1',
-                           'gateway6': 'dead:beef::1',
-                           'ip6s': [{'enabled': '1',
-                                     'ip': 'dead:beef::dcad:beff:feef:0',
-                                           'netmask': '64'}],
-                           'ips': [{'enabled': '1',
-                                    'ip': '192.168.0.100',
-                                    'netmask': '255.255.255.0'}],
-                           'label': 'fake',
-                           'mac': 'DE:AD:BE:EF:00:00',
-                           'rxtx_cap': 3})]
-        conn.finish_resize(instance, dict(base_copy='hurr', cow='durr'),
-                                                           network_info)
+    def test_finish_migrate(self):
+        instance = db.instance_create(self.context, self.values)
+        self.called = False
+
+        def fake_vdi_resize(*args, **kwargs):
+            self.called = True
+
+        self.stubs.Set(stubs.FakeSessionForMigrationTests,
+                "VDI_resize_online", fake_vdi_resize)
+        stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests)
+        stubs.stubout_loopingcall_start(self.stubs)
+        conn = xenapi_conn.get_connection(False)
+        network_info = [({'bridge': 'fa0', 'id': 0, 'injected': False},
+                          {'broadcast': '192.168.0.255',
+                           'dns': ['192.168.0.1'],
+                           'gateway': '192.168.0.1',
+                           'gateway6': 'dead:beef::1',
+                           'ip6s': [{'enabled': '1',
+                                     'ip': 'dead:beef::dcad:beff:feef:0',
+                                           'netmask': '64'}],
+                           'ips': [{'enabled': '1',
+                                    'ip': '192.168.0.100',
+                                    'netmask': '255.255.255.0'}],
+                           'label': 'fake',
+                           'mac': 'DE:AD:BE:EF:00:00',
+                           'rxtx_cap': 3})]
+        conn.finish_migration(instance, dict(base_copy='hurr', cow='durr'),
+                           network_info, resize_instance=True)
+        self.assertEqual(self.called, True)
+
+    def test_finish_migrate_no_local_storage(self):
+        tiny_type_id = \
+                instance_types.get_instance_type_by_name('m1.tiny')['id']
+        self.values.update({'instance_type_id': tiny_type_id, 'local_gb': 0})
+        instance = db.instance_create(self.context, self.values)
+
+        def fake_vdi_resize(*args, **kwargs):
+            raise Exception("This shouldn't be called")
+
+        self.stubs.Set(stubs.FakeSessionForMigrationTests,
+                "VDI_resize_online", fake_vdi_resize)
+        stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests)
+        stubs.stubout_loopingcall_start(self.stubs)
+        conn = xenapi_conn.get_connection(False)
+        network_info = [({'bridge': 'fa0', 'id': 0, 'injected': False},
+                          {'broadcast': '192.168.0.255',
+                           'dns': ['192.168.0.1'],
+                           'gateway': '192.168.0.1',
+                           'gateway6': 'dead:beef::1',
+                           'ip6s': [{'enabled': '1',
+                                     'ip': 'dead:beef::dcad:beff:feef:0',
+                                           'netmask': '64'}],
+                           'ips': [{'enabled': '1',
+                                    'ip': '192.168.0.100',
+                                    'netmask': '255.255.255.0'}],
+                           'label': 'fake',
+                           'mac': 'DE:AD:BE:EF:00:00',
+                           'rxtx_cap': 3})]
+        conn.finish_migration(instance, dict(base_copy='hurr', cow='durr'),
+                           network_info, resize_instance=True)
+
+    def test_finish_migrate_no_resize_vdi(self):
+        instance = db.instance_create(self.context, self.values)
+
+        def fake_vdi_resize(*args, **kwargs):
+            raise Exception("This shouldn't be called")
+
+        self.stubs.Set(stubs.FakeSessionForMigrationTests,
+                "VDI_resize_online", fake_vdi_resize)
+        stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests)
+        stubs.stubout_loopingcall_start(self.stubs)
+        conn = xenapi_conn.get_connection(False)
+        network_info = [({'bridge': 'fa0', 'id': 0, 'injected': False},
+                          {'broadcast': '192.168.0.255',
+                           'dns': ['192.168.0.1'],
+                           'gateway': '192.168.0.1',
+                           'gateway6': 'dead:beef::1',
+                           'ip6s': [{'enabled': '1',
+                                     'ip': 'dead:beef::dcad:beff:feef:0',
+                                           'netmask': '64'}],
+                           'ips': [{'enabled': '1',
+                                    'ip': '192.168.0.100',
+                                    'netmask': '255.255.255.0'}],
+                           'label': 'fake',
+                           'mac': 'DE:AD:BE:EF:00:00',
+                           'rxtx_cap': 3})]
+
+        # Resize instance would be determined by the compute call
+        conn.finish_migration(instance, dict(base_copy='hurr', cow='durr'),
+                           network_info, resize_instance=False)
 
 
 class XenAPIDetermineDiskImageTestCase(test.TestCase):

=== modified file 'nova/virt/driver.py'
--- nova/virt/driver.py	2011-07-22 20:11:39 +0000
+++ nova/virt/driver.py	2011-07-27 18:22:33 +0000
@@ -122,11 +122,12 @@
         """Create snapshot from a running VM instance."""
         raise NotImplementedError()
 
-    def finish_resize(self, instance, disk_info):
+    def finish_migration(self, instance, disk_info, network_info,
+                         resize_instance):
         """Completes a resize, turning on the migrated instance"""
         raise NotImplementedError()
 
-    def revert_resize(self, instance):
+    def revert_migration(self, instance):
         """Reverts a resize, powering back on the instance"""
         raise NotImplementedError()
 

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-07-27 17:07:52 +0000
+++ nova/virt/xenapi/vmops.py	2011-07-27 18:22:33 +0000
@@ -110,17 +110,19 @@
                 instance_infos.append(instance_info)
         return instance_infos
 
-    def revert_resize(self, instance):
+    def revert_migration(self, instance):
         vm_ref = VMHelper.lookup(self._session, instance.name)
         self._start(instance, vm_ref)
 
-    def finish_resize(self, instance, disk_info, network_info):
+    def finish_migration(self, instance, disk_info, network_info,
+                      resize_instance):
         vdi_uuid = self.link_disks(instance, disk_info['base_copy'],
                 disk_info['cow'])
         vm_ref = self._create_vm(instance,
                                  [dict(vdi_type='os', vdi_uuid=vdi_uuid)],
                                  network_info)
-        self.resize_instance(instance, vdi_uuid)
+        if resize_instance:
+            self.resize_instance(instance, vdi_uuid, resize_instance)
         self._spawn(instance, vm_ref)
 
     def _start(self, instance, vm_ref=None):
@@ -568,18 +570,22 @@
         return new_cow_uuid
 
     def resize_instance(self, instance, vdi_uuid):
-        """Resize a running instance by changing it's RAM and disk size."""
+        """Resize a running instance by changing its RAM and disk size."""
         #TODO(mdietz): this will need to be adjusted for swap later
         #The new disk size must be in bytes
 
-        new_disk_size = str(instance.local_gb * 1024 * 1024 * 1024)
-        instance_name = instance.name
-        instance_local_gb = instance.local_gb
-        LOG.debug(_("Resizing VDI %(vdi_uuid)s for instance %(instance_name)s."
-                " Expanding to %(instance_local_gb)d GB") % locals())
-        vdi_ref = self._session.call_xenapi('VDI.get_by_uuid', vdi_uuid)
-        self._session.call_xenapi('VDI.resize_online', vdi_ref, new_disk_size)
-        LOG.debug(_("Resize instance %s complete") % (instance.name))
+        new_disk_size = instance.local_gb * 1024 * 1024 * 1024
+        if new_disk_size > 0:
+            instance_name = instance.name
+            instance_local_gb = instance.local_gb
+            LOG.debug(_("Resizing VDI %(vdi_uuid)s for instance"
+                        "%(instance_name)s. Expanding to %(instance_local_gb)d"
+                        " GB") % locals())
+            vdi_ref = self._session.call_xenapi('VDI.get_by_uuid', vdi_uuid)
+            # for an instance with no local storage
+            self._session.call_xenapi('VDI.resize_online', vdi_ref,
+                    str(new_disk_size))
+            LOG.debug(_("Resize instance %s complete") % (instance.name))
 
     def reboot(self, instance):
         """Reboot VM instance."""

=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2011-07-27 17:07:52 +0000
+++ nova/virt/xenapi_conn.py	2011-07-27 18:22:33 +0000
@@ -191,13 +191,15 @@
         """Create VM instance"""
         self._vmops.spawn(instance, network_info)
 
-    def revert_resize(self, instance):
+    def revert_migration(self, instance):
         """Reverts a resize, powering back on the instance"""
         self._vmops.revert_resize(instance)
 
-    def finish_resize(self, instance, disk_info, network_info):
+    def finish_migration(self, instance, disk_info, network_info,
+                         resize_instance=False):
         """Completes a resize, turning on the migrated instance"""
-        self._vmops.finish_resize(instance, disk_info, network_info)
+        self._vmops.finish_migration(instance, disk_info, network_info,
+                                  resize_instance)
 
     def snapshot(self, instance, image_id):
         """ Create snapshot from a running VM instance """