c8d: implement classic builder by laurazard · Pull Request #45322 · moby/moby

laurazard · 2023-04-13T12:43:53Z

- What I did

Made the classic builder work with the containerd store

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

daemon/containerd/cache.go

thaJeztah · 2023-04-13T13:51:44Z

Oh! Some failure in a mock;

builder/dockerfile/buildargs.go:1: : # github.com/docker/docker/builder/dockerfile [github.com/docker/docker/builder/dockerfile.test]
builder/dockerfile/dispatchers_test.go:37:18: cannot use mockBackend (variable of type *MockBackend) as builder.Backend value in struct literal: *MockBackend does not implement builder.Backend (wrong type for method CreateImage)

If it's useful, a nice trick to make those satisfy the interface (for methods that won't be called) is to embed the actual interface;

type fakeSomething {
    somepackage.TheInterface
}

func (foo fakeSomething) SomeMethod() {
}

// .. etc

Also some other minor linting failures;

daemon/containerd/image_builder.go:158:4: ineffectual assignment to err (ineffassign)
			err = nil
			^
daemon/containerd/image_builder.go:275:24: ineffectual assignment to err (ineffassign)
		parentImageManifest, err := containerdimages.Manifest(ctx, i.client.ContentStore(), parentDesc, platforms.Default())
		                     ^
daemon/containerd/image_builder.go:318:30: SA1019: dimage.IDFromDigest is deprecated: cast to an ID using ID(digest). (staticcheck)
	newImage := dimage.NewImage(dimage.IDFromDigest(createdImage.Target.Digest))
	                            ^

daemon/containerd/image_commit.go

daemon/containerd/image_list.go

tianon · 2023-04-18T23:23:17Z

daemon/containerd/image_builder.go

+		if opts.Platform != nil {
+			os = opts.Platform.OS
+		}
+		if !system.IsOSSupported(os) {


Some discussion on this in rumpl#135 (review) that we should move over here to get more eyes/opinions.

The short version is that OS checks here are probably to prevent us from pulling/unpacking/trying to run Windows images on Linux, because they now pull and even unpack successfully, but will fail to run. The inverse is true on Windows (something something LCOW), except there the Linux images might even fail to unpack, and the Windows images are enormous so the pain is much more acute on Linux.

With runtimes like WASM, this becomes problematic and we do want to pull and even run non-matching images. IMO, this needs some larger design changes in the engine to support fully (some way to specify a default-runtime-per-platform or something?) but it is an interesting problem to consider in general that the pulling/resolving logic needs to be somehow aware of what's possible to run (more than just OS+arch+variant).

thaJeztah · 2023-04-26T20:32:02Z

@laurazard 🙈 needs a rebase

tianon · 2023-05-09T21:37:48Z

In playing with this a bit, one (not at all unexpected!) quirk is that all the intermediate images of a build show up in docker images as "dangling" images (unlike the old classic builder, where we have explicit parental relationships and thus those intermediate images still exist but are trivial to hide unless/until their children are removed).

Do you think there's some way we could store the data reasonably such that docker images could hide those intermediate dangling images?

Essentially if it's both dangling and has children, it should hide; probably too expensive of a calculation since we're not storing explicit relationships?

To be clear, I don't think this is a blocker in the slightest.

tianon · 2023-05-09T21:40:42Z

daemon/containerd/cache.go

+		}
+	}
+
+	children, err := ic.c.Children(ctx, parent.ID())


Apologies for not already knowing (or knowing exactly where to look 🙈), but is this call already returning results in "reverse created" order such that we make sure the "freshest" cache is considered first? 😅

Aha, nope:

moby/daemon/containerd/image_children.go

Lines 17 to 82 in 563fc92

// Children returns a slice of image ID which rootfs is a superset of the

// rootfs of the given image ID, excluding images with exactly the same rootfs.

// Called from list.go to filter containers.

func (i *ImageService) Children(ctx context.Context, id image.ID) ([]image.ID, error) {

target, err := i.resolveDescriptor(ctx, id.String())

if err != nil {

return []image.ID{}, errors.Wrap(err, "failed to get parent image")

}

cs := i.client.ContentStore()

allPlatforms, err := containerdimages.Platforms(ctx, cs, target)

if err != nil {

return []image.ID{}, errdefs.System(errors.Wrap(err, "failed to list platforms supported by image"))

}

parentRootFS := []ocispec.RootFS{}

for _, platform := range allPlatforms {

rootfs, err := platformRootfs(ctx, cs, target, platform)

if err != nil {

if !cerrdefs.IsNotFound(err) {

logrus.WithFields(logrus.Fields{

logrus.ErrorKey: err,

"image": target.Digest,

"platform": platform,

}).Warning("failed to get platform-specific rootfs")

}

continue

}

parentRootFS = append(parentRootFS, rootfs)

}

imgs, err := i.client.ImageService().List(ctx)

if err != nil {

return []image.ID{}, errdefs.System(errors.Wrap(err, "failed to list all images"))

}

children := []image.ID{}

for _, img := range imgs {

nextImage:

for _, platform := range allPlatforms {

rootfs, err := platformRootfs(ctx, cs, img.Target, platform)

if err != nil {

if !cerrdefs.IsNotFound(err) {

logrus.WithFields(logrus.Fields{

logrus.ErrorKey: err,

"image": img.Target.Digest,

"platform": platform,

}).Warning("failed to get platform-specific rootfs")

}

continue

}

for _, parentRoot := range parentRootFS {

if isRootfsChildOf(rootfs, parentRoot) {

children = append(children, image.ID(img.Target.Digest))

break nextImage

}

}

}

}

return children, nil

}

So we probably need a similar TODO like this one?

moby/daemon/containerd/image_list.go

Line 39 in 563fc92

// TODO(thaJeztah): sort the results by created (descending); see https://github.com/moby/moby/issues/43848

Co-authored-by: Djordje Lukic <djordje.lukic@docker.com> Signed-off-by: Laura Brehm <laurabrehm@hey.com>

Signed-off-by: Laura Brehm <laurabrehm@hey.com>

thaJeztah · 2023-05-11T17:28:29Z

FWIW; with this PR we're down to (on Jenkins) only 3 failures with containerd-integration enabled ❗❗❗

