Improve resilience of extension downloads #1463

narrieta · 2019-02-13T01:31:05Z

Description

Expanding the extension zip should be done inside the loop that tries the different uris
Added 5 tries at 1 min interval
Unzipping a bad file throws Exception, not IoError
Fixed cleanup logic when a failure occurs

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.
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.

pgombar

Assuming the worst scenario (when the download sequence fails for all uris, after all retries), we would raise an exception with the message "Failed to download extension". However, this complete failure can be because:

we actually fail to download from all uris, or
we download a corrupt zip every time (the publisher messed up) and we fail to expand.
Currently, we are not explicitly addressing 2). Would it be worth it to add another type of exception for this case?

Also, should we be testing if we've properly cleaned up invalid downloads/expansions?

pgombar · 2019-02-13T20:03:22Z

azurelinuxagent/ga/exthandlers.py

-                    file_downloaded = self.protocol.download_ext_handler_pkg(uri.uri, destination)
+        if not package_exists:
+            downloaded = False
+            i = 0


nit: a better name for this variable might be "retry_count"?

Thanks. The loop is short and very simple; I think in this case the longer variable name wouldn't add much.

pgombar · 2019-02-13T20:39:37Z

tests/ga/test_exthandlers_download_extension.py

+        self.download_failures = 0
+
+        def download_ext_handler_pkg(_uri, destination):
+            if self.download_failures < 3:


Can we use NUMBER_OF_DOWNLOAD_RETRIES - 2 instead of 3? It seems like an arbitrary number otherwise.

Or is this related to the number of uris you initialize in the test case set up method? In that case, I would also refer to len(self.pkg.uris).

It is an arbitrary number :) - Read it as "fail a few times, then succeed".

added comment to code

pgombar · 2019-02-13T20:40:29Z

tests/ga/test_exthandlers_download_extension.py

+        self.download_failures = 0
+
+        def download_ext_handler_pkg(_uri, destination):
+            if self.download_failures < 3:


Same as above.

pgombar · 2019-02-13T20:40:37Z

tests/ga/test_exthandlers_download_extension.py

+        self.download_failures = 0
+
+        def download_ext_handler_pkg(_uri, destination):
+            if self.download_failures < 3:


Same as above.

narrieta · 2019-02-13T21:09:31Z

@pgombar

2. we download a corrupt zip every time (the publisher messed up) and we fail to expand.
Currently, we are not explicitly addressing 2). Would it be worth it to add another type of exception for this case?

We are addressing both cases (with the same exception). The log will contain the specific errors. There is a different work item to add telemetry for download failures; over there it may be useful to differentiate both cases.

narrieta · 2019-02-13T21:15:59Z

@pgombar

Also, should we be testing if we've properly cleaned up invalid downloads/expansions?

test_it_should_ignore_existing_extension_package_when_it_is_invalid verifies that, if for any reason we don't cleanup an invalid zip, we re-download.

I'll add a couple of asserts to test_it_should_raise_an_exception_when_all_downloads_fail to check that the zip & extension dir are gone

pgombar · 2019-02-13T23:05:00Z

Thanks, Norberto. LGTM.

vrdmr

LGTM. Minor comments. Not blocker, but can be done as a part of later releases.

vrdmr · 2019-02-14T01:39:00Z

azurelinuxagent/ga/exthandlers.py

+            return False
+        return True
+
+    def _unzip_extension_package(self, source_file, target_directory):


Just a clarification - Would the issue of corrupt zip arise for Agent as well? and we need similar logic in azurelinuxagent.ga.update.GuestAgent#_unpack?

Yup, and I intend to fix that as well. However, this is a hotfix release, so I do not want to add any other changes to it. Thanks for pointing it out.

vrdmr · 2019-02-14T01:42:06Z

azurelinuxagent/ga/exthandlers.py

+        except Exception as exception:
+            self.logger.info("Error downloading extension package: {0}", ustr(exception))
+            if os.path.exists(target_file):
+                os.remove(target_file)


The fileutil.clean_ioerror already does this partially. We should change (overload) this as util and use that in both the place (756, 766 and 768).

That function seems to be meant to handle very specific errors and cannot handle this one in particular - it has a check for the exception type. Since that function is used in many places in the code, I decided to leave it alone for the moment.

vrdmr · 2019-02-14T01:43:27Z

azurelinuxagent/ga/exthandlers.py

-        if not file_downloaded:
-            raise ExtensionDownloadError("Failed to download extension", code=1001)
+                self.logger.info("Failed to download the extension package from all uris, will retry after a minute")
+                time.sleep(60)


