From 52f4e9b1342d80f2af6459316653c75af34e19d1 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 24 Sep 2019 17:53:39 +0000 Subject: [PATCH 1/3] daemon/config: fix filter type in BuildKit GC config For backwards compatibility, the old incorrect object format for builder.GC.Rule.Filter still works but is deprecated in favor of array of strings akin to what needs to be passed on the CLI. Signed-off-by: Tibor Vass (cherry picked from commit fbdd437d295595e88466b33a550a8707b9ebb709) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 1e26b431c944402e62f0e652362b54ac24925cfc Component: engine --- .../engine/builder/builder-next/controller.go | 3 +- components/engine/daemon/config/builder.go | 34 ++++++++++++-- .../engine/daemon/config/builder_test.go | 44 +++++++++++++++++++ 3 files changed, 76 insertions(+), 5 deletions(-) create mode 100644 components/engine/daemon/config/builder_test.go diff --git a/components/engine/builder/builder-next/controller.go b/components/engine/builder/builder-next/controller.go index e740a76583b..4b33412ba45 100644 --- a/components/engine/builder/builder-next/controller.go +++ b/components/engine/builder/builder-next/controller.go @@ -8,6 +8,7 @@ import ( "github.com/containerd/containerd/content/local" "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/filters" "github.com/docker/docker/builder/builder-next/adapters/containerimage" "github.com/docker/docker/builder/builder-next/adapters/localinlinecache" "github.com/docker/docker/builder/builder-next/adapters/snapshot" @@ -232,7 +233,7 @@ func getGCPolicy(conf config.BuilderConfig, root string) ([]client.PruneInfo, er gcPolicy[i], err = toBuildkitPruneInfo(types.BuildCachePruneOptions{ All: p.All, KeepStorage: b, - Filters: p.Filter, + Filters: filters.Args(p.Filter), }) if err != nil { return nil, err diff --git a/components/engine/daemon/config/builder.go b/components/engine/daemon/config/builder.go index ac85e76b303..5b774d2f079 100644 --- a/components/engine/daemon/config/builder.go +++ b/components/engine/daemon/config/builder.go @@ -1,12 +1,38 @@ package config -import "github.com/docker/docker/api/types/filters" +import ( + "encoding/json" + "strings" + + "github.com/docker/docker/api/types/filters" +) // BuilderGCRule represents a GC rule for buildkit cache type BuilderGCRule struct { - All bool `json:",omitempty"` - Filter filters.Args `json:",omitempty"` - KeepStorage string `json:",omitempty"` + All bool `json:",omitempty"` + Filter BuilderGCFilter `json:",omitempty"` + KeepStorage string `json:",omitempty"` +} + +type BuilderGCFilter filters.Args + +func (x *BuilderGCFilter) UnmarshalJSON(data []byte) error { + var arr []string + f := filters.NewArgs() + if err := json.Unmarshal(data, &arr); err != nil { + // backwards compat for deprecated buggy form + err := json.Unmarshal(data, &f) + *x = BuilderGCFilter(f) + return err + } + for _, s := range arr { + fields := strings.SplitN(s, "=", 2) + name := strings.ToLower(strings.TrimSpace(fields[0])) + value := strings.TrimSpace(fields[1]) + f.Add(name, value) + } + *x = BuilderGCFilter(f) + return nil } // BuilderGCConfig contains GC config for a buildkit builder diff --git a/components/engine/daemon/config/builder_test.go b/components/engine/daemon/config/builder_test.go new file mode 100644 index 00000000000..db3225fdd03 --- /dev/null +++ b/components/engine/daemon/config/builder_test.go @@ -0,0 +1,44 @@ +package config + +import ( + "testing" + + "github.com/docker/docker/api/types/filters" + "github.com/google/go-cmp/cmp" + "gotest.tools/assert" + "gotest.tools/fs" +) + +func TestBuilderGC(t *testing.T) { + tempFile := fs.NewFile(t, "config", fs.WithContent(`{ + "builder": { + "gc": { + "enabled": true, + "policy": [ + {"keepStorage": "10GB", "filter": ["unused-for=2200h"]}, + {"keepStorage": "50GB", "filter": {"unused-for": {"3300h": true}}}, + {"keepStorage": "100GB", "all": true} + ] + } + } +}`)) + defer tempFile.Remove() + configFile := tempFile.Path() + + cfg, err := MergeDaemonConfigurations(&Config{}, nil, configFile) + assert.NilError(t, err) + assert.Assert(t, cfg.Builder.GC.Enabled) + f1 := filters.NewArgs() + f1.Add("unused-for", "2200h") + f2 := filters.NewArgs() + f2.Add("unused-for", "3300h") + expectedPolicy := []BuilderGCRule{ + {KeepStorage: "10GB", Filter: BuilderGCFilter(f1)}, + {KeepStorage: "50GB", Filter: BuilderGCFilter(f2)}, /* parsed from deprecated form */ + {KeepStorage: "100GB", All: true}, + } + assert.DeepEqual(t, cfg.Builder.GC.Policy, expectedPolicy, cmp.AllowUnexported(BuilderGCFilter{})) + // double check to please the skeptics + assert.Assert(t, filters.Args(cfg.Builder.GC.Policy[0].Filter).UniqueExactMatch("unused-for", "2200h")) + assert.Assert(t, filters.Args(cfg.Builder.GC.Policy[1].Filter).UniqueExactMatch("unused-for", "3300h")) +} From ff0c0a3af14bdf031ebc5ff7371c8d5652054b99 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Thu, 3 Oct 2019 00:09:50 +0000 Subject: [PATCH 2/3] daemon/config: add MarshalJSON for future proofing If anything marshals the daemon config now or in the future this commit ensures the correct canonical form for the builder GC policies' filters. Signed-off-by: Tibor Vass (cherry picked from commit 85733620ebea3da75abe7d732043354aa0883f8a) Signed-off-by: Sebastiaan van Stijn Upstream-commit: dae4436d1c742c88bba1a4e50a46f38f87f7ae17 Component: engine --- components/engine/api/types/filters/parse.go | 8 ++++++++ components/engine/daemon/config/builder.go | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/components/engine/api/types/filters/parse.go b/components/engine/api/types/filters/parse.go index 1f75403f78d..bef96d92c93 100644 --- a/components/engine/api/types/filters/parse.go +++ b/components/engine/api/types/filters/parse.go @@ -36,6 +36,14 @@ func NewArgs(initialArgs ...KeyValuePair) Args { return args } +func (args Args) Keys() []string { + keys := make([]string, 0, len(args.fields)) + for k := range args.fields { + keys = append(keys, k) + } + return keys +} + // MarshalJSON returns a JSON byte representation of the Args func (args Args) MarshalJSON() ([]byte, error) { if len(args.fields) == 0 { diff --git a/components/engine/daemon/config/builder.go b/components/engine/daemon/config/builder.go index 5b774d2f079..d9c65e7622a 100644 --- a/components/engine/daemon/config/builder.go +++ b/components/engine/daemon/config/builder.go @@ -2,6 +2,8 @@ package config import ( "encoding/json" + "fmt" + "sort" "strings" "github.com/docker/docker/api/types/filters" @@ -16,6 +18,20 @@ type BuilderGCRule struct { type BuilderGCFilter filters.Args +func (x *BuilderGCFilter) MarshalJSON() ([]byte, error) { + f := filters.Args(*x) + keys := f.Keys() + sort.Strings(keys) + arr := make([]string, 0, len(keys)) + for _, k := range keys { + values := f.Get(k) + for _, v := range values { + arr = append(arr, fmt.Sprintf("%s=%s", k, v)) + } + } + return json.Marshal(arr) +} + func (x *BuilderGCFilter) UnmarshalJSON(data []byte) error { var arr []string f := filters.NewArgs() From b2a43eed8f972bd0f466678d571ddae1dff4ae02 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 21 Oct 2019 21:34:05 +0200 Subject: [PATCH 3/3] Add GoDoc to fix linting validation The validate step in CI was broken, due to a combination of 086b4541cf9d27d9c2654f316a6f69b0d9caedd9, fbdd437d295595e88466b33a550a8707b9ebb709, and 85733620ebea3da75abe7d732043354aa0883f8a being merged to master. ``` api/types/filters/parse.go:39:1: exported method `Args.Keys` should have comment or be unexported (golint) func (args Args) Keys() []string { ^ daemon/config/builder.go:19:6: exported type `BuilderGCFilter` should have comment or be unexported (golint) type BuilderGCFilter filters.Args ^ daemon/config/builder.go:21:1: exported method `BuilderGCFilter.MarshalJSON` should have comment or be unexported (golint) func (x *BuilderGCFilter) MarshalJSON() ([]byte, error) { ^ daemon/config/builder.go:35:1: exported method `BuilderGCFilter.UnmarshalJSON` should have comment or be unexported (golint) func (x *BuilderGCFilter) UnmarshalJSON(data []byte) error { ^ ``` Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 9d726f1c18216a127572310fccb0fab8fcfdc678) Signed-off-by: Sebastiaan van Stijn Upstream-commit: e5a0bc6a50ef924ed6c0f333693857ab22ca47fa Component: engine --- components/engine/api/types/filters/parse.go | 1 + components/engine/daemon/config/builder.go | 3 +++ 2 files changed, 4 insertions(+) diff --git a/components/engine/api/types/filters/parse.go b/components/engine/api/types/filters/parse.go index bef96d92c93..0bd2e1e1853 100644 --- a/components/engine/api/types/filters/parse.go +++ b/components/engine/api/types/filters/parse.go @@ -36,6 +36,7 @@ func NewArgs(initialArgs ...KeyValuePair) Args { return args } +// Keys returns all the keys in list of Args func (args Args) Keys() []string { keys := make([]string, 0, len(args.fields)) for k := range args.fields { diff --git a/components/engine/daemon/config/builder.go b/components/engine/daemon/config/builder.go index d9c65e7622a..53e3056a0d5 100644 --- a/components/engine/daemon/config/builder.go +++ b/components/engine/daemon/config/builder.go @@ -16,8 +16,10 @@ type BuilderGCRule struct { KeepStorage string `json:",omitempty"` } +// BuilderGCFilter contains garbage-collection filter rules for a BuildKit builder type BuilderGCFilter filters.Args +// MarshalJSON returns a JSON byte representation of the BuilderGCFilter func (x *BuilderGCFilter) MarshalJSON() ([]byte, error) { f := filters.Args(*x) keys := f.Keys() @@ -32,6 +34,7 @@ func (x *BuilderGCFilter) MarshalJSON() ([]byte, error) { return json.Marshal(arr) } +// UnmarshalJSON fills the BuilderGCFilter values structure from JSON input func (x *BuilderGCFilter) UnmarshalJSON(data []byte) error { var arr []string f := filters.NewArgs()