From bd6868557d7ceb58d1d4717737e9da755cad87e5 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 8 May 2023 23:51:40 +0100 Subject: c8d/builder: implement cache Signed-off-by: Laura Brehm --- daemon/containerd/cache.go | 78 ++++++++++++++++++++------------------ daemon/containerd/image_builder.go | 26 +++++++++++-- daemon/containerd/image_commit.go | 9 +++-- 3 files changed, 69 insertions(+), 44 deletions(-) (limited to 'daemon') diff --git a/daemon/containerd/cache.go b/daemon/containerd/cache.go index e9fa697932..8bd01768f8 100644 --- a/daemon/containerd/cache.go +++ b/daemon/containerd/cache.go @@ -3,6 +3,7 @@ package containerd import ( "context" "reflect" + "strings" "github.com/docker/docker/api/types/container" imagetype "github.com/docker/docker/api/types/image" @@ -30,54 +31,57 @@ type imageCache struct { func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) { ctx := context.TODO() - cfgCpy := *cfg - i, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{}) + parent, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{}) if err != nil { - for _, ii := range ic.images { - if ii.ID().String() == parentID { - if compare(ii.RunConfig(), &cfgCpy) { - return ii.ID().String(), nil - } - } + return "", err + } + + for _, localCachedImage := range ic.images { + if isMatch(localCachedImage, parent, cfg) { + return localCachedImage.ID().String(), nil } - } else { - children, err := ic.c.Children(ctx, i.ID()) + } + + children, err := ic.c.Children(ctx, parent.ID()) + if err != nil { + return "", err + } + + for _, children := range children { + childImage, err := ic.c.GetImage(ctx, children.String(), imagetype.GetImageOpts{}) if err != nil { return "", err } - for _, ch := range children { - childImage, err := ic.c.GetImage(context.TODO(), ch.String(), imagetype.GetImageOpts{}) - if err != nil { - return "", err - } - // this implementation looks correct but it's currently not working - // with the containerd store as we're not storing the image ContainerConfig - // and so intermediate images with ContainerConfigs such as - // #(nop) COPY file:c6ab44934e83eeb07289a211582c6faa25dea7d06dae077b6ef76029e92400ce in ... - // are not getting a hit - if compare(&childImage.ContainerConfig, &cfgCpy) { - return ch.String(), nil - } + + if isMatch(childImage, parent, cfg) { + return children.String(), nil } } + return "", nil } -// compare two Config structs. Do not consider the "Hostname" field as it -// defaults to the randomly generated short container ID or "Image" as it -// represents the name of the image as it was passed in (symbolic) and does -// not provide any meaningful information about whether the image is usable -// as cache. -// If OpenStdin is set, then it differs -func compare(a, b *container.Config) bool { - if a == nil || b == nil || - a.OpenStdin || b.OpenStdin { +// isMatch checks whether a given target can be used as cache for the given +// parent image/config combination. +// A target can only be an immediate child of the given parent image. For +// a parent image with `n` history entries, a valid target must have `n+1` +// entries and the extra entry must match the provided config +func isMatch(target, parent *image.Image, cfg *container.Config) bool { + if target == nil || parent == nil || cfg == nil { + return false + } + + if len(target.History) != len(parent.History)+1 || + len(target.RootFS.DiffIDs) != len(parent.RootFS.DiffIDs)+1 { return false } - a.Image = "" - a.Hostname = "" - b.Image = "" - b.Hostname = "" - return reflect.DeepEqual(a, b) + for i := range parent.History { + if !reflect.DeepEqual(parent.History[i], target.History[i]) { + return false + } + } + + childCreatedBy := target.History[len(target.History)-1].CreatedBy + return childCreatedBy == strings.Join(cfg.Cmd, " ") } diff --git a/daemon/containerd/image_builder.go b/daemon/containerd/image_builder.go index 53adde38ad..c92bd8bf3e 100644 --- a/daemon/containerd/image_builder.go +++ b/daemon/containerd/image_builder.go @@ -402,6 +402,18 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st exposedPorts[string(k)] = v } + var ociHistory []ocispec.History + for _, history := range imgToCreate.History { + created := history.Created + ociHistory = append(ociHistory, ocispec.History{ + Created: &created, + CreatedBy: history.CreatedBy, + Author: history.Author, + Comment: history.Comment, + EmptyLayer: history.EmptyLayer, + }) + } + // make an ocispec.Image from the docker/image.Image ociImgToCreate := ocispec.Image{ Created: &imgToCreate.Created, @@ -422,9 +434,8 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st Labels: imgToCreate.Config.Labels, StopSignal: imgToCreate.Config.StopSignal, }, - RootFS: rootfs, - // TODO(laurazard) - History: []ocispec.History{}, + RootFS: rootfs, + History: ociHistory, } var layers []ocispec.Descriptor @@ -457,6 +468,14 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st }, ) + // necessary to prevent the contents from being GC'd + // between writing them here and creating an image + ctx, done, err := i.client.WithLease(ctx, leases.WithRandomID(), leases.WithExpiration(1*time.Hour)) + if err != nil { + return nil, err + } + defer done(ctx) + commitManifestDesc, err := writeContentsForImage(ctx, i.snapshotter, i.client.ContentStore(), ociImgToCreate, layers) if err != nil { return nil, err @@ -487,5 +506,6 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st newImage := dimage.NewImage(dimage.ID(createdImage.Target.Digest)) newImage.V1Image = imgToCreate.V1Image newImage.V1Image.ID = string(createdImage.Target.Digest) + newImage.History = imgToCreate.History return newImage, nil } diff --git a/daemon/containerd/image_commit.go b/daemon/containerd/image_commit.go index a721c41970..632f28bc6b 100644 --- a/daemon/containerd/image_commit.go +++ b/daemon/containerd/image_commit.go @@ -141,10 +141,11 @@ func generateCommitImageConfig(baseConfig ocispec.Image, diffID digest.Digest, o DiffIDs: append(baseConfig.RootFS.DiffIDs, diffID), }, History: append(baseConfig.History, ocispec.History{ - Created: &createdTime, - CreatedBy: strings.Join(opts.ContainerConfig.Cmd, " "), - Author: opts.Author, - Comment: opts.Comment, + Created: &createdTime, + CreatedBy: strings.Join(opts.ContainerConfig.Cmd, " "), + Author: opts.Author, + Comment: opts.Comment, + // TODO(laurazard): this check might be incorrect EmptyLayer: diffID == "", }), } -- cgit v1.2.1