Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new freeBytes gc policy #5079

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Jun 24, 2024

This is just a proposal for now, the code still needs some tidying up and testing, but figured I'd share what I have for now.

We've had some issues with users having difficulty understanding how the cache policy works in dagger, typically run in container environments. Additionally, I've also had issues explaining to users how to configure this in buildkit. One of the problems is with setting a reasonable default - for example, on a development machine, where buildkit has access to the full disk, setting 10% max disk is totally reasonable. But in docker desktop, where you might expect to run a lot of builds, that same 10% max disk is completely inappropriate and far too small. In GitHub actions, we'd also want to use much more of the available space.

Actually, a useful setting would be to be able to configure at how much free disk space the GC should kick in, rather than the consumed disk space. For example, I might prefer to configure the GC to kick in when there is less that 20% total disk space free. This new metric also takes into account other applications that might consume disk space as well (now I can configure buildkit without needing to be aware of what else is going to be taking up lots of space), and make the most of the space on the machine.

This PR is a quick sketch of what this might look like, proposed under the config entry freeBytes - bikeshedding for naming welcome, no brilliant name springs to mind.

Note

An alternative to this would be to instead have a keepTotalBytes option, that is like keepBytes, but instead of computing the total bytes as the sum of all the consumed cache records, would compute it as the total amount of disk space currently being consumed (i.e. roughly total - free).

Signed-off-by: Justin Chadwell <me@jedevc.com>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, but which setting should take priority? I guess if I have keepBytes set to 20GB but only 10GB free space then it should still try to use up to 20 and fail if running out of space. Otherwise, if you accidentally run low on space because of some other tool, buildkit will just start to perform very poorly but there is no indication of why that is.

@jedevc
Copy link
Member Author

jedevc commented Jun 25, 2024

IMO, I think both settings should be followed - I think it would be quite useful and powerful to allow pruning at both constraints.

e.g. I might want to have keepBytes at 50GB, to consume max 50GB of cache - but if disk space is low, then I'd want it to use less, and always leave 10GB left.

Ideally, if we took this approach, we could modify the defaults for storage, and have freeBytes set to 1GB by default - just to prevent the accidental scenario where buildkit may consume all available space.

I'd argue that the performance metrics should be surfaced around progress/etc, docker desktop, dagger cloud, etc - this kind of poor performance already happens today on tooling like docker desktop when (naively) starting a docker container driver manually - the default disk space is small, and the 10% default is not great for large builds.

return 0, 0, err
}
diskSize := int64(st.Bsize) * int64(st.Blocks)
freeSize := int64(st.Bsize) * int64(st.Bfree)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super familiar with this API - do we want to use free or available here?

@tonistiigi
Copy link
Member

e.g. I might want to have keepBytes at 50GB, to consume max 50GB of cache - but if disk space is low, then I'd want it to use less, and always leave 10GB left.

I guess then we need min-storage, max-storage and free. We can keep the existing storage limit as one but need to reevaluate the defaults.

@jedevc
Copy link
Member Author

jedevc commented Jun 26, 2024

What would min-storage be in the above case? Is that the minimum amount of storage that the gc policy can consume (that doesn't make sense) 🤔

I think we would just need max-storage and free.

@tonistiigi
Copy link
Member

tonistiigi commented Jun 26, 2024

min-storage - amount BuildKit is always allowed to consume, even if free space constraint would say it should start deleting things. To make sure that BuildKit has some reserved storage that it can use for cache even if other apps are misbehaving.
max-storage - BuildKit will not go over this amount even if there is still plenty of free space on disk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants