Merge "xenapi: Always set other_config for VDIs"

3 files changed, 251 insertions, 40 deletions

diff --git a/nova/tests/xenapi/test_vm_utils.py b/nova/tests/xenapi/test_vm_utils.py
index 1f540709e..694be1713 100644
--- a/nova/tests/xenapi/test_vm_utils.py
+++ b/nova/tests/xenapi/test_vm_utils.py
@@ -14,6 +14,18 @@ XENSM_TYPE = 'xensm'
 ISCSI_TYPE = 'iscsi'
 
 
+class FakeSession():
+    def call_xenapi(self, operation, *args, **kwargs):
+        # VDI.add_to_other_config -> VDI_add_to_other_config
+        method = getattr(self, operation.replace('.', '_'), None)
+        if method:
+            return method(*args, **kwargs)
+
+        self.operation = operation
+        self.args = args
+        self.kwargs = kwargs
+
+
 def get_fake_connection_data(sr_type):
     fakes = {XENSM_TYPE: {'sr_uuid': 'falseSR',
                           'name_label': 'fake_storage',
@@ -209,11 +221,6 @@ class BittorrentTestCase(stubs.XenAPITestBase):
 class CreateVBDTestCase(test.TestCase):
     def setUp(self):
         super(CreateVBDTestCase, self).setUp()
-
-        class FakeSession():
-            def call_xenapi(*args):
-                pass
-
         self.session = FakeSession()
         self.mock = mox.Mox()
         self.mock.StubOutWithMock(self.session, 'call_xenapi')
@@ -284,3 +291,87 @@ class CreateVBDTestCase(test.TestCase):
         result = vm_utils.attach_cd(self.session, "vm_ref", "vdi_ref", 1)
         self.assertEquals(result, "vbd_ref")
         self.mock.VerifyAll()
+
+
+class VDIOtherConfigTestCase(stubs.XenAPITestBase):
+    """Tests to ensure that the code is populating VDI's `other_config`
+    attribute with the correct metadta.
+    """
+
+    def setUp(self):
+        super(VDIOtherConfigTestCase, self).setUp()
+        self.session = FakeSession()
+        self.context = context.get_admin_context()
+        self.fake_instance = {'uuid': 'aaaa-bbbb-cccc-dddd',
+                              'name': 'myinstance'}
+
+    def test_create_vdi(self):
+        # Some images are registered with XenServer explicitly by calling
+        # `create_vdi`
+        vm_utils.create_vdi(self.session, 'sr_ref', self.fake_instance,
+                            'myvdi', 'root', 1024, read_only=True)
+
+        expected = {'nova_disk_type': 'root',
+                    'nova_instance_uuid': 'aaaa-bbbb-cccc-dddd'}
+
+        self.assertEqual(expected, self.session.args[0]['other_config'])
+
+    def test_create_image(self):
+        # Other images are registered implicitly when they are dropped into
+        # the SR by a dom0 plugin or some other process
+        self.flags(cache_images='none')
+
+        def fake_fetch_image(*args):
+            return {'root': {'uuid': 'fake-uuid'}}
+
+        self.stubs.Set(vm_utils, '_fetch_image', fake_fetch_image)
+
+        other_config = {}
+
+        def VDI_add_to_other_config(ref, key, value):
+            other_config[key] = value
+
+        def VDI_get_record(ref):
+            return {'other_config': {}}
+
+        # Stubbing on the session object and not class so we don't pollute
+        # other tests
+        self.session.VDI_add_to_other_config = VDI_add_to_other_config
+        self.session.VDI_get_record = VDI_get_record
+
+        vm_utils._create_image(self.context, self.session, self.fake_instance,
+                'myvdi', 'image1', vm_utils.ImageType.DISK_VHD)
+
+        expected = {'nova_disk_type': 'root',
+                    'nova_instance_uuid': 'aaaa-bbbb-cccc-dddd'}
+
+        self.assertEqual(expected, other_config)
+
+    def test_move_disks(self):
+        # Migrated images should preserve the `other_config`
+        other_config = {}
+
+        def VDI_add_to_other_config(ref, key, value):
+            other_config[key] = value
+
+        def VDI_get_record(ref):
+            return {'other_config': {}}
+
+        def call_plugin_serialized(*args, **kwargs):
+            return {'root': {'uuid': 'aaaa-bbbb-cccc-dddd'}}
+
+        # Stubbing on the session object and not class so we don't pollute
+        # other tests
+        self.session.VDI_add_to_other_config = VDI_add_to_other_config
+        self.session.VDI_get_record = VDI_get_record
+        self.session.call_plugin_serialized = call_plugin_serialized
+
+        self.stubs.Set(vm_utils, 'get_sr_path', lambda *a, **k: None)
+        self.stubs.Set(vm_utils, 'scan_default_sr', lambda *a, **k: None)
+
+        vm_utils.move_disks(self.session, self.fake_instance, {})
+
+        expected = {'nova_disk_type': 'root',
+                    'nova_instance_uuid': 'aaaa-bbbb-cccc-dddd'}
+
+        self.assertEqual(expected, other_config)
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index 9832dca84..904f12e30 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -450,11 +450,6 @@ def safe_destroy_vdis(session, vdi_refs):
 def create_vdi(session, sr_ref, instance, name_label, disk_type, virtual_size,
                read_only=False):
     """Create a VDI record and returns its reference."""
-    # create_vdi may be called simply while creating a volume
-    # hence information about instance may or may not be present
-    otherconf = {'nova_disk_type': disk_type}
-    if instance:
-        otherconf['nova_instance_uuid'] = instance['uuid']
     vdi_ref = session.call_xenapi("VDI.create",
          {'name_label': name_label,
           'name_description': disk_type,
@@ -464,7 +459,7 @@ def create_vdi(session, sr_ref, instance, name_label, disk_type, virtual_size,
           'sharable': False,
           'read_only': read_only,
           'xenstore_data': {},
-          'other_config': otherconf,
+          'other_config': _get_vdi_other_config(disk_type, instance=instance),
           'sm_config': {},
           'tags': []})
     LOG.debug(_('Created VDI %(vdi_ref)s (%(name_label)s,'
@@ -597,11 +592,36 @@ def _clone_vdi(session, vdi_to_clone_ref):
     return vdi_ref
 
 
-def set_vdi_name(session, vdi_uuid, label, description, vdi_ref=None):
-    vdi_ref = vdi_ref or session.call_xenapi('VDI.get_by_uuid', vdi_uuid)
-    session.call_xenapi('VDI.set_name_label', vdi_ref, label)
+def _get_vdi_other_config(disk_type, instance=None):
+    """Return metadata to store in VDI's other_config attribute.
+
+    `nova_instance_uuid` is used to associate a VDI with a particular instance
+    so that, if it becomes orphaned from an unclean shutdown of a
+    compute-worker, we can safely detach it.
+    """
+    other_config = {'nova_disk_type': disk_type}
+
+    # create_vdi may be called simply while creating a volume
+    # hence information about instance may or may not be present
+    if instance:
+        other_config['nova_instance_uuid'] = instance['uuid']
+
+    return other_config
+
+
+def _set_vdi_info(session, vdi_ref, vdi_type, name_label, description,
+                  instance):
+    vdi_rec = session.call_xenapi('VDI.get_record', vdi_ref)
+
+    session.call_xenapi('VDI.set_name_label', vdi_ref, name_label)
     session.call_xenapi('VDI.set_name_description', vdi_ref, description)
 
+    other_config = _get_vdi_other_config(vdi_type, instance=instance)
+    for key, value in other_config.iteritems():
+        if key not in vdi_rec['other_config']:
+            session.call_xenapi(
+                    "VDI.add_to_other_config", vdi_ref, key, value)
+
 
 def get_vdi_for_vm_safely(session, vm_ref):
     """Retrieves the primary VDI for a VM."""
@@ -933,25 +953,25 @@ def _create_cached_image(context, session, instance, name_label,
                       "type %(sr_type)s. Ignoring the cow flag.")
                       % locals())
 
-    root_vdi_ref = _find_cached_image(session, image_id, sr_ref)
-    if root_vdi_ref is None:
+    cache_vdi_ref = _find_cached_image(session, image_id, sr_ref)
+    if cache_vdi_ref is None:
         vdis = _fetch_image(context, session, instance, name_label,
                             image_id, image_type)
-        root_vdi = vdis['root']
-        root_vdi_ref = session.call_xenapi('VDI.get_by_uuid',
-                                           root_vdi['uuid'])
-        set_vdi_name(session, root_vdi['uuid'], 'Glance Image %s' % image_id,
-                     'root', vdi_ref=root_vdi_ref)
+
+        cache_vdi_ref = session.call_xenapi(
+                'VDI.get_by_uuid', vdis['root']['uuid'])
+
+        session.call_xenapi('VDI.set_name_label', cache_vdi_ref,
+                            'Glance Image %s' % image_id)
+        session.call_xenapi('VDI.set_name_description', cache_vdi_ref, 'root')
         session.call_xenapi('VDI.add_to_other_config',
-                            root_vdi_ref, 'image-id', str(image_id))
+                            cache_vdi_ref, 'image-id', str(image_id))
 
     if CONF.use_cow_images and sr_type == 'ext':
-        new_vdi_ref = _clone_vdi(session, root_vdi_ref)
+        new_vdi_ref = _clone_vdi(session, cache_vdi_ref)
     else:
-        new_vdi_ref = _safe_copy_vdi(session, sr_ref, instance, root_vdi_ref)
+        new_vdi_ref = _safe_copy_vdi(session, sr_ref, instance, cache_vdi_ref)
 
-    # Set the name label for the image we just created and remove image id
-    # field from other-config.
     session.call_xenapi('VDI.remove_from_other_config',
                         new_vdi_ref, 'image-id')
 
@@ -996,10 +1016,10 @@ def _create_image(context, session, instance, name_label, image_id,
         vdis = _fetch_image(context, session, instance, name_label,
                             image_id, image_type)
 
-    # Set the name label and description to easily identify what
-    # instance and disk it's for
     for vdi_type, vdi in vdis.iteritems():
-        set_vdi_name(session, vdi['uuid'], name_label, vdi_type)
+        vdi_ref = session.call_xenapi('VDI.get_by_uuid', vdi['uuid'])
+        _set_vdi_info(session, vdi_ref, vdi_type, name_label, vdi_type,
+                      instance)
 
     return vdis
 
@@ -1131,13 +1151,7 @@ def _fetch_vhd_image(context, session, instance, image_id):
     sr_ref = safe_find_sr(session)
     _scan_sr(session, sr_ref)
 
-    # Pull out the UUID of the root VDI
-    root_vdi_uuid = vdis['root']['uuid']
-
-    # Set the name-label to ease debugging
-    set_vdi_name(session, root_vdi_uuid, instance['name'], 'root')
-
-    _check_vdi_size(context, session, instance, root_vdi_uuid)
+    _check_vdi_size(context, session, instance, vdis['root']['uuid'])
     return vdis
 
 
@@ -2315,13 +2329,13 @@ def move_disks(session, instance, disk_info):
     # Now we rescan the SR so we find the VHDs
     scan_default_sr(session)
 
-    # Set name-label so we can find if we need to clean up a failed
-    # migration
     root_uuid = imported_vhds['root']['uuid']
-    set_vdi_name(session, root_uuid, instance['name'], 'root')
-
     root_vdi_ref = session.call_xenapi('VDI.get_by_uuid', root_uuid)
 
+    # Set name-label so we can find if we need to clean up a failed migration
+    _set_vdi_info(session, root_vdi_ref, 'root', instance['name'], 'root',
+                  instance)
+
     return {'uuid': root_uuid, 'ref': root_vdi_ref}
 
 
diff --git a/tools/xenserver/populate_other_config.py b/tools/xenserver/populate_other_config.py
new file mode 100644
index 000000000..7151fee61
--- /dev/null
+++ b/tools/xenserver/populate_other_config.py
@@ -0,0 +1,106 @@
+#!/usr/bin/env python
+
+# Copyright 2013 OpenStack Foundation
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""
+One-time script to populate VDI.other_config.
+
+We use metadata stored in VDI.other_config to associate a VDI with a given
+instance so that we may safely cleanup orphaned VDIs.
+
+We had a bug in the code that meant that the vast majority of VDIs created
+would not have the other_config populated.
+
+After deploying the fixed code, this script is intended to be run against all
+compute-workers in a cluster so that existing VDIs can have their other_configs
+populated.
+
+Run on compute-worker (not Dom0):
+
+    python ./tools/xenserver/populate_other_config.py [--dry-run|--verbose]
+"""
+import gettext
+gettext.install('nova', unicode=1)
+
+import os
+import sys
+
+possible_topdir = os.getcwd()
+if os.path.exists(os.path.join(possible_topdir, "nova", "__init__.py")):
+        sys.path.insert(0, possible_topdir)
+
+from nova import config
+from nova.openstack.common import uuidutils
+from nova.virt import virtapi
+from nova.virt.xenapi import driver as xenapi_driver
+from nova.virt.xenapi import vm_utils
+from oslo.config import cfg
+
+cli_opts = [
+    cfg.BoolOpt('dry-run',
+               default=False,
+               help='Whether to actually update other_config.'),
+]
+
+CONF = cfg.CONF
+CONF.register_cli_opts(cli_opts)
+
+
+def main():
+    config.parse_args(sys.argv)
+
+    xenapi = xenapi_driver.XenAPIDriver(virtapi.VirtAPI())
+    session = xenapi._session
+
+    vdi_refs = session.call_xenapi('VDI.get_all')
+    for vdi_ref in vdi_refs:
+        vdi_rec = session.call_xenapi('VDI.get_record', vdi_ref)
+
+        other_config = vdi_rec['other_config']
+
+        # Already set...
+        if 'nova_instance_uuid' in other_config:
+            continue
+
+        name_label = vdi_rec['name_label']
+
+        # We only want name-labels of form instance-<UUID>-[optional-suffix]
+        if not name_label.startswith('instance-'):
+            continue
+
+        # Parse out UUID
+        instance_uuid = name_label.replace('instance-', '')[:36]
+        if not uuidutils.is_uuid_like(instance_uuid):
+            print "error: name label '%s' wasn't UUID-like" % name_label
+            continue
+
+        vdi_type = vdi_rec['name_description']
+
+        # We don't need a full instance record, just the UUID
+        instance = {'uuid': instance_uuid}
+
+        if not CONF.dry_run:
+            vm_utils._set_vdi_info(session, vdi_ref, vdi_type, name_label,
+                                   vdi_type, instance)
+
+        if CONF.verbose:
+            print "Setting other_config for instance_uuid=%s vdi_uuid=%s" % (
+                    instance_uuid, vdi_rec['uuid'])
+
+        if CONF.dry_run:
+            print "Dry run completed"
+
+
+if __name__ == "__main__":
+    main()