upgrade: Add pre-flight disk space check #1995
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the disk space check by moving check_disk_space() to deploy.rs for reuse across install and upgrade operations. This is a great improvement, adding a pre-flight check to all upgrade modes that download layers, which prevents wasted bandwidth and provides immediate feedback on insufficient disk space. The code is now more modular and robust. I have one suggestion to further improve the robustness of the disk space calculation.
| imgref: &ImageReference, | ||
| ) -> Result<()> { | ||
| let stat = rustix::fs::fstatvfs(repo_fd)?; | ||
| let bytes_avail: u64 = stat.f_bsize * stat.f_bavail; |
There was a problem hiding this comment.
The multiplication stat.f_bsize * stat.f_bavail could overflow on very large filesystems, as both operands are u64. This would cause a panic in debug builds or wrap around in release builds, leading to incorrect disk space checks. Using checked_mul() prevents this by saturating at u64::MAX on overflow, which is a safe upper bound for available space.
| let bytes_avail: u64 = stat.f_bsize * stat.f_bavail; | |
| let bytes_avail = stat.f_bsize.checked_mul(stat.f_bavail).unwrap_or(u64::MAX); |
Extends PR bootc-dev#1245's approach to all bootc upgrade modes that download layers (default, --apply, --download-only). Moves check_disk_space() to deploy.rs for reuse by both install and upgrade operations. This prevents wasted bandwidth and provides immediate feedback when insufficient disk space is available, matching the install behavior. Related: BIFROST-1088 Assisted-by: AI Signed-off-by: Wei Shi <wshi@redhat.com>
shi2wei3
force-pushed
the
upgrade-preflight-disk-check
branch
from
80ea26c to
db67688
Compare
|
@cgwalters @ckyrouac Would you mind help me to review it, since this MR is heavily based on yours. |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Looks sane to me. BTW I think it'd be relatively easy to have an integration test for this that makes a fake image in an oci: directory that claims to have a very large layer or so and verify we fail before we try to fetch the layers
| let bytes_avail: u64 = stat.f_bsize * stat.f_bavail; | ||
| tracing::trace!("bytes_avail: {bytes_avail}"); | ||
|
|
||
| if image_meta.bytes_to_fetch > bytes_avail { |
There was a problem hiding this comment.
Note that ostree has a slightly stricter check and has a configurable min-free-space in the repository: the rationale is that we basically never want to get close to filling up a disk with an OS update.
I'm OK with this as is, but how about just a
// TODO consider also syncing with ostree min-free-space
| } | ||
| PreparedPullResult::Ready(prepared_image_meta) => { | ||
| // Check disk space before attempting to pull | ||
| check_disk_space(repo.dfd_borrow(), &prepared_image_meta, imgref)?; |
There was a problem hiding this comment.
One thing of course here is this doesn't apply to the composefs backend.
Hmm, I wonder if container-libs has a similar check? If we get to #20 then the check would need to be there.
3 participants
Extends PR #1245's approach to all bootc upgrade modes that download layers (default, --apply, --download-only). Moves check_disk_space() to deploy.rs for reuse by both install and upgrade operations.
This prevents wasted bandwidth and provides immediate feedback when insufficient disk space is available, matching the install behavior.
Related: BIFROST-1088
Assisted-by: AI