Skip to content

Commit

Permalink
Cleanup args, rename jobDetails
Browse files Browse the repository at this point in the history 
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
  • Loading branch information
@gabriel-samfira
gabriel-samfira committed Aug 30, 2023
1 parent b6b322b commit cc5657c
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 61 deletions.
39 changes: 18 additions & 21 deletions executor/containerdexecutor/executor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import (
"io"
"os"
"path/filepath"
"runtime"
"strings"
"sync"
"syscall"
"time"
Expand Down Expand Up @@ -34,7 +32,7 @@ type containerdExecutor struct {
networkProviders map[pb.NetMode]network.Provider
cgroupParent string
dnsConfig *oci.DNSConfig
running map[string]*jobDetails
running map[string]*containerState
mu sync.Mutex
apparmorProfile string
selinux bool
Expand Down Expand Up @@ -67,15 +65,15 @@ func New(client *containerd.Client, root, cgroup string, networkProviders map[pb
networkProviders: networkProviders,
cgroupParent: cgroup,
dnsConfig: dnsConfig,
running: make(map[string]*jobDetails),
running: make(map[string]*containerState),
apparmorProfile: apparmorProfile,
selinux: selinux,
traceSocket: traceSocket,
rootless: rootless,
}
}

type jobDetails struct {
type containerState struct {
done chan error
// On linux the rootfsPath is used to ensure the CWD exists, to fetch user information
// and as a bind mount for the root FS of the container.
Expand All @@ -92,7 +90,7 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M

startedOnce := sync.Once{}
done := make(chan error, 1)
details := &jobDetails{
details := &containerState{
done: done,
}
w.mu.Lock()
Expand All @@ -112,12 +110,14 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M
}()

meta := process.Meta
releasers, resolvConf, hostsFile, err := w.prepareExecutionEnv(ctx, root, mounts, meta, details)
resolvConf, hostsFile, releasers, err := w.prepareExecutionEnv(ctx, root, mounts, meta, details)
if err != nil {
releasers()
return nil, err
}
defer releasers()

if releasers != nil {
defer releasers()
}

if err := w.ensureCWD(ctx, details, meta); err != nil {
return nil, err
Expand All @@ -137,12 +137,13 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M
bklog.G(ctx).Info("enabling HostNetworking")
}

spec, specReleasers, err := w.getOCISpec(ctx, id, resolvConf, hostsFile, namespace, mounts, meta, details)
spec, releaseSpec, err := w.createOCISpec(ctx, id, resolvConf, hostsFile, namespace, mounts, meta, details)
if err != nil {
specReleasers()
return nil, err
}
defer specReleasers()
if releaseSpec != nil {
defer releaseSpec()
}

container, err := w.client.NewContainer(ctx, id,
containerd.WithSpec(spec),
Expand All @@ -163,7 +164,7 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M
cioOpts = append(cioOpts, cio.WithTerminal)
}

taskOpts, err := w.getTaskOpts(ctx, details)
taskOpts, err := details.getTaskOpts()
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -254,14 +255,10 @@ func (w *containerdExecutor) Exec(ctx context.Context, id string, process execut
}

proc.Terminal = meta.Tty

if runtime.GOOS == "windows" {
// On Windows passing in Args will lead to double escaping by hcsshim, which leads to errors.
// The recommendation is to use CommandLine.
proc.CommandLine = strings.Join(meta.Args, " ")
} else {
proc.Args = meta.Args
}
// setArgs will set the proper command line arguments for this process.
// On Windows, this will set the CommandLine field. On Linux it will set the
// Args field.
setArgs(proc, meta.Args)

if meta.Cwd != "" {
spec.Process.Cwd = meta.Cwd
Expand Down
68 changes: 40 additions & 28 deletions executor/containerdexecutor/executor_unix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func getUserSpec(user, rootfsPath string) (specs.User, error) {
}, nil
}

func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount executor.Mount, mounts []executor.Mount, meta executor.Meta, details *jobDetails) (func(), string, string, error) {
func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount executor.Mount, mounts []executor.Mount, meta executor.Meta, details *containerState) (string, string, func(), error) {
var releasers []func()
releaseAll := func() {
for i := len(releasers) - 1; i >= 0; i-- {
Expand All @@ -52,24 +52,28 @@ func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount

resolvConf, err := oci.GetResolvConf(ctx, w.root, nil, w.dnsConfig)
if err != nil {
return releaseAll, "", "", err
releaseAll()
return "", "", nil, err
}

hostsFile, clean, err := oci.GetHostsFile(ctx, w.root, meta.ExtraHosts, nil, meta.Hostname)
if err != nil {
return releaseAll, "", "", err
releaseAll()
return "", "", nil, err
}
if clean != nil {
releasers = append(releasers, clean)
}
mountable, err := rootMount.Src.Mount(ctx, false)
if err != nil {
return releaseAll, "", "", err
releaseAll()
return "", "", nil, err
}

rootMounts, release, err := mountable.Mount()
if err != nil {
return releaseAll, "", "", err
releaseAll()
return "", "", nil, err
}
details.rootMounts = rootMounts

Expand All @@ -83,7 +87,8 @@ func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount
lm := snapshot.LocalMounterWithMounts(rootMounts)
rootfsPath, err := lm.Mount()
if err != nil {
return releaseAll, "", "", err
releaseAll()
return "", "", nil, err
}
details.rootfsPath = rootfsPath
releasers = append(releasers, func() {
Expand All @@ -93,26 +98,10 @@ func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount
})
releasers = append(releasers, executor.MountStubsCleaner(ctx, details.rootfsPath, mounts, meta.RemoveMountStubsRecursive))

return releaseAll, resolvConf, hostsFile, nil
return resolvConf, hostsFile, releaseAll, nil
}

func (w *containerdExecutor) getTaskOpts(ctx context.Context, details *jobDetails) (containerd.NewTaskOpts, error) {
rootfs := containerd.WithRootFS([]mount.Mount{{
Source: details.rootfsPath,
Type: "bind",
Options: []string{"rbind"},
}})
if runtime.GOOS == "freebsd" {
rootfs = containerd.WithRootFS([]mount.Mount{{
Source: details.rootfsPath,
Type: "nullfs",
Options: []string{},
}})
}
return rootfs, nil
}

func (w *containerdExecutor) ensureCWD(ctx context.Context, details *jobDetails, meta executor.Meta) error {
func (w *containerdExecutor) ensureCWD(ctx context.Context, details *containerState, meta executor.Meta) error {
newp, err := fs.RootPath(details.rootfsPath, meta.Cwd)
if err != nil {
return errors.Wrapf(err, "working dir %s points to invalid target", newp)
Expand All @@ -136,7 +125,7 @@ func (w *containerdExecutor) ensureCWD(ctx context.Context, details *jobDetails,
return nil
}

func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hostsFile string, namespace network.Namespace, mounts []executor.Mount, meta executor.Meta, details *jobDetails) (*specs.Spec, func(), error) {
func (w *containerdExecutor) createOCISpec(ctx context.Context, id, resolvConf, hostsFile string, namespace network.Namespace, mounts []executor.Mount, meta executor.Meta, details *containerState) (*specs.Spec, func(), error) {
var releasers []func()
releaseAll := func() {
for i := len(releasers) - 1; i >= 0; i-- {
Expand All @@ -146,7 +135,8 @@ func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hos

uid, gid, sgids, err := oci.GetUser(details.rootfsPath, meta.User)
if err != nil {
return nil, releaseAll, err
releaseAll()
return nil, nil, err
}

opts := []containerdoci.SpecOpts{oci.WithUIDGID(uid, gid, sgids)}
Expand All @@ -157,14 +147,36 @@ func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hos
processMode := oci.ProcessSandbox // FIXME(AkihiroSuda)
spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, resolvConf, hostsFile, namespace, w.cgroupParent, processMode, nil, w.apparmorProfile, w.selinux, w.traceSocket, opts...)
if err != nil {
return nil, releaseAll, err
releaseAll()
return nil, nil, err
}
releasers = append(releasers, cleanup)
spec.Process.Terminal = meta.Tty
if w.rootless {
if err := rootlessspecconv.ToRootless(spec); err != nil {
return nil, releaseAll, err
releaseAll()
return nil, nil, err
}
}
return spec, releaseAll, nil
}

func (d *containerState) getTaskOpts() (containerd.NewTaskOpts, error) {
rootfs := containerd.WithRootFS([]mount.Mount{{
Source: d.rootfsPath,
Type: "bind",
Options: []string{"rbind"},
}})
if runtime.GOOS == "freebsd" {
rootfs = containerd.WithRootFS([]mount.Mount{{
Source: d.rootfsPath,
Type: "nullfs",
Options: []string{},
}})
}
return rootfs, nil
}

func setArgs(spec *specs.Process, args []string) {
spec.Args = args
}
28 changes: 17 additions & 11 deletions executor/containerdexecutor/executor_windows.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package containerdexecutor
import (
"context"
"os"
"strings"

"github.com/containerd/containerd"
containerdoci "github.com/containerd/containerd/oci"
Expand All @@ -23,7 +24,7 @@ func getUserSpec(user, rootfsPath string) (specs.User, error) {
}, nil
}

func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount executor.Mount, mounts []executor.Mount, meta executor.Meta, details *jobDetails) (func(), string, string, error) {
func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount executor.Mount, mounts []executor.Mount, meta executor.Meta, details *containerState) (string, string, func(), error) {
var releasers []func() error
releaseAll := func() {
for _, release := range releasers {
Expand All @@ -33,24 +34,20 @@ func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount

mountable, err := rootMount.Src.Mount(ctx, false)
if err != nil {
return releaseAll, "", "", err
return "", "", releaseAll, err
}

rootMounts, release, err := mountable.Mount()
if err != nil {
return releaseAll, "", "", err
return "", "", releaseAll, err
}
details.rootMounts = rootMounts
releasers = append(releasers, release)

return releaseAll, "", "", nil
return "", "", releaseAll, nil
}

func (w *containerdExecutor) getTaskOpts(ctx context.Context, details *jobDetails) (containerd.NewTaskOpts, error) {
return containerd.WithRootFS(details.rootMounts), nil
}

func (w *containerdExecutor) ensureCWD(ctx context.Context, details *jobDetails, meta executor.Meta) (err error) {
func (w *containerdExecutor) ensureCWD(ctx context.Context, details *containerState, meta executor.Meta) (err error) {
// TODO(gabriel-samfira): Use a snapshot?
identity, err := windows.ResolveUsernameToSID(ctx, w, details.rootMounts, meta.User)
if err != nil {
Expand All @@ -77,7 +74,7 @@ func (w *containerdExecutor) ensureCWD(ctx context.Context, details *jobDetails,
return nil
}

func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hostsFile string, namespace network.Namespace, mounts []executor.Mount, meta executor.Meta, details *jobDetails) (*specs.Spec, func(), error) {
func (w *containerdExecutor) createOCISpec(ctx context.Context, id, resolvConf, hostsFile string, namespace network.Namespace, mounts []executor.Mount, meta executor.Meta, details *containerState) (*specs.Spec, func(), error) {
var releasers []func()
releaseAll := func() {
for _, release := range releasers {
Expand All @@ -92,8 +89,17 @@ func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hos
processMode := oci.ProcessSandbox // FIXME(AkihiroSuda)
spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, "", "", namespace, "", processMode, nil, "", false, w.traceSocket, opts...)
if err != nil {
return nil, releaseAll, err
releaseAll()
return nil, nil, err
}
releasers = append(releasers, cleanup)
return spec, releaseAll, nil
}

func (d *containerState) getTaskOpts() (containerd.NewTaskOpts, error) {
return containerd.WithRootFS(d.rootMounts), nil
}

func setArgs(spec *specs.Process, args []string) {
spec.CommandLine = strings.Join(args, " ")
}
3 changes: 2 additions & 1 deletion executor/oci/spec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package oci

import (
"context"
"path"
"path/filepath"
"runtime"
"strings"
Expand Down Expand Up @@ -224,7 +225,7 @@ type submounts struct {
}

func (s *submounts) subMount(m mount.Mount, subPath string) (mount.Mount, error) {
if filepath.ToSlash(filepath.Join("/", subPath)) == "/" {
if path.Join("/", subPath) == "/" {
return m, nil
}
if s.m == nil {
Expand Down

0 comments on commit cc5657c

Please sign in to comment.