From f3c12d55618ee99f7557a717ab9c899ca616873d Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Wed, 8 Jan 2025 16:19:09 +0000 Subject: [PATCH 1/4] Set default concurrency to 10 and make timeout configurable --- packages/artifact/__tests__/config.test.ts | 16 ++++++++++++ .../artifact/src/internal/shared/config.ts | 25 ++++++++++--------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/packages/artifact/__tests__/config.test.ts b/packages/artifact/__tests__/config.test.ts index b9ef643c..11bbe396 100644 --- a/packages/artifact/__tests__/config.test.ts +++ b/packages/artifact/__tests__/config.test.ts @@ -30,3 +30,19 @@ describe('isGhes', () => { expect(config.isGhes()).toBe(true) }) }) + +describe('uploadChunkTimeoutEnv', () => { + it('should return default 300000 when no env set', () => { + expect(config.getUploadChunkTimeout()).toBe(300000) + }) + it('should return value set in ACTIONS_UPLOAD_TIMEOUT_MS', () => { + process.env.ACTIONS_UPLOAD_TIMEOUT_MS = '150000' + expect(config.getUploadChunkTimeout()).toBe(150000) + }) + it('should throw if value set in ACTIONS_UPLOAD_TIMEOUT_MS is invalid', () => { + process.env.ACTIONS_UPLOAD_TIMEOUT_MS = 'abc' + expect(() => { + config.getUploadChunkTimeout() + }).toThrow() + }) +}) diff --git a/packages/artifact/src/internal/shared/config.ts b/packages/artifact/src/internal/shared/config.ts index 047d3b98..75bbf8b5 100644 --- a/packages/artifact/src/internal/shared/config.ts +++ b/packages/artifact/src/internal/shared/config.ts @@ -44,20 +44,21 @@ export function getGitHubWorkspaceDir(): string { return ghWorkspaceDir } -// Mimics behavior of azcopy: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azcopy-optimize -// If your machine has fewer than 5 CPUs, then the value of this variable is set to 32. -// Otherwise, the default value is equal to 16 multiplied by the number of CPUs. The maximum value of this variable is 300. +// From testing, setting this value to 10 yielded best results in terms of reliability and there are no impact on performance either export function getConcurrency(): number { - const numCPUs = os.cpus().length - - if (numCPUs <= 4) { - return 32 - } - - const concurrency = 16 * numCPUs - return concurrency > 300 ? 300 : concurrency + return 10 } export function getUploadChunkTimeout(): number { - return 300_000 // 5 minutes + const timeoutVar = process.env['ACTIONS_UPLOAD_TIMEOUT_MS'] + if (!timeoutVar) { + return 300000 // 5 minutes + } + + const timeout = parseInt(timeoutVar) + if (isNaN(timeout)) { + throw new Error('Invalid value set for ACTIONS_UPLOAD_TIMEOUT_MS env variable') + } + + return timeout } From ede05b95d7f7bb1e0e09edd39a3419e8ac5286ab Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Wed, 8 Jan 2025 17:53:44 +0000 Subject: [PATCH 2/4] Make concurrency change opt-in, but can only go lower --- packages/artifact/__tests__/config.test.ts | 45 +++++++++++++++++++ .../artifact/src/internal/shared/config.ts | 40 +++++++++++++++-- 2 files changed, 81 insertions(+), 4 deletions(-) diff --git a/packages/artifact/__tests__/config.test.ts b/packages/artifact/__tests__/config.test.ts index 11bbe396..4057ec3e 100644 --- a/packages/artifact/__tests__/config.test.ts +++ b/packages/artifact/__tests__/config.test.ts @@ -1,4 +1,10 @@ import * as config from '../src/internal/shared/config' +import os from 'os' + +// Mock the 'os' module +jest.mock('os', () => ({ + cpus: jest.fn() +})) beforeEach(() => { jest.resetModules() @@ -35,10 +41,12 @@ describe('uploadChunkTimeoutEnv', () => { it('should return default 300000 when no env set', () => { expect(config.getUploadChunkTimeout()).toBe(300000) }) + it('should return value set in ACTIONS_UPLOAD_TIMEOUT_MS', () => { process.env.ACTIONS_UPLOAD_TIMEOUT_MS = '150000' expect(config.getUploadChunkTimeout()).toBe(150000) }) + it('should throw if value set in ACTIONS_UPLOAD_TIMEOUT_MS is invalid', () => { process.env.ACTIONS_UPLOAD_TIMEOUT_MS = 'abc' expect(() => { @@ -46,3 +54,40 @@ describe('uploadChunkTimeoutEnv', () => { }).toThrow() }) }) + +describe('uploadConcurrencyEnv', () => { + it('should return default 32 when cpu num is <= 4', () => { + ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) + expect(config.getConcurrency()).toBe(32) + }) + + it('should return 16 * num of cpu when cpu num is > 4', () => { + ;(os.cpus as jest.Mock).mockReturnValue(new Array(6)) + expect(config.getConcurrency()).toBe(96) + }) + + it('should return up to 300 max value', () => { + ;(os.cpus as jest.Mock).mockReturnValue(new Array(32)) + expect(config.getConcurrency()).toBe(300) + }) + + it('should return override value when ACTIONS_UPLOAD_CONCURRENCY is set', () => { + ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) + process.env.ACTIONS_UPLOAD_CONCURRENCY = '10' + expect(config.getConcurrency()).toBe(10) + }) + + it('should throw with invalid value of ACTIONS_UPLOAD_CONCURRENCY', () => { + ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) + process.env.ACTIONS_UPLOAD_CONCURRENCY = 'abc' + expect(() => { + config.getConcurrency() + }).toThrow() + }) + + it('cannot go over currency cap when override value is greater', () => { + ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) + process.env.ACTIONS_UPLOAD_CONCURRENCY = '40' + expect(config.getConcurrency()).toBe(32) + }) +}) diff --git a/packages/artifact/src/internal/shared/config.ts b/packages/artifact/src/internal/shared/config.ts index 75bbf8b5..d9d9ae35 100644 --- a/packages/artifact/src/internal/shared/config.ts +++ b/packages/artifact/src/internal/shared/config.ts @@ -1,4 +1,5 @@ import os from 'os' +import {info} from '@actions/core' // Used for controlling the highWaterMark value of the zip that is being streamed // The same value is used as the chunk size that is use during upload to blob storage @@ -44,20 +45,51 @@ export function getGitHubWorkspaceDir(): string { return ghWorkspaceDir } -// From testing, setting this value to 10 yielded best results in terms of reliability and there are no impact on performance either +// Mimics behavior of azcopy: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azcopy-optimize +// If your machine has fewer than 5 CPUs, then the value of this variable is set to 32. +// Otherwise, the default value is equal to 16 multiplied by the number of CPUs. The maximum value of this variable is 300. +// This value can be lowered with ACTIONS_UPLOAD_CONCURRENCY variable. export function getConcurrency(): number { - return 10 + const numCPUs = os.cpus().length + let concurrencyCap = 32 + + if (numCPUs > 4) { + const concurrency = 16 * numCPUs + concurrencyCap = concurrency > 300 ? 300 : concurrency + } + + const concurrencyOverride = process.env['ACTIONS_UPLOAD_CONCURRENCY'] + if (concurrencyOverride) { + const concurrency = parseInt(concurrencyOverride) + if (isNaN(concurrency)) { + throw new Error( + 'Invalid value set for ACTIONS_UPLOAD_CONCURRENCY env variable' + ) + } + + if (concurrency < concurrencyCap) { + return concurrency + } + + info( + `ACTIONS_UPLOAD_CONCURRENCY is higher than the cap of ${concurrencyCap} based on the number of cpus. Lowering it to the cap.` + ) + } + + return concurrencyCap } export function getUploadChunkTimeout(): number { - const timeoutVar = process.env['ACTIONS_UPLOAD_TIMEOUT_MS'] + const timeoutVar = process.env['ACTIONS_UPLOAD_TIMEOUT_MS'] if (!timeoutVar) { return 300000 // 5 minutes } const timeout = parseInt(timeoutVar) if (isNaN(timeout)) { - throw new Error('Invalid value set for ACTIONS_UPLOAD_TIMEOUT_MS env variable') + throw new Error( + 'Invalid value set for ACTIONS_UPLOAD_TIMEOUT_MS env variable' + ) } return timeout From d4385a64a79e01e0b5eff991f721f234bdcd7620 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Wed, 8 Jan 2025 18:14:04 +0000 Subject: [PATCH 3/4] Concurrency has a min of 1 --- packages/artifact/__tests__/config.test.ts | 8 ++++++++ packages/artifact/src/internal/shared/config.ts | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/artifact/__tests__/config.test.ts b/packages/artifact/__tests__/config.test.ts index 4057ec3e..579fed6e 100644 --- a/packages/artifact/__tests__/config.test.ts +++ b/packages/artifact/__tests__/config.test.ts @@ -85,6 +85,14 @@ describe('uploadConcurrencyEnv', () => { }).toThrow() }) + it('should throw if ACTIONS_UPLOAD_CONCURRENCY is < 1', () => { + ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) + process.env.ACTIONS_UPLOAD_CONCURRENCY = '0' + expect(() => { + config.getConcurrency() + }).toThrow() + }) + it('cannot go over currency cap when override value is greater', () => { ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) process.env.ACTIONS_UPLOAD_CONCURRENCY = '40' diff --git a/packages/artifact/src/internal/shared/config.ts b/packages/artifact/src/internal/shared/config.ts index d9d9ae35..44547451 100644 --- a/packages/artifact/src/internal/shared/config.ts +++ b/packages/artifact/src/internal/shared/config.ts @@ -61,7 +61,7 @@ export function getConcurrency(): number { const concurrencyOverride = process.env['ACTIONS_UPLOAD_CONCURRENCY'] if (concurrencyOverride) { const concurrency = parseInt(concurrencyOverride) - if (isNaN(concurrency)) { + if (isNaN(concurrency) || concurrency < 1) { throw new Error( 'Invalid value set for ACTIONS_UPLOAD_CONCURRENCY env variable' ) From e55409315fb4946675f6859c12243eff19570ed4 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Wed, 8 Jan 2025 20:32:45 +0000 Subject: [PATCH 4/4] Rename the prefix to be more specific --- packages/artifact/__tests__/config.test.ts | 22 +++++++++---------- .../artifact/src/internal/shared/config.ts | 12 +++++----- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/artifact/__tests__/config.test.ts b/packages/artifact/__tests__/config.test.ts index 579fed6e..9fc4543d 100644 --- a/packages/artifact/__tests__/config.test.ts +++ b/packages/artifact/__tests__/config.test.ts @@ -42,13 +42,13 @@ describe('uploadChunkTimeoutEnv', () => { expect(config.getUploadChunkTimeout()).toBe(300000) }) - it('should return value set in ACTIONS_UPLOAD_TIMEOUT_MS', () => { - process.env.ACTIONS_UPLOAD_TIMEOUT_MS = '150000' + it('should return value set in ACTIONS_ARTIFACT_UPLOAD_TIMEOUT_MS', () => { + process.env.ACTIONS_ARTIFACT_UPLOAD_TIMEOUT_MS = '150000' expect(config.getUploadChunkTimeout()).toBe(150000) }) - it('should throw if value set in ACTIONS_UPLOAD_TIMEOUT_MS is invalid', () => { - process.env.ACTIONS_UPLOAD_TIMEOUT_MS = 'abc' + it('should throw if value set in ACTIONS_ARTIFACT_UPLOAD_TIMEOUT_MS is invalid', () => { + process.env.ACTIONS_ARTIFACT_UPLOAD_TIMEOUT_MS = 'abc' expect(() => { config.getUploadChunkTimeout() }).toThrow() @@ -71,23 +71,23 @@ describe('uploadConcurrencyEnv', () => { expect(config.getConcurrency()).toBe(300) }) - it('should return override value when ACTIONS_UPLOAD_CONCURRENCY is set', () => { + it('should return override value when ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY is set', () => { ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) - process.env.ACTIONS_UPLOAD_CONCURRENCY = '10' + process.env.ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY = '10' expect(config.getConcurrency()).toBe(10) }) - it('should throw with invalid value of ACTIONS_UPLOAD_CONCURRENCY', () => { + it('should throw with invalid value of ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY', () => { ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) - process.env.ACTIONS_UPLOAD_CONCURRENCY = 'abc' + process.env.ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY = 'abc' expect(() => { config.getConcurrency() }).toThrow() }) - it('should throw if ACTIONS_UPLOAD_CONCURRENCY is < 1', () => { + it('should throw if ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY is < 1', () => { ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) - process.env.ACTIONS_UPLOAD_CONCURRENCY = '0' + process.env.ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY = '0' expect(() => { config.getConcurrency() }).toThrow() @@ -95,7 +95,7 @@ describe('uploadConcurrencyEnv', () => { it('cannot go over currency cap when override value is greater', () => { ;(os.cpus as jest.Mock).mockReturnValue(new Array(4)) - process.env.ACTIONS_UPLOAD_CONCURRENCY = '40' + process.env.ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY = '40' expect(config.getConcurrency()).toBe(32) }) }) diff --git a/packages/artifact/src/internal/shared/config.ts b/packages/artifact/src/internal/shared/config.ts index 44547451..7aeb2378 100644 --- a/packages/artifact/src/internal/shared/config.ts +++ b/packages/artifact/src/internal/shared/config.ts @@ -48,7 +48,7 @@ export function getGitHubWorkspaceDir(): string { // Mimics behavior of azcopy: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azcopy-optimize // If your machine has fewer than 5 CPUs, then the value of this variable is set to 32. // Otherwise, the default value is equal to 16 multiplied by the number of CPUs. The maximum value of this variable is 300. -// This value can be lowered with ACTIONS_UPLOAD_CONCURRENCY variable. +// This value can be lowered with ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY variable. export function getConcurrency(): number { const numCPUs = os.cpus().length let concurrencyCap = 32 @@ -58,12 +58,12 @@ export function getConcurrency(): number { concurrencyCap = concurrency > 300 ? 300 : concurrency } - const concurrencyOverride = process.env['ACTIONS_UPLOAD_CONCURRENCY'] + const concurrencyOverride = process.env['ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY'] if (concurrencyOverride) { const concurrency = parseInt(concurrencyOverride) if (isNaN(concurrency) || concurrency < 1) { throw new Error( - 'Invalid value set for ACTIONS_UPLOAD_CONCURRENCY env variable' + 'Invalid value set for ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY env variable' ) } @@ -72,7 +72,7 @@ export function getConcurrency(): number { } info( - `ACTIONS_UPLOAD_CONCURRENCY is higher than the cap of ${concurrencyCap} based on the number of cpus. Lowering it to the cap.` + `ACTIONS_ARTIFACT_UPLOAD_CONCURRENCY is higher than the cap of ${concurrencyCap} based on the number of cpus. Lowering it to the cap.` ) } @@ -80,7 +80,7 @@ export function getConcurrency(): number { } export function getUploadChunkTimeout(): number { - const timeoutVar = process.env['ACTIONS_UPLOAD_TIMEOUT_MS'] + const timeoutVar = process.env['ACTIONS_ARTIFACT_UPLOAD_TIMEOUT_MS'] if (!timeoutVar) { return 300000 // 5 minutes } @@ -88,7 +88,7 @@ export function getUploadChunkTimeout(): number { const timeout = parseInt(timeoutVar) if (isNaN(timeout)) { throw new Error( - 'Invalid value set for ACTIONS_UPLOAD_TIMEOUT_MS env variable' + 'Invalid value set for ACTIONS_ARTIFACT_UPLOAD_TIMEOUT_MS env variable' ) }