bols: implement ReadFileByBlock and unify handler across daemons [infra/infra : main]
0 views
Skip to first unread message
Otabek Kasimov (Gerrit)
Oct 22, 2026, 1:18:17 PM (3 days ago) Oct 22
to chromium...@chromium.org, infra-rev...@chromium.org
Otabek Kasimov has uploaded change 8634333 for review![]( "Open in Gerrit")
Commit message
bols: implement ReadFileByBlock and unify handler across daemons
Change-Id: I5dd7ad4b6a0bb1c6f02f91d8a2a3e228edb2750c
Change diff
Change information
Files:
- M go/src/infra/cros/cmd/cft/bols_labstation/internal/server/service.go
- M go/src/infra/cros/lib/bols/file_service.go
- M go/src/infra/cros/lib/bols/file_service_test.go
Change size: L
Delta: 3 files changed, 280 insertions(+), 105 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Oct 22, 2026, 1:18:28 PM (3 days ago) Oct 22
to chromium...@chromium.org, infra-rev...@chromium.org
Otabek Kasimov voted Commit-Queue+1 on change 8634333![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Oct 22, 2026, 1:18:37 PM (3 days ago) Oct 22
to chromium...@chromium.org, infra-rev...@chromium.org
Otabek Kasimov voted Auto-Submit+1 on change 8634333![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Oct 22, 2026, 1:21:10 PM (3 days ago) Oct 22
to LUCI CQ, chromium...@chromium.org, infra-rev...@chromium.org
Otabek Kasimov voted Auto-Submit+1 on change 8634333![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Oct 22, 2026, 1:23:25 PM (3 days ago) Oct 22
to LUCI CQ, chromium...@chromium.org, infra-rev...@chromium.org
Otabek Kasimov voted Commit-Queue+1 on change 8634333![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
See Wai Fu (Gerrit)
Oct 22, 2026, 1:32:14 PM (3 days ago) Oct 22
to Otabek Kasimov, Rafał Gaweł, LUCI CQ, chromium...@chromium.org, infra-rev...@chromium.org
Attention needed from Otabek Kasimov
See Wai Fu voted and added 1 comment on change 8634333![]( "Open in Gerrit")
See Wai Fu <seew...@google.com> added votes with a message
+1 - LGTM.
- Host path is a faithful move of the labstation handler (same dd args, same kill/wait on send/read errors, same stderr surfacing). Dispatch is sound: bols_labstation embeds a bare bols.Service{} (no IPCache / dockerClientFactory) so isContainerMode() is false there, while bols_satlab / bols_linux go through NewService and get IPCache - the same split WriteFileByBlock already relies on.
- Container path genuinely streams: docker.execSDK demuxes with stdcopy.StdCopy straight into the supplied req.Stdout, so nothing is buffered server-side, and readBlockStreamWriter re-chunks to <= 1 MiB and propagates a Send error back through StdCopy so Exec fails fast.
- Built all three daemons, vet, and ran lib/bols + the three daemon test packages locally at PS2: the new Read/Write block tests pass. (CQ dry run for PS2 still in flight.)
One doc nit inline, unresolved. Unrelated aside: TestGetServoTopology_ReturnsFtdiMauiCableAsRoot fails on any workstation that has /sys/bus/usb/devices/1-4 (fails identically on main without this CL) because the host-mode path runs filepath.Glob against the real sysfs - worth a separate fix since it makes local `go test ./cros/lib/bols/...` red for developers.
| Code-Review | +1 |
1 comment
File go/src/infra/cros/lib/bols/file_service.go
Line 722, Patchset 2 (Latest):
// - NumberOfBlocks: number of blocks to read ("dd count=<number_of_blocks>"); if <= 0, reads until EOF.See Wai Fu . unresolved
nit (doc, non-blocking): worth one sentence here on the timeout asymmetry, since this line advertises "reads until EOF" (i.e. potentially a whole USB device):
- container mode passes timeRemaining(ctx) to docker Exec, which falls back to a 1-minute default when the caller set no deadline, and a cancel does not kill the dd inside the container;
- host mode is unbounded and exec.CommandContext kills dd on cancel.
So a PARIS/bols_testing caller doing an until-EOF read of a large device in container mode must set a deadline or the stream dies at 60s with a generic "exceeded timeout". Same applies to WriteFileByBlock, so a shared note is fine.
Related details
Attention is currently required from:
- Otabek Kasimov
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Oct 22, 2026, 1:34:58 PM (3 days ago) Oct 22
to See Wai Fu, Rafał Gaweł, LUCI CQ, chromium...@chromium.org, infra-rev...@chromium.org
Otabek Kasimov added 1 comment on change 8634333![]( "Open in Gerrit")
File go/src/infra/cros/lib/bols/file_service.go
Line 722, Patchset 2:
// - NumberOfBlocks: number of blocks to read ("dd count=<number_of_blocks>"); if <= 0, reads until EOF.See Wai Fu . resolved
nit (doc, non-blocking): worth one sentence here on the timeout asymmetry, since this line advertises "reads until EOF" (i.e. potentially a whole USB device):
- container mode passes timeRemaining(ctx) to docker Exec, which falls back to a 1-minute default when the caller set no deadline, and a cancel does not kill the dd inside the container;
- host mode is unbounded and exec.CommandContext kills dd on cancel.
So a PARIS/bols_testing caller doing an until-EOF read of a large device in container mode must set a deadline or the stream dies at 60s with a generic "exceeded timeout". Same applies to WriteFileByBlock, so a shared note is fine.
Otabek Kasimov
Done. Added the timeout caveat to the docstring noting the 1-minute fallback in container mode and bounded cancellation in host mode.
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Oct 22, 2026, 1:35:20 PM (3 days ago) Oct 22
to See Wai Fu, Rafał Gaweł, LUCI CQ, chromium...@chromium.org, infra-rev...@chromium.org
Otabek Kasimov voted Commit-Queue+2 on change 8634333![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
LUCI CQ (Gerrit)
Oct 22, 2026, 1:46:37 PM (3 days ago) Oct 22
to Otabek Kasimov, See Wai Fu, Rafał Gaweł, chromium...@chromium.org, infra-rev...@chromium.org
LUCI CQ submitted change 8634333 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: go/src/infra/cros/lib/bols/file_service.go
Insertions: 4, Deletions: 0.
@@ -720,6 +720,10 @@
// - FilePath: required path to the regular file or block device (e.g., "/dev/sda").
// - ByteSize: block size in bytes passed to "dd bs=<byte_size>" (defaults to 512 if <= 0).
// - NumberOfBlocks: number of blocks to read ("dd count=<number_of_blocks>"); if <= 0, reads until EOF.
+// Timeout caveat: in container mode, docker Exec enforces a 1-minute default timeout if ctx
+// has no deadline (cancelling ctx does not kill dd inside the container). Callers performing
+// large until-EOF reads in container mode must configure an explicit ctx deadline. In host mode,
+// execution is bounded by ctx and dd is killed on cancel.
//
// Example use cases in PARIS (infra/cros/recovery):
// - USB-drive health audit (audit_usb): reading 1 MiB or 4 MiB blocks from a
```
Change information
Commit message:
bols: implement ReadFileByBlock and unify handler across daemons
Summary:
Implement the ReadFileByBlock streaming gRPC handler on *bols.Service in
infra/cros/lib/bols and remove the duplicate ReadFileByBlock handler from
bols_labstation so bols_satlab, bols_linux, and bols_labstation share a
single implementation.
Why / Reasoning:
Previously, ReadFileByBlock was only implemented in bols_labstation while
bols_satlab and bols_linux returned Unimplemented. Moving ReadFileByBlock
into infra/cros/lib/bols/file_service.go with host-mode and container-mode
dispatch eliminates duplicate gRPC stream handling across the three BOLS
daemons.
Associated Bugs:
b/572930938
Other Details:
Documented the logic difference between host mode (bols_labstation, where
dd reads directly from the host filesystem) and container mode
(bols_satlab and bols_linux, where dd executes inside the target Docker
container and streams stdout chunks via an io.Writer adapter) along with
PARIS recovery usage examples.
How Tested:
1. Ran unit tests and golangci-lint across cros/lib/bols and all three
BOLS daemon packages.
2. Verified end-to-end BOLS RPC suite on Satlab s2 and Labstation
chromeos15-row1-rack1-labstation via SSH port-forwarding.
BUG=b:572930938
TEST=go test ./cros/lib/bols/... ./cros/cmd/cft/bols_labstation/... ./cros/cmd/cft/bols_satlab/... ./cros/cmd/cft/bols_linux/...
TEST=make -C cros/cmd/cft/testing/bols_testing run-on-satlab-dev SATLAB_HOSTNAME=s2 NEW_FEATURES=true
Change-Id: I5dd7ad4b6a0bb1c6f02f91d8a2a3e228edb2750c
Commit-Queue: Otabek Kasimov <ota...@google.com>
Reviewed-by: See Wai Fu <seew...@google.com>
Auto-Submit: Otabek Kasimov <ota...@google.com>
Cr-Commit-Position: refs/heads/main@{#84768}
Files:
- M go/src/infra/cros/cmd/cft/bols_labstation/internal/server/service.go
- M go/src/infra/cros/lib/bols/file_service.go
- M go/src/infra/cros/lib/bols/file_service_test.go
Change size: L
Delta: 3 files changed, 281 insertions(+), 105 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by See Wai Fu
Review-Enforcement: Code-Review+1 by See Wai Fu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages