c8d: Implement RWLayer#49120
Merged
thaJeztah merged 7 commits intomoby:masterfrom Jan 10, 2025
Merged
Conversation
vvoland
added
status/2-code-review
kind/refactor
vvoland
force-pushed
the
c8d-rwlayer
branch
9 times, most recently
from
896f55c to
0c6dd65
Compare
Contributor
Author
|
Next thing I'd like to do is split the Useful for #49044 |
thaJeztah
reviewed
Jan 6, 2025
Member
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Looks like I had some comments that I didn't submit 🙈
thaJeztah
reviewed
Jan 8, 2025
Comment on lines
+176
to
+181
| l, ok := rwlayer.(layer.RWLayer) | ||
| if !ok { | ||
| return fmt.Errorf("unexpected RWLayer type: %T", rwlayer) | ||
| } | ||
|
|
||
| metaData, err := i.layerStore.ReleaseRWLayer(l) |
Member
There was a problem hiding this comment.
I don't think this is correct; after some of the follow-up commits, layer.RWLayer is never satisfied by container.RWLayer, so it will always be rejected? Removing the code above will fail to compile be cause it doesn't the interface that i.layerStore.ReleaseRWLayer(l) expects
Contributor
Author
There was a problem hiding this comment.
No, it should never be rejected, because CreateLayer always creates layer.RWLayer (return i.layerStore.CreateRWLayer(container.ID, layerID, rwLayerOpts)).
And since each image service should only be able to release layer it created, it's safe to assume that this layer will be of this type and allow the image service to have access to a wider interface.
The container.RWLayer is a subset of that and is minimized only to the methods used outside of the image service.
We could avoid that if we removed the RWLayer from the container and put it entirely behind image service (see #46771). However, that approach heads us more coupling between container and image service, which I think is the opposite direction we should be headed.
Comment on lines
+20
to
+24
| rwLayer, ok := container.RWLayer.(layer.RWLayer) | ||
| if !ok { | ||
| return nil, fmt.Errorf("container %s has an unexpected RWLayer type: %T", container.Name, container.RWLayer) | ||
| } | ||
| return rwLayer.Changes() |
Member
There was a problem hiding this comment.
Ideally, we'd match this at compile-time, but I guess that's complicated because importing layer into the container package was exactly the part you were trying to avoid 🤔
Contributor
Author
There was a problem hiding this comment.
Hmm, I think we could remove the Changes from the image service completely and instead keep the Changes method in the container.RWLayer interface.
Member
There was a problem hiding this comment.
I think, taking a few steps back, our goal effectively is;
- We have some methods accepting a
layer.RWLayerinterface - But that's a rather large interface, including bits that may not be needed by those methods accepting them
- So we want to reduce the interface they require, so that we can split things up, and in some cases, reduce what has to be implemented
Introduce a separate `RWLayer` interface for the `container.RWLayer` to remove coupling with the graphdriver implementation. Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
Implement containerd image store backed `RWLayer` and remove the containerd-specific `PrepareSnapshot` method from the ImageService interface. Signed-off-by: Paweł Gronowski <[email protected]>
After implementing `RWLayer` for containerd image store, implementation of these methods is identical for both stores. Move the logic out of the image service into the daemon. Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
dmcgowan
approved these changes
Jan 10, 2025
Member
|
I had a quick try, but this one is gonna conflict massively with @LaurentGoderre's PR; First commit of that PR is mostly doable, but second commit is a bit more hairy (in Should we try get that PR in first, then handle the conflicts in this PR, or is one of you able to assist @LaurentGoderre to get his PR in a mergeable shape again if we merge this one first? Conflicts in frist commit; Detailsdiff --cc daemon/containerd/service.go
index 584b3ef5a6,68ad818f71..0000000000
--- a/daemon/containerd/service.go
+++ b/daemon/containerd/service.go
@@@ -20,6 -20,8 +20,11 @@@ import
dimages "github.com/docker/docker/daemon/images"
"github.com/docker/docker/daemon/snapshotter"
"github.com/docker/docker/errdefs"
++<<<<<<< HEAD
++=======
+ "github.com/docker/docker/image"
+ "github.com/docker/docker/layer"
++>>>>>>> 67d0068dda (Implement mount from image)
"github.com/docker/docker/pkg/idtools"
"github.com/docker/docker/registry"
"github.com/pkg/errors"
@@@ -107,6 -109,17 +112,20 @@@ func (i *ImageService) CountImages(ctx
return len(imgs)
}
++<<<<<<< HEAD
++=======
+ // CreateLayer creates a filesystem layer for a container.
+ // called from create.go
+ // TODO: accept an opt struct instead of container?
+ func (i *ImageService) CreateLayer(container *container.Container, initFunc layer.MountInit) (layer.RWLayer, error) {
+ return nil, errdefs.NotImplemented(errdefs.NotImplemented(errors.New("not implemented")))
+ }
+
+ func (i *ImageService) CreateLayerFromImage(img *image.Image, layerName string, rwLayerOpts *layer.CreateRWLayerOpts) (layer.RWLayer, error) {
+ return nil, errdefs.NotImplemented(errdefs.NotImplemented(errors.New("not implemented")))
+ }
+
++>>>>>>> 67d0068dda (Implement mount from image)
// LayerStoreStatus returns the status for each layer store
// called from info.go
func (i *ImageService) LayerStoreStatus() [][2]string {
diff --cc daemon/image_service.go
index 1417cf7353,859f8ea933..0000000000
--- a/daemon/image_service.go
+++ b/daemon/image_service.go
@@@ -48,13 -48,19 +48,18 @@@ type ImageService interface
// Layers
GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ROLayer, error)
++<<<<<<< HEAD
+ CreateLayer(container *container.Container, initFunc layer.MountInit) (container.RWLayer, error)
+ GetLayerByID(cid string) (container.RWLayer, error)
++=======
+ CreateLayer(container *container.Container, initFunc layer.MountInit) (layer.RWLayer, error)
+ CreateLayerFromImage(img *image.Image, layerName string, rwLayerOpts *layer.CreateRWLayerOpts) (layer.RWLayer, error)
++>>>>>>> 67d0068dda (Implement mount from image)
LayerStoreStatus() [][2]string
GetLayerMountID(cid string) (string, error)
- ReleaseLayer(rwlayer layer.RWLayer) error
+ ReleaseLayer(rwlayer container.RWLayer) error
LayerDiskUsage(ctx context.Context) (int64, error)
GetContainerLayerSize(ctx context.Context, containerID string) (int64, int64, error)
- Mount(ctx context.Context, container *container.Container) error
- Unmount(ctx context.Context, container *container.Container) error
Changes(ctx context.Context, container *container.Container) ([]archive.Change, error)
// Windows specific
diff --cc daemon/images/service.go
index 4af7ab4a91,77ac2dfe5a..0000000000
--- a/daemon/images/service.go
+++ b/daemon/images/service.go
@@@ -118,10 -117,10 +118,15 @@@ func (i *ImageService) Children(_ conte
// CreateLayer creates a filesystem layer for a container.
// called from create.go
// TODO: accept an opt struct instead of container?
++<<<<<<< HEAD
+func (i *ImageService) CreateLayer(container *container.Container, initFunc layer.MountInit) (container.RWLayer, error) {
+ var layerID layer.ChainID
++=======
+ func (i *ImageService) CreateLayer(container *container.Container, initFunc layer.MountInit) (layer.RWLayer, error) {
+ var img *image.Image
++>>>>>>> 67d0068dda (Implement mount from image)
if container.ImageID != "" {
- img, err := i.imageStore.Get(container.ImageID)
+ containerImg, err := i.imageStore.Get(container.ImageID)
if err != nil {
return nil, err
}And second commit; Detailsdiff --cc daemon/containerd/image_snapshot.go
index 8c179be51b,01ce8a1732..0000000000
--- a/daemon/containerd/image_snapshot.go
+++ b/daemon/containerd/image_snapshot.go
@@@ -9,43 -10,56 +9,90 @@@ import
"github.com/containerd/containerd/mount"
"github.com/containerd/containerd/snapshots"
cerrdefs "github.com/containerd/errdefs"
++<<<<<<< HEAD
+ "github.com/containerd/log"
+ "github.com/docker/docker/container"
+ "github.com/docker/docker/daemon/snapshotter"
++=======
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
"github.com/docker/docker/errdefs"
+ "github.com/docker/docker/layer"
"github.com/opencontainers/image-spec/identity"
- ocispec "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
)
++<<<<<<< HEAD
+// CreateLayer creates a new layer for a container.
+// TODO(vvoland): Decouple from container
+func (i *ImageService) CreateLayer(ctr *container.Container, initFunc layer.MountInit) (container.RWLayer, error) {
+ ctx := context.TODO()
+
+ var parentSnapshot string
+ if ctr.ImageManifest != nil {
+ img := c8dimages.Image{
+ Target: *ctr.ImageManifest,
+ }
+ platformImg, err := i.NewImageManifest(ctx, img, img.Target)
+ if err != nil {
+ return nil, err
+ }
+ unpacked, err := platformImg.IsUnpacked(ctx, i.snapshotter)
++=======
+ func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform, setupInit func(string) error) error {
+ var platformImg containerd.Image
+ if parentImage != "" {
+ img, err := i.resolveImage(ctx, parentImage)
+ if err != nil {
+ return err
+ }
+
+ platformImg = i.NewImageWithPlatform(img, platform)
+ }
+
+ _, err := i.PrepareSnapshotFromImage(ctx, id, platformImg, setupInit)
+ if err != nil {
+ return err
+ }
+
+ return nil
+ }
+
+ // PrepareSnapshot prepares a snapshot from a parent image for a container
+ func (i *ImageService) PrepareSnapshotFromImage(ctx context.Context, id string, image containerd.Image, setupInit func(string) error) ([]mount.Mount, error) {
+ var parentSnapshot string
+ if image != nil {
+ cs := i.content
+
+ unpacked, err := image.IsUnpacked(ctx, i.snapshotter)
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
if err != nil {
return nil, err
}
if !unpacked {
++<<<<<<< HEAD
+ if err := platformImg.Unpack(ctx, i.snapshotter); err != nil {
++=======
+ if err := image.Unpack(ctx, i.snapshotter); err != nil {
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
return nil, err
}
}
++<<<<<<< HEAD
+ diffIDs, err := platformImg.RootFS(ctx)
+ if err != nil {
+ return nil, err
++=======
+ desc, err := image.Config(ctx)
+ if err != nil {
+ return nil, err
+ }
+
+ diffIDs, err := c8dimages.RootFS(ctx, cs, desc)
+ if err != nil {
+ return nil, err
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
}
parentSnapshot = identity.ChainID(diffIDs).String()
@@@ -65,147 -77,17 +112,157 @@@
}
ctx = leases.WithLease(ctx, lease.ID)
++<<<<<<< HEAD
+ if err := i.prepareInitLayer(ctx, id, parentSnapshot, initFunc); err != nil {
++=======
+ snapshotter := i.client.SnapshotService(i.StorageDriver())
+
+ if err := i.prepareInitLayer(ctx, id, parentSnapshot, setupInit); err != nil {
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
return nil, err
}
+ sn := i.client.SnapshotService(i.StorageDriver())
if !i.idMapping.Empty() {
- return i.remapSnapshot(ctx, snapshotter, id, id+"-init")
+ err = i.remapSnapshot(ctx, sn, id, id+"-init")
+ } else {
+ _, err = sn.Prepare(ctx, id, id+"-init")
}
++<<<<<<< HEAD
+ if err != nil {
+ return nil, err
+ }
+
+ return &rwLayer{
+ id: id,
+ snapshotterName: i.StorageDriver(),
+ snapshotter: sn,
+ refCountMounter: i.refCountMounter,
+ lease: lease,
+ }, nil
+}
+
+type rwLayer struct {
+ id string
+ snapshotter snapshots.Snapshotter
+ snapshotterName string
+ refCountMounter snapshotter.Mounter
+ root string
+ lease leases.Lease
+}
+
+func (l *rwLayer) mounts(ctx context.Context) ([]mount.Mount, error) {
+ return l.snapshotter.Mounts(ctx, l.id)
+}
+
+func (l *rwLayer) Mount(mountLabel string) (string, error) {
+ ctx := context.TODO()
+
+ // TODO: Investigate how we can handle mountLabel
+ _ = mountLabel
+ mounts, err := l.mounts(ctx)
+ if err != nil {
+ return "", err
+ }
+
+ var root string
+ if root, err = l.refCountMounter.Mount(mounts, l.id); err != nil {
+ return "", fmt.Errorf("failed to mount %s: %w", root, err)
+ }
+ l.root = root
+
+ log.G(ctx).WithFields(log.Fields{"container": l.id, "root": root, "snapshotter": l.snapshotterName}).Debug("container mounted via snapshotter")
+ return root, nil
+}
+
+// GetLayerByID returns a layer by ID
+// called from daemon.go Daemon.restore().
+func (i *ImageService) GetLayerByID(cid string) (container.RWLayer, error) {
+ ctx := context.TODO()
+
+ sn := i.client.SnapshotService(i.StorageDriver())
+ if _, err := sn.Stat(ctx, cid); err != nil {
+ if !cerrdefs.IsNotFound(err) {
+ return nil, fmt.Errorf("failed to stat snapshot %s: %w", cid, err)
+ }
+ return nil, errdefs.NotFound(fmt.Errorf("RW layer for container %s not found", cid))
+ }
+
+ ls := i.client.LeasesService()
+ lss, err := ls.List(ctx, "id=="+cid)
+ if err != nil {
+ return nil, err
+ }
+
+ switch len(lss) {
+ case 0:
+ return nil, errdefs.NotFound(errors.New("rw layer lease not found for container " + cid))
+ default:
+ log.G(ctx).WithFields(log.Fields{"container": cid, "leases": lss}).Warn("multiple leases with the same id found, this should not happen")
+ case 1:
+ }
+
+ root, err := i.refCountMounter.Mounted(cid)
+ if err != nil {
+ log.G(ctx).WithField("container", cid).Warn("failed to determine if container is already mounted")
+ }
+
+ return &rwLayer{
+ id: cid,
+ snapshotterName: i.StorageDriver(),
+ snapshotter: sn,
+ refCountMounter: i.refCountMounter,
+ lease: lss[0],
+ root: root,
+ }, nil
+
+}
+
+func (l *rwLayer) Unmount() error {
+ ctx := context.TODO()
+
+ if l.root == "" {
+ target, err := l.refCountMounter.Mounted(l.id)
+ if err != nil {
+ log.G(ctx).WithField("id", l.id).Warn("failed to determine if container is already mounted")
+ }
+ if target == "" {
+ return errors.New("layer not mounted")
+ }
+ l.root = target
+ }
+
+ if err := l.refCountMounter.Unmount(l.root); err != nil {
+ log.G(ctx).WithField("container", l.id).WithError(err).Error("error unmounting container")
+ return fmt.Errorf("failed to unmount %s: %w", l.root, err)
+ }
+
+ return nil
+}
+
+func (l rwLayer) Metadata() (map[string]string, error) {
+ return nil, nil
+}
+
+// ReleaseLayer releases a layer allowing it to be removed
+// called from delete.go Daemon.cleanupContainer(), and Daemon.containerExport()
+func (i *ImageService) ReleaseLayer(rwlayer container.RWLayer) error {
+ c8dLayer, ok := rwlayer.(*rwLayer)
+ if !ok {
+ return fmt.Errorf("invalid layer type %T", rwlayer)
+ }
+
+ ls := i.client.LeasesService()
+ if err := ls.Delete(context.Background(), c8dLayer.lease, leases.SynchronousDelete); err != nil {
+ if !cerrdefs.IsNotFound(err) {
+ return err
+ }
+ }
+ return nil
++=======
+ return snapshotter.Prepare(ctx, id, id+"-init")
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
}
func (i *ImageService) prepareInitLayer(ctx context.Context, id string, parent string, setupInit func(string) error) error {
diff --cc daemon/containerd/service.go
index 3165716ebe,7b33bd24d5..0000000000
--- a/daemon/containerd/service.go
+++ b/daemon/containerd/service.go
@@@ -21,8 -21,10 +21,9 @@@ import
"github.com/docker/docker/daemon/snapshotter"
"github.com/docker/docker/errdefs"
"github.com/docker/docker/image"
- "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"
)
@@@ -108,6 -110,19 +109,22 @@@ func (i *ImageService) CountImages(ctx
return len(imgs)
}
++<<<<<<< HEAD
++=======
+ // NewImageWithPlatform gets the platform specific image.
+ func (i *ImageService) NewImageWithPlatform(img c8dimages.Image, platform *ocispec.Platform) containerd.Image {
+ matcher := i.matchRequestedOrDefault(platforms.Only, platform)
+ return containerd.NewImageWithPlatform(i.client, img, matcher)
+ }
+
+ // CreateLayer creates a filesystem layer for a container.
+ // called from create.go
+ // TODO: accept an opt struct instead of container?
+ func (i *ImageService) CreateLayer(container *container.Container, initFunc layer.MountInit) (layer.RWLayer, error) {
+ return nil, errdefs.NotImplemented(errdefs.NotImplemented(errors.New("not implemented")))
+ }
+
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
func (i *ImageService) CreateLayerFromImage(img *image.Image, layerName string, rwLayerOpts *layer.CreateRWLayerOpts) (layer.RWLayer, error) {
return nil, errdefs.NotImplemented(errdefs.NotImplemented(errors.New("not implemented")))
}
@@@ -140,6 -155,16 +157,19 @@@ func (i *ImageService) StorageDriver()
return i.snapshotter
}
++<<<<<<< HEAD
++=======
+ func (i *ImageService) Mounter() snapshotter.Mounter {
+ return i.refCountMounter
+ }
+
+ // ReleaseLayer releases a layer allowing it to be removed
+ // called from delete.go Daemon.cleanupContainer(), and Daemon.containerExport()
+ func (i *ImageService) ReleaseLayer(rwlayer layer.RWLayer) error {
+ return errdefs.NotImplemented(errors.New("not implemented"))
+ }
+
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
// LayerDiskUsage returns the number of bytes used by layer stores
// called from disk_usage.go
func (i *ImageService) LayerDiskUsage(ctx context.Context) (int64, error) {
diff --cc daemon/image_service.go
index 4f5f8ee0b7,491021358b..0000000000
--- a/daemon/image_service.go
+++ b/daemon/image_service.go
@@@ -45,6 -47,11 +47,14 @@@ type ImageService interface
SquashImage(id, parent string) (string, error)
ImageInspect(ctx context.Context, refOrID string, opts backend.ImageInspectOpts) (*imagetype.InspectResponse, error)
++<<<<<<< HEAD
++=======
+ // Containerd related methods
+
+ PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform, setupInit func(string) error) error
+ PrepareSnapshotFromImage(ctx context.Context, id string, image containerd.Image, setupInit func(string) error) ([]mount.Mount, error)
+
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
// Layers
GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ROLayer, error)
diff --cc daemon/images/image.go
index cbd69d8517,65b1ab48b4..0000000000
--- a/daemon/images/image.go
+++ b/daemon/images/image.go
@@@ -45,6 -47,16 +47,19 @@@ type manifest struct
Config ocispec.Descriptor `json:"config"`
}
++<<<<<<< HEAD
++=======
+ func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform, setupInit func(string) error) error {
+ // Only makes sense when containerd image store is used
+ panic("not implemented")
+ }
+
+ func (i *ImageService) PrepareSnapshotFromImage(ctx context.Context, id string, image containerd.Image, setupInit func(string) error) ([]mount.Mount, error) {
+ // Only makes sense when containerd image store is used
+ panic("not implemented")
+ }
+
++>>>>>>> b5e927f070 (Implement image mount for the snapshotter)
func (i *ImageService) manifestMatchesPlatform(ctx context.Context, img *image.Image, platform ocispec.Platform) (bool, error) {
ls, err := i.leases.ListResources(ctx, leases.Lease{ID: imageKey(img.ID().String())})
if err != nil { |
Member
|
I would say get this one in, there could be a new interface which handles both cases. It would be better to cleanup the interface that PR is using to avoid exposing more internals and forking the logic further in |
Member
|
Yeah, makes sense; would you be able to help @LaurentGoderre with that? |
Member
|
Of course |
Member
|
Thanks! Mergeconflicts coming up 🙈 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- What I did
Refactor image service to use a new, smaller
container.RWLayerinterface oflayer.RWLayerto allow containerd-image store implementation.This also gets rid of some of the hacky
if daemon.UsesSnapshotter()branches in the daemon code.- How I did it
- How to verify it
CI
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)