Addressing Rick Clark's comments.

9 files changed, 95 insertions, 89 deletions

diff --git a/nova/tests/db/fakes.py b/nova/tests/db/fakes.py
index 62c7cd794..c46b75aa2 100644
--- a/nova/tests/db/fakes.py
+++ b/nova/tests/db/fakes.py
@@ -99,9 +99,7 @@ def stub_out_db_instance_api(stubs, injected=True):
         return FakeModel(network_fields)
 
     def fake_network_get_all_by_instance(context, instance_id):
-        l = []
-        l.append(FakeModel(network_fields))
-        return l
+        return [FakeModel(network_fields)]
 
     def fake_instance_get_fixed_address(context, instance_id):
         return FakeModel(fixed_ip_fields).address
diff --git a/nova/tests/fake_utils.py b/nova/tests/fake_utils.py
index 8982f50be..823c775cb 100644
--- a/nova/tests/fake_utils.py
+++ b/nova/tests/fake_utils.py
@@ -33,7 +33,6 @@ _fake_execute_log = []
 
 
 def fake_execute_get_log():
-    global _fake_execute_log
     return _fake_execute_log
 
 
@@ -55,7 +54,7 @@ def fake_execute_default_reply_handler(*ignore_args, **ignore_kwargs):
     return '', ''
 
 
-def fake_execute(*cmd, **kwargs):
+def fake_execute(*cmd_parts, **kwargs):
     """This function stubs out execute, optionally executing
     a preconfigued function to return expected data
     """
@@ -64,8 +63,7 @@ def fake_execute(*cmd, **kwargs):
     process_input = kwargs.get('process_input', None)
     addl_env = kwargs.get('addl_env', None)
     check_exit_code = kwargs.get('check_exit_code', 0)
-    cmd_map = map(str, cmd)
-    cmd_str = ' '.join(cmd_map)
+    cmd_str = ' '.join(str(part) for part in cmd_parts)
 
     LOG.debug(_("Faking execution of cmd (subprocess): %s"), cmd_str)
     _fake_execute_log.append(cmd_str)
@@ -78,13 +76,13 @@ def fake_execute(*cmd, **kwargs):
             LOG.debug(_('Faked command matched %s') % fake_replier[0])
             break
 
-    if isinstance(reply_handler, types.StringTypes):
+    if isinstance(reply_handler, basestring):
         # If the reply handler is a string, return it as stdout
         reply = reply_handler, ''
     else:
         try:
             # Alternative is a function, so call it
