From 207c4d537c6b0bd45c6798b5c08ff1b05603fd1b Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Sat, 16 Sep 2023 20:06:12 +0200 Subject: [PATCH] c8d: Fix `docker diff` Diffing a container yielded some extra changes that come from the files/directories that we mount inside the container (/etc/resolv.conf for example). To avoid that we create an intermediate snapshot that has these files, with this we can now diff the container fs with its parent and only get the differences that were made inside the container. Signed-off-by: Djordje Lukic --- daemon/containerd/image_changes.go | 53 ++++----------------- daemon/containerd/image_snapshot.go | 42 +++++++++++----- daemon/containerd/image_snapshot_unix.go | 17 +------ daemon/containerd/image_snapshot_windows.go | 3 +- daemon/containerd/service.go | 12 ----- daemon/create.go | 2 +- daemon/image_service.go | 2 +- daemon/images/image.go | 2 +- integration/container/diff_test.go | 26 ++++++++-- 9 files changed, 65 insertions(+), 94 deletions(-) diff --git a/daemon/containerd/image_changes.go b/daemon/containerd/image_changes.go index 9d2802520b..d51de8a06f 100644 --- a/daemon/containerd/image_changes.go +++ b/daemon/containerd/image_changes.go @@ -2,68 +2,35 @@ package containerd import ( "context" - "encoding/json" - "github.com/containerd/containerd/content" "github.com/containerd/containerd/log" "github.com/containerd/containerd/mount" "github.com/docker/docker/container" "github.com/docker/docker/pkg/archive" - "github.com/google/uuid" - "github.com/opencontainers/image-spec/identity" - ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) func (i *ImageService) Changes(ctx context.Context, container *container.Container) ([]archive.Change, error) { - cs := i.client.ContentStore() - - imageManifest, err := getContainerImageManifest(container) - if err != nil { - return nil, err - } - - imageManifestBytes, err := content.ReadBlob(ctx, cs, imageManifest) - if err != nil { - return nil, err - } - var manifest ocispec.Manifest - if err := json.Unmarshal(imageManifestBytes, &manifest); err != nil { - return nil, err - } - - imageConfigBytes, err := content.ReadBlob(ctx, cs, manifest.Config) - if err != nil { - return nil, err - } - var image ocispec.Image - if err := json.Unmarshal(imageConfigBytes, &image); err != nil { - return nil, err - } - - rnd, err := uuid.NewRandom() - if err != nil { - return nil, err - } - snapshotter := i.client.SnapshotService(container.Driver) - - diffIDs := image.RootFS.DiffIDs - parent, err := snapshotter.View(ctx, rnd.String(), identity.ChainID(diffIDs).String()) + info, err := snapshotter.Stat(ctx, container.ID) if err != nil { return nil, err } + + imageMounts, _ := snapshotter.View(ctx, container.ID+"-parent-view", info.Parent) + defer func() { - if err := snapshotter.Remove(ctx, rnd.String()); err != nil { - log.G(ctx).WithError(err).WithField("key", rnd.String()).Warn("remove temporary snapshot") + if err := snapshotter.Remove(ctx, container.ID+"-parent-view"); err != nil { + log.G(ctx).WithError(err).Warn("error removing the parent view snapshot") } }() var changes []archive.Change - err = i.PerformWithBaseFS(ctx, container, func(containerRootfs string) error { - return mount.WithReadonlyTempMount(ctx, parent, func(parentRootfs string) error { - changes, err = archive.ChangesDirs(containerRootfs, parentRootfs) + err = i.PerformWithBaseFS(ctx, container, func(containerRoot string) error { + return mount.WithReadonlyTempMount(ctx, imageMounts, func(imageRoot string) error { + changes, err = archive.ChangesDirs(containerRoot, imageRoot) return err }) }) + return changes, err } diff --git a/daemon/containerd/image_snapshot.go b/daemon/containerd/image_snapshot.go index 40eeccdd0c..190c2e0321 100644 --- a/daemon/containerd/image_snapshot.go +++ b/daemon/containerd/image_snapshot.go @@ -8,6 +8,7 @@ import ( cerrdefs "github.com/containerd/containerd/errdefs" containerdimages "github.com/containerd/containerd/images" "github.com/containerd/containerd/leases" + "github.com/containerd/containerd/mount" "github.com/containerd/containerd/platforms" "github.com/containerd/containerd/snapshots" "github.com/docker/docker/errdefs" @@ -19,7 +20,7 @@ import ( const remapSuffix = "-remap" // PrepareSnapshot prepares a snapshot from a parent image for a container -func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform) error { +func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform, setupInit func(string) error) error { var parentSnapshot string if parentImage != "" { img, err := i.resolveImage(ctx, parentImage) @@ -59,27 +60,42 @@ func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, parentIma parentSnapshot = identity.ChainID(diffIDs).String() } - // Add a lease so that containerd doesn't garbage collect our snapshot ls := i.client.LeasesService() lease, err := ls.Create(ctx, leases.WithID(id)) if err != nil { return err } - if err := ls.AddResource(ctx, lease, leases.Resource{ - ID: id, - Type: "snapshots/" + i.StorageDriver(), + ctx = leases.WithLease(ctx, lease.ID) + + snapshotter := i.client.SnapshotService(i.StorageDriver()) + + if err := i.prepareInitLayer(ctx, id, parentSnapshot, setupInit); err != nil { + return err + } + + if !i.idMapping.Empty() { + return i.remapSnapshot(ctx, snapshotter, id, id+"-init") + } + + _, err = snapshotter.Prepare(ctx, id, id+"-init") + return err +} + +func (i *ImageService) prepareInitLayer(ctx context.Context, id string, parent string, setupInit func(string) error) error { + snapshotter := i.client.SnapshotService(i.StorageDriver()) + + mounts, err := snapshotter.Prepare(ctx, id+"-init-key", parent) + if err != nil { + return err + } + + if err := mount.WithTempMount(ctx, mounts, func(root string) error { + return setupInit(root) }); err != nil { return err } - snapshotter := i.client.SnapshotService(i.StorageDriver()) - - if !i.idMapping.Empty() { - return i.remapSnapshot(ctx, snapshotter, id, parentSnapshot, lease) - } - - _, err = snapshotter.Prepare(ctx, id, parentSnapshot) - return err + return snapshotter.Commit(ctx, id+"-init", id+"-init-key") } // calculateSnapshotParentUsage returns the usage of all ancestors of the diff --git a/daemon/containerd/image_snapshot_unix.go b/daemon/containerd/image_snapshot_unix.go index d5e2977601..ee85bc8839 100644 --- a/daemon/containerd/image_snapshot_unix.go +++ b/daemon/containerd/image_snapshot_unix.go @@ -9,15 +9,13 @@ import ( "path/filepath" "syscall" - "github.com/containerd/containerd/leases" "github.com/containerd/containerd/log" "github.com/containerd/containerd/mount" "github.com/containerd/containerd/snapshots" "github.com/docker/docker/pkg/idtools" ) -func (i *ImageService) remapSnapshot(ctx context.Context, snapshotter snapshots.Snapshotter, id string, parentSnapshot string, lease leases.Lease) error { - ls := i.client.LeasesService() +func (i *ImageService) remapSnapshot(ctx context.Context, snapshotter snapshots.Snapshotter, id string, parentSnapshot string) error { rootPair := i.idMapping.RootPair() usernsID := fmt.Sprintf("%s-%d-%d", parentSnapshot, rootPair.UID, rootPair.GID) remappedID := usernsID + remapSuffix @@ -28,19 +26,6 @@ func (i *ImageService) remapSnapshot(ctx context.Context, snapshotter snapshots. return err } - if err := ls.AddResource(ctx, lease, leases.Resource{ - ID: remappedID, - Type: "snapshots/" + i.StorageDriver(), - }); err != nil { - return err - } - if err := ls.AddResource(ctx, lease, leases.Resource{ - ID: usernsID, - Type: "snapshots/" + i.StorageDriver(), - }); err != nil { - return err - } - mounts, err := snapshotter.Prepare(ctx, remappedID, parentSnapshot) if err != nil { return err diff --git a/daemon/containerd/image_snapshot_windows.go b/daemon/containerd/image_snapshot_windows.go index 7330efd17f..bb4ed3ac12 100644 --- a/daemon/containerd/image_snapshot_windows.go +++ b/daemon/containerd/image_snapshot_windows.go @@ -3,10 +3,9 @@ package containerd import ( "context" - "github.com/containerd/containerd/leases" "github.com/containerd/containerd/snapshots" ) -func (i *ImageService) remapSnapshot(ctx context.Context, snapshotter snapshots.Snapshotter, id string, parentSnapshot string, lease leases.Lease) error { +func (i *ImageService) remapSnapshot(ctx context.Context, snapshotter snapshots.Snapshotter, id string, parentSnapshot string) error { return nil } diff --git a/daemon/containerd/service.go b/daemon/containerd/service.go index c751c81f96..0fee95df9f 100644 --- a/daemon/containerd/service.go +++ b/daemon/containerd/service.go @@ -21,7 +21,6 @@ import ( "github.com/docker/docker/layer" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/registry" - ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" ) @@ -186,14 +185,3 @@ func (i *ImageService) GetContainerLayerSize(ctx context.Context, containerID st // TODO(thaJeztah): include content-store size for the image (similar to "GET /images/json") return rwLayerUsage.Size, rwLayerUsage.Size + unpackedUsage.Size, nil } - -// getContainerImageManifest safely dereferences ImageManifest. -// ImageManifest can be nil for containers created with Docker Desktop with old -// containerd image store integration enabled which didn't set this field. -func getContainerImageManifest(ctr *container.Container) (ocispec.Descriptor, error) { - if ctr.ImageManifest == nil { - return ocispec.Descriptor{}, errdefs.InvalidParameter(errors.New("container is missing ImageManifest (probably created on old version), please recreate it")) - } - - return *ctr.ImageManifest, nil -} diff --git a/daemon/create.go b/daemon/create.go index 81c9a481ca..4bff0b51ed 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -195,7 +195,7 @@ func (daemon *Daemon) create(ctx context.Context, daemonCfg *config.Config, opts ctr.ImageManifest = imgManifest if daemon.UsesSnapshotter() { - if err := daemon.imageService.PrepareSnapshot(ctx, ctr.ID, opts.params.Config.Image, opts.params.Platform); err != nil { + if err := daemon.imageService.PrepareSnapshot(ctx, ctr.ID, opts.params.Config.Image, opts.params.Platform, setupInitLayer(daemon.idMapping)); err != nil { return nil, err } } else { diff --git a/daemon/image_service.go b/daemon/image_service.go index 7024ea5352..7fb41161d3 100644 --- a/daemon/image_service.go +++ b/daemon/image_service.go @@ -47,7 +47,7 @@ type ImageService interface { // Containerd related methods - PrepareSnapshot(ctx context.Context, id string, image string, platform *ocispec.Platform) error + PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform, setupInit func(string) error) error GetImageManifest(ctx context.Context, refOrID string, options imagetype.GetImageOpts) (*ocispec.Descriptor, error) // Layers diff --git a/daemon/images/image.go b/daemon/images/image.go index 4ac75c7592..6c53cda35d 100644 --- a/daemon/images/image.go +++ b/daemon/images/image.go @@ -46,7 +46,7 @@ type manifest struct { Config ocispec.Descriptor `json:"config"` } -func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, image string, platform *ocispec.Platform) error { +func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform, setupInit func(string) error) error { // Only makes sense when conatinerd image store is used panic("not implemented") } diff --git a/integration/container/diff_test.go b/integration/container/diff_test.go index e01a90e5c3..29068c2513 100644 --- a/integration/container/diff_test.go +++ b/integration/container/diff_test.go @@ -12,22 +12,38 @@ import ( ) func TestDiff(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows", "cannot diff a running container on Windows") + ctx := setupTest(t) + apiClient := testEnv.APIClient() + + cID := container.Run(ctx, t, apiClient, container.WithCmd("sh", "-c", `mkdir /foo; echo xyzzy > /foo/bar`)) + + expected := []containertypes.FilesystemChange{ + {Kind: containertypes.ChangeAdd, Path: "/foo"}, + {Kind: containertypes.ChangeAdd, Path: "/foo/bar"}, + } + + items, err := apiClient.ContainerDiff(ctx, cID) + assert.NilError(t, err) + assert.DeepEqual(t, expected, items) +} + +func TestDiffStoppedContainer(t *testing.T) { + // There's no way in Windows to differentiate between an Add or a Modify, + // and all files are under a "Files/" prefix. skip.If(t, testEnv.DaemonInfo.OSType == "windows", "FIXME") ctx := setupTest(t) apiClient := testEnv.APIClient() cID := container.Run(ctx, t, apiClient, container.WithCmd("sh", "-c", `mkdir /foo; echo xyzzy > /foo/bar`)) - // Wait for it to exit as cannot diff a running container on Windows, and - // it will take a few seconds to exit. Also there's no way in Windows to - // differentiate between an Add or a Modify, and all files are under - // a "Files/" prefix. + poll.WaitOn(t, container.IsInState(ctx, apiClient, cID, "exited"), poll.WithDelay(100*time.Millisecond), poll.WithTimeout(60*time.Second)) + expected := []containertypes.FilesystemChange{ {Kind: containertypes.ChangeAdd, Path: "/foo"}, {Kind: containertypes.ChangeAdd, Path: "/foo/bar"}, } if testEnv.DaemonInfo.OSType == "windows" { - poll.WaitOn(t, container.IsInState(ctx, apiClient, cID, "exited"), poll.WithDelay(100*time.Millisecond), poll.WithTimeout(60*time.Second)) expected = []containertypes.FilesystemChange{ {Kind: containertypes.ChangeModify, Path: "Files/foo"}, {Kind: containertypes.ChangeModify, Path: "Files/foo/bar"},