builder: add prune options to the API #37651

tiborvass · 2018-08-15T22:16:43Z

Signed-off-by: Tibor Vass tibor@docker.com

tiborvass · 2018-08-15T22:25:56Z

api/types/types.go

+type BuildCachePruneOptions struct {
+	All          bool
+	KeepDuration time.Duration
+	KeepStorage  float64 // in MB


@AkihiroSuda @vdemeester I'm not sure if I want KeepStorage here or KeepBytes like in Buildkit API, though on the CLIs it's --keep-storage. Let me know what you think.

either is fine, but I think the type should be int64 in bytes

@tiborvass same as @AkihiroSuda 😝, KeepStorage is fine 😉

AkihiroSuda · 2018-08-16T03:44:46Z

api/server/router/build/build_routes.go

+
+	opts := types.BuildCachePruneOptions{
+		All:          httputils.BoolValue(r, "all"),
+		KeepDuration: time.Duration(kd),


kd * time.Second

or errors.Wrap(err, "keep-duration expects an integer number in milliseconds")

caervs · 2018-08-16T22:47:09Z

api/types/types.go

+// BuildCachePruneOptions hold parameters to prune the build cache
+type BuildCachePruneOptions struct {
+	All          bool
+	KeepDuration time.Duration


Gonna summarize a conversation that @tiborvass and I had offline. If we've understood the criteria for pruning correctly a given cache layer will be kept if it meets the KeepDuration criterion and meets the KeepStorage criterion. In other words keep rules are conjunctive and by De Morgan's law, purge rules are disjunctive 😜

If that's the case I would rename KeepDuration to KeepMaxAge. Duration implies that the cached item will be kept around for that amount of time, whereas max age implies it will be kept for a maximum of that amount of time given all other criteria are also met.

andrewhsu · 2018-08-19T21:01:00Z

@tiborvass Hmm...janky no likey:

integration/build/build_session_test.go:1::warning: file is not goimported (goimports)
api/server/router/build/build_routes.go:177:31:warning: invalid operation: mismatched types int and time.Duration (gosimple)
api/server/router/build/build_routes.go:178:17:warning: cannot use ks (variable of type float64) as int64 value in struct literal (interfacer)
api/server/router/build/build_routes.go:177:31:warning: invalid operation: mismatched types int and time.Duration (interfacer)
api/server/router/build/build_routes.go:178:17:warning: cannot use ks (variable of type float64) as int64 value in struct literal (unconvert)
api/server/router/build/build_routes.go:177:31:warning: invalid operation: mismatched types int and time.Duration (unconvert)
Build step 'Execute shell' marked build as failure

anusha-ragunathan · 2018-08-20T15:28:06Z

api/server/router/build/build_routes.go

+	}
+
+	opts := types.BuildCachePruneOptions{
+		All:          httputils.BoolValue(r, "all"),


For my clarification:
Isnt all mutually exclusive with the Keep* options?
Also, what happens when KeepDuration is met but KeepStorage isnt or vice versa?

andrewhsu · 2018-08-21T00:56:39Z

Only one failure on z known to be flakey:

FAIL: docker_cli_swarm_test.go:1376: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey

andrewhsu · 2018-08-21T01:00:12Z

SGTM wrt getting apis for pruning things that are built

tiborvass · 2018-08-21T01:26:53Z

For my clarification:
Isnt all mutually exclusive with the Keep* options?
Also, what happens when KeepDuration is met but KeepStorage isnt or vice versa?

@anusha-ragunathan There are different type of caches in buildkit: for regular cache, frontend images, internal images. By default only regular cache is pruned, but if you specify All then all types are pruned. So you can mix that with KeepMaxAge (renamed from KeepDuration) and KeepStorage. The rule is that if the cache matches keepmaxage AND keepstorage, then it is kept, otherwise it is purged. If you want to do an OR, you do 2 separate operations, one with keepmaxage, another one with keepstorage.

tonistiigi · 2018-08-28T22:45:10Z

Talked with @tiborvass about this. The conclusion was to move the maxage to a filter, leave everything else as is as other fields are not really filters. For the gc, it is a list of these same options that just apply automatically and if the user wants to override they can do it with the config file initially.

tiborvass · 2018-08-31T22:19:55Z

@tonistiigi @AkihiroSuda @vdemeester So this round of changes include:

Changed max-age filter to unused-for because I realized the filter was backwards: we want to prune everything that wasn't used for .
Fixed a bug where prune would be equivalent to prune --all
Shows deleted IDs
Mutable was removed from BuildCache API, and Type and Shared were added.
mutable and immutable are no longer valid filters for prune, but type now is.

tiborvass · 2018-08-31T22:20:22Z

Failures are unrelated.

tonistiigi · 2018-08-31T22:36:43Z

api/server/router/build/build_routes.go

+	}
+	ks, err := strconv.Atoi(r.FormValue("keep-storage"))
+	if err != nil {
+		return errors.Wrap(err, "keep-storage is in bytes and expects an integer")


nit: print values on parse errors

Signed-off-by: Tibor Vass <tibor@docker.com>

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass · 2018-09-01T23:46:16Z

@tonistiigi I pushed your fix for Shared. Also updated the CLI branch: docker/cli#1327

vdemeester

LGTM 🐯

thaJeztah · 2018-10-25T21:20:46Z

Looks like we forgot to update the API changelog document in this PR

tiborvass added area/api area/builder impact/api labels Aug 15, 2018

tiborvass requested review from dnephin and tonistiigi as code owners August 15, 2018 22:16

GordonTheTurtle added the status/0-triage label Aug 15, 2018

tiborvass mentioned this pull request Aug 15, 2018

[WIP] Add new builder subcommand and implement builder prune to prune build cache docker/cli#1292

Closed

1 task

tiborvass commented Aug 15, 2018

View reviewed changes

AkihiroSuda reviewed Aug 16, 2018

View reviewed changes

caervs reviewed Aug 16, 2018

View reviewed changes

tiborvass force-pushed the new-builder-prune branch 2 times, most recently from ac354f6 to dc9b62f Compare August 17, 2018 08:35

anusha-ragunathan reviewed Aug 20, 2018

View reviewed changes

tiborvass force-pushed the new-builder-prune branch 7 times, most recently from a39e158 to 88f575d Compare August 20, 2018 22:24

tiborvass changed the title ~~builder: add prune options to the API + buildkit vendor~~ builder: add prune options to the API Aug 20, 2018

DaddyCal approved these changes Aug 26, 2018

View reviewed changes

tiborvass force-pushed the new-builder-prune branch from 88f575d to a60ee5d Compare August 29, 2018 00:48

GordonTheTurtle assigned coolljt0725 Aug 30, 2018

tiborvass force-pushed the new-builder-prune branch from a60ee5d to 16f3ae0 Compare August 30, 2018 06:28

tiborvass force-pushed the new-builder-prune branch 4 times, most recently from 3af5dbc to 5340314 Compare August 31, 2018 19:15

tonistiigi approved these changes Aug 31, 2018

View reviewed changes

AkihiroSuda added rebuild/experimental labels Sep 1, 2018

GordonTheTurtle removed rebuild/experimental labels Sep 1, 2018

Tibor Vass added 2 commits September 1, 2018 22:01

builder: add prune options to the API

8ff7847

Signed-off-by: Tibor Vass <tibor@docker.com>

builder: fix pruning all cache

d47435a

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the new-builder-prune branch from 5340314 to efdaf61 Compare September 1, 2018 22:01

builder: implement ref checker

354c241

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the new-builder-prune branch from efdaf61 to 354c241 Compare September 1, 2018 23:40

tiborvass added the rebuild/janky label Sep 2, 2018

GordonTheTurtle removed the rebuild/janky label Sep 2, 2018

vdemeester approved these changes Sep 3, 2018

View reviewed changes

yongtang added status/2-code-review rebuild/experimental and removed status/0-triage labels Sep 3, 2018

GordonTheTurtle removed the rebuild/experimental label Sep 3, 2018

tiborvass merged commit 6ba1e91 into moby:master Sep 4, 2018

tiborvass mentioned this pull request Sep 4, 2018

[18.09] Buildkit cherry-picks docker-archive/engine#42

Merged

thaJeztah added the docs/revisit label Oct 25, 2018

tiborvass deleted the new-builder-prune branch July 17, 2019 00:34

thaJeztah mentioned this pull request Jul 27, 2020

Incorrect API documentation for SystemDataUsage #41264

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder: add prune options to the API #37651

builder: add prune options to the API #37651

tiborvass commented Aug 15, 2018 •

edited

Loading

tiborvass Aug 15, 2018

AkihiroSuda Aug 16, 2018

vdemeester Aug 16, 2018

AkihiroSuda Aug 16, 2018

AkihiroSuda Aug 16, 2018

caervs Aug 16, 2018

andrewhsu commented Aug 19, 2018

anusha-ragunathan Aug 20, 2018

andrewhsu commented Aug 21, 2018

andrewhsu commented Aug 21, 2018

tiborvass commented Aug 21, 2018

tonistiigi commented Aug 28, 2018

tiborvass commented Aug 31, 2018

tiborvass commented Aug 31, 2018

tonistiigi Aug 31, 2018

tiborvass Sep 1, 2018

tiborvass commented Sep 1, 2018

vdemeester left a comment

thaJeztah commented Oct 25, 2018

builder: add prune options to the API #37651

builder: add prune options to the API #37651

Conversation

tiborvass commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewhsu commented Aug 19, 2018

Choose a reason for hiding this comment

andrewhsu commented Aug 21, 2018

andrewhsu commented Aug 21, 2018

tiborvass commented Aug 21, 2018

tonistiigi commented Aug 28, 2018

tiborvass commented Aug 31, 2018

tiborvass commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented Sep 1, 2018

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 25, 2018

tiborvass commented Aug 15, 2018 •

edited

Loading