Skip to content

fix: Remove Orphan AWS runners #79

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 2 commits into from
Jul 21, 2020
Merged

fix: Remove Orphan AWS runners #79

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dde244
Remove Orphan AWS runners
npalm Jul 20, 2020
421cfeb
Remove duplicated check for time exceede
npalm Jul 21, 2020
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
34 changes: 24 additions & 10 deletions modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface TestData {
}

const environment = 'unit-test-environment';
const minimumRunningTimeInMinutes = 15;
const TEST_DATA: TestData = {
repositoryName: 'hello-world',
repositoryOwner: 'Codertocat',
Expand All @@ -58,7 +59,17 @@ const DEFAULT_RUNNERS = [
},
{
instanceId: 'i-not-registered-104',
launchTime: moment(new Date()).subtract(5, 'minutes').toDate(),
launchTime: moment(new Date())
.subtract(minimumRunningTimeInMinutes - 1, 'minutes')
.toDate(),
repo: `doe/another-repo`,
org: undefined,
},
{
instanceId: 'i-not-registered-105',
launchTime: moment(new Date())
.subtract(minimumRunningTimeInMinutes + 5, 'minutes')
.toDate(),
repo: `doe/another-repo`,
org: undefined,
},
Expand All @@ -73,7 +84,7 @@ const DEFAULT_REGISTERED_RUNNERS: any = {
},
{
id: 102,
name: 'i-idle-101',
name: 'i-idle-102',
},
{
id: 103,
Expand All @@ -91,8 +102,7 @@ describe('scaleDown', () => {
process.env.GITHUB_APP_CLIENT_SECRET = 'TEST_CLIENT_SECRET';
process.env.RUNNERS_MAXIMUM_COUNT = '3';
process.env.ENVIRONMENT = environment;
const minimumRunningTimeInMinutes = '15';
process.env.MINIMUM_RUNNING_TIME_IN_MINUTES = minimumRunningTimeInMinutes;
process.env.MINIMUM_RUNNING_TIME_IN_MINUTES = minimumRunningTimeInMinutes.toString();
jest.clearAllMocks();
mockOctokit.apps.getOrgInstallation.mockImplementation(() => ({
data: {
Expand Down Expand Up @@ -166,15 +176,17 @@ describe('scaleDown', () => {
});
});

it('Terminate 2 of 4 runners for repo.', async () => {
it('Terminate 3 of 5 runners for repo.', async () => {
await scaleDown();
expect(listRunners).toBeCalledWith({
environment: environment,
});

expect(mockOctokit.apps.getRepoInstallation).toBeCalled();

expect(terminateRunner).toBeCalledTimes(2);
expect(terminateRunner).toBeCalledTimes(3);
for (const toTerminate of [DEFAULT_RUNNERS[0], DEFAULT_RUNNERS[1], DEFAULT_RUNNERS[4]]) {
expect(terminateRunner).toHaveBeenCalledWith(toTerminate);
}
});
});

Expand All @@ -187,15 +199,17 @@ describe('scaleDown', () => {
});
});

it('Terminate 2 of 4 runners for org.', async () => {
it('Terminate 3 of 5 runners for org.', async () => {
await scaleDown();
expect(listRunners).toBeCalledWith({
environment: environment,
});

expect(mockOctokit.apps.getOrgInstallation).toBeCalled();

expect(terminateRunner).toBeCalledTimes(2);
expect(terminateRunner).toBeCalledTimes(3);
for (const toTerminate of [DEFAULT_RUNNERS[0], DEFAULT_RUNNERS[1], DEFAULT_RUNNERS[4]]) {
expect(terminateRunner).toHaveBeenCalledWith(toTerminate);
}
});
});
});
12 changes: 12 additions & 0 deletions modules/runners/lambdas/runners/src/scale-runners/scale-down.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,11 @@ export async function scaleDown(): Promise<void> {
repo: repo.repoName,
});

let orphanEc2Runner = true;
for (const ghRunner of registered.data.runners) {
const runnerName = ghRunner.name as string;
if (runnerName === ec2runner.instanceId && runnerMinimumTimeExceeded(ec2runner, minimumRunningTimeInMinutes)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split up this if statement to determine if a EC2 instance is orphaned. Then only try to delete if the runnerMinimumTimeExceeded function returns true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gertjanmaas created a fix

orphanEc2Runner = false;
try {
const result = enableOrgLevel
? await githubAppClient.actions.deleteSelfHostedRunnerFromOrg({
Expand All @@ -100,5 +102,15 @@ export async function scaleDown(): Promise<void> {
}
}
}

// Remove orphan AWS runners.
if (orphanEc2Runner && runnerMinimumTimeExceeded(ec2runner, minimumRunningTimeInMinutes)) {
console.info(`Runner '${ec2runner.instanceId}' is orphan, and will be removed.`);
try {
await terminateRunner(ec2runner);
} catch (e) {
console.debug(`Orphan runner '${ec2runner.instanceId}' cannot be removed.`);
}
}
}
}