Fleet agent update improvements#41
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new ChangesAgent download gate scenario
Sequence Diagram(s)sequenceDiagram
participant Runner as runFleetAutomationCorrectness
participant Simulator as Fleet simulator
participant Director as /dl endpoint
Runner->>Simulator: fleetSimDLProbe(device 0)
Simulator->>Director: /dl probe
Director-->>Simulator: HTTP status
Simulator-->>Runner: histogram
Runner->>Runner: wait until histogram["0"] is absent
loop each DownloadGate probe
Runner->>Simulator: fleetSimDLProbe(device id)
Simulator-->>Runner: histogram
Runner->>Runner: compare observed status with ExpectCode
end
opt throttle_count > 0
Runner->>Simulator: concurrent fleetSimDLProbe calls
Simulator-->>Runner: histogram with 429 counts
Runner->>Runner: assert minimum 429 count
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
4115-4132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHistogram-to-status extraction relies on map iteration but is safe under current call pattern.
gotis derived by scanning the histogram map for any key withn > 0; withcount=1per probe there should only ever be one such key, so this is correct today, but it's a bit fragile ifprobeis ever called withcount > 1for per-device checks. Not urgent given current usage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runner/runner.go` around lines 4115 - 4132, In the DownloadGate check inside runner.go, the `got` status is inferred by iterating the histogram map returned from `probe`, which is only safe because `probe(p.DeviceID, 1, 1)` currently guarantees a single hit. Make the extraction in this loop deterministic by choosing the status from the actual probe result structure or by explicitly validating there is exactly one nonzero bucket before assigning `got`, so `probe`, `DownloadGate`, and the `/dl` comparison don’t depend on fragile map iteration if the call pattern changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/case.go`:
- Around line 1372-1380: The agent_download_gate validation in case.go only
checks download_gate and throttle_count, but it also needs to require the
throttle companion fields when throttling is enabled. Update the validation
logic in the agent_download_gate case to enforce that tc.Fleet.ThrottleMin429 is
greater than zero and tc.Fleet.ThrottleDeviceID is set whenever
tc.Fleet.ThrottleCount is greater than zero, using the existing case.Name and
Fleet fields in this branch so the scenario cannot silently pass with a vacuous
429 check or a blocked default device.
In `@internal/runner/runner.go`:
- Around line 4137-4151: The throttle burst path in runner.go is defaulting to a
blocked device by calling fleetStr(fc.ThrottleDeviceID, "0"), which prevents the
burst from ever reaching the 429 check when throttle_device_id is omitted.
Update the throttle probe setup in the logic around probe and ThrottleCount so
it no longer silently falls back to device "0"; instead, enforce that
ThrottleDeviceID is present whenever ThrottleCount is greater than zero, and
surface a validation error earlier in the flow using the same
fc.ThrottleCount/fc.ThrottleDeviceID fields and the validateFleet-related
handling.
- Around line 4099-4111: The readiness wait around probe in runner.go is
recomputing scenarioDeadline() on every loop iteration, which makes the timeout
slide forward instead of expiring at a fixed settle-based deadline. Snapshot the
deadline once before the loop, then use that fixed value in the waiting
condition so the /dl readiness check fails fast like the other wait sites in
this flow.
---
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 4115-4132: In the DownloadGate check inside runner.go, the `got`
status is inferred by iterating the histogram map returned from `probe`, which
is only safe because `probe(p.DeviceID, 1, 1)` currently guarantees a single
hit. Make the extraction in this loop deterministic by choosing the status from
the actual probe result structure or by explicitly validating there is exactly
one nonzero bucket before assigning `got`, so `probe`, `DownloadGate`, and the
`/dl` comparison don’t depend on fragile map iteration if the call pattern
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c3d7e4c-2ed5-4d9b-9b0b-a8ea4ada6d5f
📒 Files selected for processing (2)
internal/config/case.gointernal/runner/runner.go
Deploying pipebench with
|
| Latest commit: |
c873610
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://184eb7c5.pipebench.pages.dev |
| Branch Preview URL: | https://dev-fleet-update.pipebench.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To test VirtualMetric's Agent update process
Summary by CodeRabbit
New Features
Bug Fixes