Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option for concurrent cache downloads with timeout #1484

Merged
merged 3 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
}
],
"eslint-comments/no-use": "off",
"no-constant-condition": ["error", { "checkLoops": false }],
"github/no-then": "off",
"import/no-namespace": "off",
"no-shadow": "off",
Expand Down
4 changes: 4 additions & 0 deletions packages/cache/RELEASES.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,7 @@
### 3.2.1

- Updated @azure/storage-blob to `v12.13.0`

### 3.2.2

- Add new default cache download method to improve performance and reduce hangs [#1484](https://github.com/actions/toolkit/pull/1484)
27 changes: 21 additions & 6 deletions packages/cache/__tests__/cacheHttpClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,18 +84,24 @@ test('downloadCache uses storage SDK for Azure storage URLs', async () => {
'downloadCacheStorageSDK'
)

const downloadCacheHttpClientConcurrentMock = jest.spyOn(
downloadUtils,
'downloadCacheHttpClientConcurrent'
)

const archiveLocation = 'http://foo.blob.core.windows.net/bar/baz'
const archivePath = '/foo/bar'

await downloadCache(archiveLocation, archivePath)

expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(1)
expect(downloadCacheStorageSDKMock).toHaveBeenCalledWith(
expect(downloadCacheHttpClientConcurrentMock).toHaveBeenCalledTimes(1)
expect(downloadCacheHttpClientConcurrentMock).toHaveBeenCalledWith(
archiveLocation,
archivePath,
getDownloadOptions()
)

expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(0)
expect(downloadCacheHttpClientMock).toHaveBeenCalledTimes(0)
})

Expand All @@ -109,20 +115,26 @@ test('downloadCache passes options to download methods', async () => {
'downloadCacheStorageSDK'
)

const downloadCacheHttpClientConcurrentMock = jest.spyOn(
downloadUtils,
'downloadCacheHttpClientConcurrent'
)

const archiveLocation = 'http://foo.blob.core.windows.net/bar/baz'
const archivePath = '/foo/bar'
const options: DownloadOptions = {downloadConcurrency: 4}

await downloadCache(archiveLocation, archivePath, options)

expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(1)
expect(downloadCacheStorageSDKMock).toHaveBeenCalled()
expect(downloadCacheStorageSDKMock).toHaveBeenCalledWith(
expect(downloadCacheHttpClientConcurrentMock).toHaveBeenCalledTimes(1)
expect(downloadCacheHttpClientConcurrentMock).toHaveBeenCalled()
expect(downloadCacheHttpClientConcurrentMock).toHaveBeenCalledWith(
archiveLocation,
archivePath,
getDownloadOptions(options)
)

expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(0)
expect(downloadCacheHttpClientMock).toHaveBeenCalledTimes(0)
})

Expand All @@ -138,7 +150,10 @@ test('downloadCache uses http-client when overridden', async () => {

const archiveLocation = 'http://foo.blob.core.windows.net/bar/baz'
const archivePath = '/foo/bar'
const options: DownloadOptions = {useAzureSdk: false}
const options: DownloadOptions = {
useAzureSdk: false,
concurrentBlobDownloads: false
}

await downloadCache(archiveLocation, archivePath, options)

Expand Down
7 changes: 5 additions & 2 deletions packages/cache/__tests__/options.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import {
getUploadOptions
} from '../src/options'

const useAzureSdk = true
const useAzureSdk = false
const concurrentBlobDownloads = true
const downloadConcurrency = 8
const timeoutInMs = 30000
const segmentTimeoutInMs = 600000
Expand All @@ -18,6 +19,7 @@ test('getDownloadOptions sets defaults', async () => {

expect(actualOptions).toEqual({
useAzureSdk,
concurrentBlobDownloads,
downloadConcurrency,
timeoutInMs,
segmentTimeoutInMs,
Expand All @@ -27,7 +29,8 @@ test('getDownloadOptions sets defaults', async () => {

test('getDownloadOptions overrides all settings', async () => {
const expectedOptions: DownloadOptions = {
useAzureSdk: false,
useAzureSdk: true,
concurrentBlobDownloads: false,
downloadConcurrency: 14,
timeoutInMs: 20000,
segmentTimeoutInMs: 3600000,
Expand Down
18 changes: 9 additions & 9 deletions packages/cache/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions packages/cache/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@actions/cache",
"version": "3.2.1",
"version": "3.2.2",
"preview": true,
"description": "Actions cache lib",
"keywords": [
Expand Down Expand Up @@ -40,7 +40,7 @@
"@actions/core": "^1.10.0",
"@actions/exec": "^1.0.1",
"@actions/glob": "^0.1.0",
"@actions/http-client": "^2.0.1",
"@actions/http-client": "^2.1.1",
"@actions/io": "^1.0.1",
"@azure/abort-controller": "^1.1.0",
"@azure/ms-rest-js": "^2.6.0",
Expand Down
32 changes: 24 additions & 8 deletions packages/cache/src/internal/cacheHttpClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ import {
ITypedResponseWithError,
ArtifactCacheList
} from './contracts'
import {downloadCacheHttpClient, downloadCacheStorageSDK} from './downloadUtils'
import {
downloadCacheHttpClient,
downloadCacheHttpClientConcurrent,
downloadCacheStorageSDK
} from './downloadUtils'
import {
DownloadOptions,
UploadOptions,
Expand Down Expand Up @@ -171,14 +175,26 @@ export async function downloadCache(
const archiveUrl = new URL(archiveLocation)
const downloadOptions = getDownloadOptions(options)

if (
downloadOptions.useAzureSdk &&
archiveUrl.hostname.endsWith('.blob.core.windows.net')
) {
// Use Azure storage SDK to download caches hosted on Azure to improve speed and reliability.
await downloadCacheStorageSDK(archiveLocation, archivePath, downloadOptions)
if (archiveUrl.hostname.endsWith('.blob.core.windows.net')) {
if (downloadOptions.useAzureSdk) {
// Use Azure storage SDK to download caches hosted on Azure to improve speed and reliability.
await downloadCacheStorageSDK(
archiveLocation,
archivePath,
downloadOptions
)
} else if (downloadOptions.concurrentBlobDownloads) {
// Use concurrent implementation with HttpClient to work around blob SDK issue
await downloadCacheHttpClientConcurrent(
archiveLocation,
archivePath,
downloadOptions
)
} else {
// Otherwise, download using the Actions http-client.
await downloadCacheHttpClient(archiveLocation, archivePath)
}
} else {
// Otherwise, download using the Actions http-client.
await downloadCacheHttpClient(archiveLocation, archivePath)
}
}
Expand Down