ensure failure to inject user files results in startup error

This was the case in Essex but was inadvertantly changed in:
folsom-2-95-g0d166ca.

* nova/virt/disk/api.py: Refactor to allow specifying
mandatory injection items, that result in an exception
on failure to inject.
* nova/virt/libvirt/driver.py: Specify that user 'files'
are mandatory items and thus result in VM startup failure
unless injected successfully.
* nova/tests/test_virt_disk.py: A new test for the
separate warning and error cases.

Fixes bug: 1095744
Change-Id: Idab5c4294c1cb52098ce44a7aae957a44fb2674f

3 files changed, 82 insertions, 30 deletions

diff --git a/nova/tests/test_virt_disk.py b/nova/tests/test_virt_disk.py
index 902d49704..e6a57e085 100644
--- a/nova/tests/test_virt_disk.py
+++ b/nova/tests/test_virt_disk.py
@@ -14,8 +14,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import os
 import sys
 
+from nova import exception
 from nova import test
 from nova.tests import fakeguestfs
 from nova.virt.disk import api as diskapi
@@ -29,6 +31,25 @@ class VirtDiskTest(test.TestCase):
         sys.modules['guestfs'] = fakeguestfs
         vfsguestfs.guestfs = fakeguestfs
 
+    def test_inject_data(self):
+
+        self.assertTrue(diskapi.inject_data("/some/file", use_cow=True))
+
+        self.assertTrue(diskapi.inject_data("/some/file",
+                                            mandatory=('files',)))
+
+        self.assertTrue(diskapi.inject_data("/some/file", key="mysshkey",
+                                            mandatory=('key',)))
+
+        os_name = os.name
+        os.name = 'nt'  # Cause password injection to fail
+        self.assertRaises(exception.NovaException,
+                          diskapi.inject_data,
+                          "/some/file", admin_password="p",
+                          mandatory=('admin_password',))
+        self.assertFalse(diskapi.inject_data("/some/file", admin_password="p"))
+        os.name = os_name
+
     def test_inject_data_key(self):
 
         vfs = vfsguestfs.VFSGuestFS("/some/file", "qcow2")
diff --git a/nova/virt/disk/api.py b/nova/virt/disk/api.py
index 26fb86f1e..dd255bac1 100644
--- a/nova/virt/disk/api.py
+++ b/nova/virt/disk/api.py
@@ -267,14 +267,19 @@ class _DiskImage(object):
 # Public module functions
 
 def inject_data(image, key=None, net=None, metadata=None, admin_password=None,
-                files=None, partition=None, use_cow=False):
-    """Injects a ssh key and optionally net data into a disk image.
+                files=None, partition=None, use_cow=False, mandatory=()):
+    """Inject the specified items into a disk image.
+
+    If an item name is not specified in the MANDATORY iterable, then a warning
+    is logged on failure to inject that item, rather than raising an exception.
 
     it will mount the image as a fully partitioned disk and attempt to inject
     into the specified partition number.
 
-    If partition is not specified it mounts the image as a single partition.
+    If PARTITION is not specified the image is mounted as a single partition.
 
+    Returns True if all requested operations completed without issue.
+    Raises an exception if a mandatory item can't be injected.
     """
     LOG.debug(_("Inject data image=%(image)s key=%(key)s net=%(net)s "
                 "metadata=%(metadata)s admin_password=ha-ha-not-telling-you "
@@ -283,11 +288,23 @@ def inject_data(image, key=None, net=None, metadata=None, admin_password=None,
     fmt = "raw"
     if use_cow:
         fmt = "qcow2"
-    fs = vfs.VFS.instance_for_image(image, fmt, partition)
-    fs.setup()
     try:
-        inject_data_into_fs(fs,
-                            key, net, metadata, admin_password, files)
+        fs = vfs.VFS.instance_for_image(image, fmt, partition)
+        fs.setup()
+    except Exception as e:
+        # If a mandatory item is passed to this function,
+        # then reraise the exception to indicate the error.
+        for inject in mandatory:
+            inject_val = locals()[inject]
+            if inject_val:
+                raise
+        LOG.warn(_('Ignoring error injecting data into image '
+                   '(%(e)s)') % locals())
+        return False
+
+    try:
+        return inject_data_into_fs(fs, key, net, metadata,
+                                   admin_password, files, mandatory)
     finally:
         fs.teardown()
 
@@ -320,22 +337,37 @@ def teardown_container(container_dir):
         LOG.exception(_('Failed to unmount container filesystem: %s'), exn)
 
 
-def inject_data_into_fs(fs, key, net, metadata, admin_password, files):
+def inject_data_into_fs(fs, key, net, metadata, admin_password, files,
+                        mandatory=()):
     """Injects data into a filesystem already mounted by the caller.
     Virt connections can call this directly if they mount their fs
-    in a different way to inject_data
+    in a different way to inject_data.
+
+    If an item name is not specified in the MANDATORY iterable, then a warning
+    is logged on failure to inject that item, rather than raising an exception.
+
+    Returns True if all requested operations completed without issue.
+    Raises an exception if a mandatory item can't be injected.
     """
-    if key:
-        _inject_key_into_fs(key, fs)
-    if net:
-        _inject_net_into_fs(net, fs)
-    if metadata:
-        _inject_metadata_into_fs(metadata, fs)
-    if admin_password:
-        _inject_admin_password_into_fs(admin_password, fs)
-    if files:
-        for (path, contents) in files:
-            _inject_file_into_fs(fs, path, contents)
+    status = True
+    for inject in ('key', 'net', 'metadata', 'admin_password', 'files'):
+        inject_val = locals()[inject]
+        inject_func = globals()['_inject_%s_into_fs' % inject]
+        if inject_val:
+            try:
+                inject_func(inject_val, fs)
+            except Exception as e:
+                if inject in mandatory:
+                    raise
+                LOG.warn(_('Ignoring error injecting %(inject)s into image '
+                           '(%(e)s)') % locals())
+                status = False
+    return status
+
+
+def _inject_files_into_fs(files, fs):
+    for (path, contents) in files:
+        _inject_file_into_fs(fs, path, contents)
 
 
 def _inject_file_into_fs(fs, path, contents, append=False):
diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py
index 9931f8e4c..7c8abc6ec 100644
--- a/nova/virt/libvirt/driver.py
+++ b/nova/virt/libvirt/driver.py
@@ -1403,22 +1403,21 @@ class LibvirtDriver(driver.ComputeDriver):
             injection_path = image('disk').path
             img_id = instance['image_ref']
 
-            for injection in ('metadata', 'key', 'net', 'admin_pass',
-                              'files'):
-                if locals()[injection]:
-                    LOG.info(_('Injecting %(injection)s into image'
+            for inject in ('key', 'net', 'metadata', 'admin_pass', 'files'):
+                if locals()[inject]:
+                    LOG.info(_('Injecting %(inject)s into image'
                                ' %(img_id)s'), locals(), instance=instance)
             try:
                 disk.inject_data(injection_path,
                                  key, net, metadata, admin_pass, files,
                                  partition=target_partition,
-                                 use_cow=CONF.use_cow_images)
-
+                                 use_cow=CONF.use_cow_images,
+                                 mandatory=('files',))
             except Exception as e:
-                # This could be a windows image, or a vmdk format disk
-                LOG.warn(_('Ignoring error injecting data into image '
-                           '%(img_id)s (%(e)s)') % locals(),
-                         instance=instance)
+                LOG.error(_('Error injecting data into image '
+                            '%(img_id)s (%(e)s)') % locals(),
+                            instance=instance)
+                raise
 
         if CONF.libvirt_type == 'uml':
             libvirt_utils.chown(image('disk').path, 'root')