Use Azure storage SDK to download cache by dhadka · Pull Request #497 · actions/toolkit

dhadka · 2020-06-08T16:55:28Z

Use the Azure Storage SDK for Node (https://github.com/Azure/azure-storage-node) when downloading cache content hosted on Azure blob storage. This is intended to improve reliability and performance because the download call, downloadToBuffer, downloads the file in 4 MB chunks which can be independently retried and parallelized.

In testing, we observed download speeds reduced by about 50% using this approach and a significant improvement in reliability. Additionally, this follows the suggestions made by Azure in our investigation into download flakiness (actions/cache#267).

One limitation of downloadToBuffer is Node only supports buffers up to ~1 GB on 32-bit systems and ~2 GBs on 64-bit systems. While the vast majority of caches are < 1 GB, there are a good number of caches > 1 GB and we technically allow up to 5 GBs. As a result, any download that exceeds the buffer limit will be split up and downloaded in multiple calls to downloadToBuffer.

Proxies are still supported using the existing http_proxy and https_proxy environment variables.

joshmgross

Overall looks good to me, left some comments around making this option more friendly to other action authors.

Could we add some more context to the PR description? Would be good to have some background and link to the actions/cache issue that this relates to

joshmgross · 2020-06-12T17:41:12Z

+  const disableAzureSdk = process.env['DISABLE_AZURE_SDK'] ?? ''
+
+  if (
+    archiveUrl.hostname.endsWith('.blob.core.windows.net') &&


Do we have any guarantees that all blob storage downloads will be at this address?

Guarantee? No. This is what they've always used and I don't see it changing anytime soon. If it were to change, we would just fall back to http-client.

dhadka · 2020-06-16T20:33:23Z

@joshmgross Any idea why the toolkit npm test fails with:

    packages/cache/src/internal/cacheHttpClient.ts:290:35 - error TS2339: Property 'constants' does not exist on type 'typeof import("buffer")'.

    290     const maxSegmentSize = buffer.constants.MAX_LENGTH

When the action runs fine in the cache tests?

joshmgross · 2020-06-16T20:48:07Z

@joshmgross Any idea why the toolkit npm test fails with:
    packages/cache/src/internal/cacheHttpClient.ts:290:35 - error TS2339: Property 'constants' does not exist on type 'typeof import("buffer")'.

    290     const maxSegmentSize = buffer.constants.MAX_LENGTH
When the action runs fine in the cache tests?

Looks like it was just a type issue with @types/node. Since all actions run on the LTS version of Node 12, I installed the latest version of @types/node@12

joshmgross · 2020-06-16T20:49:41Z

  "devDependencies": {
    "@types/jest": "^24.0.11",
-    "@types/node": "^11.13.5",
+    "@types/node": "^12.12.47",


@thboop @ericsciple FYI I bumped this package version since all actions run on Node 12 and it was causing issues in the test, see #497 (comment)

Restructure files and add tests

dhadka · 2020-06-18T15:50:33Z

@joshmgross Can you please take one more pass? My last commit added some testing. In order to make it work with jest, I had to move around some methods, including adding the following:

requestUtils.ts - Contains the retry logic and methods that check HTTP status codes (needed to avoid circular reference)
downloadUtils.ts - Contains the actual download logic

If you're good, then this should be ready to merge (I don't have write permission on this repo so I can't merge).

joshmgross

Changes look good to me, but moving the exported functions to new files will break existing imports. We can either bump the major version or keep the functions where they are and have them call the new function locations.

This package is well tested, so I'm fine with releasing a version 1

joshmgross · 2020-06-18T17:23:34Z

+    retryOptions: {
+      // Override the timeout used when downloading each 4 MB chunk
+      // The default is 2 min / MB, which is way too slow
+      tryTimeoutInMs: options?.timeoutInMs ?? 30000


I think we should do something similar to https://github.com/actions/toolkit/blob/master/packages/glob/src/internal-glob-options-helper.ts so that we can initialize a default options object and keep all the defaults in a single place.

Done. The one issue I hit with this approach is that even though we fill in the default value if it's undefined, the interface still shows the properties as optional, so we still need some check if it's undefined. I ended up adding an assertDefined method that throws if the property is undefined, but that should never happen in practice...it's just to make TypeScript happy.

If you know of a better way to do this, please let me know.

(I also tried using the non-null operator !, but lint doesn't like it)

joshmgross · 2020-06-19T14:26:43Z

+- Fix to await async function getCompressionMethod
+
+### 1.0.0
+- Downloads Azure-hosted caches using the Azure SDK for speed and reliability


Could we list the breaking changes here?

Done. Are there any other breaking changes we would want to make now?

I'm working on a follow-on PR to display progress. Let's not release 1.0.0 just yet.

@joshmgross Here's the follow-on PR - https://github.com/dhadka/toolkit/pull/2

…-sdk

yaananth · 2020-07-10T13:42:20Z

+    return false
+  }
+  const retryableStatusCodes = [
+    HttpCodes.BadGateway,


Do we get throttled here? Should we retry on 429?

I don't think we should retry on throttling - it probably will make it worse, no? :D

(Note that I didn't add this code, just moved it to a different file for testing purposes)

I believe it's this way because a 429 would generally mean we need to wait for some period of time. The question then becomes whether it's better for us to wait and retry, or just "cache miss" and let the user rebuild the cache content. These retries are only used when accessing the ArtifactCache API, so this would mean RU is throttling the host and that host would likely remain throttled for some time.

yaananth · 2020-07-10T13:43:46Z

+  }
+
+  if (copy) {
+    if (typeof copy.useAzureSdk === 'boolean') {


any reason for checking these types? we are already in typescript, we should have good typechecking. I guess are these sent by customers and they can send anything they want? Why don't we validate them and fail if that's the case

I think the purpose of this if condition is to check if the value is set (not undefined and not null). So I think this is just an easier expression. (This pattern is used elsewhere, such as

toolkit/packages/glob/src/internal-glob-options-helper.ts

Line 15 in 1a2c592

if (typeof copy.followSymbolicLinks === 'boolean') {

)

aiqiaoy

LGTM

konradpabjan

Looks good! 👍

The @actions/cache version currently being used has performance issue with large gem caches. See actions/cache#267 This was addressed here actions/toolkit#497 Bump the @actions/cache version to a newer release that contains that fix.

dhadka added 7 commits June 4, 2020 16:03

Adds option to download using AzCopy

0f71f6b

Bump version number and add release notes

824c885

Ensure we use at least v10

353580d

Negate env var so it disables AzCopy

b6f111e

Use Azure storage SDK to download cache

012303c

Use same level of parallelism as AzCopy

85163cb

Fix naming of variable

69bc73d

joshmgross approved these changes Jun 12, 2020

View reviewed changes

React to feedback

b9ef148

Bump Node types to Node 12

58c0a53

joshmgross reviewed Jun 16, 2020

View reviewed changes

dhadka and others added 5 commits June 16, 2020 16:09

Make linter happy

173d97d

Pass options into restoreCache method

5bc9c7e

Fix tests

c6e67d0

Restructure files and add tests

c4ef939

Merge pull request #1 from dhadka/cache-tests

96f7093

Restructure files and add tests

dhadka mentioned this pull request Jun 18, 2020

Networking issues impacting cache save/restore actions/cache#267

Closed

joshmgross suggested changes Jun 18, 2020

View reviewed changes

timoschilling approved these changes Jun 18, 2020

View reviewed changes

Add method to get the default download and upload options

cccec7b

joshmgross reviewed Jun 19, 2020

View reviewed changes

dhadka added 2 commits June 19, 2020 22:31

Include breaking changes in RELEASES.md

4327c3b

Merge branch 'master' of http://github.com/actions/toolkit into cache…

172bcdc

…-sdk

yaananth reviewed Jul 10, 2020

View reviewed changes

aiqiaoy approved these changes Jul 10, 2020

View reviewed changes

konradpabjan approved these changes Jul 10, 2020

View reviewed changes

konradpabjan merged commit 4964b0c into actions:master Jul 10, 2020

dhadka mentioned this pull request Jul 16, 2020

actions/cache is much slower on Windows actions/cache#336

Closed

scottjacobsen mentioned this pull request Aug 21, 2020

Automated Bundler caching ruby/setup-ruby#52

Closed

scottjacobsen mentioned this pull request Aug 21, 2020

Bump @actions/cache version ruby/setup-ruby#79

Merged

Conversation

dhadka commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshmgross left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhadka commented Jun 16, 2020

Uh oh!

joshmgross commented Jun 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhadka commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshmgross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhadka Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiqiaoy left a comment

Choose a reason for hiding this comment

Uh oh!

konradpabjan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dhadka commented Jun 8, 2020 •

edited

Loading

dhadka commented Jun 18, 2020 •

edited

Loading

dhadka Jun 19, 2020 •

edited

Loading