-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
builder: use buildkit's GC for build cache #37846
Conversation
builder/builder-next/controller.go
Outdated
wopt := mobyworker.Opt{ | ||
ID: "moby", | ||
Root: root, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: remove this, not needed.
e24c7dc
to
81363a2
Compare
Codecov Report
@@ Coverage Diff @@
## master #37846 +/- ##
=========================================
Coverage ? 36.09%
=========================================
Files ? 610
Lines ? 45115
Branches ? 0
=========================================
Hits ? 16284
Misses ? 26591
Partials ? 2240 |
e482e74
to
e6ac871
Compare
builder/builder-next/controller.go
Outdated
if conf.GC.DefaultKeepStorage != "" { | ||
defaultKeepStorage, err = units.RAMInBytes(conf.GC.DefaultKeepStorage) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: wrap these parse errors
e6ac871
to
c82df7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this, just a few comments but in general looks very good 👼
builder/builder-next/controller.go
Outdated
err error | ||
) | ||
|
||
if conf.GC.DefaultKeepStorage != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this only be done only if conf.GC.Enabled
is true
?
builder/builder-next/controller.go
Outdated
} | ||
} | ||
} else { | ||
gcPolicy = mobyworker.DefaultGCPolicy(root, defaultKeepStorage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not initializing gcPolicy
to this directly instead of having an else statement? then if the user wants to set it the make
at 194 can override it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fntlnz It's a small matter of preference: with what you're suggesting, in the case where user wants to set it, it would have already called DefaultGCPolicy unnecessarily. The way it is now, only one of the two codepaths is executed once. If readability is what concerns you, I could change it to if conf.GC.Policy == nil {
first.
c82df7c
to
380f52e
Compare
@fntlnz updated |
LGTM |
if err := syscall.Statfs(root, &st); err != nil { | ||
return defaultCap | ||
} | ||
diskSize := st.Bsize * int64(st.Blocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is complaining that on s390x (z linux) Bsize
is int32.
https://github.com/golang/go/blob/master/src/syscall/ztypes_linux_s390x.go#L119
07:19:55 builder/builder-next/worker/gc_unix.go:14:23: invalid operation: st.Bsize * int64(st.Blocks) (mismatched types uint32 and int64)
I think we need an int32 version of this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiborvass ^^
380f52e
to
f341564
Compare
This allows users to configure the buildkit GC. The following enables the default GC: ``` { "builder": { "gc": { "enabled": true } } } ``` The default GC policy has a simple config: ``` { "builder": { "gc": { "enabled": true, "defaultKeepStorage": "30GB" } } } ``` A custom GC policy can be used instead by specifying a list of cache prune rules: ``` { "builder": { "gc": { "enabled": true, "policy": [ {"keepStorage": "512MB", "filter": ["unused-for=1400h"]]}, {"keepStorage": "30GB", "all": true} ] } } } ``` Signed-off-by: Tibor Vass <tibor@docker.com>
f341564
to
4a776d0
Compare
All tests passed so merging. |
Can you tell me exactly which config file I need to edit to set the options from the original post? |
This allows users to configure the buildkit GC.
The following enables the default GC:
The default GC policy has a simple config:
A custom GC policy can be used instead by specifying a list of cache prune rules:
Signed-off-by: Tibor Vass tibor@docker.com