Repository navigation
RSDK-14354: Add disk space monitor for viam-agent - #293
Conversation
nandini-swami
changed the title
|
Manual tested on raspberry pi and Windows machine Windows: Pi: |
Cheuk (cheukt)
left a comment
Cheuk (cheukt)
left a comment
There was a problem hiding this comment.
I think the code looks good! let's just export the rdk function so we aren't duplicating code too much (it does mean you'll have to do an rdk pr as well, but hopefully nothing crazy). that should also hopefully make the tests a little simpler
| // Callers must not pass a file path: on Windows GetDiskFreeSpaceExW rejects one, and | ||
| // diskusage.Usage stops walking up at the first path that exists, which is the file itself | ||
| // once a partial download is on disk. | ||
| func warnIfLowDiskSpace(logger logging.Logger, path, desc string, required uint64, extraFields ...any) { |
There was a problem hiding this comment.
let's go ahead and export rdk's robot/packages.checkDiskSpace instead of basically reimplementing it here. maybe with a signature of diskusage.WarnIfLow(logger, path, desc, required, extra...) (low bool, err error)?
|
Manual testing on Windows 
|
|
Manual testing on Pi |
051876f to
75295b4
Compare
|
Manually tested on Pi Default - warning comes from CheckDiskSpace in rdk, and the download proceeds. With |
|
Manually tested on Windows |
75295b4 to
48e5692
Compare
| // download and record the sha of the download itself | ||
| verData.DlPath, err = utils.DownloadFile(ctx, verData.URL, c.logger) | ||
| verData.DlPath, err = utils.DownloadFile(ctx, verData.URL, c.logger, c.blockOnLowDisk) | ||
| if err != nil { |
There was a problem hiding this comment.
I think if err is diskusage.ErrInsufficientDiskSpace we don't want to set data.brokenTarget = true since the target isn't actually broken, we're just out of space. Otherwise, even if we make more disk space, we won't re-download/install the custom URL version until we changed the URL back and forth.
There was a problem hiding this comment.
oh good catch, that would've stuck permanently. fixed it, thx!
| m.cache.SetBlockOnLowDisk(m.cfg.AdvancedSettings.BlockDownloadsOnLowDisk.Get()) | ||
|
|
||
| // Agent | ||
| needRestart, err := m.cache.UpdateBinary(ctx, SubsystemName) |
There was a problem hiding this comment.
Can we maybe just pass m.cfg.AdvancedSettings.BlockDownloadsOnLowDisk.Get() to UpdateBinary instead of having it be a field with a getter on the cache? It was a little hard to follow how it was being used and I think this is clearer.
Also small nit: looks like most (all?) advanced config settings have a getter wrapper instead of doing a raw Get().
There was a problem hiding this comment.
yes, that's clearer. fixed nit as well!
Daniel Botros (danielbotros)
left a comment
Daniel Botros (danielbotros)
left a comment
There was a problem hiding this comment.
LGTM, nice job!
Cheuk (cheukt)
left a comment
Cheuk (cheukt)
left a comment
There was a problem hiding this comment.
looks good! just a small change and then good to approve
| defer res.Body.Close() //nolint:errcheck | ||
| // we remove surrounding quotes if present | ||
| return strings.Trim(res.Header.Get("ETag"), `"`), nil | ||
| return strings.Trim(res.Header.Get("ETag"), `"`), res.ContentLength, nil |
There was a problem hiding this comment.
this is pre-existing but let's check the status code of the response (that it's a 200) before returning a size
| go.viam.com/rdk v1.2.0/go.mod h1:rl0RdP/baQRLFSkkUQHGF3SzbKgTajjUuoAr9K1kN7A= | ||
| go.viam.com/api v0.1.579 h1:KQz5abVMsNxLxvs7Xw0dZqjSYhSqZns9xVFT9B+d7ec= | ||
| go.viam.com/api v0.1.579/go.mod h1:4wA8A7g955PH03er3PNSJAi32bu3fpj9/kFp6AbrDNY= | ||
| go.viam.com/rdk v1.7.1-0.20260909213157-0dc7ed903416 h1:fYqJN2JyEqfCZqoBPwp6ZbtbjYYKiX+td8Bd4Dgi55o= |
There was a problem hiding this comment.
let's wait until next week to bump rdk to a proper release before merging
3 participants
RSDK-14354:
Add a disk-space check before viam-agent downloads.
The agent checks free space before it downloads or copies a binary. This covers viam-server downloads and agent self-updates. Both go through utils.DownloadFile.
The check requires the remaining download size plus a 10 MB floor. It reads the size from the HEAD request that already runs before every download. If a partial download is on disk and the agent can resume it, the check subtracts those bytes, because only the remaining bytes need to fit. If the server does not report a size, the check uses the just the 10 MB floor.
The warning includes the path, the available space, the required space, and the download size.
Adds
block_downloads_on_low_disktoadvanced_settingsconfig for agent. Unset (the default) is log-only: the warning is logged and the download continues, matching viam-server's default (RSDK-13166, rdk#6075). When set to true, a low-disk result refuses the download and the agent logs the error instead.Tests cover the sizing math for both paths, unknown content length, a resumable partial, a stale partial that must not be subtracted, and refusal in both branches when blocking is on.
Manually verified on a Raspberry Pi (log-only and blocking) and a Windows 11 VM (log-only).