XenAPI: Fix race-condition with cached images.

The core problem is that XenServer's `VDI.copy` call drops the
destination file directly into the SR. This means that half-completed
files are visible with no way to distinguish these from fully-copied
files.

We had some code that attempted to mitigate this issue by checking
physical_utilisation against an expected value. The problem with this
code is that it didn't account for VDI chaining where the
physical_utilisation would not necessarily match the parent.

The net effect of this was that 'cloned' VDIs would never be found
because their physical_utilisation was far below what was expected.

The work around is to create our own `_safe_copy_vdi` which is isolated
and atomic. Long term, `VDI.copy` should be fixed so that half-completed
files are never stored in the SR.

Change-Id: I6eb3cb5259f9ee1c7394e58f76105a8b39bfc720

4 files changed, 141 insertions, 36 deletions

diff --git a/nova/tests/test_xenapi.py b/nova/tests/test_xenapi.py
index a5b88bd0d..f858fed25 100644
--- a/nova/tests/test_xenapi.py
+++ b/nova/tests/test_xenapi.py
@@ -288,6 +288,15 @@ class XenAPIVMTestCase(stubs.XenAPITestBase):
         self.stubs.Set(vmops.VMOps, 'inject_instance_metadata',
                        fake_inject_instance_metadata)
 
+        def fake_safe_copy_vdi(session, sr_ref, instance, vdi_to_copy_ref):
+            name_label = "fakenamelabel"
+            disk_type = "fakedisktype"
+            virtual_size = 777
+            return vm_utils.create_vdi(
+                    session, sr_ref, instance, name_label, disk_type,
+                    virtual_size)
+        self.stubs.Set(vm_utils, '_safe_copy_vdi', fake_safe_copy_vdi)
+
     def tearDown(self):
         super(XenAPIVMTestCase, self).tearDown()
         fake_image.FakeImageService_reset()
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index 47c5a1e85..6635558f9 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -428,11 +428,63 @@ def get_vdis_for_instance(context, session, instance, name_label, image,
                         image_type)
 
 
-def _copy_vdi(session, sr_ref, vdi_to_copy_ref):
-    """Copy a VDI and return the new VDIs reference."""
-    vdi_ref = session.call_xenapi('VDI.copy', vdi_to_copy_ref, sr_ref)
-    LOG.debug(_('Copied VDI %(vdi_ref)s from VDI '
-                '%(vdi_to_copy_ref)s on %(sr_ref)s.') % locals())
+@contextlib.contextmanager
+def _dummy_vm(session, instance, vdi_ref):
+    """This creates a temporary VM so that we can snapshot a VDI.
+
+    VDI's can't be snapshotted directly since the API expects a `vm_ref`. To
+    work around this, we need to create a temporary VM and then map the VDI to
+    the VM using a temporary VBD.
+    """
+    name_label = "dummy"
+    vm_ref = create_vm(session, instance, name_label, None, None)
+    try:
+        vbd_ref = create_vbd(session, vm_ref, vdi_ref, 'autodetect',
+                             read_only=True)
+        try:
+            yield vm_ref
+        finally:
+            try:
+                destroy_vbd(session, vbd_ref)
+            except volume_utils.StorageError:
+                # destroy_vbd() will log error
+                pass
+    finally:
+        destroy_vm(session, instance, vm_ref)
+
+
+def _safe_copy_vdi(session, sr_ref, instance, vdi_to_copy_ref):
+    """Copy a VDI and return the new VDIs reference.
+
+    This function differs from the XenAPI `VDI.copy` call in that the copy is
+    atomic and isolated, meaning we don't see half-downloaded images. It
+    accomplishes this by copying the VDI's into a temporary directory and then
+    atomically renaming them into the SR when the copy is completed.
+
+    The correct long term solution is to fix `VDI.copy` so that it is atomic
+    and isolated.
+    """
+    with _dummy_vm(session, instance, vdi_to_copy_ref) as vm_ref:
+        label = "snapshot"
+
+        with snapshot_attached_here(
+                session, instance, vm_ref, label) as vdi_uuids:
+            params = {'sr_path': get_sr_path(session),
+                      'vdi_uuids': vdi_uuids,
+                      'uuid_stack': _make_uuid_stack()}
+
+            kwargs = {'params': pickle.dumps(params)}
+            result = session.call_plugin(
+                    'workarounds', 'safe_copy_vdis', kwargs)
+            imported_vhds = jsonutils.loads(result)
+
+    root_uuid = imported_vhds['root']['uuid']
+
+    # TODO(sirp): for safety, we should probably re-scan the SR after every
+    # call to a dom0 plugin, since there is a possibility that the underlying
+    # VHDs changed
+    scan_default_sr(session)
+    vdi_ref = session.call_xenapi('VDI.get_by_uuid', root_uuid)
     return vdi_ref
 
 
@@ -526,22 +578,7 @@ def find_cached_image(session, image_id, sr_ref):
     """Returns the vdi-ref of the cached image."""
     for vdi_ref, vdi_rec in _get_all_vdis_in_sr(session, sr_ref):
         other_config = vdi_rec['other_config']
-
-        try:
-            image_id_match = other_config['image-id'] == image_id
-        except KeyError:
-            image_id_match = False
-
-        # NOTE(sirp): `VDI.copy` stores the partially-completed file in the SR.
-        # In order to avoid these half-baked files, we compare its current size
-        # to the expected size pulled from the original cache file.
-        try:
-            size_match = (other_config['expected_physical_utilisation'] ==
-                          vdi_rec['physical_utilisation'])
-        except KeyError:
-            size_match = False
-
-        if image_id_match and size_match:
+        if image_id == other_config.get('image-id'):
             return vdi_ref
 
     return None
@@ -764,24 +801,16 @@ def _create_cached_image(context, session, instance, name_label,
         session.call_xenapi('VDI.add_to_other_config',
                             root_vdi_ref, 'image-id', str(image_id))
 
-        for vdi_type, vdi in vdis.iteritems():
-            vdi_ref = session.call_xenapi('VDI.get_by_uuid',
-                                          vdi['uuid'])
-
-            vdi_rec = session.call_xenapi('VDI.get_record', vdi_ref)
-            session.call_xenapi('VDI.add_to_other_config',
-                                vdi_ref, 'expected_physical_utilisation',
-                                vdi_rec['physical_utilisation'])
-
-            if vdi_type == 'swap':
-                session.call_xenapi('VDI.add_to_other_config',
-                                    root_vdi_ref, 'swap-disk',
-                                    str(vdi['uuid']))
+        swap_vdi = vdis.get('swap')
+        if swap_vdi:
+            session.call_xenapi(
+                    'VDI.add_to_other_config', root_vdi_ref, 'swap-disk',
+                    str(swap_vdi['uuid']))
 
     if FLAGS.use_cow_images and sr_type == 'ext':
         new_vdi_ref = _clone_vdi(session, root_vdi_ref)
     else:
-        new_vdi_ref = _copy_vdi(session, sr_ref, root_vdi_ref)
+        new_vdi_ref = _safe_copy_vdi(session, sr_ref, instance, root_vdi_ref)
 
     # Set the name label for the image we just created and remove image id
     # field from other-config.
@@ -799,7 +828,8 @@ def _create_cached_image(context, session, instance, name_label,
         swap_disk_uuid = vdi_rec['other_config']['swap-disk']
         swap_vdi_ref = session.call_xenapi('VDI.get_by_uuid',
                                            swap_disk_uuid)
-        new_swap_vdi_ref = _copy_vdi(session, sr_ref, swap_vdi_ref)
+        new_swap_vdi_ref = _safe_copy_vdi(
+                session, sr_ref, instance, swap_vdi_ref)
         new_swap_vdi_uuid = session.call_xenapi('VDI.get_uuid',
                                                 new_swap_vdi_ref)
         vdis['swap'] = dict(uuid=new_swap_vdi_uuid, file=None)
diff --git a/plugins/xenserver/xenapi/contrib/rpmbuild/SPECS/openstack-xen-plugins.spec b/plugins/xenserver/xenapi/contrib/rpmbuild/SPECS/openstack-xen-plugins.spec
index 8f4415311..63b5e71d3 100644
--- a/plugins/xenserver/xenapi/contrib/rpmbuild/SPECS/openstack-xen-plugins.spec
+++ b/plugins/xenserver/xenapi/contrib/rpmbuild/SPECS/openstack-xen-plugins.spec
@@ -33,6 +33,7 @@ rm -rf $RPM_BUILD_ROOT
 /etc/xapi.d/plugins/kernel
 /etc/xapi.d/plugins/migration
 /etc/xapi.d/plugins/pluginlib_nova.py
+/etc/xapi.d/plugins/workarounds
 /etc/xapi.d/plugins/xenhost
 /etc/xapi.d/plugins/xenstore.py
 /etc/xapi.d/plugins/utils.py
diff --git a/plugins/xenserver/xenapi/etc/xapi.d/plugins/workarounds b/plugins/xenserver/xenapi/etc/xapi.d/plugins/workarounds
new file mode 100755
index 000000000..611436539
--- /dev/null
+++ b/plugins/xenserver/xenapi/etc/xapi.d/plugins/workarounds
@@ -0,0 +1,65 @@
+#!/usr/bin/env python
+
+# Copyright (c) 2012 Openstack, LLC
+# All Rights Reserved.
+#
+#    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.
+
+"""Handle the uploading and downloading of images via Glance."""
+
+import cPickle as pickle
+try:
+    import json
+except ImportError:
+    import simplejson as json
+import os
+import shutil
+
+import XenAPIPlugin
+
+import utils
+
+#FIXME(sirp): should this use pluginlib from 5.6?
+from pluginlib_nova import *
+configure_logging('hacks')
+
+
+def _copy_vdis(sr_path, staging_path, vdi_uuids):
+    seq_num = 0
+    for vdi_uuid in vdi_uuids:
+        src = os.path.join(sr_path, "%s.vhd" % vdi_uuid)
+        dst = os.path.join(staging_path, "%d.vhd" % seq_num)
+        shutil.copyfile(src, dst)
+        seq_num += 1
+
+
+def safe_copy_vdis(session, args):
+    params = pickle.loads(exists(args, 'params'))
+    sr_path = params["sr_path"]
+    vdi_uuids = params["vdi_uuids"]
+    uuid_stack = params["uuid_stack"]
+
+    staging_path = utils.make_staging_area(sr_path)
+    try:
+        _copy_vdis(sr_path, staging_path, vdi_uuids)
+        imported_vhds = utils.import_vhds(sr_path, staging_path, uuid_stack)
+    finally:
+        utils.cleanup_staging_area(staging_path)
+
+    # Right now, it's easier to return a single string via XenAPI,
+    # so we'll json encode the list of VHDs.
+    return json.dumps(imported_vhds)
+
+
+if __name__ == '__main__':
+    XenAPIPlugin.dispatch({'safe_copy_vdis': safe_copy_vdis})