Repository: Azure/WALinuxAgent
Pull request #1504 (2019-04-13 00:00:13)

Suppress 'Errno 2' messages when deleting extensions
<!-- DO NOT DELETE THIS TEMPLATE -->

DCR reports this error intermittently:

> Failed to remove extension handler directory: [Errno 2] No such file or directory: 'omsagent-1.6.1-3.universal.x64.deb'

Apparently, some extensions do part of their uninstall asynchronously so they may be deleting their files while the agent is trying to do the same.

Added code to suppress those errors (they can prevent full deletion of the extension directory).

<!--
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).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/azure/walinuxagent/1504)
<!-- Reviewable:end -->


larohra: 



___
*[tests/ga/test_exthandlers_exthandlerinstance.py, line 11 at r1](https://reviewable.io/reviews/azure/walinuxagent/1504#-LcXPIYe940LN7J-vlCj:-LcXPIYe940LN7J-vlCk:b-e7dr4m) ([raw file](https://github.com/azure/walinuxagent/blob/7cc28ebf1189d78fc48650851a3f201f424d1b8c/tests/ga/test_exthandlers_exthandlerinstance.py#L11)):*
> ```Python
> class ExtHandlerInstanceTestCase(AgentTestCase):
>     def setUp(self):
>         AgentTestCase.setUp(self)
> ```

Just curious, why do some function calls follow CamelCase conventions and some underscore convention? Is there a pattern I'm missing?


<!-- Sent from Reviewable.io -->

narrieta: 



___
*[tests/ga/test_exthandlers_exthandlerinstance.py, line 11 at r1](https://reviewable.io/reviews/azure/walinuxagent/1504#-LcXPIYe940LN7J-vlCj:-LcXRykt2w81j1BcHYaU:b9t4u5j) ([raw file](https://github.com/azure/walinuxagent/blob/7cc28ebf1189d78fc48650851a3f201f424d1b8c/tests/ga/test_exthandlers_exthandlerinstance.py#L11)):*
<details><summary><i>Previously, larohra wrote…</i></summary><blockquote>

Just curious, why do some function calls follow CamelCase conventions and some underscore convention? Is there a pattern I'm missing?
</blockquote></details>

not sure if we are being consistent (likely not), but could always try to follow https://www.python.org/dev/peps/pep-0008/#package-and-module-names


<!-- Sent from Reviewable.io -->

larohra: 



___
*[tests/ga/test_exthandlers_exthandlerinstance.py, line 11 at r1](https://reviewable.io/reviews/azure/walinuxagent/1504#-LcXPIYe940LN7J-vlCj:-LcXTtKnDBGrbORZhZqe:brckp7b) ([raw file](https://github.com/azure/walinuxagent/blob/7cc28ebf1189d78fc48650851a3f201f424d1b8c/tests/ga/test_exthandlers_exthandlerinstance.py#L11)):*
<details><summary><i>Previously, narrieta (Norberto Arrieta) wrote…</i></summary><blockquote>

not sure if we are being consistent (likely not), but could always try to follow https://www.python.org/dev/peps/pep-0008/#package-and-module-names
</blockquote></details>

Awesome sounds good


<!-- Sent from Reviewable.io -->

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504?src=pr&el=h1) Report
> Merging [#1504](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504?src=pr&el=desc) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/4c7c8925cb2a4109e9e7ba05800fe53f0b93596e?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `40%`.

[![Impacted file tree graph](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504/graphs/tree.svg?width=650&token=ae90E2KYCw&height=150&src=pr)](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           develop    #1504      +/-   ##
===========================================
- Coverage    60.82%   60.81%   -0.02%     
===========================================
  Files           75       75              
  Lines        10878    10882       +4     
  Branches      1559     1560       +1     
===========================================
+ Hits          6617     6618       +1     
- Misses        3946     3949       +3     
  Partials       315      315
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/ga/exthandlers.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2dhL2V4dGhhbmRsZXJzLnB5) | `80.64% <40%> (-0.25%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504?src=pr&el=footer). Last update [4c7c892...d4494e7](https://codecov.io/gh/Azure/WALinuxAgent/pull/1504?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

narrieta: Unit Tests are fixed - PR is ready for review.... Thanks!

Patch:
diff --git a/azurelinuxagent/ga/exthandlers.py b/azurelinuxagent/ga/exthandlers.py
index 4797ac90..8c4d31ef 100644
--- a/azurelinuxagent/ga/exthandlers.py
+++ b/azurelinuxagent/ga/exthandlers.py
@@ -921,7 +921,14 @@ class ExtHandlerInstance(object):
             if os.path.isdir(base_dir):
                 self.logger.info("Remove extension handler directory: {0}",
                                  base_dir)
-                shutil.rmtree(base_dir)
+
+                # some extensions uninstall asynchronously so ignore error 2 while removing them
+                def on_rmtree_error(_, __, exc_info):
+                    _, exception, _ = exc_info
+                    if not isinstance(exception, OSError) or exception.errno != 2:  # [Errno 2] No such file or directory
+                        raise exception
+
+                shutil.rmtree(base_dir, onerror=on_rmtree_error)
         except IOError as e:
             message = "Failed to remove extension handler directory: {0}".format(e)
             self.report_event(message=message, is_success=False)


Test patch:
diff --git a/tests/ga/test_exthandlers_exthandlerinstance.py b/tests/ga/test_exthandlers_exthandlerinstance.py
new file mode 100644
index 00000000..96352fd7
--- /dev/null
+++ b/tests/ga/test_exthandlers_exthandlerinstance.py
@@ -0,0 +1,109 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the Apache License.
+
+from azurelinuxagent.ga.exthandlers import ExtHandlerInstance
+from azurelinuxagent.common.protocol.restapi import ExtHandler, ExtHandlerProperties
+from tests.tools import *
+
+
+class ExtHandlerInstanceTestCase(AgentTestCase):
+    def setUp(self):
+        AgentTestCase.setUp(self)
+
+        ext_handler_properties = ExtHandlerProperties()
+        ext_handler_properties.version = "1.2.3"
+        ext_handler = ExtHandler(name='foo')
+        ext_handler.properties = ext_handler_properties
+        self.ext_handler_instance = ExtHandlerInstance(ext_handler=ext_handler, protocol=None)
+
+        self.extension_directory = os.path.join(self.tmp_dir, "extension_directory")
+        self.mock_get_base_dir = patch.object(self.ext_handler_instance, "get_base_dir", return_value=self.extension_directory)
+        self.mock_get_base_dir.start()
+
+    def tearDown(self):
+        self.mock_get_base_dir.stop()
+
+    def test_rm_ext_handler_dir_should_remove_the_extension_directory(self):
+        os.mkdir(self.extension_directory)
+        os.mknod(os.path.join(self.extension_directory, "extension_file1"))
+        os.mknod(os.path.join(self.extension_directory, "extension_file2"))
+        os.mknod(os.path.join(self.extension_directory, "extension_file3"))
+
+        self.ext_handler_instance.rm_ext_handler_dir()
+
+        self.assertFalse(os.path.exists(self.extension_directory))
+
+    def test_rm_ext_handler_dir_should_not_report_an_event_if_the_extension_directory_does_not_exist(self):
+        if os.path.exists(self.extension_directory):
+            os.rmdir(self.extension_directory)
+
+        with patch.object(self.ext_handler_instance, "report_event") as mock_report_event:
+            self.ext_handler_instance.rm_ext_handler_dir()
+
+        mock_report_event.assert_not_called()
+
+    def test_rm_ext_handler_dir_should_not_report_an_event_if_a_child_is_removed_asynchronously_while_deleting_the_extension_directory(self):
+        os.mkdir(self.extension_directory)
+        os.mknod(os.path.join(self.extension_directory, "extension_file1"))
+        os.mknod(os.path.join(self.extension_directory, "extension_file2"))
+        os.mknod(os.path.join(self.extension_directory, "extension_file3"))
+
+        #
+        # Some extensions uninstall asynchronously and the files we are trying to remove may be removed
+        # while shutil.rmtree is traversing the extension's directory. Mock this by deleting a file
+        # twice (the second call will produce "[Errno 2] No such file or directory", which should not be
+        # reported as a telemetry event.
+        # In order to mock this, we need to know that rm_ext_handler_dir invokes Pyhon's shutil.rmtree,
+        # which in turn invokes os.unlink (Python 3) or os.remove (Python 2)
+        #
+        remove_api_name = "unlink" if sys.version_info >= (3, 0) else "remove"
+
+        original_remove_api = getattr(shutil.os, remove_api_name)
+
+        extension_directory = self.extension_directory
+
+        def mock_remove(path, dir_fd=None):
+            if dir_fd is not None:  # path is relative, make it absolute
+                path = os.path.join(extension_directory, path)
+
+            if path.endswith("extension_file2"):
+                original_remove_api(path)
+                mock_remove.file_deleted_asynchronously = True
+            original_remove_api(path)
+
+        mock_remove.file_deleted_asynchronously = False
+
+        with patch.object(shutil.os, remove_api_name, mock_remove):
+            with patch.object(self.ext_handler_instance, "report_event") as mock_report_event:
+                self.ext_handler_instance.rm_ext_handler_dir()
+
+        mock_report_event.assert_not_called()
+
+        # The next 2 asserts are checks on the mock itself, in case the implementation of rm_ext_handler_dir changes (mocks may need to be updated then)
+        self.assertTrue(mock_remove.file_deleted_asynchronously)  # verify the mock was actually called
+        self.assertFalse(os.path.exists(self.extension_directory))  # verify the error produced by the mock did not prevent the deletion
+
+    def test_rm_ext_handler_dir_should_report_an_event_if_an_error_occurs_while_deleting_the_extension_directory(self):
+        os.mkdir(self.extension_directory)
+        os.mknod(os.path.join(self.extension_directory, "extension_file1"))
+        os.mknod(os.path.join(self.extension_directory, "extension_file2"))
+        os.mknod(os.path.join(self.extension_directory, "extension_file3"))
+
+        # The mock below relies on the knowledge that rm_ext_handler_dir invokes Pyhon's shutil.rmtree,
+        # which in turn invokes os.unlink (Python 3) or os.remove (Python 2)
+        remove_api_name = "unlink" if sys.version_info >= (3, 0) else "remove"
+
+        original_remove_api = getattr(shutil.os, remove_api_name)
+
+        def mock_remove(path, dir_fd=None):
+            if path.endswith("extension_file2"):
+                raise IOError("A mocked error")
+            original_remove_api(path)
+
+        with patch.object(shutil.os, remove_api_name, mock_remove):
+            with patch.object(self.ext_handler_instance, "report_event") as mock_report_event:
+                self.ext_handler_instance.rm_ext_handler_dir()
+
+        args, kwargs = mock_report_event.call_args
+        self.assertTrue("A mocked error" in kwargs["message"])
+