Browse code

container/controller: avoid cancellation with forked pull context

Context cancellations were previously causing `Prepare` to fail
completely on re-entrant calls. To prevent this, we filtered out cancels
and deadline errors. While this allowed the service to proceed without
errors, it had the possibility of interrupting long pulls, causing the
pull to happen twice.

This PR forks the context of the pull to match the lifetime of
`Controller`, ensuring that for each task, the pull is only performed
once. It also ensures that multiple calls to `Prepare` are re-entrant,
ensuring that the pull resumes from its original position.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
(cherry picked from commit d8d71ad5b94d44a2778f2d8989424259cac94e9b)
Signed-off-by: Tibor Vass <tibor@docker.com>

Stephen J Day authored on 2016/07/26 12:59:02
Showing 1 changed files
... ...
@@ -23,6 +23,10 @@ type controller struct {
23 23
 	adapter *containerAdapter
24 24
 	closed  chan struct{}
25 25
 	err     error
26
+
27
+	pulled     chan struct{} // closed after pull
28
+	cancelPull func()        // cancels pull context if not nil
29
+	pullErr    error         // pull error, only read after pulled closed
26 30
 }
27 31
 
28 32
 var _ exec.Controller = &controller{}
... ...
@@ -84,24 +88,40 @@ func (r *controller) Prepare(ctx context.Context) error {
84 84
 		return err
85 85
 	}
86 86
 
87
-	if err := r.adapter.pullImage(ctx); err != nil {
88
-		cause := errors.Cause(err)
89
-		if cause == context.Canceled || cause == context.DeadlineExceeded {
90
-			return err
91
-		}
87
+	if r.pulled == nil {
88
+		// Fork the pull to a different context to allow pull to continue
89
+		// on re-entrant calls to Prepare. This ensures that Prepare can be
90
+		// idempotent and not incur the extra cost of pulling when
91
+		// cancelled on updates.
92
+		var pctx context.Context
93
+
94
+		r.pulled = make(chan struct{})
95
+		pctx, r.cancelPull = context.WithCancel(context.Background()) // TODO(stevvooe): Bind a context to the entire controller.
96
+
97
+		go func() {
98
+			defer close(r.pulled)
99
+			r.pullErr = r.adapter.pullImage(pctx) // protected by closing r.pulled
100
+		}()
101
+	}
92 102
 
93
-		// NOTE(stevvooe): We always try to pull the image to make sure we have
94
-		// the most up to date version. This will return an error, but we only
95
-		// log it. If the image truly doesn't exist, the create below will
96
-		// error out.
97
-		//
98
-		// This gives us some nice behavior where we use up to date versions of
99
-		// mutable tags, but will still run if the old image is available but a
100
-		// registry is down.
101
-		//
102
-		// If you don't want this behavior, lock down your image to an
103
-		// immutable tag or digest.
104
-		log.G(ctx).WithError(err).Error("pulling image failed")
103
+	select {
104
+	case <-ctx.Done():
105
+		return ctx.Err()
106
+	case <-r.pulled:
107
+		if r.pullErr != nil {
108
+			// NOTE(stevvooe): We always try to pull the image to make sure we have
109
+			// the most up to date version. This will return an error, but we only
110
+			// log it. If the image truly doesn't exist, the create below will
111
+			// error out.
112
+			//
113
+			// This gives us some nice behavior where we use up to date versions of
114
+			// mutable tags, but will still run if the old image is available but a
115
+			// registry is down.
116
+			//
117
+			// If you don't want this behavior, lock down your image to an
118
+			// immutable tag or digest.
119
+			log.G(ctx).WithError(r.pullErr).Error("pulling image failed")
120
+		}
105 121
 	}
106 122
 
107 123
 	if err := r.adapter.create(ctx, r.backend); err != nil {
... ...
@@ -249,6 +269,10 @@ func (r *controller) Shutdown(ctx context.Context) error {
249 249
 		return err
250 250
 	}
251 251
 
252
+	if r.cancelPull != nil {
253
+		r.cancelPull()
254
+	}
255
+
252 256
 	if err := r.adapter.shutdown(ctx); err != nil {
253 257
 		if isUnknownContainer(err) || isStoppedContainer(err) {
254 258
 			return nil
... ...
@@ -266,6 +290,10 @@ func (r *controller) Terminate(ctx context.Context) error {
266 266
 		return err
267 267
 	}
268 268
 
269
+	if r.cancelPull != nil {
270
+		r.cancelPull()
271
+	}
272
+
269 273
 	if err := r.adapter.terminate(ctx); err != nil {
270 274
 		if isUnknownContainer(err) {
271 275
 			return nil
... ...
@@ -283,6 +311,10 @@ func (r *controller) Remove(ctx context.Context) error {
283 283
 		return err
284 284
 	}
285 285
 
286
+	if r.cancelPull != nil {
287
+		r.cancelPull()
288
+	}
289
+
286 290
 	// It may be necessary to shut down the task before removing it.
287 291
 	if err := r.Shutdown(ctx); err != nil {
288 292
 		if isUnknownContainer(err) {
... ...
@@ -317,6 +349,10 @@ func (r *controller) Close() error {
317 317
 	case <-r.closed:
318 318
 		return r.err
319 319
 	default:
320
+		if r.cancelPull != nil {
321
+			r.cancelPull()
322
+		}
323
+
320 324
 		r.err = exec.ErrControllerClosed
321 325
 		close(r.closed)
322 326
 	}