Move snapshot image property inheritance

The xenapi snapshotting code had support for inheriting properties
from the base image of the snapshot (by way of the system metadata
set on the snapshotted instance).  The problem, however, came in the
fact that some image properties were set in nova.compute.api, but
those properties were not excluded from this inheritance logic
except through a configuration option called
"non_inheritable_image_properties".  I had previously updated the
default setting for this option to work around the bugs introduced
by setting image properties in two different locations, but now it
is time for the real fix.

This change moves the inheritance logic into
nova.compute.api:API._create_image.  Note that two properties are
still set through the xenapi snapshotting logic: the "os_type" and
the "auto_disk_config" properties, which are presumed to be xenapi
specific.  The change also alters the inheritance logic to ensure
that the work-around listing of image properties in
non_inheritable_image_properties is no longer necessary; the
default for this configuration option is updated accordingly.

(Note: It will not harm anything to have these image properties
still listed in non_inheritable_image_properties, so configurations
that override this option do not need to be altered.)

Change-Id: I3514da432cc10c75418e1de9752f60640d579136

5 files changed, 43 insertions, 24 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 2818a9eb8..20523170f 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1233,6 +1233,22 @@ class API(base.Base):
                 sent_meta['min_disk'] = min_disk
 
         properties.update(extra_properties or {})
+
+        # Now inherit image properties from the base image
+        prefix = 'image_'
+        for key, value in system_meta.items():
+            # Trim off the image_ prefix
+            if key.startswith(prefix):
+                key = key[len(prefix):]
+
+            # Skip properties that are non-inheritable
+            if key in FLAGS.non_inheritable_image_properties:
+                continue
+
+            # By using setdefault, we ensure that the properties set
+            # up above will not be overwritten by inherited values
+            properties.setdefault(key, value)
+
         sent_meta['properties'] = properties
 
         recv_meta = self.image_service.create(context, sent_meta)
diff --git a/nova/flags.py b/nova/flags.py
index 9ab59bbe3..e9057d8db 100644
--- a/nova/flags.py
+++ b/nova/flags.py
@@ -401,12 +401,6 @@ global_opts = [
                help='The strategy to use for auth: noauth or keystone.'),
     cfg.ListOpt('non_inheritable_image_properties',
                 default=['cache_in_nova',
-                         'instance_uuid',
-                         'user_id',
-                         'image_type',
-                         'backup_type',
-                         'min_ram',
-                         'min_disk',
                          'bittorrent'],
                 help='These are image properties which a snapshot should not'
                      ' inherit from an instance'),
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 78968afab..cad27b544 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -3532,6 +3532,27 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(self.context, instance['uuid'])
 
+    def test_snapshot_image_metadata_inheritance(self):
+        # Ensure image snapshots inherit metadata from the base image
+        self.flags(non_inheritable_image_properties=['spam'])
+
+        def fake_instance_system_metadata_get(context, uuid):
+            return dict(image_a=1, image_b=2, image_c='c', d='d', spam='spam')
+
+        self.stubs.Set(db, 'instance_system_metadata_get',
+                       fake_instance_system_metadata_get)
+
+        instance = self._create_fake_instance()
+        image = self.compute_api.snapshot(self.context, instance, 'snap1',
+                                          {'extra_param': 'value1'})
+
+        properties = image['properties']
+        self.assertEqual(properties['a'], 1)
+        self.assertEqual(properties['b'], 2)
+        self.assertEqual(properties['c'], 'c')
+        self.assertEqual(properties['d'], 'd')
+        self.assertFalse('spam' in properties)
+
     def test_backup(self):
         """Can't backup an instance which is already being backed up."""
         instance = self._create_fake_instance()
diff --git a/nova/tests/test_xenapi.py b/nova/tests/test_xenapi.py
index 5f967862b..adacffbb5 100644
--- a/nova/tests/test_xenapi.py
+++ b/nova/tests/test_xenapi.py
@@ -2296,9 +2296,6 @@ class VmUtilsTestCase(test.TestCase):
     """Unit tests for xenapi utils."""
 
     def test_upload_image(self):
-        """Ensure image properties include instance system metadata
-           as well as few local settings."""
-
         def fake_instance_system_metadata_get(context, uuid):
             return dict(image_a=1, image_b=2, image_c='c', d='d')
 
@@ -2337,8 +2334,8 @@ class VmUtilsTestCase(test.TestCase):
         vm_utils.upload_image(ctx, session, instance, "vmi uuids", "image id")
 
         actual = self.kwargs['properties']
-        expected = dict(a=1, b=2, c='c', d='d',
-                        auto_disk_config='auto disk config',
+        # Inheritance happens in another place, now
+        expected = dict(auto_disk_config='auto disk config',
                         os_type='os type')
         self.assertEquals(expected, actual)
 
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index bd8fad474..69dd77a87 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -684,19 +684,10 @@ def upload_image(context, session, instance, vdi_uuids, image_id):
     glance_api_servers = glance.get_api_servers()
     glance_host, glance_port, glance_use_ssl = glance_api_servers.next()
 
-    # TODO(sirp): this inherit-image-property code should probably go in
-    # nova/compute/manager so it can be shared across hypervisors
-    sys_meta = db.instance_system_metadata_get(context, instance['uuid'])
-    properties = {}
-    prefix = 'image_'
-    for key, value in sys_meta.iteritems():
-        if key.startswith(prefix):
-            key = key[len(prefix):]
-        if key in FLAGS.non_inheritable_image_properties:
-            continue
-        properties[key] = value
-    properties['auto_disk_config'] = instance['auto_disk_config']
-    properties['os_type'] = instance['os_type'] or FLAGS.default_os_type
+    properties = {
+        'auto_disk_config': instance['auto_disk_config'],
+        'os_type': instance['os_type'] or FLAGS.default_os_type,
+    }
 
     params = {'vdi_uuids': vdi_uuids,
               'image_id': image_id,