Good call! 👍

vrdmr · 2019-02-14T01:43:56Z

azurelinuxagent/ga/exthandlers.py

-            fileutil.clean_ioerror(e, paths=[self.get_base_dir(), self.pkg_file])
-            raise ExtensionError(u"Failed to unzip extension package", e, code=1001)
+            if not downloaded:
+                raise ExtensionDownloadError("Failed to download extension", code=1001)


I'll open a task. We need to document these magical numbers. (such as 1001).

vrdmr · 2019-02-14T01:54:26Z

tests/ga/test_exthandlers_download_extension.py

+        protocol = WireProtocol("http://Microsoft.CPlat.Core.RunCommandLinux/foo-bar")
+
+        self.pkg = ExtHandlerPackage()
+        self.pkg.uris = [ ExtHandlerVersionUri(), ExtHandlerVersionUri(), ExtHandlerVersionUri(), ExtHandlerVersionUri(), ExtHandlerVersionUri() ]


If you change ExtHandlerVersionUri to the following, It would make both the class as well as the test a little cleaner:

class ExtHandlerVersionUri(DataContract): def __init__(self, uri=None): self.uri = uri

self.pkg.uris = [ ExtHandlerVersionUri('https://zrdfepirv2cy4prdstr00a.blob.core.windows.net/f72653efd9e349ed9842c8b99e4c1712-foobar/Microsoft.CPlat.Core__RunCommandLinux__1.0.0'), ExtHandlerVersionUri('https://zrdfepirv2cy4prdstr01a.blob.core.windows.net/f72653efd9e349ed9842c8b99e4c1712-foobar/Microsoft.CPlat.Core__RunCommandLinux__1.0.0'), ExtHandlerVersionUri('https://zrdfepirv2cy4prdstr02a.blob.core.windows.net/f72653efd9e349ed9842c8b99e4c1712-foobar/Microsoft.CPlat.Core__RunCommandLinux__1.0.0'), ExtHandlerVersionUri('https://zrdfepirv2cy4prdstr02a.blob.core.windows.net/f72653efd9e349ed9842c8b99e4c1712-foobar/Microsoft.CPlat.Core__RunCommandLinux__1.0.0'), ExtHandlerVersionUri('https://zrdfepirv2cy4prdstr02a.blob.core.windows.net/f72653efd9e349ed9842c8b99e4c1712-foobar/Microsoft.CPlat.Core__RunCommandLinux__1.0.0') ]

ExtHandlerVersionUri is meant to be used to deserialize from xml, etc. The class has other members other than uri. I think it would not be worth it to add all of those parameters to init just for the sake of this test.

Improve resilience of extension downloads

0c09040

narrieta requested review from vrdmr and pgombar February 13, 2019 01:31

Fix log messages

8ac6788

pgombar reviewed Feb 13, 2019

View reviewed changes

Test improvements

a9617aa

vrdmr approved these changes Feb 14, 2019

View reviewed changes

narrieta merged commit 0f7f375 into Azure:release-2.2.37 Feb 14, 2019

narrieta deleted the download-error branch February 14, 2019 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve resilience of extension downloads #1463

Improve resilience of extension downloads #1463

narrieta commented Feb 13, 2019

pgombar left a comment

pgombar Feb 13, 2019

narrieta Feb 13, 2019

pgombar Feb 13, 2019

pgombar Feb 13, 2019

narrieta Feb 13, 2019

narrieta Feb 13, 2019

pgombar Feb 13, 2019

narrieta Feb 13, 2019

pgombar Feb 13, 2019

narrieta Feb 13, 2019

narrieta commented Feb 13, 2019

narrieta commented Feb 13, 2019

pgombar commented Feb 13, 2019

vrdmr left a comment

vrdmr Feb 14, 2019

narrieta Feb 14, 2019

vrdmr Feb 14, 2019

narrieta Feb 14, 2019

vrdmr Feb 14, 2019

vrdmr Feb 14, 2019

narrieta Feb 14, 2019

vrdmr Feb 14, 2019

narrieta Feb 14, 2019

Improve resilience of extension downloads #1463

Improve resilience of extension downloads #1463

Conversation

narrieta commented Feb 13, 2019

Description

PR information

Quality of Code and Contribution Guidelines

pgombar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta commented Feb 13, 2019

narrieta commented Feb 13, 2019

pgombar commented Feb 13, 2019

vrdmr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment