Repository navigation
Conversation
|
I openend it for review but i think i this should be written on top of #5464. Once this is merged, i will rebase it |
640d292 to
159a208
Compare
| const logger = createAwsSsmStorageLogger('runner-config-housekeeper'); | ||
| const DELETE_BATCH_SIZE = 10; | ||
| // Pacing is per invocation; other housekeepers share the account/Region quota. | ||
| const DELETE_BATCH_DELAY_MS = 350; |
There was a problem hiding this comment.
Because AWS’s default limit for SSM delete operations is 3 requests per second. AWS quotas (https://docs.aws.amazon.com/general/latest/gr/ssm.html#limits_ssm)
1,000 ms ÷ 3 ≈ 333 ms between requests. I went with 350ms to leave a small margin
| if (remainingTime() < 10000) return; | ||
| const page = await client.send(new GetParametersByPathCommand({ Path: options.tokenPath, NextToken: nextToken })); | ||
| const page = await client.send( | ||
| new GetParametersByPathCommand({ Path: options.tokenPath, NextToken: nextToken, MaxResults: DELETE_BATCH_SIZE }), |
There was a problem hiding this comment.
We won't be able to delete all parameters in a page. Can we get bigger page sizes and build batches which are sent when we reach BATCH_SIZE, plus once more at the end?
There was a problem hiding this comment.
GetParametersByPath supports at most 10 parameters per page (https://docs.aws.amazon.com/systems-manager/latest/APIReference/API_GetParametersByPath.html#systemsmanager-GetParametersByPath-request-MaxResults). We could switch to DescribeParameters i think (https://docs.aws.amazon.com/systems-manager/latest/APIReference/API_DescribeParameters.html#systemsmanager-DescribeParameters-request-MaxResults), which supports up to 50 and returns the metadata we need, but that would also require IAM and listing changes. I’d suggest handling that separately.
I implemented the 2nd part though build batches and clean up when we reach batch size
34d2e8b to
c7bd33d
Compare
Reapplies github-aws-runners#5464, which was unintentionally reverted by github-aws-runners#5477. That commit carried housekeeper files from a branch that predated github-aws-runners#5464, restoring the list-everything-then-delete loop and dropping the remaining-time guard, its Lambda wiring, tests, and README notes.
c7bd33d to
033dacc
Compare
|
#5477 unintentionally reverted #5464. Its squash commit (b2ac2e5) brought over the SSM housekeeper files from the #5472 branch, which predated #5464. As a result, main is back to listing every parameter before deleting any, with no remaining-time guard. It also lost #5464's Lambda wiring, tests and README notes. 762583d fix(ssm): restore page-by-page housekeeper cleanup re-applies #5464 unchanged, and the batching commits are rebased on top of it. If you'd rather restore #5464 in its own PR, I can split that commit out. |
|
Confirmed that #5477 ( Once #5508 merges, this PR can rebase onto main and drop the duplicate restoration commit. Validation on the separate restoration: 127 storage-provider tests, 22 Lambda wrapper tests, lint/format checks, and the control-plane Lambda bundle build passed. |
| } | ||
| logger.info('Deleting expired runner configuration', { parameterName: parameter.Name, dryRun: options.dryRun }); | ||
| if (!options.dryRun) { | ||
| pendingNames.push(parameter.Name); |
There was a problem hiding this comment.
LGTM on batching and confirmed-deletion logging, but I think we should flush partial batches before listing consumes the cleanup budget. With six expired names on the first page and enough young/empty pages afterward to hit the runtime guard, this only flushes at ten names or in finally; by then flushPendingNames() refuses to run because less than ten seconds remain. I reproduced three fresh scans of a finite 20-page inventory (60-second budget, 5 seconds per listing call), each finding the same six candidates and issuing zero deletes. This loses the incremental progress restored by #5464. Could we bound how long a partial batch waits, or reserve time to flush it before continuing the scan, and add a repeated runtime-limited scan regression test?
This branch has not been deployed
3 participants
SSM housekeeper cleanup can fall behind when individual parameter deletions are throttled. This change batches up to 10 eligible names across listing pages into each DeleteParameters request, with 350 ms pacing per batch and a remaining-runtime guard.
Partial batches are flushed when listing finishes or fails if runtime permits. Age filtering, dry-run behavior, empty-page pagination, and fresh scans on later invocations are preserved. SDK retries handle retryable failures; exhausted requests and invalid names are logged without stopping subsequent batches. Account/Region quotas are shared, so concurrent housekeepers can still throttle.
Confirmed deletions now emit a success log with deletedCount based exclusively on AWS's DeletedParameters response. A cleanup summary reports attempted, deleted, failed, skipped, and pending parameter counts, dryRun, and whether the scan completed, stopped at the runtime guard, or failed while listing. Per-batch success logs retain progress when a hard timeout prevents the final summary. Sum deletedCount for confirmed parameter deletions; counting success entries counts batches.
Both housekeeper IAM policies grant ssm:DeleteParameters. Deploy the IAM update alongside the Lambda; custom policies also need the batch action. Native expiration remains complementary.
Related: #5243.
Validation