Merge lp:~johannes.erdfelt/nova/lp821046 into lp:~hudson-openstack/nova/trunk

[Johannes Erdfelt]

Be more tolerant of agent failures. It is often the case there is only a problem with the agent, not with the instance, so don't claim it failed to boot so quickly.

[Rick Harris (rconradharris)]

lgtm.

review: Approve

[Matt Dietz (cerberus)]

ditto

review: Approve

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-08-08 20:06:54 +0000
+++ nova/virt/xenapi/vmops.py	2011-08-09 17:41:24 +0000
@@ -282,6 +282,7 @@
                         'architecture': instance.architecture})
 
         def _check_agent_version():
+            LOG.debug(_("Querying agent version"))
             if instance.os_type == 'windows':
                 # Windows will generally perform a setup process on first boot
                 # that can take a couple of minutes and then reboot. So we
@@ -292,7 +293,6 @@
             else:
                 version = self.get_agent_version(instance)
             if not version:
-                LOG.info(_('No agent version returned by instance'))
                 return
 
             LOG.info(_('Instance agent version: %s') % version)
@@ -327,6 +327,10 @@
                 LOG.debug(_("Setting admin password"))
                 self.set_admin_password(instance, admin_password)
 
+        def _reset_network():
+            LOG.debug(_("Resetting network"))
+            self.reset_network(instance, vm_ref)
+
         # NOTE(armando): Do we really need to do this in virt?
         # NOTE(tr3buchet): not sure but wherever we do it, we need to call
         #                  reset_network afterwards
@@ -341,7 +345,7 @@
                     _check_agent_version()
                     _inject_files()
                     _set_admin_password()
-                    self.reset_network(instance, vm_ref)
+                    _reset_network()
                     return True
             except Exception, exc:
                 LOG.warn(exc)
@@ -597,13 +601,13 @@
             transaction_id = str(uuid.uuid4())
             args = {'id': transaction_id}
             resp = self._make_agent_call('version', instance, '', args)
-            if resp is None:
-                # No response from the agent
-                return
-            resp_dict = json.loads(resp)
+            if resp['returncode'] != '0':
+                LOG.error(_('Failed to query agent version: %(resp)r') %
+                          locals())
+                return None
             # Some old versions of the Windows agent have a trailing \\r\\n
             # (ie CRLF escaped) for some reason. Strip that off.
-            return resp_dict['message'].replace('\\r\\n', '')
+            return resp['message'].replace('\\r\\n', '')
 
         if timeout:
             vm_ref = self._get_vm_opaque_ref(instance)
@@ -634,13 +638,10 @@
         transaction_id = str(uuid.uuid4())
         args = {'id': transaction_id, 'url': url, 'md5sum': md5sum}
         resp = self._make_agent_call('agentupdate', instance, '', args)
-        if resp is None:
-            # No response from the agent
-            return
-        resp_dict = json.loads(resp)
-        if resp_dict['returncode'] != '0':
-            raise RuntimeError(resp_dict['message'])
-        return resp_dict['message']
+        if resp['returncode'] != '0':
+            LOG.error(_('Failed to update agent: %(resp)r') % locals())
+            return None
+        return resp['message']
 
     def set_admin_password(self, instance, new_pass):
         """Set the root/admin password on the VM instance.
@@ -659,18 +660,13 @@
         key_init_args = {'id': key_init_transaction_id,
                          'pub': str(dh.get_public())}
         resp = self._make_agent_call('key_init', instance, '', key_init_args)
-        if resp is None:
-            # No response from the agent
-            return
-        resp_dict = json.loads(resp)
         # Successful return code from key_init is 'D0'
-        if resp_dict['returncode'] != 'D0':
-            # There was some sort of error; the message will contain
-            # a description of the error.
-            raise RuntimeError(resp_dict['message'])
+        if resp['returncode'] != 'D0':
+            LOG.error(_('Failed to exchange keys: %(resp)r') % locals())
+            return None
         # Some old versions of the Windows agent have a trailing \\r\\n
         # (ie CRLF escaped) for some reason. Strip that off.
-        agent_pub = int(resp_dict['message'].replace('\\r\\n', ''))
+        agent_pub = int(resp['message'].replace('\\r\\n', ''))
         dh.compute_shared(agent_pub)
         # Some old versions of Linux and Windows agent expect trailing \n
         # on password to work correctly.
@@ -679,17 +675,14 @@
         password_transaction_id = str(uuid.uuid4())
         password_args = {'id': password_transaction_id, 'enc_pass': enc_pass}
         resp = self._make_agent_call('password', instance, '', password_args)
-        if resp is None:
-            # No response from the agent
-            return
-        resp_dict = json.loads(resp)
         # Successful return code from password is '0'
-        if resp_dict['returncode'] != '0':
-            raise RuntimeError(resp_dict['message'])
+        if resp['returncode'] != '0':
+            LOG.error(_('Failed to update password: %(resp)r') % locals())
+            return None
         db.instance_update(nova_context.get_admin_context(),
                                   instance['id'],
                                   dict(admin_pass=new_pass))
-        return resp_dict['message']
+        return resp['message']
 
     def inject_file(self, instance, path, contents):
         """Write a file to the VM instance.
@@ -712,12 +705,10 @@
         # If the agent doesn't support file injection, a NotImplementedError
         # will be raised with the appropriate message.
         resp = self._make_agent_call('inject_file', instance, '', args)
-        resp_dict = json.loads(resp)
-        if resp_dict['returncode'] != '0':
-            # There was some other sort of error; the message will contain
-            # a description of the error.
-            raise RuntimeError(resp_dict['message'])
-        return resp_dict['message']
+        if resp['returncode'] != '0':
+            LOG.error(_('Failed to inject file: %(resp)r') % locals())
+            return None
+        return resp['message']
 
     def _shutdown(self, instance, vm_ref, hard=True):
         """Shutdown an instance."""
@@ -1178,8 +1169,19 @@
 
     def _make_agent_call(self, method, vm, path, addl_args=None):
         """Abstracts out the interaction with the agent xenapi plugin."""
-        return self._make_plugin_call('agent', method=method, vm=vm,
+        ret = self._make_plugin_call('agent', method=method, vm=vm,
                 path=path, addl_args=addl_args)
+        if isinstance(ret, dict):
+            return ret
+        try:
+            return json.loads(ret)
+        except TypeError:
+            instance_id = vm.id
+            LOG.error(_('The agent call to %(method)s returned an invalid'
+                      ' response: %(ret)r. VM id=%(instance_id)s;'
+                      ' path=%(path)s; args=%(addl_args)r') % locals())
+            return {'returncode': 'error',
+                    'message': 'unable to deserialize response'}
 
     def _make_plugin_call(self, plugin, method, vm, path, addl_args=None,
                                                           vm_ref=None):
@@ -1197,20 +1199,20 @@
             ret = self._session.wait_for_task(task, instance_id)
         except self.XenAPI.Failure, e:
             ret = None
-            err_trace = e.details[-1]
-            err_msg = err_trace.splitlines()[-1]
-            strargs = str(args)
+            err_msg = e.details[-1].splitlines()[-1]
             if 'TIMEOUT:' in err_msg:
                 LOG.error(_('TIMEOUT: The call to %(method)s timed out. '
-                        'VM id=%(instance_id)s; args=%(strargs)s') % locals())
+                        'VM id=%(instance_id)s; args=%(args)r') % locals())
+                return {'returncode': 'timeout', 'message': err_msg}
             elif 'NOT IMPLEMENTED:' in err_msg:
                 LOG.error(_('NOT IMPLEMENTED: The call to %(method)s is not'
                         ' supported by the agent. VM id=%(instance_id)s;'
-                        ' args=%(strargs)s') % locals())
-                raise NotImplementedError(err_msg)
+                        ' args=%(args)r') % locals())
+                return {'returncode': 'notimplemented', 'message': err_msg}
             else:
                 LOG.error(_('The call to %(method)s returned an error: %(e)s. '
-                        'VM id=%(instance_id)s; args=%(strargs)s') % locals())
+                        'VM id=%(instance_id)s; args=%(args)r') % locals())
+                return {'returncode': 'error', 'message': err_msg}
         return ret
 
     def add_to_xenstore(self, vm, path, key, value):