[don't merge] ci: enable TEST_INTEGRATION_USE_SNAPSHOTTER #45232

thaJeztah

LGTM

we discussed this, and as it doesn't impact non-containerd code, we'd like to get this in and have a wider range of users give it a spin.

laurazard requested a review from tonistiigi as a code owner April 13, 2023 12:43

vvoland reviewed Apr 13, 2023

View reviewed changes

daemon/containerd/cache.go Outdated Show resolved Hide resolved

thaJeztah added area/builder Build impact/changelog area/builder/classic-builder Build containerd-integration Issues and PRs related to containerd integration labels Apr 13, 2023

thaJeztah added this to the 24.0.0 milestone Apr 13, 2023

laurazard force-pushed the c8d-upstream-classic-builder branch 5 times, most recently from 8c63648 to 48e341a Compare April 14, 2023 09:42

thaJeztah mentioned this pull request Apr 14, 2023

[don't merge] ci: enable TEST_INTEGRATION_USE_SNAPSHOTTER #45232

Closed

laurazard force-pushed the c8d-upstream-classic-builder branch 2 times, most recently from 53d04b9 to a106bff Compare April 17, 2023 09:38

thaJeztah reviewed Apr 17, 2023

View reviewed changes

daemon/containerd/image_commit.go Outdated Show resolved Hide resolved

thaJeztah reviewed Apr 17, 2023

View reviewed changes

daemon/containerd/image_list.go Outdated Show resolved Hide resolved

laurazard force-pushed the c8d-upstream-classic-builder branch 3 times, most recently from 58d1248 to fb3217e Compare April 18, 2023 18:12

tianon reviewed Apr 18, 2023

View reviewed changes

laurazard force-pushed the c8d-upstream-classic-builder branch from fb3217e to d6b1b3e Compare April 19, 2023 16:25

rumpl modified the milestones: 24.0.0, v-future Apr 24, 2023

thaJeztah mentioned this pull request Apr 29, 2023

c8d: commit: generateCommitImageConfig: don't merge image config #45438

Merged

laurazard force-pushed the c8d-upstream-classic-builder branch 2 times, most recently from a667438 to a06a139 Compare May 9, 2023 15:05

laurazard mentioned this pull request May 9, 2023

c8d: add image history #45501

Merged

tianon mentioned this pull request May 9, 2023

Use OCI "History" type instead of inventing our own copy #45504

Merged

tianon reviewed May 9, 2023

View reviewed changes

laurazard mentioned this pull request May 10, 2023

c8d: classic builder rumpl/moby#135

Closed

c8d: implement classic builder

e46674b

Co-authored-by: Djordje Lukic <djordje.lukic@docker.com> Signed-off-by: Laura Brehm <laurabrehm@hey.com>

laurazard force-pushed the c8d-upstream-classic-builder branch from a06a139 to 7749c74 Compare May 11, 2023 10:58

c8d/builder: implement cache

bd68685

Signed-off-by: Laura Brehm <laurabrehm@hey.com>

laurazard force-pushed the c8d-upstream-classic-builder branch from 7749c74 to bd68685 Compare May 11, 2023 11:02

rumpl approved these changes May 11, 2023

View reviewed changes

vvoland approved these changes May 11, 2023

View reviewed changes

tianon approved these changes May 11, 2023

View reviewed changes

thaJeztah approved these changes May 11, 2023

View reviewed changes

tianon merged commit 46ce4ec into moby:master May 11, 2023

cpuguy83 approved these changes May 11, 2023

View reviewed changes

laurazard mentioned this pull request May 11, 2023

[24.0 backport] c8d: implement classic builder #45526

Merged

thaJeztah modified the milestones: v-future, 25.0.0 Jun 1, 2023

neersighted added the process/cherry-picked label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d: implement classic builder#45322

c8d: implement classic builder#45322
tianon merged 2 commits intomoby:masterfrom
laurazard:c8d-upstream-classic-builder

laurazard commented Apr 13, 2023

Uh oh!

Uh oh!

thaJeztah commented Apr 13, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

tianon Apr 18, 2023

Uh oh!

thaJeztah commented Apr 26, 2023

Uh oh!

tianon commented May 9, 2023

Uh oh!

tianon May 9, 2023

Uh oh!

tianon May 9, 2023

Uh oh!

thaJeztah commented May 11, 2023

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	// Children returns a slice of image ID which rootfs is a superset of the
	// rootfs of the given image ID, excluding images with exactly the same rootfs.
	// Called from list.go to filter containers.
	func (i *ImageService) Children(ctx context.Context, id image.ID) ([]image.ID, error) {
	target, err := i.resolveDescriptor(ctx, id.String())
	if err != nil {
	return []image.ID{}, errors.Wrap(err, "failed to get parent image")
	}

	cs := i.client.ContentStore()

	allPlatforms, err := containerdimages.Platforms(ctx, cs, target)
	if err != nil {
	return []image.ID{}, errdefs.System(errors.Wrap(err, "failed to list platforms supported by image"))
	}

	parentRootFS := []ocispec.RootFS{}
	for _, platform := range allPlatforms {
	rootfs, err := platformRootfs(ctx, cs, target, platform)
	if err != nil {
	if !cerrdefs.IsNotFound(err) {
	logrus.WithFields(logrus.Fields{
	logrus.ErrorKey: err,
	"image": target.Digest,
	"platform": platform,
	}).Warning("failed to get platform-specific rootfs")
	}
	continue
	}

	parentRootFS = append(parentRootFS, rootfs)
	}

	imgs, err := i.client.ImageService().List(ctx)
	if err != nil {
	return []image.ID{}, errdefs.System(errors.Wrap(err, "failed to list all images"))
	}

	children := []image.ID{}
	for _, img := range imgs {
	nextImage:
	for _, platform := range allPlatforms {
	rootfs, err := platformRootfs(ctx, cs, img.Target, platform)
	if err != nil {
	if !cerrdefs.IsNotFound(err) {
	logrus.WithFields(logrus.Fields{
	logrus.ErrorKey: err,
	"image": img.Target.Digest,
	"platform": platform,
	}).Warning("failed to get platform-specific rootfs")
	}
	continue
	}

	for _, parentRoot := range parentRootFS {
	if isRootfsChildOf(rootfs, parentRoot) {
	children = append(children, image.ID(img.Target.Digest))
	break nextImage
	}
	}
	}

	}

	return children, nil
	}

Conversation

laurazard commented Apr 13, 2023

Uh oh!

Uh oh!

thaJeztah commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tianon Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 26, 2023

Uh oh!

tianon commented May 9, 2023

Uh oh!

tianon May 9, 2023

Choose a reason for hiding this comment

Uh oh!

tianon May 9, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 11, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thaJeztah commented Apr 13, 2023 •

edited

Loading