Logging a disable error instead of Download error on extension update…

azurelinuxagent/common/exception.py

@@ -67,6 +67,15 @@ def __init__(self, msg=None, inner=None, code=-1):
         self.code = code
 
 
+class ExtensionUpdateError(ExtensionError):
+    """
+    When failed to update an extension
+    """
+
+    def __init__(self, msg=None, inner=None, code=-1):
+        super(ExtensionUpdateError, self).__init__(msg, inner, code)
+
+
 class ExtensionDownloadError(ExtensionError):
     """
     When failed to download and setup an extension

azurelinuxagent/ga/exthandlers.py

@@ -41,7 +41,7 @@
 from azurelinuxagent.common.errorstate import ErrorState, ERROR_STATE_DELTA_INSTALL
 from azurelinuxagent.common.event import add_event, WALAEventOperation, elapsed_milliseconds, report_event
 from azurelinuxagent.common.exception import ExtensionError, ProtocolError, ProtocolNotFoundError, \
-    ExtensionDownloadError, ExtensionOperationError, ExtensionErrorCodes
+    ExtensionDownloadError, ExtensionOperationError, ExtensionErrorCodes, ExtensionUpdateError
 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.protocol import get_protocol_util
 from azurelinuxagent.common.protocol.restapi import ExtHandlerStatus, \
@@ -436,6 +436,9 @@ def handle_ext_handler(self, ext_handler, etag):
             else:
                 message = u"Unknown ext handler state:{0}".format(state)
                 raise ExtensionError(message)
+        except ExtensionUpdateError as e:
+            # Not reporting the error as it has already been reported from the old version
+            self.handle_ext_handler_error(ext_handler_i, e, e.code, report_telemetry_event=False)
         except ExtensionOperationError as e:
             self.handle_ext_handler_error(ext_handler_i, e, e.code)
         except ExtensionDownloadError as e:
@@ -445,10 +448,12 @@ def handle_ext_handler(self, ext_handler, etag):
         except Exception as e:
             self.handle_ext_handler_error(ext_handler_i, e)
 
-    def handle_ext_handler_error(self, ext_handler_i, e, code=-1):
+    def handle_ext_handler_error(self, ext_handler_i, e, code=-1, report_telemetry_event=True):
         msg = ustr(e)
         ext_handler_i.set_handler_status(message=msg, code=code)
-        ext_handler_i.report_event(message=msg, is_success=False, log_event=True)
+
+        if report_telemetry_event:
+            ext_handler_i.report_event(message=msg, is_success=False, log_event=True)
 
     def handle_ext_handler_download_error(self, ext_handler_i, e, code=-1):
         msg = ustr(e)
@@ -476,20 +481,41 @@ def handle_enable(self, ext_handler_i):
             if old_ext_handler_i is None:
                 ext_handler_i.install()
             elif ext_handler_i.version_ne(old_ext_handler_i):
-                old_ext_handler_i.disable()
-                ext_handler_i.copy_status_files(old_ext_handler_i)
-                if ext_handler_i.version_gt(old_ext_handler_i):
-                    ext_handler_i.update()
-                else:
-                    old_ext_handler_i.update(version=ext_handler_i.ext_handler.properties.version)
-                old_ext_handler_i.uninstall()
-                old_ext_handler_i.remove_ext_handler()
-                ext_handler_i.update_with_install()
+                ExtHandlersHandler._update_extension_handler(old_ext_handler_i, ext_handler_i)
         else:
             ext_handler_i.update_settings()
 
         ext_handler_i.enable()
 
+    @staticmethod
+    def _update_extension_handler(old_ext_handler_i, ext_handler_i):
+
+        def execute_old_handler_command(func):
+            """
+            Created a common wrapper to execute all commands that need to be executed from the old handler
+            so that it can have a common exception handling mechanism
+            :param func: The command to be executed on the old handler
+            """
+            try:
+                func()
+            except ExtensionError as e:
+                # Reporting the event with the old handler and raising a new ExtensionUpdateError to set the
+                # handler status on the new version
+                msg = ustr(e)
+                old_ext_handler_i.report_event(message=msg, is_success=False)
+                raise ExtensionUpdateError(msg)
+
+        execute_old_handler_command(lambda: old_ext_handler_i.disable())
+        ext_handler_i.copy_status_files(old_ext_handler_i)
+        if ext_handler_i.version_gt(old_ext_handler_i):
+            ext_handler_i.update()
+        else:
+            old_ext_handler_i.update(version=ext_handler_i.ext_handler.properties.version)
+        # Currently uninstall failures swallows the exception so we wont catch anything here
+        execute_old_handler_command(lambda: old_ext_handler_i.uninstall())
+        old_ext_handler_i.remove_ext_handler()
+        ext_handler_i.update_with_install()
+
     def handle_disable(self, ext_handler_i):
         self.log_process = True
         handler_state = ext_handler_i.get_handler_state()

tests/ga/test_extension.py

@@ -1460,6 +1460,52 @@ def test__extension_upgrade_failure_when_prev_version_disable_fails_incorrect_zi
         # On the next iteration, update should not be retried
         self._assert_handler_status(protocol.report_vm_status, "NotReady", expected_ext_count=0, version="1.0.1")
 
+    @patch('azurelinuxagent.ga.exthandlers.HandlerManifest.get_disable_command')
+    def test__old_handler_reports_failure_on_disable_fail_on_update(self, patch_get_disable_command, *args):
+        test_data = WireProtocolData(DATA_FILE_EXT_SINGLE)
+        exthandlers_handler, protocol = self._create_mock(test_data, *args)
+        old_version, new_version = "1.0.0", "1.0.1"
+
+        # Ensure initial install and enable is successful
+        exthandlers_handler.run()
+
+        self.assertEqual(0, patch_get_disable_command.call_count)
+
+        self._assert_handler_status(protocol.report_vm_status, "Ready", expected_ext_count=1, version=old_version)
+        self._assert_ext_status(protocol.report_ext_status, "success", 0)
+
+        # Next incarnation, update version
+        test_data.goal_state = test_data.goal_state.replace("<Incarnation>1<", "<Incarnation>2<")
+        test_data.ext_conf = test_data.ext_conf.replace('version="%s"' % old_version, 'version="%s"' % new_version)
+        test_data.manifest = test_data.manifest.replace(old_version, new_version)
+
+        with patch.object(ExtHandlerInstance, "report_event", autospec=True) as patch_report_event:
+            # Disable of the old extn fails
+            patch_get_disable_command.return_value = "exit 1"
+            exthandlers_handler.run()  # Download the new update the first time, and then we patch the download method.
+            self.assertEqual(1, patch_get_disable_command.call_count)
+
+            old_version_args, old_version_kwargs = patch_report_event.call_args
+            new_version_args, new_version_kwargs = patch_report_event.call_args_list[0]
+
+            self.assertEqual(new_version_args[0].ext_handler.properties.version, new_version,
+                             "The first call to report event should be from the new version of the ext-handler "
+                             "to report download succeeded")
+
+            self.assertEqual(new_version_kwargs['message'], "Download succeeded",
+                             "The message should be Download Succedded")
+
+            self.assertEqual(old_version_args[0].ext_handler.properties.version, old_version,
+                             "The last report event call should be from the old version ext-handler "
+                             "to report the event from the previous version")
+
+            self.assertFalse(old_version_kwargs['is_success'], "The last call to report event should be for a failure")
+
+            self.assertTrue(old_version_kwargs['message'].startswith('[ExtensionError]'), "No error reported")
+
+            # This is ensuring that the error status is being written to the new version
+        self._assert_handler_status(protocol.report_vm_status, "NotReady", expected_ext_count=0, version=new_version)
+
     @patch('azurelinuxagent.ga.exthandlers.ExtHandlersHandler.handle_ext_handler_error')
     @patch('azurelinuxagent.ga.exthandlers.HandlerManifest.get_update_command')
     def test_upgrade_failure_with_exception_handling(self, patch_get_update_command,