Repository navigation
Conversation
Measure how long waitForClaimSandboxReady takes to notice a claim that binds to a ready sandbox shortly after the wait starts, and how many reads it issues. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: manaspros <manasnandchoudhary@gmail.com>
waitForClaimSandboxReady read the claim on a fixed 1s tick, so a claim bound a few milliseconds after the first read still took a full second to return. Start at 100ms and double up to the old 1s interval. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: manaspros <manasnandchoudhary@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
==========================================
+ Coverage 59.98% 60.50% +0.51%
==========================================
Files 36 35 -1
Lines 3589 3621 +32
==========================================
+ Hits 2153 2191 +38
+ Misses 1216 1210 -6
Partials 220 220
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The fake-client benchmark shows the polling delay, but do you have a before/after comparison with a real warm pool to show whether the latency reduction justifies the extra GETs? |
| // claimReadyPollInitialInterval is the delay before the second read of a | ||
| // SandboxClaim while waiting for it to bind a ready Sandbox. A claim served | ||
| // from a warm pool is usually bound within a few hundred milliseconds, so | ||
| // the first retries come quickly. |
There was a problem hiding this comment.
Just a nit
| // claimReadyPollInitialInterval is the delay before the second read of a | |
| // SandboxClaim while waiting for it to bind a ready Sandbox. A claim served | |
| // from a warm pool is usually bound within a few hundred milliseconds, so | |
| // the first retries come quickly. | |
| // claimReadyPollInitialInterval is the initial polling interval while waiting for a SandboxClaim | |
| // to bind to a Ready Sandbox. A claim served from a warm pool is usually bound within | |
| // a few hundred milliseconds, so the first retries come quickly. |
| defer cancel() | ||
| ticker := time.NewTicker(1 * time.Second) | ||
| defer ticker.Stop() | ||
| interval := claimReadyPollInitialInterval |
There was a problem hiding this comment.
The first check still runs immediately after the claim is created, right?
I agree that a watch-based approach should remain our long-term goal, even if we keep the backoff for now because of the RBAC implications. Claiming a sandbox is a "fast path", so we should minimize avoidable delays at each step.
I found some performance data from upstream; Hope it helps. |
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
waitForClaimSandboxReadyWithTimeoutread the SandboxClaim on a fixed 1 second ticker. A claim that got bound right after the first read still waited a full second before we noticed.Now the wait starts at 100 ms and doubles up to the old 1 second: 100, 200, 400, 800, 1000, 1000 and so on. After the first second it behaves the same as before. It's a
time.Timerreset each round. The context and timeout handling is untouched.There are two commits. The first only adds the benchmark, so the "before" numbers can be reproduced from it.
The benchmark uses a fake dynamic client, the same kind the existing tests use. It binds the claim 20 ms or 300 ms after the wait starts, then serves a ready sandbox. So sec/op here is wait latency. I picked those bind delays myself. They aren't measurements of the real controller.
The cost is extra reads early on. A claim that stays unbound gets reads at 0, 0.1, 0.3, 0.7 and 1.5 s instead of 0 and 1 s, so 3 extra GETs in the first 1.5 s. After that it's one per second, as before.
Which issue(s) this PR fixes:
Fixes #482
Special notes for your reviewer:
A watch would be the cleaner end state; agent-sandbox's Go client does that. With auth on, the wait runs with the caller's own client, so a watch would need
watchon sandboxclaims for every caller. I went with the smaller change first. I'm happy to do the watch instead if you prefer it.TestWaitForClaimSandboxReadyNoticesQuickBindingWithoutWaitingASecondchecks that a claim bound after 20 ms returns in under 700 ms. It's timing based, so tell me if you'd rather not have it.go test -race ./pkg/...passes.Not tested: a real cluster, a real warm pool, or API server load with many concurrent creates.
AI use: I used Claude Code (Claude Sonnet 5.5 and Claude Opus 5.5) to find this and draft the code, benchmark and test. I've read and run all of it and can explain each line.
Does this PR introduce a user-facing change?: