api: Create logic for automatically cleaning up dead streams #2013
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Ready for review! Could you take a look @leszko ? |
victorges
force-pushed
the
vg/feat/active-cleanup-cron
branch
from
076f89b
to
0dd1ea3
Compare
There was a problem hiding this comment.
Good work @victorges !
Added some comments, but in general I really like the approach and love that we'll start having this functionality!
| @@ -0,0 +1,15 @@ | |||
| name: "cron: Active streams cleanup" | |||
There was a problem hiding this comment.
I wonder if we shouldn't run it as a kubernetes Cron Jobs. I know that currently we don't have a clear agreement on whether cron jobs should be GH Actions or Kubernetes CronJobs, but the advantages I see for integrating it in our Infra:
- Alerts integration (Grafana Pager Duty)
- (super minor) GH Actions load on the GH workers
- (minor) security and passing admin LP token
Anyway, I leave it up to you.
There was a problem hiding this comment.
Even though I agree it would be much better to run cronjobs, I'd leave that as a separate change for the future, given we have other (more critical) workflows already running on GitHub actions, mainly usage billing. We should probably bring this up as a tech debt to be prioritized eventually though.
| @@ -229,6 +234,7 @@ function activeCleanupOne( | |||
| } | |||
|
|
|||
| setImmediate(async () => { | |||
| await cleanupSemaphore.wait(); | |||
There was a problem hiding this comment.
The "right" solution would be to have a separate connection pool for the cleanup work. This is a workaround. I wonder how difficult would it be to implement a separate connection pool for the cleanup.
There was a problem hiding this comment.
I've implemented the separate connection pool here!
ef6df93
I'd recommend reviewing that commit separately, since I touched several different files (also moved webhooks, tasks and usage logic to the jobs conn pool).
|
|
||
| on: | ||
| schedule: | ||
| - cron: "*/10 * * * *" # run every 10 minutes |
There was a problem hiding this comment.
How about running it every 5 min and getting rid of all the opportunistic cleanup? I think this job should be the only place where we do the stream cleanup.
Otherwise, we risk not exposing a lot of bugs + it's super wrong that making a GET call to studio makes a change in the Studio DB.
There was a problem hiding this comment.
Super scary, but I also would love to remove the opportunistic logic. Implemented here! f70dfe3
Notice that with a 5 minute frequency, the worst case delay for a stream to be activecleanedup will be 10 minutes. I think making the github action more frequent would not be that good though given limited runners etc, so we can consider that once we move to an actual cron in our infra. WDYT?
victorges
force-pushed
the
vg/feat/active-cleanup-cron
branch
from
0dd1ea3
to
22e0160
Compare
victorges
force-pushed
the
vg/feat/active-cleanup-cron
branch
from
007bbc1
to
f2a4ad4
Compare
victorges
force-pushed
the
vg/feat/active-cleanup-cron
branch
from
f2a4ad4
to
10eea44
Compare
victorges
force-pushed
the
vg/feat/active-cleanup-cron
branch
from
10eea44
to
fb9aa19
Compare
victorges
force-pushed
the
vg/feat/active-cleanup-cron
branch
from
fb9aa19
to
eec5d11
Compare
| let hooksCalled = 0; | ||
| for (let i = 0; i < 5 && hooksCalled < 2; i++) { | ||
| await sleep(500); | ||
|
|
||
| hooksCalled = 0; | ||
| for (const id of [webhookResJson.id, webhookResJson2.id]) { | ||
| const res = await client.get(`/webhook/${id}`); | ||
| expect(res.status).toBe(200); | ||
| const { status } = (await res.json()) as Webhook; | ||
| if (status.lastTriggeredAt >= now) { | ||
| hooksCalled++; | ||
| } | ||
| } | ||
| } | ||
| expect(hooksCalled).toBe(2); |
There was a problem hiding this comment.
FTR: this was the very cryptic failure I was getting from the tests. What has happening was that, after creating the separate jobs DB pool, the test below (records webhook logs) started failing because the webhook created there was begin called twice, one additional time with the event triggered in this test.
The problem was that this test never really waited (or even checked) that the webhooks created here were actually triggered. So the test finished while leaving the background runner calling the webhooks for it. This probably caused some flakiness every now and then as well.
What has happening was that, after switching to the separate DB pool, the very first event handled by the WebhookCannon took a little longer since it had to setup a new connection with Postgres (that hadn't been already initialized from the usual test setup). This small delay was already enough for the next test to start and create another webhook, which then got called for the same event. I could reproduce this by making a dummy query on the jobsDb in this test, which then caused the failure to stop cause the pool was then already initialized.
Fixed this by correcting this test logic tho, which was not even checking that the webhooks were being called. Explaining here only cause it seems so unrelated to the changes here, but they actually were.
Don't filter on the outer level, but rather make sure the pipeline supports an array of child streams.
Now we clean all of them, but we merge before the cleanup processing only the session once. No easy way to catch that on tests.
Also parallelize parent streams
There are some bugged sessions that got isActive set to them
This reverts commit 8621089. It was a staging only thing, likely from a development version.
What does this pull request do? Explain your changes. (required)
This creates an API and a corresponding cronjob to clean-up stream that are marked active
but haven't been updated in a while. This is an improvement from the lazy approach that
only tried to clean-up the streams once they were accessed via the API, which also had the
undesirable effect of triggering expensive mutations from read-only requests.
Specific updates (required)
How did you test each of these updates (required)
yarn testDoes this pull request close any open issues?
Implements ENG-869
Checklist