Skip to content

Commit 4f3e1b2

Browse files
committed
Add the log uploader bot
**Impact:** none until `LOG_UPLOADER_REPOS` is set -- an unset allowlist disables the handler **Risk:** low ## What A new Probot handler on `workflow_job` that, when a job completes, asks the `gha-log-uploader` lambda to archive its log. Adds `lib/lambda.ts`, which wraps the async invoke, and adds the AWS SDK `client-lambda` package. Which repos are enabled is controlled by `LOG_UPLOADER_REPOS`, a comma-separated list of `owner/repo` or `owner/*`. Unset means the handler does nothing. ghstack-source-id: cfbf717 Pull-Request: #8594
1 parent d07d621 commit 4f3e1b2

6 files changed

Lines changed: 515 additions & 1 deletion

File tree

torchci/lib/bot/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import ciflowPushTrigger from "./ciflowPushTrigger";
88
import codevNoWritePerm from "./codevNoWritePermBot";
99
import crcrOncallBot from "./crcrOncallBot";
1010
import drciBot from "./drciBot";
11+
import logUploader from "./logUploader";
1112
import nitpickBot from "./nitpickBot";
1213
import pytorchBot from "./pytorchBot";
1314
import retryBot from "./retryBot";
@@ -25,6 +26,7 @@ export default function bot(app: Probot) {
2526
codevNoWritePerm(app);
2627
crcrOncallBot(app);
2728
drciBot(app);
29+
logUploader(app);
2830
nitpickBot(app);
2931
pytorchBot(app);
3032
retryBot(app);

torchci/lib/bot/logUploader.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { invokeLogUploader } from "lib/lambda";
2+
import { Context, Probot } from "probot";
3+
import { isPyTorchbotSupportedOrg, isVLLM } from "./utils";
4+
5+
/**
6+
* Comma-separated list of repos whose logs this handler uploads. Entries are
7+
* either `owner/repo` or `owner/*`; an empty or unset value disables the
8+
* handler entirely.
9+
*
10+
* This exists so the cutover from the github-status-test webhook can be done a
11+
* repo at a time. While a repo still has that webhook, both paths write the same
12+
* S3 key -- harmless, but it doubles classifier calls, so the allowlist is what
13+
* bounds the overlap. Rolling back is editing this variable.
14+
*/
15+
export function parseRepoAllowlist(raw: string | undefined): Set<string> {
16+
return new Set(
17+
(raw ?? "")
18+
.split(",")
19+
.map((entry) => entry.trim().toLowerCase())
20+
.filter((entry) => entry.length > 0)
21+
);
22+
}
23+
24+
export function isRepoEnabled(
25+
allowlist: Set<string>,
26+
owner: string,
27+
repo: string
28+
): boolean {
29+
return (
30+
allowlist.has(`${owner}/${repo}`.toLowerCase()) ||
31+
allowlist.has(`${owner.toLowerCase()}/*`)
32+
);
33+
}
34+
35+
async function handleCompletedJob(event: Context<"workflow_job">) {
36+
if (event.payload.action !== "completed") {
37+
return;
38+
}
39+
40+
const owner = event.payload.repository.owner.login;
41+
const repo = event.payload.repository.name;
42+
if (!isPyTorchbotSupportedOrg(owner) && !isVLLM(owner)) {
43+
event.log(`${__filename} isn't enabled on ${owner}'s repos`);
44+
return;
45+
}
46+
47+
const allowlist = parseRepoAllowlist(process.env.LOG_UPLOADER_REPOS);
48+
if (!isRepoEnabled(allowlist, owner, repo)) {
49+
return;
50+
}
51+
52+
try {
53+
await invokeLogUploader({
54+
repo: event.payload.repository.full_name,
55+
job_id: event.payload.workflow_job.id,
56+
conclusion: event.payload.workflow_job.conclusion,
57+
});
58+
} catch (error) {
59+
// Never fail the webhook over a log. GitHub would redeliver the whole event,
60+
// re-running every other handler, to retry something Dr.CI already repairs
61+
// on its own via backfillMissingLog.
62+
event.log.error(
63+
`Failed to queue a log upload for ${event.payload.repository.full_name} ` +
64+
`job ${event.payload.workflow_job.id}: ${error}`
65+
);
66+
}
67+
}
68+
69+
export default function logUploader(app: Probot) {
70+
app.on("workflow_job", handleCompletedJob);
71+
}

torchci/lib/lambda.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import {
2+
InvocationType,
3+
InvokeCommand,
4+
LambdaClient,
5+
} from "@aws-sdk/client-lambda";
6+
7+
export const GHA_LOG_UPLOADER_FUNCTION = "gha-log-uploader";
8+
9+
export function getLambdaClient(): LambdaClient {
10+
return new LambdaClient({
11+
region: "us-east-1",
12+
credentials: {
13+
accessKeyId: process.env.OUR_AWS_ACCESS_KEY_ID!,
14+
secretAccessKey: process.env.OUR_AWS_SECRET_ACCESS_KEY!,
15+
},
16+
});
17+
}
18+
19+
export interface LogUploadRequest {
20+
repo: string;
21+
job_id: number;
22+
conclusion?: string | null;
23+
}
24+
25+
/**
26+
* Ask gha-log-uploader to archive a job's log to S3.
27+
*
28+
* Invoked with InvocationType Event, so this returns as soon as Lambda accepts
29+
* the payload rather than waiting on the GitHub download. That matters because
30+
* the caller is a Probot webhook handler: Probot only acks GitHub once every
31+
* handler resolves, and nothing runs after a Vercel function returns, so the
32+
* handoff has to be both awaited and bounded.
33+
*
34+
* Delivery failures are Lambda's problem from here -- it retries twice and then
35+
* DLQs. Failures to hand off at all are the caller's, and are non-fatal: Dr.CI
36+
* re-requests a missing log through backfillMissingLog on its next run.
37+
*/
38+
export async function invokeLogUploader(
39+
request: LogUploadRequest
40+
): Promise<void> {
41+
await getLambdaClient().send(
42+
new InvokeCommand({
43+
FunctionName: GHA_LOG_UPLOADER_FUNCTION,
44+
InvocationType: InvocationType.Event,
45+
Payload: Buffer.from(JSON.stringify(request)),
46+
})
47+
);
48+
}

torchci/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
},
1616
"dependencies": {
1717
"@aws-sdk/client-dynamodb": "^3.347.1",
18+
"@aws-sdk/client-lambda": "^3.347.1",
1819
"@aws-sdk/client-s3": "^3.347.1",
1920
"@aws-sdk/lib-dynamodb": "^3.72.0",
2021
"@clickhouse/client": "^1.11.1",

torchci/test/logUploader.test.ts

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
import logUploader, {
2+
isRepoEnabled,
3+
parseRepoAllowlist,
4+
} from "lib/bot/logUploader";
5+
import * as lambda from "lib/lambda";
6+
import nock from "nock";
7+
import { Probot } from "probot";
8+
import * as utils from "./utils";
9+
10+
nock.disableNetConnect();
11+
12+
function workflowJobPayload({
13+
owner = "meta-pytorch",
14+
name = "torchcomms",
15+
action = "completed",
16+
id = 12345,
17+
conclusion = "failure" as string | null,
18+
} = {}) {
19+
return {
20+
action,
21+
workflow_job: { id, conclusion },
22+
repository: {
23+
full_name: `${owner}/${name}`,
24+
name,
25+
owner: { login: owner },
26+
},
27+
};
28+
}
29+
30+
describe("parseRepoAllowlist", () => {
31+
test("an unset value disables the handler", () => {
32+
expect(parseRepoAllowlist(undefined).size).toBe(0);
33+
expect(parseRepoAllowlist("").size).toBe(0);
34+
});
35+
36+
test("entries are trimmed and lowercased", () => {
37+
expect(parseRepoAllowlist(" Pytorch/PyTorch , meta-pytorch/* ")).toEqual(
38+
new Set(["pytorch/pytorch", "meta-pytorch/*"])
39+
);
40+
});
41+
42+
test("empty entries from a trailing comma are dropped", () => {
43+
expect(parseRepoAllowlist("pytorch/pytorch,,").size).toBe(1);
44+
});
45+
});
46+
47+
describe("isRepoEnabled", () => {
48+
test("matches an exact repo", () => {
49+
const allowlist = parseRepoAllowlist("pytorch/pytorch");
50+
expect(isRepoEnabled(allowlist, "pytorch", "pytorch")).toBe(true);
51+
expect(isRepoEnabled(allowlist, "pytorch", "executorch")).toBe(false);
52+
});
53+
54+
test("matches a whole org via a wildcard", () => {
55+
const allowlist = parseRepoAllowlist("meta-pytorch/*");
56+
expect(isRepoEnabled(allowlist, "meta-pytorch", "torchcomms")).toBe(true);
57+
expect(isRepoEnabled(allowlist, "meta-pytorch", "monarch")).toBe(true);
58+
expect(isRepoEnabled(allowlist, "pytorch", "pytorch")).toBe(false);
59+
});
60+
61+
test("an org wildcard does not leak into a similarly named org", () => {
62+
const allowlist = parseRepoAllowlist("pytorch/*");
63+
expect(isRepoEnabled(allowlist, "meta-pytorch", "torchcomms")).toBe(false);
64+
});
65+
66+
test("comparison is case insensitive", () => {
67+
const allowlist = parseRepoAllowlist("PyTorch/PyTorch");
68+
expect(isRepoEnabled(allowlist, "pytorch", "pytorch")).toBe(true);
69+
});
70+
});
71+
72+
describe("logUploader", () => {
73+
let probot: Probot;
74+
let invoke: jest.SpyInstance;
75+
76+
beforeEach(() => {
77+
probot = utils.testProbot();
78+
probot.load(logUploader);
79+
invoke = jest.spyOn(lambda, "invokeLogUploader").mockResolvedValue();
80+
process.env.LOG_UPLOADER_REPOS = "meta-pytorch/*,pytorch/pytorch";
81+
});
82+
83+
afterEach(() => {
84+
jest.restoreAllMocks();
85+
nock.cleanAll();
86+
delete process.env.LOG_UPLOADER_REPOS;
87+
});
88+
89+
async function receive(payload: any) {
90+
await probot.receive({ name: "workflow_job", payload, id: "1" } as any);
91+
}
92+
93+
test("queues an upload for a completed job on an allowlisted repo", async () => {
94+
await receive(workflowJobPayload());
95+
96+
expect(invoke).toHaveBeenCalledWith({
97+
repo: "meta-pytorch/torchcomms",
98+
job_id: 12345,
99+
conclusion: "failure",
100+
});
101+
});
102+
103+
test("ignores anything but the completed action", async () => {
104+
// workflow_job also fires on queued and in_progress, where there is no log
105+
// to fetch yet. Uploading then would archive a truncated log.
106+
await receive(workflowJobPayload({ action: "queued" }));
107+
await receive(workflowJobPayload({ action: "in_progress" }));
108+
109+
expect(invoke).not.toHaveBeenCalled();
110+
});
111+
112+
test("skips a repo that is not on the allowlist", async () => {
113+
await receive(workflowJobPayload({ owner: "pytorch", name: "executorch" }));
114+
115+
expect(invoke).not.toHaveBeenCalled();
116+
});
117+
118+
test("skips an org the bot does not serve, even if allowlisted", async () => {
119+
// The allowlist narrows the org gate, it must not widen it.
120+
process.env.LOG_UPLOADER_REPOS = "someoneelse/*";
121+
await receive(workflowJobPayload({ owner: "someoneelse", name: "repo" }));
122+
123+
expect(invoke).not.toHaveBeenCalled();
124+
});
125+
126+
test("does nothing when the allowlist is unset", async () => {
127+
delete process.env.LOG_UPLOADER_REPOS;
128+
await receive(workflowJobPayload());
129+
130+
expect(invoke).not.toHaveBeenCalled();
131+
});
132+
133+
test("passes a null conclusion through rather than dropping the job", async () => {
134+
await receive(workflowJobPayload({ conclusion: null }));
135+
136+
expect(invoke).toHaveBeenCalledWith(
137+
expect.objectContaining({ conclusion: null })
138+
);
139+
});
140+
141+
test("a failed invoke does not fail the webhook", async () => {
142+
// Throwing here would make GitHub redeliver the event and re-run every other
143+
// handler, to retry something Dr.CI repairs on its own.
144+
invoke.mockRejectedValue(new Error("AccessDeniedException"));
145+
146+
await expect(receive(workflowJobPayload())).resolves.not.toThrow();
147+
});
148+
});

0 commit comments

Comments
 (0)