Repository: Azure/WALinuxAgent
Pull request #1953 (2020-07-17 19:07:04)

Disable only when enabled for version update
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description

Fixed bug to further feature parity with the windows guest agent; during version update, `disableCommand` is now only called of the preexisting extension is in an `ExtHandlerState.Enabled` state.

This change broke some tests in `tests.ga.test_extension.TestExtensionUpdateOnFailure` that did not correctly set the preexisting extension's state before simulating an upgrade. This PR addresses this by manually setting the extension's state, but this isn't an ideal solution; the root cause of these tests breaking is that they call a private method inside of `ga.exthandlers.ExtHandlersHandler` instead of the `run` method of that same class. The private method relies on state assigned by earlier calls in `run`, and this is something that should probably be revisited in the future.


Issue # <!-- if any -->
<!--
Please add an informative description that covers that changes made by the pull request. 
This checklist is used to make sure that common issues in a pull request are addressed.
This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
-->

---

### PR information
- [ ] The title of the PR is clear and informative.
- [ ] There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, [see this page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [ ] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [ ] If applicable, the PR references the bug/issue that it fixes in the description.
- [ ] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [ ] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).


Patch:
diff --git a/azurelinuxagent/ga/exthandlers.py b/azurelinuxagent/ga/exthandlers.py
index 74c93192..fa87054d 100644
--- a/azurelinuxagent/ga/exthandlers.py
+++ b/azurelinuxagent/ga/exthandlers.py
@@ -516,8 +516,13 @@ class ExtHandlersHandler(object):
                 logger.info("Continue on Update failure flag is set, proceeding with update")
             return exit_code
 
-        disable_exit_code = execute_old_handler_command_and_return_if_succeeds(
-            func=lambda: old_ext_handler_i.disable())
+        disable_exit_code = NOT_RUN
+        # We only want to disable the old handler if it is currently enabled; no
+        # other state makes sense.
+        if old_ext_handler_i.get_handler_state() == ExtHandlerState.Enabled:
+            disable_exit_code = execute_old_handler_command_and_return_if_succeeds(
+                func=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(disable_exit_code=disable_exit_code,


Test patch:
diff --git a/tests/ga/test_extension.py b/tests/ga/test_extension.py
index c6cf0cf4..5c5f44f0 100644
--- a/tests/ga/test_extension.py
+++ b/tests/ga/test_extension.py
@@ -2580,6 +2580,7 @@ class TestExtensionUpdateOnFailure(ExtensionTestCase):
     def test_uninstall_failed_env_variable_should_set_for_install_when_continue_on_update_failure_is_true(
             self, *args):
         old_handler_i = self._get_ext_handler_instance('foo', '1.0.0')
+        old_handler_i.set_handler_state(ExtHandlerState.Enabled)
         new_handler_i = self._get_ext_handler_instance('foo', '1.0.1', continue_on_update_failure=True)
 
         with patch.object(CGroupConfigurator.get_instance(), "start_extension_command",
@@ -2594,6 +2595,7 @@ class TestExtensionUpdateOnFailure(ExtensionTestCase):
 
     def test_extension_error_should_be_raised_when_continue_on_update_failure_is_false_on_disable_failure(self, *args):
         old_handler_i = self._get_ext_handler_instance('foo', '1.0.0')
+        old_handler_i.set_handler_state(ExtHandlerState.Enabled)
         new_handler_i = self._get_ext_handler_instance('foo', '1.0.1', continue_on_update_failure=False)
 
         with patch.object(ExtHandlerInstance, "disable", side_effect=ExtensionError("Disable Failed")):
@@ -2645,6 +2647,7 @@ class TestExtensionUpdateOnFailure(ExtensionTestCase):
     @patch("azurelinuxagent.common.cgroupconfigurator.handle_process_completion", side_effect="Process Successful")
     def test_env_variable_should_not_set_when_continue_on_update_failure_is_false(self, *args):
         old_handler_i = self._get_ext_handler_instance('foo', '1.0.0')
+        old_handler_i.set_handler_state(ExtHandlerState.Enabled)
         new_handler_i = self._get_ext_handler_instance('foo', '1.0.1', continue_on_update_failure=False)
 
         # When Disable Fails
@@ -2723,6 +2726,8 @@ class TestExtensionUpdateOnFailure(ExtensionTestCase):
         old_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance('foo', '1.0.0', handler={
             "disableCommand": test_failure_file_name,
             "uninstallCommand": test_failure_file_name})
+        old_handler_i.set_handler_state(ExtHandlerState.Enabled)
+
         new_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance(
             'foo', '1.0.1',
             handler={"updateCommand": test_env_file_name,
@@ -2759,6 +2764,7 @@ class TestExtensionUpdateOnFailure(ExtensionTestCase):
         old_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance('foo', '1.0.0', handler={
             "disableCommand": test_failure_file_name,
             "uninstallCommand": test_failure_file_name})
+        old_handler_i.set_handler_state(ExtHandlerState.Enabled)
         new_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance(
             'foo', '1.0.1',
             handler={"updateCommand": test_env_file_name + " -u", "installCommand": test_env_file_name + " -i"},
@@ -2802,6 +2808,7 @@ class TestExtensionUpdateOnFailure(ExtensionTestCase):
         old_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance('foo', '1.0.0', handler={
             "disableCommand": test_success_file_name,
             "uninstallCommand": test_success_file_name})
+        old_handler_i.set_handler_state(ExtHandlerState.Enabled)
         new_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance(
             'foo', '1.0.1',
             handler={"updateCommand": test_env_file_name + " -u", "installCommand": test_env_file_name + " -i"},
@@ -2834,6 +2841,19 @@ class TestExtensionUpdateOnFailure(ExtensionTestCase):
             self.assertTrue(test_env_file_name + " -u" in update_kwargs['message'] and "%s=%s" % (
                 ExtCommandEnvVariable.DisableReturnCode, exit_code) in update_kwargs['message'])
 
+    @patch('time.sleep', side_effect=lambda _: mock_sleep(0.0001))
+    def test_disable_should_not_be_called_during_version_upgrade_if_not_enabled(self, _):
+
+        old_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance('foo', '1.0.0')
+        old_handler_i.set_handler_state(ExtHandlerState.Installed)  # Something other than Enabled.
+        new_handler_i = TestExtensionUpdateOnFailure._get_ext_handler_instance('foo', '1.0.1', continue_on_update_failure=False)
+
+        with patch.object(CGroupConfigurator.get_instance(), "start_extension_command", return_value="[stdout]\n\n\n[stderr]\n\n\n"):
+            with patch.object(old_handler_i, 'disable', autospec=True) as mock_disable:
+                uninstall_rc = ExtHandlersHandler._update_extension_handler_and_return_if_failed(old_handler_i,
+                                                                                                new_handler_i)
+                mock_disable.mock.assert_not_called() # Python2.6's mock library doesn't forward assert_not_called, so we have to do it ourselves.
+
 
 @patch('time.sleep', side_effect=lambda _: mock_sleep(0.001))
 class TestCollectExtensionStatus(ExtensionTestCase):