fingerprint: update storage fingerprint freebytes calculation #27019
Conversation
Storage fingerprinting was using the detected available disk space from the df command. This value would lead to incorrect disk space calculations when the fingerprinting client had existing allocations that had written to disk.
| // SPDX-License-Identifier: BUSL-1.1 | ||
|
|
||
| // MACHINE GENERATED BY 'go generate' COMMAND; DO NOT EDIT | ||
| // Code generated by 'go generate'; DO NOT EDIT. |
There was a problem hiding this comment.
This code was using deprecated functions so I regenerated it. Tested on a windows machine to validate it worked correctly.
client/fingerprint/storage.go
Outdated
|
|
||
| resp.AddAttribute("unique.storage.volume", volume) | ||
| resp.AddAttribute("unique.storage.bytestotal", strconv.FormatUint(total, 10)) | ||
| resp.AddAttribute("unique.storage.bytesfree", strconv.FormatUint(free, 10)) |
There was a problem hiding this comment.
I wonder if we should remove this attribute?
There was a problem hiding this comment.
I'm now wondering if it makes sense to just completely remove disk_free_mb. This setting doesn't really make sense with regard to the fingerprint anymore. It would basically become another override for total disk space, which we already have disk_total_mb for.
There was a problem hiding this comment.
Makes sense. We can deprecate it, but if you remove it outright I'm pretty sure users will hit validation errors during upgrade. So we'll have to remove it later on.
There was a problem hiding this comment.
Added a warning to the client config finalizer for users.
| return fmt.Errorf("failed to determine disk space for %s: %v", storageDir, err) | ||
| } | ||
|
|
||
| if cfg.DiskTotalMB > 0 { |
There was a problem hiding this comment.
I wonder if we should also deprecate this field as well. It was added in the same PR as DiskFreeMB. I think the only 2 use cases for this would be if we could not detect the clients total disk space for some reason (but I think this would be a Nomad bug) or if a user wanted to "overprovision" their storage for scheduling sake.
Co-authored-by: Aimee Ukasick <[email protected]>
client/fingerprint/storage.go
Outdated
|
|
||
| resp.AddAttribute("unique.storage.volume", volume) | ||
| resp.AddAttribute("unique.storage.bytestotal", strconv.FormatUint(total, 10)) | ||
| resp.AddAttribute("unique.storage.bytesfree", strconv.FormatUint(free, 10)) |
We aren't adding or removing any values from |
3 participants
Description
Storage fingerprinting was using the detected available disk space from the
dfcommand. This value would lead to incorrect disk space calculations when the fingerprinting client had existing allocations that had written to disk.Testing & Reproduction steps
Links
Fixes GH-24914, GH-6172
Contributor Checklist
changelog entry using the
make clcommand.ensure regressions will be caught.
and job configuration, please update the Nomad website documentation to reflect this. Refer to
the website README for docs guidelines. Please also consider whether the
change requires notes within the upgrade guide.
Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.