From 2a5e4c4be74fdd1589b98960f2c6caa1bcf4bb30 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Fri, 29 Mar 2019 16:57:47 +0200 Subject: [PATCH 1/3] Skip rootfs unmount when no mounts are provided Co-authored-by: Julia Nedialkova Signed-off-by: Georgi Sabev --- runtime/v1/linux/proc/init.go | 10 ++++++---- runtime/v1/shim/service.go | 12 ++++++++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/runtime/v1/linux/proc/init.go b/runtime/v1/linux/proc/init.go index 10787ed87..38c32fe66 100644 --- a/runtime/v1/linux/proc/init.go +++ b/runtime/v1/linux/proc/init.go @@ -304,10 +304,12 @@ func (p *Init) delete(ctx context.Context) error { } p.io.Close() } - if err2 := mount.UnmountAll(p.Rootfs, 0); err2 != nil { - log.G(ctx).WithError(err2).Warn("failed to cleanup rootfs mount") - if err == nil { - err = errors.Wrap(err2, "failed rootfs umount") + if p.Rootfs != "" { + if err2 := mount.UnmountAll(p.Rootfs, 0); err2 != nil { + log.G(ctx).WithError(err2).Warn("failed to cleanup rootfs mount") + if err == nil { + err = errors.Wrap(err2, "failed rootfs umount") + } } } return err diff --git a/runtime/v1/shim/service.go b/runtime/v1/shim/service.go index 701dab435..894a5b450 100644 --- a/runtime/v1/shim/service.go +++ b/runtime/v1/shim/service.go @@ -138,13 +138,13 @@ func (s *Service) Create(ctx context.Context, r *shimapi.CreateTaskRequest) (_ * Options: r.Options, } rootfs := filepath.Join(r.Bundle, "rootfs") - defer func() { + defer func(rootfs string) { if err != nil { if err2 := mount.UnmountAll(rootfs, 0); err2 != nil { log.G(ctx).WithError(err2).Warn("Failed to cleanup rootfs mount") } } - }() + }(rootfs) for _, rm := range mounts { m := &mount.Mount{ Type: rm.Type, @@ -159,6 +159,10 @@ func (s *Service) Create(ctx context.Context, r *shimapi.CreateTaskRequest) (_ * s.mu.Lock() defer s.mu.Unlock() + if len(mounts) == 0 { + rootfs = "" + } + process, err := newInit( ctx, s.config.Path, @@ -169,6 +173,7 @@ func (s *Service) Create(ctx context.Context, r *shimapi.CreateTaskRequest) (_ * s.config.SystemdCgroup, s.platform, config, + rootfs, ) if err != nil { return nil, errdefs.ToGRPC(err) @@ -632,7 +637,7 @@ func getTopic(ctx context.Context, e interface{}) string { return runtime.TaskUnknownTopic } -func newInit(ctx context.Context, path, workDir, runtimeRoot, namespace, criu string, systemdCgroup bool, platform rproc.Platform, r *proc.CreateConfig) (*proc.Init, error) { +func newInit(ctx context.Context, path, workDir, runtimeRoot, namespace, criu string, systemdCgroup bool, platform rproc.Platform, r *proc.CreateConfig, rootfs string) (*proc.Init, error) { var options runctypes.CreateOptions if r.Options != nil { v, err := typeurl.UnmarshalAny(r.Options) @@ -642,7 +647,6 @@ func newInit(ctx context.Context, path, workDir, runtimeRoot, namespace, criu st options = *v.(*runctypes.CreateOptions) } - rootfs := filepath.Join(path, "rootfs") runtime := proc.NewRunc(runtimeRoot, path, namespace, r.Runtime, criu, systemdCgroup) p := proc.New(r.ID, runtime, rproc.Stdio{ Stdin: r.Stdin, From c0f0b21314b93a1bd22ddfed701d572dbf4d71d7 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Thu, 4 Apr 2019 18:40:30 +0300 Subject: [PATCH 2/3] Apply PR feedback * Rootfs dir is created during container creation not during bundle creation * Add support for v2 * UnmountAll is a no-op when the path to unmount (i.e. the rootfs dir) does not exist or is invalid Co-authored-by: Danail Branekov Signed-off-by: Georgi Sabev --- mount/mount_linux.go | 3 +++ runtime/v1/linux/bundle.go | 3 --- runtime/v1/linux/proc/init.go | 10 ++++------ runtime/v1/shim/service.go | 17 ++++++++++------- runtime/v2/bundle.go | 4 ---- runtime/v2/runc/container.go | 15 ++++++++++++--- 6 files changed, 29 insertions(+), 23 deletions(-) diff --git a/mount/mount_linux.go b/mount/mount_linux.go index b5a16148a..1c61c73d5 100644 --- a/mount/mount_linux.go +++ b/mount/mount_linux.go @@ -112,6 +112,9 @@ func unmount(target string, flags int) error { // are no mounts remaining (EINVAL is returned by mount), which is // useful for undoing a stack of mounts on the same mount point. func UnmountAll(mount string, flags int) error { + if _, err := os.Stat(mount); err != nil { + return nil + } for { if err := unmount(mount, flags); err != nil { // EINVAL is returned if the target is not a diff --git a/runtime/v1/linux/bundle.go b/runtime/v1/linux/bundle.go index 7fcf36a3d..ae0e73f28 100644 --- a/runtime/v1/linux/bundle.go +++ b/runtime/v1/linux/bundle.go @@ -65,9 +65,6 @@ func newBundle(id, path, workDir string, spec []byte) (b *bundle, err error) { os.RemoveAll(workDir) } }() - if err := os.Mkdir(filepath.Join(path, "rootfs"), 0711); err != nil { - return nil, err - } err = ioutil.WriteFile(filepath.Join(path, configFilename), spec, 0666) return &bundle{ id: id, diff --git a/runtime/v1/linux/proc/init.go b/runtime/v1/linux/proc/init.go index 38c32fe66..10787ed87 100644 --- a/runtime/v1/linux/proc/init.go +++ b/runtime/v1/linux/proc/init.go @@ -304,12 +304,10 @@ func (p *Init) delete(ctx context.Context) error { } p.io.Close() } - if p.Rootfs != "" { - if err2 := mount.UnmountAll(p.Rootfs, 0); err2 != nil { - log.G(ctx).WithError(err2).Warn("failed to cleanup rootfs mount") - if err == nil { - err = errors.Wrap(err2, "failed rootfs umount") - } + if err2 := mount.UnmountAll(p.Rootfs, 0); err2 != nil { + log.G(ctx).WithError(err2).Warn("failed to cleanup rootfs mount") + if err == nil { + err = errors.Wrap(err2, "failed rootfs umount") } } return err diff --git a/runtime/v1/shim/service.go b/runtime/v1/shim/service.go index 894a5b450..4d2578ad0 100644 --- a/runtime/v1/shim/service.go +++ b/runtime/v1/shim/service.go @@ -124,6 +124,14 @@ func (s *Service) Create(ctx context.Context, r *shimapi.CreateTaskRequest) (_ * }) } + rootfs := "" + if len(mounts) > 0 { + rootfs = filepath.Join(r.Bundle, "rootfs") + if err := os.Mkdir(rootfs, 0711); err != nil { + return nil, err + } + } + config := &proc.CreateConfig{ ID: r.ID, Bundle: r.Bundle, @@ -137,14 +145,13 @@ func (s *Service) Create(ctx context.Context, r *shimapi.CreateTaskRequest) (_ * ParentCheckpoint: r.ParentCheckpoint, Options: r.Options, } - rootfs := filepath.Join(r.Bundle, "rootfs") - defer func(rootfs string) { + defer func() { if err != nil { if err2 := mount.UnmountAll(rootfs, 0); err2 != nil { log.G(ctx).WithError(err2).Warn("Failed to cleanup rootfs mount") } } - }(rootfs) + }() for _, rm := range mounts { m := &mount.Mount{ Type: rm.Type, @@ -159,10 +166,6 @@ func (s *Service) Create(ctx context.Context, r *shimapi.CreateTaskRequest) (_ * s.mu.Lock() defer s.mu.Unlock() - if len(mounts) == 0 { - rootfs = "" - } - process, err := newInit( ctx, s.config.Path, diff --git a/runtime/v2/bundle.go b/runtime/v2/bundle.go index 71394102c..7897a1c64 100644 --- a/runtime/v2/bundle.go +++ b/runtime/v2/bundle.go @@ -89,10 +89,6 @@ func NewBundle(ctx context.Context, root, state, id string, spec []byte) (b *Bun } } paths = append(paths, work) - // create rootfs dir - if err := os.Mkdir(filepath.Join(b.Path, "rootfs"), 0711); err != nil { - return nil, err - } // symlink workdir if err := os.Symlink(work, filepath.Join(b.Path, "work")); err != nil { return nil, err diff --git a/runtime/v2/runc/container.go b/runtime/v2/runc/container.go index 0606381bc..92ca16f2e 100644 --- a/runtime/v2/runc/container.go +++ b/runtime/v2/runc/container.go @@ -21,6 +21,7 @@ package runc import ( "context" "io/ioutil" + "os" "path/filepath" "sync" @@ -63,6 +64,15 @@ func NewContainer(ctx context.Context, platform rproc.Platform, r *task.CreateTa Options: m.Options, }) } + + rootfs := "" + if len(mounts) > 0 { + rootfs = filepath.Join(r.Bundle, "rootfs") + if err := os.Mkdir(rootfs, 0711); err != nil { + return nil, err + } + } + config := &proc.CreateConfig{ ID: r.ID, Bundle: r.Bundle, @@ -80,7 +90,6 @@ func NewContainer(ctx context.Context, platform rproc.Platform, r *task.CreateTa if err := WriteRuntime(r.Bundle, opts.BinaryName); err != nil { return nil, err } - rootfs := filepath.Join(r.Bundle, "rootfs") defer func() { if err != nil { if err2 := mount.UnmountAll(rootfs, 0); err2 != nil { @@ -107,6 +116,7 @@ func NewContainer(ctx context.Context, platform rproc.Platform, r *task.CreateTa platform, config, &opts, + rootfs, ) if err != nil { return nil, errdefs.ToGRPC(err) @@ -146,8 +156,7 @@ func WriteRuntime(path, runtime string) error { } func newInit(ctx context.Context, path, workDir, namespace string, platform rproc.Platform, - r *proc.CreateConfig, options *options.Options) (*proc.Init, error) { - rootfs := filepath.Join(path, "rootfs") + r *proc.CreateConfig, options *options.Options, rootfs string) (*proc.Init, error) { runtime := proc.NewRunc(options.Root, path, namespace, options.BinaryName, options.CriuPath, options.SystemdCgroup) p := proc.New(r.ID, runtime, rproc.Stdio{ Stdin: r.Stdin, From ae5ca8177d6b0ab85923787f91d2aa717e1f6fd2 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Tue, 9 Apr 2019 16:20:05 +0300 Subject: [PATCH 3/3] Refactor mount path check and add comments Co-authored-by: Danail Branekov Signed-off-by: Georgi Sabev --- mount/mount_linux.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/mount/mount_linux.go b/mount/mount_linux.go index 1c61c73d5..6bbc50bbf 100644 --- a/mount/mount_linux.go +++ b/mount/mount_linux.go @@ -111,10 +111,18 @@ func unmount(target string, flags int) error { // UnmountAll repeatedly unmounts the given mount point until there // are no mounts remaining (EINVAL is returned by mount), which is // useful for undoing a stack of mounts on the same mount point. +// UnmountAll all is noop when the first argument is an empty string. +// This is done when the containerd client did not specify any rootfs +// mounts (e.g. because the rootfs is managed outside containerd) +// UnmountAll is noop when the mount path does not exist. func UnmountAll(mount string, flags int) error { - if _, err := os.Stat(mount); err != nil { + if mount == "" { return nil } + if _, err := os.Stat(mount); os.IsNotExist(err) { + return nil + } + for { if err := unmount(mount, flags); err != nil { // EINVAL is returned if the target is not a