From cc5657c60026c990295ee1e04b2a8bfbd89ab302 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Wed, 30 Aug 2023 13:09:54 +0300 Subject: [PATCH] Cleanup args, rename jobDetails Signed-off-by: Gabriel Adrian Samfira --- executor/containerdexecutor/executor.go | 39 +++++------ executor/containerdexecutor/executor_unix.go | 68 +++++++++++-------- .../containerdexecutor/executor_windows.go | 28 +++++--- executor/oci/spec.go | 3 +- 4 files changed, 77 insertions(+), 61 deletions(-) diff --git a/executor/containerdexecutor/executor.go b/executor/containerdexecutor/executor.go index b5e797e3b1b8..0fe2d30296c5 100644 --- a/executor/containerdexecutor/executor.go +++ b/executor/containerdexecutor/executor.go @@ -5,8 +5,6 @@ import ( "io" "os" "path/filepath" - "runtime" - "strings" "sync" "syscall" "time" @@ -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 @@ -67,7 +65,7 @@ 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, @@ -75,7 +73,7 @@ func New(client *containerd.Client, root, cgroup string, networkProviders map[pb } } -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. @@ -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() @@ -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 @@ -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), @@ -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 } @@ -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 diff --git a/executor/containerdexecutor/executor_unix.go b/executor/containerdexecutor/executor_unix.go index 42f17749f1a1..0018360b7cce 100644 --- a/executor/containerdexecutor/executor_unix.go +++ b/executor/containerdexecutor/executor_unix.go @@ -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-- { @@ -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 @@ -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() { @@ -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) @@ -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-- { @@ -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)} @@ -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 +} diff --git a/executor/containerdexecutor/executor_windows.go b/executor/containerdexecutor/executor_windows.go index bb02dd798bfb..5bb4e12374a8 100644 --- a/executor/containerdexecutor/executor_windows.go +++ b/executor/containerdexecutor/executor_windows.go @@ -3,6 +3,7 @@ package containerdexecutor import ( "context" "os" + "strings" "github.com/containerd/containerd" containerdoci "github.com/containerd/containerd/oci" @@ -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 { @@ -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 { @@ -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 { @@ -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, " ") +} diff --git a/executor/oci/spec.go b/executor/oci/spec.go index 17f5adf523ef..96aff5ffac55 100644 --- a/executor/oci/spec.go +++ b/executor/oci/spec.go @@ -2,6 +2,7 @@ package oci import ( "context" + "path" "path/filepath" "runtime" "strings" @@ -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 {