Fix scaleUp not waiting for rateLimit response and improved testing (#…

…5797)

terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/gh-runners.test.ts

@@ -1,13 +1,14 @@
 import {
-  GhRunners,
   createGitHubClientForRunnerInstallId,
   createGitHubClientForRunnerOrg,
   createGitHubClientForRunnerRepo,
   createRegistrationTokenOrg,
   createRegistrationTokenRepo,
+  getGitHubRateLimit,
   getRunnerOrg,
   getRunnerRepo,
   getRunnerTypes,
+  GhRunners,
   listGithubRunnersOrg,
   listGithubRunnersRepo,
   removeGithubRunnerOrg,
@@ -20,7 +21,7 @@ import { ScaleUpMetrics } from './metrics';
 import { Config } from './config';
 import { Octokit } from '@octokit/rest';
 import { mocked } from 'ts-jest/utils';
-import { locallyCached } from './cache';
+import { locallyCached, redisCached } from './cache';
 import nock from 'nock';
 
 const mockEC2 = {
@@ -1081,3 +1082,51 @@ describe('createRegistrationToken', () => {
     });
   });
 });
+
+describe('getGitHubRateLimit', () => {
+  const repo = { owner: 'owner', repo: 'repo' };
+
+  it('succeeds, make sure it is using the cache mechanism', async () => {
+    const mocckedRedisCache = mocked(redisCached).mockImplementationOnce((_, __, ___, ____, f) => f());
+    const mockCreateGithubAuth = mocked(createGithubAuth);
+    const mockCreateOctoClient = mocked(createOctoClient);
+    const getRepoInstallation = jest.fn().mockResolvedValue({
+      data: { id: 'mockReturnValueOnce1' },
+    });
+    const mockedOctokit = {
+      actions: {
+        deleteSelfHostedRunnerFromRepo: jest.fn().mockResolvedValue({
+          status: 204,
+        }),
+      },
+      apps: { getRepoInstallation: getRepoInstallation },
+      rateLimit: {
+        get: jest.fn().mockResolvedValue({
+          data: {},
+          headers: {
+            'x-ratelimit-limit': '5000',
+            'x-ratelimit-remaining': '4999',
+            'x-ratelimit-used': '1',
+          },
+          status: 200,
+        }),
+      },
+    };
+
+    mockCreateGithubAuth.mockResolvedValueOnce('token1');
+    mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);
+    mockCreateGithubAuth.mockResolvedValueOnce('token2');
+    mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);
+
+    await resetGHRunnersCaches();
+    const response = await getGitHubRateLimit(repo, metrics);
+
+    expect(mocckedRedisCache).toBeCalledTimes(2);
+    expect(mockedOctokit.rateLimit.get).toBeCalledTimes(1);
+    expect(response).toEqual({
+      limit: 5000,
+      remaining: 4999,
+      used: 1,
+    });
+  });
+});

terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/gh-runners.ts

@@ -12,6 +12,12 @@ import YAML from 'yaml';
 const ghMainClientCache = new LRU({ maxAge: 10 * 1000 });
 const ghClientCache = new LRU({ maxAge: 10 * 1000 });
 
+export interface GHRateLimitInfo {
+  limit: number;
+  remaining: number;
+  used: number;
+}
+
 export async function resetGHRunnersCaches() {
   await redisClearCacheKeyPattern('ghRunners', '');
   clearLocalCacheNamespace('ghRunners');
@@ -366,13 +372,15 @@ export async function getRunnerTypes(
 
       Array.from(result.keys()).forEach((key) => {
         const runnerType = result.get(key);
+        /* istanbul ignore next */
         if (runnerType?.variants === undefined) {
           return;
         }
 
         if (runnerType.variants.size > 0) {
           Array.from(runnerType.variants.keys()).forEach((variant) => {
             const variantType = runnerType.variants?.get(variant);
+            /* istanbul ignore next */
             if (!variantType) {
               return;
             }
@@ -400,6 +408,7 @@ export async function getRunnerTypes(
               typeof runnerType.instance_type === 'string' &&
               alphaNumericStr.test(runnerType.instance_type) &&
               ['linux', 'windows'].includes(runnerType.os) &&
+              /* istanbul ignore next */
               (runnerType.labels?.every((label) => typeof label === 'string' && alphaNumericStr.test(label)) ?? true) &&
               (typeof runnerType.disk_size === 'number' || runnerType.disk_size === undefined) &&
               (typeof runnerType.min_available === 'number' || runnerType.min_available === undefined) &&
@@ -517,9 +526,9 @@ export async function createRegistrationTokenOrg(
   }
 }
 
-export async function getGitHubRateLimit(repo: Repo, installationId: number, metrics: Metrics): Promise<void> {
+export async function getGitHubRateLimit(repo: Repo, metrics: Metrics): Promise<GHRateLimitInfo> {
   try {
-    const { used, limit, remaining } = await locallyCached('ghRunners', 'getGitHubRateLimit', 10, async () => {
+    return await redisCached('ghRunners', 'getGitHubRateLimit', 10, 0.5, async () => {
       try {
         const client = await createGitHubClientForRunnerRepo(repo, metrics);
 
@@ -535,13 +544,13 @@ export async function getGitHubRateLimit(repo: Repo, installationId: number, met
 
         return { used, limit, remaining };
       } catch (e) {
+        /* istanbul ignore next */
         console.error(`[getGitHubRateLimit]: <anonymous> ${e}`);
         throw e;
       }
     });
-
-    metrics.gitHubRateLimitStats(limit, remaining, used);
   } catch (e) {
+    /* istanbul ignore next */
     console.error(`[getGitHubRateLimit]: ${e}`);
     throw e;
   }

terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts

@@ -2,6 +2,7 @@ import { createRunner } from './runners';
 import {
   createRegistrationTokenOrg,
   createRegistrationTokenRepo,
+  getGitHubRateLimit,
   getRunnerTypes,
   listGithubRunnersOrg,
   listGithubRunnersRepo,
@@ -23,6 +24,8 @@ beforeEach(() => {
   jest.clearAllMocks();
   jest.restoreAllMocks();
   nock.disableNetConnect();
+
+  mocked(getGitHubRateLimit).mockResolvedValue({ limit: 5000, remaining: 4999, used: 1 });
 });
 
 const baseCfg = {

terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts

@@ -55,7 +55,13 @@ export async function scaleUp(
   metrics.runRepo(repo);
   metrics.run();
 
-  getGitHubRateLimit(repo, Number(payload.installationId), metrics);
+  try {
+    const ghLimitInfo = await getGitHubRateLimit(repo, metrics);
+    metrics.gitHubRateLimitStats(ghLimitInfo.limit, ghLimitInfo.remaining, ghLimitInfo.used);
+  } catch (e) {
+    /* istanbul ignore next */
+    console.error(`Error getting GitHub rate limit: ${e}`);
+  }
 
   const scaleConfigRepo = {
     owner: repo.owner,