Skip to content

Commit cc5657c

Browse files
gabriel-samfiragabriel-samfira
committed
Cleanup args, rename jobDetails
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
1 parent b6b322b commit cc5657c

File tree

4 files changed

+77
-61
lines changed

4 files changed

+77
-61
lines changed

‎executor/containerdexecutor/executor.go

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ import (
55
"io"
66
"os"
77
"path/filepath"
8-
"runtime"
9-
"strings"
108
"sync"
119
"syscall"
1210
"time"
@@ -34,7 +32,7 @@ type containerdExecutor struct {
3432
networkProviders map[pb.NetMode]network.Provider
3533
cgroupParent string
3634
dnsConfig *oci.DNSConfig
37-
running map[string]*jobDetails
35+
running map[string]*containerState
3836
mu sync.Mutex
3937
apparmorProfile string
4038
selinux bool
@@ -67,15 +65,15 @@ func New(client *containerd.Client, root, cgroup string, networkProviders map[pb
6765
networkProviders: networkProviders,
6866
cgroupParent: cgroup,
6967
dnsConfig: dnsConfig,
70-
running: make(map[string]*jobDetails),
68+
running: make(map[string]*containerState),
7169
apparmorProfile: apparmorProfile,
7270
selinux: selinux,
7371
traceSocket: traceSocket,
7472
rootless: rootless,
7573
}
7674
}
7775

78-
type jobDetails struct {
76+
type containerState struct {
7977
done chan error
8078
// On linux the rootfsPath is used to ensure the CWD exists, to fetch user information
8179
// 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
9290

9391
startedOnce := sync.Once{}
9492
done := make(chan error, 1)
95-
details := &jobDetails{
93+
details := &containerState{
9694
done: done,
9795
}
9896
w.mu.Lock()
@@ -112,12 +110,14 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M
112110
}()
113111

114112
meta := process.Meta
115-
releasers, resolvConf, hostsFile, err := w.prepareExecutionEnv(ctx, root, mounts, meta, details)
113+
resolvConf, hostsFile, releasers, err := w.prepareExecutionEnv(ctx, root, mounts, meta, details)
116114
if err != nil {
117-
releasers()
118115
return nil, err
119116
}
120-
defer releasers()
117+
118+
if releasers != nil {
119+
defer releasers()
120+
}
121121

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

140-
spec, specReleasers, err := w.getOCISpec(ctx, id, resolvConf, hostsFile, namespace, mounts, meta, details)
140+
spec, releaseSpec, err := w.createOCISpec(ctx, id, resolvConf, hostsFile, namespace, mounts, meta, details)
141141
if err != nil {
142-
specReleasers()
143142
return nil, err
144143
}
145-
defer specReleasers()
144+
if releaseSpec != nil {
145+
defer releaseSpec()
146+
}
146147

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

166-
taskOpts, err := w.getTaskOpts(ctx, details)
167+
taskOpts, err := details.getTaskOpts()
167168
if err != nil {
168169
return nil, err
169170
}
@@ -254,14 +255,10 @@ func (w *containerdExecutor) Exec(ctx context.Context, id string, process execut
254255
}
255256

256257
proc.Terminal = meta.Tty
257-
258-
if runtime.GOOS == "windows" {
259-
// On Windows passing in Args will lead to double escaping by hcsshim, which leads to errors.
260-
// The recommendation is to use CommandLine.
261-
proc.CommandLine = strings.Join(meta.Args, " ")
262-
} else {
263-
proc.Args = meta.Args
264-
}
258+
// setArgs will set the proper command line arguments for this process.
259+
// On Windows, this will set the CommandLine field. On Linux it will set the
260+
// Args field.
261+
setArgs(proc, meta.Args)
265262

266263
if meta.Cwd != "" {
267264
spec.Process.Cwd = meta.Cwd

‎executor/containerdexecutor/executor_unix.go

Lines changed: 40 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ func getUserSpec(user, rootfsPath string) (specs.User, error) {
4242
}, nil
4343
}
4444

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

5353
resolvConf, err := oci.GetResolvConf(ctx, w.root, nil, w.dnsConfig)
5454
if err != nil {
55-
return releaseAll, "", "", err
55+
releaseAll()
56+
return "", "", nil, err
5657
}
5758

5859
hostsFile, clean, err := oci.GetHostsFile(ctx, w.root, meta.ExtraHosts, nil, meta.Hostname)
5960
if err != nil {
60-
return releaseAll, "", "", err
61+
releaseAll()
62+
return "", "", nil, err
6163
}
6264
if clean != nil {
6365
releasers = append(releasers, clean)
6466
}
6567
mountable, err := rootMount.Src.Mount(ctx, false)
6668
if err != nil {
67-
return releaseAll, "", "", err
69+
releaseAll()
70+
return "", "", nil, err
6871
}
6972

7073
rootMounts, release, err := mountable.Mount()
7174
if err != nil {
72-
return releaseAll, "", "", err
75+
releaseAll()
76+
return "", "", nil, err
7377
}
7478
details.rootMounts = rootMounts
7579

@@ -83,7 +87,8 @@ func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount
8387
lm := snapshot.LocalMounterWithMounts(rootMounts)
8488
rootfsPath, err := lm.Mount()
8589
if err != nil {
86-
return releaseAll, "", "", err
90+
releaseAll()
91+
return "", "", nil, err
8792
}
8893
details.rootfsPath = rootfsPath
8994
releasers = append(releasers, func() {
@@ -93,26 +98,10 @@ func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount
9398
})
9499
releasers = append(releasers, executor.MountStubsCleaner(ctx, details.rootfsPath, mounts, meta.RemoveMountStubsRecursive))
95100

96-
return releaseAll, resolvConf, hostsFile, nil
101+
return resolvConf, hostsFile, releaseAll, nil
97102
}
98103

99-
func (w *containerdExecutor) getTaskOpts(ctx context.Context, details *jobDetails) (containerd.NewTaskOpts, error) {
100-
rootfs := containerd.WithRootFS([]mount.Mount{{
101-
Source: details.rootfsPath,
102-
Type: "bind",
103-
Options: []string{"rbind"},
104-
}})
105-
if runtime.GOOS == "freebsd" {
106-
rootfs = containerd.WithRootFS([]mount.Mount{{
107-
Source: details.rootfsPath,
108-
Type: "nullfs",
109-
Options: []string{},
110-
}})
111-
}
112-
return rootfs, nil
113-
}
114-
115-
func (w *containerdExecutor) ensureCWD(ctx context.Context, details *jobDetails, meta executor.Meta) error {
104+
func (w *containerdExecutor) ensureCWD(ctx context.Context, details *containerState, meta executor.Meta) error {
116105
newp, err := fs.RootPath(details.rootfsPath, meta.Cwd)
117106
if err != nil {
118107
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,
136125
return nil
137126
}
138127

139-
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) {
128+
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) {
140129
var releasers []func()
141130
releaseAll := func() {
142131
for i := len(releasers) - 1; i >= 0; i-- {
@@ -146,7 +135,8 @@ func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hos
146135

147136
uid, gid, sgids, err := oci.GetUser(details.rootfsPath, meta.User)
148137
if err != nil {
149-
return nil, releaseAll, err
138+
releaseAll()
139+
return nil, nil, err
150140
}
151141

152142
opts := []containerdoci.SpecOpts{oci.WithUIDGID(uid, gid, sgids)}
@@ -157,14 +147,36 @@ func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hos
157147
processMode := oci.ProcessSandbox // FIXME(AkihiroSuda)
158148
spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, resolvConf, hostsFile, namespace, w.cgroupParent, processMode, nil, w.apparmorProfile, w.selinux, w.traceSocket, opts...)
159149
if err != nil {
160-
return nil, releaseAll, err
150+
releaseAll()
151+
return nil, nil, err
161152
}
162153
releasers = append(releasers, cleanup)
163154
spec.Process.Terminal = meta.Tty
164155
if w.rootless {
165156
if err := rootlessspecconv.ToRootless(spec); err != nil {
166-
return nil, releaseAll, err
157+
releaseAll()
158+
return nil, nil, err
167159
}
168160
}
169161
return spec, releaseAll, nil
170162
}
163+
164+
func (d *containerState) getTaskOpts() (containerd.NewTaskOpts, error) {
165+
rootfs := containerd.WithRootFS([]mount.Mount{{
166+
Source: d.rootfsPath,
167+
Type: "bind",
168+
Options: []string{"rbind"},
169+
}})
170+
if runtime.GOOS == "freebsd" {
171+
rootfs = containerd.WithRootFS([]mount.Mount{{
172+
Source: d.rootfsPath,
173+
Type: "nullfs",
174+
Options: []string{},
175+
}})
176+
}
177+
return rootfs, nil
178+
}
179+
180+
func setArgs(spec *specs.Process, args []string) {
181+
spec.Args = args
182+
}

‎executor/containerdexecutor/executor_windows.go

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package containerdexecutor
33
import (
44
"context"
55
"os"
6+
"strings"
67

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

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

3435
mountable, err := rootMount.Src.Mount(ctx, false)
3536
if err != nil {
36-
return releaseAll, "", "", err
37+
return "", "", releaseAll, err
3738
}
3839

3940
rootMounts, release, err := mountable.Mount()
4041
if err != nil {
41-
return releaseAll, "", "", err
42+
return "", "", releaseAll, err
4243
}
4344
details.rootMounts = rootMounts
4445
releasers = append(releasers, release)
4546

46-
return releaseAll, "", "", nil
47+
return "", "", releaseAll, nil
4748
}
4849

49-
func (w *containerdExecutor) getTaskOpts(ctx context.Context, details *jobDetails) (containerd.NewTaskOpts, error) {
50-
return containerd.WithRootFS(details.rootMounts), nil
51-
}
52-
53-
func (w *containerdExecutor) ensureCWD(ctx context.Context, details *jobDetails, meta executor.Meta) (err error) {
50+
func (w *containerdExecutor) ensureCWD(ctx context.Context, details *containerState, meta executor.Meta) (err error) {
5451
// TODO(gabriel-samfira): Use a snapshot?
5552
identity, err := windows.ResolveUsernameToSID(ctx, w, details.rootMounts, meta.User)
5653
if err != nil {
@@ -77,7 +74,7 @@ func (w *containerdExecutor) ensureCWD(ctx context.Context, details *jobDetails,
7774
return nil
7875
}
7976

80-
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) {
77+
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) {
8178
var releasers []func()
8279
releaseAll := func() {
8380
for _, release := range releasers {
@@ -92,8 +89,17 @@ func (w *containerdExecutor) getOCISpec(ctx context.Context, id, resolvConf, hos
9289
processMode := oci.ProcessSandbox // FIXME(AkihiroSuda)
9390
spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, "", "", namespace, "", processMode, nil, "", false, w.traceSocket, opts...)
9491
if err != nil {
95-
return nil, releaseAll, err
92+
releaseAll()
93+
return nil, nil, err
9694
}
9795
releasers = append(releasers, cleanup)
9896
return spec, releaseAll, nil
9997
}
98+
99+
func (d *containerState) getTaskOpts() (containerd.NewTaskOpts, error) {
100+
return containerd.WithRootFS(d.rootMounts), nil
101+
}
102+
103+
func setArgs(spec *specs.Process, args []string) {
104+
spec.CommandLine = strings.Join(args, " ")
105+
}

‎executor/oci/spec.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package oci
22

33
import (
44
"context"
5+
"path"
56
"path/filepath"
67
"runtime"
78
"strings"
@@ -224,7 +225,7 @@ type submounts struct {
224225
}
225226

226227
func (s *submounts) subMount(m mount.Mount, subPath string) (mount.Mount, error) {
227-
if filepath.ToSlash(filepath.Join("/", subPath)) == "/" {
228+
if path.Join("/", subPath) == "/" {
228229
return m, nil
229230
}
230231
if s.m == nil {

0 commit comments

Comments
 (0)