Skip to content

Commit 4f18ac5

Browse files
HenryNguyen5HenryNguyen5
authored
Fix pagination with listing self-hosted runners (#202)
* Fix pagination with listing self-hosted runners * Remove unused function * Add function memoization to api calls
1 parent 4c08f9f commit 4f18ac5

File tree

2 files changed

+91
-55
lines changed

2 files changed

+91
-55
lines changed

‎modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts

Lines changed: 17 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,7 @@
1-
import { mocked } from 'ts-jest/utils';
2-
import { scaleDown } from './scale-down';
31
import moment from 'moment';
4-
import { createAppAuth } from '@octokit/auth-app';
5-
import { Octokit } from '@octokit/rest';
2+
import { mocked } from 'ts-jest/utils';
63
import { listRunners, terminateRunner } from './runners';
7-
import { stringify } from 'querystring';
8-
import { ScalingDownConfig } from './scale-down-config';
4+
import { scaleDown } from './scale-down';
95

106
jest.mock('@octokit/auth-app', () => ({
117
createAppAuth: jest.fn().mockImplementation(() => jest.fn().mockImplementation(() => ({ token: 'Blaat' }))),
@@ -21,6 +17,7 @@ const mockOctokit = {
2117
deleteSelfHostedRunnerFromOrg: jest.fn(),
2218
deleteSelfHostedRunnerFromRepo: jest.fn(),
2319
},
20+
paginate: jest.fn(),
2421
};
2522
jest.mock('@octokit/rest', () => ({
2623
Octokit: jest.fn().mockImplementation(() => mockOctokit),
@@ -91,24 +88,20 @@ const RUNNERS_WITH_AUTO_SCALING_CONFIG = DEFAULT_RUNNERS.filter(
9188

9289
const RUNNERS_TO_BE_REMOVED_WITH_AUTO_SCALING_CONFIG = DEFAULT_RUNNERS.filter((r) => r.instanceId.includes('oldest'));
9390

94-
const DEFAULT_REGISTERED_RUNNERS: any = {
95-
data: {
96-
runners: [
97-
{
98-
id: 101,
99-
name: 'i-idle-101',
100-
},
101-
{
102-
id: 102,
103-
name: 'i-oldest-idle-102',
104-
},
105-
{
106-
id: 103,
107-
name: 'i-running-103',
108-
},
109-
],
91+
const DEFAULT_REGISTERED_RUNNERS: any = [
92+
{
93+
id: 101,
94+
name: 'i-idle-101',
11095
},
111-
};
96+
{
97+
id: 102,
98+
name: 'i-oldest-idle-102',
99+
},
100+
{
101+
id: 103,
102+
name: 'i-running-103',
103+
},
104+
];
112105

113106
describe('scaleDown', () => {
114107
beforeEach(() => {
@@ -131,14 +124,10 @@ describe('scaleDown', () => {
131124
},
132125
}));
133126

134-
mockOctokit.actions.listSelfHostedRunnersForOrg.mockImplementation(() => {
135-
return DEFAULT_REGISTERED_RUNNERS;
136-
});
137-
mockOctokit.actions.listSelfHostedRunnersForRepo.mockImplementation(() => {
127+
mockOctokit.paginate.mockImplementationOnce(() => {
138128
return DEFAULT_REGISTERED_RUNNERS;
139129
});
140130

141-
function deRegisterRunnerGithub(id: number): any {}
142131
mockOctokit.actions.deleteSelfHostedRunnerFromRepo.mockImplementation((repo) => {
143132
if (repo.runner_id === 103) {
144133
throw Error();

‎modules/runners/lambdas/runners/src/scale-runners/scale-down.ts

Lines changed: 74 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
import { Octokit } from '@octokit/rest';
21
import { AppAuth } from '@octokit/auth-app/dist-types/types';
3-
import { listRunners, terminateRunner, RunnerInfo } from './runners';
4-
import { createGithubAppAuth, createInstallationClient } from './scale-up';
5-
import { getIdleRunnerCount, ScalingDownConfig } from './scale-down-config';
6-
import yn from 'yn';
2+
import { Octokit } from '@octokit/rest';
73
import moment from 'moment';
4+
import yn from 'yn';
5+
import { listRunners, RunnerInfo, terminateRunner } from './runners';
6+
import { getIdleRunnerCount, ScalingDownConfig } from './scale-down-config';
7+
import { createGithubAppAuth, createInstallationClient } from './scale-up';
88

99
async function createAppClient(githubAppAuth: AppAuth): Promise<Octokit> {
1010
const auth = await githubAppAuth({ type: 'app' });
@@ -22,24 +22,75 @@ function getRepo(runner: RunnerInfo, orgLevel: boolean): Repo {
2222
: { repoOwner: runner.repo?.split('/')[0] as string, repoName: runner.repo?.split('/')[1] as string };
2323
}
2424

25-
async function createGitHubClientForRunner(runner: RunnerInfo, orgLevel: boolean): Promise<Octokit> {
26-
const githubClient = await createAppClient(await createGithubAppAuth(undefined));
27-
const repo = getRepo(runner, orgLevel);
25+
function createGitHubClientForRunnerFactory(): (runner: RunnerInfo, orgLevel: boolean) => Promise<Octokit> {
26+
const cache: Map<string, Octokit> = new Map();
27+
28+
return async (runner: RunnerInfo, orgLevel: boolean) => {
29+
const githubClient = await createAppClient(await createGithubAppAuth(undefined));
30+
const repo = getRepo(runner, orgLevel);
31+
const key = orgLevel ? repo.repoOwner : repo.repoOwner + repo.repoName;
32+
const cachedOctokit = cache.get(key);
33+
34+
if (cachedOctokit) {
35+
console.debug(`[createGitHubClientForRunner] Cache hit for ${key}`);
36+
return cachedOctokit;
37+
}
38+
39+
console.debug(`[createGitHubClientForRunner] Cache miss for ${key}`);
40+
const installationId = orgLevel
41+
? (
42+
await githubClient.apps.getOrgInstallation({
43+
org: repo.repoOwner,
44+
})
45+
).data.id
46+
: (
47+
await githubClient.apps.getRepoInstallation({
48+
owner: repo.repoOwner,
49+
repo: repo.repoName,
50+
})
51+
).data.id;
52+
const octokit = await createInstallationClient(await createGithubAppAuth(installationId));
53+
cache.set(key, octokit);
54+
55+
return octokit;
56+
};
57+
}
58+
59+
/**
60+
* Extract the inner type of a promise if any
61+
*/
62+
export type UnboxPromise<T> = T extends Promise<infer U> ? U : T;
2863

29-
const installationId = orgLevel
30-
? (
31-
await githubClient.apps.getOrgInstallation({
64+
type GhRunners = UnboxPromise<ReturnType<Octokit['actions']['listSelfHostedRunnersForRepo']>>['data']['runners'];
65+
66+
function listGithubRunnersFactory(): (
67+
client: Octokit,
68+
runner: RunnerInfo,
69+
enableOrgLevel: boolean,
70+
) => Promise<GhRunners> {
71+
const cache: Map<string, GhRunners> = new Map();
72+
return async (client: Octokit, runner: RunnerInfo, enableOrgLevel: boolean) => {
73+
const repo = getRepo(runner, enableOrgLevel);
74+
const key = enableOrgLevel ? repo.repoOwner : repo.repoOwner + repo.repoName;
75+
const cachedRunners = cache.get(key);
76+
if (cachedRunners) {
77+
console.debug(`[listGithubRunners] Cache hit for ${key}`);
78+
return cachedRunners;
79+
}
80+
81+
console.debug(`[listGithubRunners] Cache miss for ${key}`);
82+
const runners = enableOrgLevel
83+
? await client.paginate(client.actions.listSelfHostedRunnersForOrg, {
3284
org: repo.repoOwner,
3385
})
34-
).data.id
35-
: (
36-
await githubClient.apps.getRepoInstallation({
86+
: await client.paginate(client.actions.listSelfHostedRunnersForRepo, {
3787
owner: repo.repoOwner,
3888
repo: repo.repoName,
39-
})
40-
).data.id;
89+
});
90+
cache.set(key, runners);
4191

42-
return createInstallationClient(await createGithubAppAuth(installationId));
92+
return runners;
93+
};
4394
}
4495

4596
function runnerMinimumTimeExceeded(runner: RunnerInfo, minimumRunningTimeInMinutes: string): boolean {
@@ -102,24 +153,20 @@ export async function scaleDown(): Promise<void> {
102153
return;
103154
}
104155

156+
const createGitHubClientForRunner = createGitHubClientForRunnerFactory();
157+
const listGithubRunners = listGithubRunnersFactory();
158+
105159
for (const ec2runner of runners) {
106160
if (!runnerMinimumTimeExceeded(ec2runner, minimumRunningTimeInMinutes)) {
107161
continue;
108162
}
109163

110164
const githubAppClient = await createGitHubClientForRunner(ec2runner, enableOrgLevel);
111-
const repo = getRepo(ec2runner, enableOrgLevel);
112-
const registered = enableOrgLevel
113-
? await githubAppClient.actions.listSelfHostedRunnersForOrg({
114-
org: repo.repoOwner,
115-
})
116-
: await githubAppClient.actions.listSelfHostedRunnersForRepo({
117-
owner: repo.repoOwner,
118-
repo: repo.repoName,
119-
});
120165

166+
const repo = getRepo(ec2runner, enableOrgLevel);
167+
const ghRunners = await listGithubRunners(githubAppClient, ec2runner, enableOrgLevel);
121168
let orphanEc2Runner = true;
122-
for (const ghRunner of registered.data.runners) {
169+
for (const ghRunner of ghRunners) {
123170
const runnerName = ghRunner.name as string;
124171
if (runnerName === ec2runner.instanceId) {
125172
orphanEc2Runner = false;

0 commit comments

Comments
 (0)