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

buildkitd: allow multiple units for gc config #3773

Merged
merged 3 commits into from
May 9, 2023

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Apr 3, 2023

In addition to parsing the raw number of bytes, we can additionally supporting reading short notations from strings, such as "50MB", or "10GB". We can also support different percentages, such as "20%", which allows for consuming a maximum percentage of the disk easily (previously, only the default was set to 10%, with no ability to change to use an arbitrary percentage).

@jedevc
Copy link
Member Author

jedevc commented Apr 3, 2023

Would we also want something for durations? To do time.ParseDuration for the cache config?

@tonistiigi
Copy link
Member

Would we also want something for durations? To do time.ParseDuration for the cache config?

sgtm

In addition to parsing the raw number of bytes, we can additionally
supporting reading short notations from strings, such as "50MB", or
"10GB". We can also support different percentages, such as "20%", which
allows for consuming a maximum percentage of the disk easily
(previously, only the default was set to 10%, with no ability to change
to use an arbitrary percentage).

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the config-allow-storage-units branch from 564c8bb to 6fe8ffc Compare April 3, 2023 17:53
@jedevc jedevc marked this pull request as ready for review April 3, 2023 17:53
@jedevc jedevc force-pushed the config-allow-storage-units branch from 6fe8ffc to 85ea36b Compare April 3, 2023 17:59
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the config-allow-storage-units branch from 85ea36b to e06c962 Compare April 3, 2023 18:10
@crazy-max
Copy link
Member

@jedevc
Copy link
Member Author

jedevc commented Apr 4, 2023

Can you update https://github.com/moby/buildkit/blob/master/docs/buildkitd.toml.md too?

Done. I think we probably want to spend some time at some point updating the entire document, there's not a lot of consistency between the different documentation styles in the comments.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the config-allow-storage-units branch from 687ffa0 to 4cd5b7b Compare April 4, 2023 09:20
@aaronlehmann
Copy link
Collaborator

The ability to specify percentages is a great improvement. It will let us replace a percentage calculation in startup scripts.

@jedevc
Copy link
Member Author

jedevc commented May 2, 2023

PTAL @tonistiigi

var st syscall.Statfs_t
if err := syscall.Statfs(root, &st); err != nil {
return defaultCap
}
diskSize := int64(st.Bsize) * int64(st.Blocks)
avail := diskSize / 10
avail := diskSize * d.Percentage / 100
return (avail/(1<<30) + 1) * 1e9 // round up
Copy link
Member

Choose a reason for hiding this comment

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

This "round up" still makes sense with user input?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. It just rounds up to the closest gigabyte, which makes sense to me.

@jedevc jedevc merged commit 07ff714 into moby:master May 9, 2023
@jedevc jedevc deleted the config-allow-storage-units branch May 9, 2023 14:53
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.

4 participants