-            reply = reply_handler(cmd,
+            reply = reply_handler(cmd_parts,
                                   process_input=process_input,
                                   addl_env=addl_env,
                                   check_exit_code=check_exit_code)
@@ -92,8 +90,10 @@ def fake_execute(*cmd, **kwargs):
             LOG.debug(_('Faked command raised an exception %s' % str(e)))
             raise
 
-    LOG.debug(_("Reply to faked command is stdout='%(0)s' stderr='%(1)s'") %
-        {'0': reply[0], '1': reply[1]})
+    stdout = reply[0]
+    stderr = reply[1]
+    LOG.debug(_("Reply to faked command is stdout='%(stdout)s' "
+                "stderr='%(stderr)s'") % locals())
 
     # Replicate the sleep call in the real function
     greenthread.sleep(0)
diff --git a/nova/tests/test_xenapi.py b/nova/tests/test_xenapi.py
index b22163e9b..d31fa27ac 100644
--- a/nova/tests/test_xenapi.py
+++ b/nova/tests/test_xenapi.py
@@ -76,7 +76,6 @@ class XenAPIVolumeTestCase(test.TestCase):
         FLAGS.xenapi_connection_url = 'test_url'
         FLAGS.xenapi_connection_password = 'test_pass'
         db_fakes.stub_out_db_instance_api(self.stubs)
-        #db_fakes.stub_out_db_network_api(self.stubs)
         stubs.stub_out_get_target(self.stubs)
         xenapi_fake.reset()
         self.values = {'id': 1,
@@ -333,7 +332,8 @@ class XenAPIVMTestCase(test.TestCase):
         self.assertEquals(self.vm['HVM_boot_policy'], '')
 
     def _test_spawn(self, image_id, kernel_id, ramdisk_id,
-        instance_type="m1.large", os_type="linux", check_injection=False):
+                    instance_type="m1.large", os_type="linux",
+                    check_injection=False):
         stubs.stubout_loopingcall_start(self.stubs)
         values = {'id': 1,
                   'project_id': self.project.id,
diff --git a/nova/tests/xenapi/stubs.py b/nova/tests/xenapi/stubs.py
index d278934c6..0f559b7f9 100644
--- a/nova/tests/xenapi/stubs.py
+++ b/nova/tests/xenapi/stubs.py
@@ -139,9 +139,9 @@ def stubout_is_vdi_pv(stubs):
 
 
 def stubout_loopingcall_start(stubs):
-    def f_1(self, interval, now=True):
+    def fake_start(self, interval, now=True):
         self.f(*self.args, **self.kw)
-    stubs.Set(utils.LoopingCall, 'start', f_1)
+    stubs.Set(utils.LoopingCall, 'start', fake_start)
 
 
 class FakeSessionForVMTests(fake.SessionBase):
diff --git a/nova/virt/disk.py b/nova/virt/disk.py
index eae667c01..ee6d3e36a 100644
--- a/nova/virt/disk.py
+++ b/nova/virt/disk.py
@@ -166,12 +166,13 @@ def _free_device(device):
 
 
 def get_injectables(inst, template=None, template_data=None):
-    #load cheetah.template if necessary
+    # Note(salvatore-orlando):
+    # it the caller does not provide template object and data
+    # we will import the Cheetah template module and load the
+    # data from the file specified by injected_network_template flag
     if not template:
-        t = __import__('Cheetah.Template', globals(), locals(), ['Template'],
-                       -1)
+        from Cheetah import Template as t
         template = t.Template
-    #load template file if necessary
     if not template_data:
         template_data = open(FLAGS.injected_network_template).read()
 
diff --git a/nova/virt/xenapi/fake.py b/nova/virt/xenapi/fake.py
index 60db86ecd..18d558058 100644
--- a/nova/virt/xenapi/fake.py
+++ b/nova/virt/xenapi/fake.py
@@ -427,7 +427,6 @@ class SessionBase(object):
                 if (field in _db_content[cls][ref]):
                     return _db_content[cls][ref][field]
             else:
-                LOG.debug(_('Raising Failure'))
                 raise Failure(['HANDLE_INVALID', cls, ref])
 
         LOG.debug(_('Raising NotImplemented'))
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index 7b6f2b5da..d429dae3c 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -683,65 +683,12 @@ class VMHelper(HelperBase):
         # everything
         mount_required = False
         key, net = disk.get_injectables(instance)
-        if key is not None or net is not None:
-            mount_required = True
-
-        if mount_required:
-
-            def _mounted_processing(device):
-                """Callback which runs with the image VDI attached"""
+        mount_required = key or net
+        if not mount_required:
+            return
 
-                dev_path = '/dev/' + device + '1'  # NB: Partition 1 hardcoded
-                tmpdir = tempfile.mkdtemp()
-                try:
-                    # Mount only Linux filesystems, to avoid disturbing
-                    # NTFS images
-                    try:
-                        out, err = utils.execute('sudo', 'mount',
-                                                 '-t', 'ext2,ext3',
-                                                 dev_path, tmpdir)
-                    except exception.ProcessExecutionError as e:
-                        err = str(e)
-                    if err:
-                        LOG.info(_('Failed to mount filesystem (expected for '
-                            'non-linux instances): %s') % err)
-                    else:
-                        try:
-                            # This try block ensures that the umount occurs
-                            xe_guest_agent_filename = os.path.join(
-                                tmpdir, FLAGS.xenapi_agent_path)
-                            if os.path.isfile(xe_guest_agent_filename):
-                                # The presence of the guest agent
-                                # file indicates that this instance can
-                                # reconfigure the network from xenstore data,
-                                # so manipulation of files in /etc is not
-                                # required
-                                LOG.info(_('XenServer tools installed in this '
-                                    'image are capable of network injection.  '
-                                    'Networking files will not be'
-                                    'manipulated'))
-                            else:
-                                xe_daemon_filename = os.path.join(tmpdir,
-                                    'usr', 'sbin', 'xe-daemon')
-                                if os.path.isfile(xe_daemon_filename):
-                                    LOG.info(_('XenServer tools are present '
-                                        'in this image but are not capable '
-                                        'of network injection'))
-                                else:
-                                    LOG.info(_('XenServer tools are not '
-                                        'installed in this image'))
-                                LOG.info(_('Manipulating interface files '
-                                         'directly'))
-                                disk.inject_data_into_fs(tmpdir, key, net,
-                                    utils.execute)
-                        finally:
-                            utils.execute('sudo', 'umount', dev_path)
-                finally:
-                    # remove temporary directory
-                    os.rmdir(tmpdir)
-
-            with_vdi_attached_here(session, vdi_ref, False,
-                _mounted_processing)
+        with_vdi_attached_here(session, vdi_ref, False,
+                               lambda dev: _mounted_processing(dev, key, net))
 
     @classmethod
     def lookup_kernel_ramdisk(cls, session, vm):
@@ -1077,3 +1024,65 @@ def _write_partition(virtual_size, dev):
 def get_name_label_for_image(image):
     # TODO(sirp): This should eventually be the URI for the Glance image
     return _('Glance image %s') % image
+
+
+def _mount_filesystem(dev_path, dir):
+    """mounts the device specified by dev_path in dir"""
+    try:
+        out, err = utils.execute('sudo', 'mount',
+                                 '-t', 'ext2,ext3',
+                                 dev_path, dir)
+    except exception.ProcessExecutionError as e:
+        err = str(e)
+    return err
+
+
+def _find_guest_agent(base_dir, agent_rel_path):
+    agent_path = os.path.join(base_dir, agent_rel_path)
+    if os.path.isfile(agent_path):
+        # The presence of the guest agent
+        # file indicates that this instance can
+        # reconfigure the network from xenstore data,
+        # so manipulation of files in /etc is not
+        # required
+        LOG.info(_('XenServer tools installed in this '
+                'image are capable of network injection.  '
+                'Networking files will not be'
+                'manipulated'))
+        return True
+    xe_daemon_filename = os.path.join(base_dir,
+        'usr', 'sbin', 'xe-daemon')
+    if os.path.isfile(xe_daemon_filename):
+        LOG.info(_('XenServer tools are present '
+                'in this image but are not capable '
+                'of network injection'))
+    else:
+        LOG.info(_('XenServer tools are not '
+                'installed in this image'))
+    return False
+
+
+def _mounted_processing(device, key, net):
+    """Callback which runs with the image VDI attached"""
+
+    dev_path = '/dev/' + device + '1'  # NB: Partition 1 hardcoded
+    tmpdir = tempfile.mkdtemp()
+    try:
+        # Mount only Linux filesystems, to avoid disturbing NTFS images
+        err = _mount_filesystem(dev_path, tmpdir)
+        if not err:
+            try:
+                # This try block ensures that the umount occurs
+                if not _find_guest_agent(tmpdir, FLAGS.xenapi_agent_path):
+                    LOG.info(_('Manipulating interface files '
+                            'directly'))
+                    disk.inject_data_into_fs(tmpdir, key, net,
+                        utils.execute)
+            finally:
+                utils.execute('sudo', 'umount', dev_path)
+        else:
+            LOG.info(_('Failed to mount filesystem (expected for '
+                'non-linux instances): %s') % err)
+    finally:
+        # remove temporary directory
+        os.rmdir(tmpdir)
diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py
index 59c5f3c13..e3810e218 100644
--- a/nova/virt/xenapi/vmops.py
+++ b/nova/virt/xenapi/vmops.py
@@ -169,7 +169,6 @@ class VMOps(object):
 
         def _wait_for_boot():
             try:
-                LOG.debug("ENTERING WAIT FOR BOOT!")
                 state = self.get_info(instance_name)['state']
                 db.instance_set_state(context.get_admin_context(),
                                       instance['id'], state)
diff --git a/nova/virt/xenapi_conn.py b/nova/virt/xenapi_conn.py
index 4f31f1071..7c3d3544f 100644
--- a/nova/virt/xenapi_conn.py
+++ b/nova/virt/xenapi_conn.py
@@ -107,17 +107,17 @@ flags.DEFINE_integer('xenapi_vhd_coalesce_max_attempts',
                      'Max number of times to poll for VHD to coalesce.'
                      '  Used only if connection_type=xenapi.')
 flags.DEFINE_bool('xenapi_inject_image',
-                     True,
-                     'Specifies whether an attempt to inject network/key'
-                     '  data into the disk image should be made.'
-                     '  Used only if connection_type=xenapi.')
+                  True,
+                  'Specifies whether an attempt to inject network/key'
+                  '  data into the disk image should be made.'
+                  '  Used only if connection_type=xenapi.')
 flags.DEFINE_string('xenapi_agent_path',
-                     'usr/sbin/xe-update-networking',
-                     'Specifies the path in which the xenapi guest agent'
-                     '  should be located. If the agent is present,'
-                     '  network configuration if not injected into the image'
-                     '  Used only if connection_type=xenapi.'
-                     '  and xenapi_inject_image=True')
+                    'usr/sbin/xe-update-networking',
+                    'Specifies the path in which the xenapi guest agent'
+                    '  should be located. If the agent is present,'
+                    '  network configuration if not injected into the image'
+                    '  Used only if connection_type=xenapi.'
+                    '  and xenapi_inject_image=True')
 
 flags.DEFINE_string('xenapi_sr_base_path', '/var/run/sr-mount',
                     'Base path to the storage repository')