Skip to content

FIX: Don't remove EC2 instance when fails to remove githubRunner #904

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 19, 2022
Merged

FIX: Don't remove EC2 instance when fails to remove githubRunner #904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91908d9
FIX: Don't remove EC2 instance when fails to remove githubRunner
jeanschmidt Oct 18, 2022
068524b
Fix scaleDown tests, and improved code-flow
jeanschmidt Oct 18, 2022
c236425
Improved loggind, add expBackOff for some high throughput functions i…
jeanschmidt Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 4 additions & 23 deletions ...rm-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/gh-runners.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
removeGithubRunnerRepo,
resetGHRunnersCaches,
} from './gh-runners';
import { RunnerInfo } from './utils';
import { createGithubAuth, createOctoClient } from './gh-auth';
import { ScaleUpMetrics } from './metrics';

Expand Down Expand Up @@ -390,12 +389,6 @@ describe('listGithubRunners', () => {
});

describe('removeGithubRunnerRepo', () => {
const repo = { owner: 'owner', repo: 'repo' };
const irrelevantRunnerInfo: RunnerInfo = {
...repo,
instanceId: '113',
};

it('succeeds', async () => {
const runnerId = 33;
const repo = { owner: 'owner', repo: 'repo' };
Expand All @@ -419,16 +412,13 @@ describe('removeGithubRunnerRepo', () => {
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);

resetGHRunnersCaches();
await removeGithubRunnerRepo(irrelevantRunnerInfo, runnerId, repo, metrics);
await removeGithubRunnerRepo(runnerId, repo, metrics);

expect(mockedOctokit.actions.deleteSelfHostedRunnerFromRepo).toBeCalledWith({
...repo,
runner_id: runnerId,
});
expect(getRepoInstallation).toBeCalled();
expect(mockEC2.terminateInstances).toBeCalledWith({
InstanceIds: [irrelevantRunnerInfo.instanceId],
});
});

it('fails', async () => {
Expand All @@ -454,23 +444,18 @@ describe('removeGithubRunnerRepo', () => {
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);

resetGHRunnersCaches();
await removeGithubRunnerRepo(irrelevantRunnerInfo, runnerId, repo, metrics);
await expect(removeGithubRunnerRepo(runnerId, repo, metrics)).rejects.toThrow();

expect(mockedOctokit.actions.deleteSelfHostedRunnerFromRepo).toBeCalledWith({
...repo,
runner_id: runnerId,
});
expect(getRepoInstallation).toBeCalled();
expect(mockEC2.terminateInstances).not.toBeCalled();
});
});

describe('removeGithubRunnerOrg', () => {
const org = 'mockedOrg';
const irrelevantRunnerInfo: RunnerInfo = {
org: org,
instanceId: '113',
};

it('succeeds', async () => {
const runnerId = 33;
Expand All @@ -494,16 +479,13 @@ describe('removeGithubRunnerOrg', () => {
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);

resetGHRunnersCaches();
await removeGithubRunnerOrg(irrelevantRunnerInfo, runnerId, org, metrics);
await removeGithubRunnerOrg(runnerId, org, metrics);

expect(mockedOctokit.actions.deleteSelfHostedRunnerFromOrg).toBeCalledWith({
org: org,
runner_id: runnerId,
});
expect(getOrgInstallation).toBeCalled();
expect(mockEC2.terminateInstances).toBeCalledWith({
InstanceIds: [irrelevantRunnerInfo.instanceId],
});
});

it('fails', async () => {
Expand All @@ -528,14 +510,13 @@ describe('removeGithubRunnerOrg', () => {
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);

resetGHRunnersCaches();
await removeGithubRunnerOrg(irrelevantRunnerInfo, runnerId, org, metrics);
await expect(removeGithubRunnerOrg(runnerId, org, metrics)).rejects.toThrow();

expect(mockedOctokit.actions.deleteSelfHostedRunnerFromOrg).toBeCalledWith({
org: org,
runner_id: runnerId,
});
expect(getOrgInstallation).toBeCalled();
expect(mockEC2.terminateInstances).not.toBeCalled();
});
});

Expand Down
115 changes: 54 additions & 61 deletions terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/gh-runners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Repo, RunnerInfo, getRepoKey } from './utils';
import { RunnerType, terminateRunner } from './runners';
import { Repo, getRepoKey, expBackOff } from './utils';
import { RunnerType } from './runners';
import { createGithubAuth, createOctoClient } from './gh-auth';

import { Config } from './config';
Expand Down Expand Up @@ -127,10 +127,10 @@ type UnboxPromise<T> = T extends Promise<infer U> ? U : T;

export type GhRunners = UnboxPromise<ReturnType<Octokit['actions']['listSelfHostedRunnersForRepo']>>['data']['runners'];

export async function removeGithubRunnerRepo(ec2runner: RunnerInfo, ghRunnerId: number, repo: Repo, metrics: Metrics) {
try {
const githubAppClient = await createGitHubClientForRunnerRepo(repo, metrics);
const result = await metrics.trackRequest(
export async function removeGithubRunnerRepo(ghRunnerId: number, repo: Repo, metrics: Metrics) {
const githubAppClient = await createGitHubClientForRunnerRepo(repo, metrics);
const result = await expBackOff(() => {
return metrics.trackRequest(
metrics.deleteSelfHostedRunnerFromRepoGHCallSuccess,
metrics.deleteSelfHostedRunnerFromRepoGHCallFailure,
() => {
Expand All @@ -140,26 +140,21 @@ export async function removeGithubRunnerRepo(ec2runner: RunnerInfo, ghRunnerId:
});
},
);
});

/* istanbul ignore next */
if (result?.status == 204) {
await terminateRunner(ec2runner, metrics);
console.info(
`AWS runner instance '${ec2runner.instanceId}' [${ec2runner.runnerType}] is terminated ` +
`and GitHub runner is de-registered. (removeGithubRunnerRepo)`,
);
}
} catch (e) {
console.warn(
`Error scaling down (removeGithubRunnerRepo) '${ec2runner.instanceId}' [${ec2runner.runnerType}]: ${e}`,
/* istanbul ignore next */
if ((result?.status ?? 0) != 204) {
throw (
`Request deleteSelfHostedRunnerFromRepoGHCallSuccess returned status code different than 204: ` +
`${result?.status ?? 0} for ${repo} ${ghRunnerId}`
);
}
}

export async function removeGithubRunnerOrg(ec2runner: RunnerInfo, ghRunnerId: number, org: string, metrics: Metrics) {
try {
const githubAppClient = await createGitHubClientForRunnerOrg(org, metrics);
const result = await metrics.trackRequest(
export async function removeGithubRunnerOrg(ghRunnerId: number, org: string, metrics: Metrics) {
const githubAppClient = await createGitHubClientForRunnerOrg(org, metrics);
const result = await expBackOff(() => {
return metrics.trackRequest(
metrics.deleteSelfHostedRunnerFromOrgGHCallSuccess,
metrics.deleteSelfHostedRunnerFromOrgGHCallFailure,
() => {
Expand All @@ -169,18 +164,13 @@ export async function removeGithubRunnerOrg(ec2runner: RunnerInfo, ghRunnerId: n
});
},
);
});

/* istanbul ignore next */
if (result?.status == 204) {
await terminateRunner(ec2runner, metrics);
console.info(
`AWS runner instance '${ec2runner.instanceId}' [${ec2runner.runnerType}] is terminated ` +
`and GitHub runner is de-registered. (removeGithubRunnerOrg)`,
);
}
} catch (e) {
console.warn(
`Error scaling down (removeGithubRunnerOrg) '${ec2runner.instanceId}' [${ec2runner.runnerType}]: ${e}`,
/* istanbul ignore next */
if ((result?.status ?? 0) != 204) {
throw (
`Request deleteSelfHostedRunnerFromRepoGHCallSuccess returned status code different than 204: ` +
`${result?.status ?? 0} for ${org} ${ghRunnerId}`
);
}
}
Expand Down Expand Up @@ -245,7 +235,7 @@ async function listGithubRunners(key: string, listCallback: () => Promise<GhRunn
}

console.debug(`[listGithubRunners] Cache miss for ${key}`);
const runners = await listCallback();
const runners = await expBackOff(listCallback);
ghRunnersCache.set(key, runners);
return runners;
} catch (e) {
Expand All @@ -261,16 +251,18 @@ export async function getRunnerRepo(repo: Repo, runnerID: string, metrics: Metri

try {
return (
await metrics.trackRequest(
metrics.getSelfHostedRunnerForRepoGHCallSuccess,
metrics.getSelfHostedRunnerForRepoGHCallFailure,
() => {
return client.actions.getSelfHostedRunnerForRepo({
...repo,
runner_id: runnerID as unknown as number,
});
},
)
await expBackOff(() => {
return metrics.trackRequest(
metrics.getSelfHostedRunnerForRepoGHCallSuccess,
metrics.getSelfHostedRunnerForRepoGHCallFailure,
() => {
return client.actions.getSelfHostedRunnerForRepo({
...repo,
runner_id: runnerID as unknown as number,
});
},
);
})
).data;
} catch (e) {
console.warn(`[getRunnerRepo <inner try>]: ${e}`);
Expand All @@ -283,16 +275,18 @@ export async function getRunnerOrg(org: string, runnerID: string, metrics: Metri

try {
return (
await metrics.trackRequest(
metrics.getSelfHostedRunnerForOrgGHCallSuccess,
metrics.getSelfHostedRunnerForOrgGHCallFailure,
() => {
return client.actions.getSelfHostedRunnerForOrg({
org: org,
runner_id: runnerID as unknown as number,
});
},
)
await expBackOff(() => {
return metrics.trackRequest(
metrics.getSelfHostedRunnerForOrgGHCallSuccess,
metrics.getSelfHostedRunnerForOrgGHCallFailure,
() => {
return client.actions.getSelfHostedRunnerForOrg({
org: org,
runner_id: runnerID as unknown as number,
});
},
);
})
).data;
} catch (e) {
console.warn(`[getRunnerOrg <inner try>]: ${e}`);
Expand All @@ -309,8 +303,9 @@ export async function getRunnerTypes(
try {
const runnerTypeKey = getRepoKey(repo);

if (runnerTypeCache.get(runnerTypeKey) !== undefined) {
return runnerTypeCache.get(runnerTypeKey) as Map<string, RunnerType>;
const runnerTypeCacheActualContent = runnerTypeCache.get(runnerTypeKey);
if (runnerTypeCacheActualContent !== undefined) {
return runnerTypeCacheActualContent as Map<string, RunnerType>;
}
console.debug(`[getRunnerTypes] cache miss for ${runnerTypeKey}`);

Expand All @@ -320,16 +315,14 @@ export async function getRunnerTypes(
? await createGitHubClientForRunnerOrg(repo.owner, metrics)
: await createGitHubClientForRunnerRepo(repo, metrics);

const response = await metrics.trackRequest(
metrics.reposGetContentGHCallSuccess,
metrics.reposGetContentGHCallFailure,
() => {
const response = await expBackOff(() => {
return metrics.trackRequest(metrics.reposGetContentGHCallSuccess, metrics.reposGetContentGHCallFailure, () => {
return githubAppClient.repos.getContent({
...repo,
path: filepath,
});
},
);
});
});

/* istanbul ignore next */
const { content } = { ...(response?.data || {}) };
Expand Down
59 changes: 59 additions & 0 deletions terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,48 @@ export class ScaleDownMetrics extends Metrics {
this.countEntry(`run.ec2runners.${org}.${ec2Runner.runnerType}.notFound`);
}

/* istanbul ignore next */
runnerGhTerminateSuccessOrg(org: string, ec2runner: RunnerInfo) {
this.countEntry(`run.ghRunner.byOrg.${org}.terminate.success`);
this.countEntry(`run.ghRunner.byOrg.${org}.byType.${ec2runner.runnerType}.terminate.success`);
this.countEntry(`run.ghRunner.byType.${ec2runner.runnerType}.terminate.success`);
}

/* istanbul ignore next */
runnerGhTerminateSuccessRepo(repo: Repo, ec2runner: RunnerInfo) {
this.countEntry(`run.ghRunner.byRepo.${repo.owner}.${repo.repo}.terminate.success`);
this.countEntry(`run.ghRunner.byRepo.${repo.owner}.${repo.repo}.byType.${ec2runner.runnerType}.terminate.success`);
this.countEntry(`run.ghRunner.byType.${ec2runner.runnerType}.terminate.success`);
}

/* istanbul ignore next */
runnerGhTerminateFailureOrg(org: string, ec2runner: RunnerInfo) {
this.countEntry(`run.ghRunner.byOrg.${org}.terminate.failure`);
this.countEntry(`run.ghRunner.byOrg.${org}.byType.${ec2runner.runnerType}.terminate.failure`);
this.countEntry(`run.ghRunner.byType.${ec2runner.runnerType}.terminate.failure`);
}

/* istanbul ignore next */
runnerGhTerminateFailureRepo(repo: Repo, ec2runner: RunnerInfo) {
this.countEntry(`run.ghRunner.byRepo.${repo.owner}.${repo.repo}.terminate.failure`);
this.countEntry(`run.ghRunner.byRepo.${repo.owner}.${repo.repo}.byType.${ec2runner.runnerType}.terminate.failure`);
this.countEntry(`run.ghRunner.byType.${ec2runner.runnerType}.terminate.failure`);
}

/* istanbul ignore next */
runnerGhTerminateNotFoundOrg(org: string, ec2runner: RunnerInfo) {
this.countEntry(`run.ghRunner.byOrg.${org}.terminate.notfound`);
this.countEntry(`run.ghRunner.byOrg.${org}.byType.${ec2runner.runnerType}.terminate.notfound`);
this.countEntry(`run.ghRunner.byType.${ec2runner.runnerType}.terminate.notfound`);
}

/* istanbul ignore next */
runnerGhTerminateNotFoundRepo(repo: Repo, ec2runner: RunnerInfo) {
this.countEntry(`run.ghRunner.byRepo.${repo.owner}.${repo.repo}.terminate.notfound`);
this.countEntry(`run.ghRunner.byRepo.${repo.owner}.${repo.repo}.byType.${ec2runner.runnerType}.terminate.notfound`);
this.countEntry(`run.ghRunner.byType.${ec2runner.runnerType}.terminate.notfound`);
}

/* istanbul ignore next */
runnerTerminateSuccess(ec2Runner: RunnerInfo) {
this.countEntry('run.ec2Runners.terminate.success');
Expand Down Expand Up @@ -724,6 +766,23 @@ export class ScaleDownMetrics extends Metrics {
this.countEntry(`run.ec2runners.${repo.owner}.${repo.repo}.terminate.failure`);
}
}

/* istanbul ignore next */
runnerTerminateSkipped(ec2Runner: RunnerInfo) {
this.countEntry('run.ec2Runners.terminate.skipped');
this.countEntry(`run.ec2runners.${ec2Runner.runnerType}.terminate.skipped`);

if (ec2Runner.org !== undefined) {
this.countEntry(`run.ec2runners.${ec2Runner.org}.${ec2Runner.runnerType}.terminate.skipped`);
this.countEntry(`run.ec2runners.${ec2Runner.org}.terminate.skipped`);
}

if (ec2Runner.repo !== undefined) {
const repo = getRepo(ec2Runner.repo as string);
this.countEntry(`run.ec2runners.${repo.owner}.${repo.repo}.${ec2Runner.runnerType}.terminate.skipped`);
this.countEntry(`run.ec2runners.${repo.owner}.${repo.repo}.terminate.skipped`);
}
}
}

export interface sendMetricsTimeoutVars {
Expand Down
Loading