From 677257f032d9d67d13b4fd0007bd2d45aa0d5b74 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 18 Aug 2017 14:40:41 -0700 Subject: [PATCH] service/snapshotter: move default to client In order to enforce strict handling of snapshotter values on the container object, the defaults have been moved to the client side. This ensures that we correctly qualify the snapshotter under use when from the container at the time it was created, rather than possibly losing the metadata on a change of default. Signed-off-by: Stephen J Day --- client.go | 1 + container.go | 4 ++++ container_opts.go | 16 +++++++++++++++ container_opts_unix.go | 1 + services/snapshot/default_linux.go | 5 ----- services/snapshot/default_unix.go | 7 ------- services/snapshot/default_windows.go | 5 ----- services/snapshot/service.go | 29 +++++++++------------------- snapshot_test.go | 2 +- snapshotter_default_linux.go | 8 ++++++++ snapshotter_default_unix.go | 10 ++++++++++ snapshotter_default_windows.go | 8 ++++++++ spec_opts_unix.go | 3 +++ 13 files changed, 61 insertions(+), 38 deletions(-) delete mode 100644 services/snapshot/default_linux.go delete mode 100644 services/snapshot/default_unix.go delete mode 100644 services/snapshot/default_windows.go create mode 100644 snapshotter_default_linux.go create mode 100644 snapshotter_default_unix.go create mode 100644 snapshotter_default_windows.go diff --git a/client.go b/client.go index d0a57199c..65c28b8b3 100644 --- a/client.go +++ b/client.go @@ -192,6 +192,7 @@ func defaultRemoteContext() *RemoteContext { Resolver: docker.NewResolver(docker.ResolverOptions{ Client: http.DefaultClient, }), + Snapshotter: DefaultSnapshotter, } } diff --git a/container.go b/container.go index 9eb66adef..9d74a9932 100644 --- a/container.go +++ b/container.go @@ -174,6 +174,10 @@ func (c *container) NewTask(ctx context.Context, ioCreate IOCreation, opts ...Ne Stderr: cfg.Stderr, } if c.c.RootFS != "" { + if c.c.Snapshotter == "" { + return nil, errors.Wrapf(errdefs.ErrInvalidArgument, "unable to resolve rootfs mounts without snapshotter on container") + } + // get the rootfs from the snapshotter and add it to the request mounts, err := c.client.SnapshotService(c.c.Snapshotter).Mounts(ctx, c.c.RootFS) if err != nil { diff --git a/container_opts.go b/container_opts.go index 8140c7889..8e90be36c 100644 --- a/container_opts.go +++ b/container_opts.go @@ -4,7 +4,9 @@ import ( "context" "github.com/containerd/containerd/containers" + "github.com/containerd/containerd/errdefs" "github.com/opencontainers/image-spec/identity" + "github.com/pkg/errors" ) // NewContainerOpts allows the caller to set additional options when creating a container @@ -38,6 +40,8 @@ func WithContainerLabels(labels map[string]string) NewContainerOpts { } // WithSnapshotter sets the provided snapshotter for use by the container +// +// This option must appear before other snapshotter options to have an effect. func WithSnapshotter(name string) NewContainerOpts { return func(ctx context.Context, client *Client, c *containers.Container) error { c.Snapshotter = name @@ -48,6 +52,7 @@ func WithSnapshotter(name string) NewContainerOpts { // WithSnapshot uses an existing root filesystem for the container func WithSnapshot(id string) NewContainerOpts { return func(ctx context.Context, client *Client, c *containers.Container) error { + setSnapshotterIfEmpty(c) // check that the snapshot exists, if not, fail on creation if _, err := client.SnapshotService(c.Snapshotter).Mounts(ctx, id); err != nil { return err @@ -65,6 +70,7 @@ func WithNewSnapshot(id string, i Image) NewContainerOpts { if err != nil { return err } + setSnapshotterIfEmpty(c) if _, err := client.SnapshotService(c.Snapshotter).Prepare(ctx, id, identity.ChainID(diffIDs).String()); err != nil { return err } @@ -77,6 +83,9 @@ func WithNewSnapshot(id string, i Image) NewContainerOpts { // WithSnapshotCleanup deletes the rootfs allocated for the container func WithSnapshotCleanup(ctx context.Context, client *Client, c containers.Container) error { if c.RootFS != "" { + if c.Snapshotter == "" { + return errors.Wrapf(errdefs.ErrInvalidArgument, "container.Snapshotter must be set to cleanup rootfs") + } return client.SnapshotService(c.Snapshotter).Remove(ctx, c.RootFS) } return nil @@ -90,6 +99,7 @@ func WithNewSnapshotView(id string, i Image) NewContainerOpts { if err != nil { return err } + setSnapshotterIfEmpty(c) if _, err := client.SnapshotService(c.Snapshotter).View(ctx, id, identity.ChainID(diffIDs).String()); err != nil { return err } @@ -98,3 +108,9 @@ func WithNewSnapshotView(id string, i Image) NewContainerOpts { return nil } } + +func setSnapshotterIfEmpty(c *containers.Container) { + if c.Snapshotter == "" { + c.Snapshotter = DefaultSnapshotter + } +} diff --git a/container_opts_unix.go b/container_opts_unix.go index be69500bd..73e5486bd 100644 --- a/container_opts_unix.go +++ b/container_opts_unix.go @@ -46,6 +46,7 @@ func WithCheckpoint(desc v1.Descriptor, rootfsID string) NewContainerOpts { if err != nil { return err } + setSnapshotterIfEmpty(c) if _, err := client.SnapshotService(c.Snapshotter).Prepare(ctx, rootfsID, identity.ChainID(diffIDs).String()); err != nil { if !errdefs.IsAlreadyExists(err) { return err diff --git a/services/snapshot/default_linux.go b/services/snapshot/default_linux.go deleted file mode 100644 index 13f75af41..000000000 --- a/services/snapshot/default_linux.go +++ /dev/null @@ -1,5 +0,0 @@ -package snapshot - -const ( - defaultSnapshotter = "overlayfs" -) diff --git a/services/snapshot/default_unix.go b/services/snapshot/default_unix.go deleted file mode 100644 index 3489cf8cf..000000000 --- a/services/snapshot/default_unix.go +++ /dev/null @@ -1,7 +0,0 @@ -// +build darwin freebsd solaris - -package snapshot - -const ( - defaultSnapshotter = "naive" -) diff --git a/services/snapshot/default_windows.go b/services/snapshot/default_windows.go deleted file mode 100644 index 680631e32..000000000 --- a/services/snapshot/default_windows.go +++ /dev/null @@ -1,5 +0,0 @@ -package snapshot - -const ( - defaultSnapshotter = "windows" -) diff --git a/services/snapshot/service.go b/services/snapshot/service.go index 6c6ef8818..293a643e3 100644 --- a/services/snapshot/service.go +++ b/services/snapshot/service.go @@ -20,11 +20,6 @@ import ( "google.golang.org/grpc" ) -type config struct { - // Default is the default snapshotter to use for the service - Default string `toml:"default,omitempty"` -} - func init() { plugin.Register(&plugin.Registration{ Type: plugin.GRPCPlugin, @@ -33,9 +28,6 @@ func init() { plugin.SnapshotPlugin, plugin.MetadataPlugin, }, - Config: &config{ - Default: defaultSnapshotter, - }, Init: newService, }) } @@ -43,9 +35,8 @@ func init() { var empty = &protoempty.Empty{} type service struct { - snapshotters map[string]snapshot.Snapshotter - defaultSnapshotterName string - publisher events.Publisher + snapshotters map[string]snapshot.Snapshotter + publisher events.Publisher } func newService(ic *plugin.InitContext) (interface{}, error) { @@ -62,26 +53,24 @@ func newService(ic *plugin.InitContext) (interface{}, error) { snapshotters[name] = metadata.NewSnapshotter(md.(*bolt.DB), name, sn.(snapshot.Snapshotter)) } - cfg := ic.Config.(*config) - _, ok := snapshotters[cfg.Default] - if !ok { - return nil, errors.Errorf("default snapshotter not loaded: %s", cfg.Default) + if len(snapshotters) == 0 { + return nil, errors.Errorf("failed to create snapshotter service: no snapshotters loaded") } return &service{ - snapshotters: snapshotters, - defaultSnapshotterName: cfg.Default, - publisher: ic.Events, + snapshotters: snapshotters, + publisher: ic.Events, }, nil } func (s *service) getSnapshotter(name string) (snapshot.Snapshotter, error) { if name == "" { - name = s.defaultSnapshotterName + return nil, errdefs.ToGRPCf(errdefs.ErrInvalidArgument, "snapshotter argument missing") } + sn, ok := s.snapshotters[name] if !ok { - return nil, errors.Errorf("snapshotter not loaded: %s", name) + return nil, errdefs.ToGRPCf(errdefs.ErrInvalidArgument, "snapshotter not loaded: %s", name) } return sn, nil } diff --git a/snapshot_test.go b/snapshot_test.go index d52bc79cc..ccf28c70c 100644 --- a/snapshot_test.go +++ b/snapshot_test.go @@ -15,7 +15,7 @@ func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, fun return nil, nil, err } - sn := client.SnapshotService("") + sn := client.SnapshotService(DefaultSnapshotter) return sn, func() { client.Close() diff --git a/snapshotter_default_linux.go b/snapshotter_default_linux.go new file mode 100644 index 000000000..c432a2d55 --- /dev/null +++ b/snapshotter_default_linux.go @@ -0,0 +1,8 @@ +package containerd + +const ( + // DefaultSnapshotter will set the default snapshotter for the platform. + // This will be based on the client compilation target, so take that into + // account when choosing this value. + DefaultSnapshotter = "overlayfs" +) diff --git a/snapshotter_default_unix.go b/snapshotter_default_unix.go new file mode 100644 index 000000000..cb8b08ac1 --- /dev/null +++ b/snapshotter_default_unix.go @@ -0,0 +1,10 @@ +// +build darwin freebsd solaris + +package containerd + +const ( + // DefaultSnapshotter will set the default snapshotter for the platform. + // This will be based on the client compilation target, so take that into + // account when choosing this value. + DefaultSnapshotter = "naive" +) diff --git a/snapshotter_default_windows.go b/snapshotter_default_windows.go new file mode 100644 index 000000000..3b7358218 --- /dev/null +++ b/snapshotter_default_windows.go @@ -0,0 +1,8 @@ +package containerd + +const ( + // DefaultSnapshotter will set the default snapshotter for the platform. + // This will be based on the client compilation target, so take that into + // account when choosing this value. + DefaultSnapshotter = "windows" +) diff --git a/spec_opts_unix.go b/spec_opts_unix.go index bf90577ff..3e35c18c8 100644 --- a/spec_opts_unix.go +++ b/spec_opts_unix.go @@ -233,6 +233,9 @@ func WithRemappedSnapshot(id string, i Image, uid, gid uint32) NewContainerOpts if err != nil { return err } + + setSnapshotterIfEmpty(c) + var ( snapshotter = client.SnapshotService(c.Snapshotter) parent = identity.ChainID(diffIDs).String()