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
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?
There was a problem hiding this comment.
Good catch, thanks. Fixed in 58bb2ab by splitting the guard: listing stops at 20s remaining, while deletes are still allowed down to 10s, so a buffered partial batch always has ~10s to flush. Added a regression test for your scenario (20 pages, 6 stale on page 1, 60s budget, 5s per listing call, 3 repeated scans) — it fails on the previous code and passes now.
#5477 (`445269c`) unintentionally reverted #5464: the SSM housekeeper again lists every parameter before deleting anything, loses candidates when the first page omits `Parameters`, and has no remaining-runtime guard. A late listing failure or timeout can therefore prevent cleanup from making progress. Reapply #5464 unchanged on current main, restoring page-by-page deletion, pagination through empty pages, isolated deletion failures, the ten-second runtime guard and its Lambda wiring, tests, and README notes. The restored patch has the same stable patch ID as #5464. #5469 already includes an equivalent restoration commit (`762583d`). This PR separates that restoration from the batching changes so it can land independently. Once merged, #5469 can rebase and drop its duplicate restoration commit. Validation: 127 storage-provider tests and 22 Lambda wrapper tests passed. ESLint and Prettier passed for the changed TypeScript files; the control-plane Lambda bundle build and `git diff --check` passed. No AWS deployment or live parameter deletion was performed.
033dacc to
e0c2c8b
Compare
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