vendor: update buildkit to v0.11.2 by tonistiigi · Pull Request #44686 · moby/moby

tonistiigi · 2022-12-22T01:34:07Z

replaces, closes vendor: github.com/moby/buildkit v0.11.0-rc2 #44529

tonistiigi · 2022-12-22T03:32:25Z

There are a couple of test failures. Investigating.

thaJeztah

thanks! left some comments/questions

thaJeztah · 2022-12-22T14:00:23Z

builder/builder-next/exporter/export.go

 }

-func (e *imageExporterInstance) Export(ctx context.Context, inp exporter.Source, sessionID string) (map[string]string, error) {
+func (e *imageExporterInstance) Export(ctx context.Context, inp *exporter.Source, sessionID string) (map[string]string, exporter.DescriptorReference, error) {


This signature is starting to look a bit awkward; I had a quick look, and it seems this is ultimately defined in the BuildKit repository. As it's being updated already, would it make sense to have a struct definition for the returned values (which contains both the map[string]string and the DescriptoReference) work?

I haven't looked very closely, but the existing map[string]string returns this;

return map[string]string{ exptypes.ExporterImageConfigDigestKey: configDigest.String(), exptypes.ExporterImageDigestKey: id.String(), }

Which feels like it was intended as a "generic" key/value list (maybe I'm wrong though), so perhaps the map could be a map[string]interface instead, OR ( if the "what should be returned" should be more formal) the map could be discarded and individual fields defined for them.

These are for different use cases. Map is for the client to handle metadata while the new field in for buildkit internal post-processing that runs after the exporter. If there are more fields for post-processing in the future, then the new field should be turned into struct instead of adding a 4th var.

thaJeztah · 2022-12-22T14:03:32Z

builder/builder-next/controller.go

+		hconf = &bkconfig.HistoryConfig{
+			MaxAge:     int64(opt.BuilderConfig.History.MaxAge),
+			MaxEntries: int64(opt.BuilderConfig.History.MaxEntries),
+		}


Should we change the opt.BuilderConfig.History type to match, so that we don't have to cast to an int64 ?

thaJeztah · 2022-12-22T14:06:08Z

daemon/config/builder.go

+// BuilderHistoryConfig contains history config for a buildkit builder
+type BuilderHistoryConfig struct {
+	MaxAge     int `json:",omitempty"`
+	MaxEntries int `json:",omitempty"`
+}


Oh! And I just realise that's a new type; any reason for using int here (instead of int64)?

Potentially this could even be an alias for the buildkit type (cmd/buildkitd/config.HistoryConfig), although if it becomes a public API, perhaps that config package should be moved outside of cmd as it looks it's no longer really a command-line config only (but options for a buildkit instance)

thaJeztah · 2022-12-22T14:08:37Z

builder/builder-next/worker/worker.go

+func init() {
+	version.Version = "v0.11.0-rc3"
+}
+


Should we have a plain var for this, so that it can be through a -X option at compile time?

If you want to set it with -X then I think you can just set the version.Version directly there. Or is there any problem with -X not working with vendored packages?

I was wondering that indeed; wasn't sure if it would allow setting things in vendor.

I think it's fine, maybe also add a comment in vendor.mod:

moby/vendor.mod

Line 54 in b9fe30d

github.com/moby/buildkit v0.10.6

to not forget to also update this.

thaJeztah · 2022-12-22T14:09:34Z

builder/builder-next/worker/worker.go

+func (w *Worker) BuildkitVersion() client.BuildkitVersion {
+	return client.BuildkitVersion{
+		Package:  version.Package,
+		Version:  version.Version + "-moby",


Still wondering a bit if -moby would already be reflected from the builder type (container vs docker / moby)

thaJeztah · 2022-12-22T14:11:31Z

builder/builder-next/worker/worker.go

+	return client.BuildkitVersion{
+		Package:  version.Package,
+		Version:  version.Version + "-moby",
+		Revision: version.Revision,


This probably needs packaging changes to propagate the version (as the version from the vendored BuildKit code won't be set) 🤔

Hum yes we might need smth like: crazy-max@59746a8

@crazy-max remind me to this one 😅

thaJeztah · 2022-12-22T14:17:07Z

builder/builder-next/controller.go

 		return nil, err
 	}

+	historyDB, err := bbolt.Open(filepath.Join(opt.Root, "history.db"), 0600, nil)


Nit; should use the new 0o format (linters start to expect these, so we may as well be ahead of some of that)

Suggested change

historyDB, err := bbolt.Open(filepath.Join(opt.Root, "history.db"), 0600, nil)

historyDB, err := bbolt.Open(filepath.Join(opt.Root, "history.db"), 0o600, nil)

Also; question: is there code somewhere to clean up these files and/or truncate them? I was wondering what these contain, and if (some of) these should be / become part of docker builder prune or docker system prune, or perhaps even should be deleted on daemon restart (maybe we do some of that already, haven't checked).

full diff: tonistiigi/fsutil@9ed6126...0127568 Signed-off-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Tonis Tiigi <[email protected]>

Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi · 2023-02-02T08:54:23Z

Green now.

thaJeztah · 2023-02-02T11:45:41Z

.github/workflows/buildkit.yml

+          echo "BUILDKIT_REPO=tonistiigi/buildkit" >> $GITHUB_ENV
+          echo "BUILDKIT_REF=db67180a1a439efb1547ecf5decd4003ec8f621b" >> $GITHUB_ENV


Note to self; we can remove this once moby/buildkit#3567 is merged and cherry-picked in the 0.11 branch

thaJeztah

LGTM, thanks!

tonistiigi force-pushed the buildkit-update-v0.11-rc3 branch from da18165 to 33cd806 Compare December 22, 2022 02:43

AkihiroSuda approved these changes Dec 22, 2022

View reviewed changes

thaJeztah reviewed Dec 22, 2022

View reviewed changes

tonistiigi force-pushed the buildkit-update-v0.11-rc3 branch from 33cd806 to 13d274b Compare December 22, 2022 17:29

crazy-max mentioned this pull request Jan 3, 2023

containerd integration: Make build work #44079

Merged

cpuguy83 approved these changes Jan 3, 2023

View reviewed changes

thaJeztah added area/builder Build status/2-code-review impact/changelog area/builder/buildkit Build labels Jan 9, 2023

tonistiigi force-pushed the buildkit-update-v0.11-rc3 branch from 13d274b to 36f4f3f Compare January 19, 2023 00:57

tonistiigi changed the title ~~vendor: update buildkit to v0.11.0-rc3~~ vendor: update buildkit to v0.11.1 Jan 19, 2023

thaJeztah added this to the v-next milestone Jan 20, 2023

tonistiigi force-pushed the buildkit-update-v0.11-rc3 branch from 36f4f3f to fdee00a Compare February 2, 2023 07:10

thaJeztah and others added 2 commits February 1, 2023 23:10

vendor: github.com/tonistiigi/fsutil v0.0.0-20221114235510-0127568185cf

85169a0

full diff: tonistiigi/fsutil@9ed6126...0127568 Signed-off-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Tonis Tiigi <[email protected]>

vendor: update buildkit to v0.11.2

666334b

Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi force-pushed the buildkit-update-v0.11-rc3 branch from fdee00a to 99ff094 Compare February 2, 2023 07:29

github: apply patch for CNI test that was incorrectly skipped

043dbc0

Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi force-pushed the buildkit-update-v0.11-rc3 branch from 99ff094 to 043dbc0 Compare February 2, 2023 07:57

tonistiigi changed the title ~~vendor: update buildkit to v0.11.1~~ vendor: update buildkit to v0.11.2 Feb 2, 2023

thaJeztah reviewed Feb 2, 2023

View reviewed changes

thaJeztah approved these changes Feb 2, 2023

View reviewed changes

crazy-max approved these changes Feb 2, 2023

View reviewed changes

vvoland approved these changes Feb 2, 2023

View reviewed changes

cpuguy83 approved these changes Feb 2, 2023

View reviewed changes

cpuguy83 merged commit 8413828 into moby:master Feb 2, 2023

This was referenced Feb 2, 2023

ci: enable BuildKit tests with containerd integration worker #44908

Merged

Set BuildKit version and revision at build time #44917

Closed

This was referenced Feb 3, 2023

[0.11 backport] integration: fix compat check for CNI DNS test moby/buildkit#3578

Merged

gha: switch buildkit back to upstream for testing #44929

Merged

thaJeztah mentioned this pull request Mar 31, 2023

Bump moby/buildkit to v0.11.4 docker/cli#4149

Closed

crazy-max mentioned this pull request Apr 11, 2023

Set BuildKit version using buildinfo #45304

Merged

	historyDB, err := bbolt.Open(filepath.Join(opt.Root, "history.db"), 0600, nil)
	historyDB, err := bbolt.Open(filepath.Join(opt.Root, "history.db"), 0o600, nil)

		echo "BUILDKIT_REPO=tonistiigi/buildkit" >> $GITHUB_ENV
		echo "BUILDKIT_REF=db67180a1a439efb1547ecf5decd4003ec8f621b" >> $GITHUB_ENV

Conversation

tonistiigi commented Dec 22, 2022 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Dec 22, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Feb 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tonistiigi commented Dec 22, 2022 •

edited by thaJeztah

Loading