Browse code

daemon: Daemon.ContainerExecStart: rename err-return, and minor refactor

- rename the error-return to prevent accidental shadowing
- remove some intermediate variables
- usee a struct-literal for specs.Process
- optimize logging-code to not use chained "WithField"
- remove punctuation from error-message

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2023/12/28 21:16:16
Showing 1 changed files
... ...
@@ -159,7 +159,7 @@ func (daemon *Daemon) ContainerExecCreate(name string, options *containertypes.E
159 159
 // ContainerExecStart starts a previously set up exec instance. The
160 160
 // std streams are set up.
161 161
 // If ctx is cancelled, the process is terminated.
162
-func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, options backend.ExecStartConfig) (err error) {
162
+func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, options backend.ExecStartConfig) (retErr error) {
163 163
 	var (
164 164
 		cStdin           io.ReadCloser
165 165
 		cStdout, cStderr io.Writer
... ...
@@ -173,13 +173,12 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, optio
173 173
 	ec.Lock()
174 174
 	if ec.ExitCode != nil {
175 175
 		ec.Unlock()
176
-		err := fmt.Errorf("Error: Exec command %s has already run", ec.ID)
177
-		return errdefs.Conflict(err)
176
+		return errdefs.Conflict(fmt.Errorf("exec command %s has already run", ec.ID))
178 177
 	}
179 178
 
180 179
 	if ec.Running {
181 180
 		ec.Unlock()
182
-		return errdefs.Conflict(fmt.Errorf("Error: Exec command %s is already running", ec.ID))
181
+		return errdefs.Conflict(fmt.Errorf("exec command %s is already running", ec.ID))
183 182
 	}
184 183
 	ec.Running = true
185 184
 	ec.Unlock()
... ...
@@ -190,7 +189,7 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, optio
190 190
 	})
191 191
 
192 192
 	defer func() {
193
-		if err != nil {
193
+		if retErr != nil {
194 194
 			ec.Lock()
195 195
 			ec.Container.ExecCommands.Delete(ec.ID)
196 196
 			ec.Running = false
... ...
@@ -210,9 +209,11 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, optio
210 210
 	if ec.OpenStdin && options.Stdin != nil {
211 211
 		r, w := io.Pipe()
212 212
 		go func() {
213
-			defer w.Close()
214
-			defer log.G(ctx).Debug("Closing buffered stdin pipe")
215
-			pools.Copy(w, options.Stdin)
213
+			defer func() {
214
+				log.G(ctx).Debug("Closing buffered stdin pipe")
215
+				_ = w.Close()
216
+			}()
217
+			_, _ = pools.Copy(w, options.Stdin)
216 218
 		}()
217 219
 		cStdin = r
218 220
 	}
... ...
@@ -231,6 +232,9 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, optio
231 231
 
232 232
 	p := &specs.Process{}
233 233
 	if runtime.GOOS != "windows" {
234
+		// TODO(thaJeztah): also enable on Windows;
235
+		//  This was added in https://github.com/moby/moby/commit/7603c22c7365d7d7150597fe396e0707d6e561da,
236
+		//  which mentions that it failed on Windows "Probably needs to wait for container to be in running state"
234 237
 		ctr, err := daemon.containerdClient.LoadContainer(ctx, ec.Container.ID)
235 238
 		if err != nil {
236 239
 			return err
... ...
@@ -239,11 +243,16 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, optio
239 239
 		if err != nil {
240 240
 			return err
241 241
 		}
242
-		spec := specs.Spec{Process: p}
242
+
243
+		// Technically, this should be a [specs.Spec], but we're only
244
+		// interested in the Process field.
245
+		spec := struct{ Process *specs.Process }{p}
243 246
 		if err := json.Unmarshal(md.Spec.GetValue(), &spec); err != nil {
244 247
 			return err
245 248
 		}
246 249
 	}
250
+
251
+	// merge/override properties of the container's process with the exec-config.
247 252
 	p.Args = append([]string{ec.Entrypoint}, ec.Args...)
248 253
 	p.Env = ec.Env
249 254
 	p.Cwd = ec.WorkingDir
... ...
@@ -308,15 +317,15 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, optio
308 308
 
309 309
 	select {
310 310
 	case <-ctx.Done():
311
-		log := log.G(ctx).
312
-			WithField("container", ec.Container.ID).
313
-			WithField("exec", ec.ID)
314
-		log.Debug("Sending KILL signal to container process")
311
+		logger := log.G(ctx).WithFields(log.Fields{
312
+			"container": ec.Container.ID,
313
+			"exeec":     ec.ID,
314
+		})
315
+		logger.Debug("Sending KILL signal to container process")
315 316
 		sigCtx, cancelFunc := context.WithTimeout(context.Background(), 30*time.Second)
316 317
 		defer cancelFunc()
317
-		err := ec.Process.Kill(sigCtx, signal.SignalMap["KILL"])
318
-		if err != nil {
319
-			log.WithError(err).Error("Could not send KILL signal to container process")
318
+		if err := ec.Process.Kill(sigCtx, signal.SignalMap["KILL"]); err != nil {
319
+			logger.WithError(err).Error("Could not send KILL signal to container process")
320 320
 		}
321 321
 		return ctx.Err()
322 322
 	case err := <-attachErr: