Browse code

Fix attach race condition, improve unit tests, make sure the container is started before unblocking Start

Guillaume J. Charmes authored on 2013/10/09 10:42:53
Showing 6 changed files
... ...
@@ -371,8 +371,8 @@ func TestGetContainersJSON(t *testing.T) {
371 371
 	if err := json.Unmarshal(r.Body.Bytes(), &containers); err != nil {
372 372
 		t.Fatal(err)
373 373
 	}
374
-	if len(containers) != 1 {
375
-		t.Fatalf("Expected %d container, %d found (started with: %d)", 1, len(containers), beginLen)
374
+	if len(containers) != beginLen+1 {
375
+		t.Fatalf("Expected %d container, %d found (started with: %d)", beginLen+1, len(containers), beginLen)
376 376
 	}
377 377
 	if containers[0].ID != container.ID {
378 378
 		t.Fatalf("Container ID mismatch. Expected: %s, received: %s\n", container.ID, containers[0].ID)
... ...
@@ -1597,12 +1597,10 @@ func (cli *DockerCli) CmdRun(args ...string) error {
1597 1597
 		cli.forwardAllSignals(runResult.ID)
1598 1598
 	}
1599 1599
 
1600
-	//start the container
1601
-	if _, _, err = cli.call("POST", "/containers/"+runResult.ID+"/start", hostConfig); err != nil {
1602
-		return err
1603
-	}
1604
-
1605
-	var wait chan struct{}
1600
+	var (
1601
+		wait  chan struct{}
1602
+		errCh chan error
1603
+	)
1606 1604
 
1607 1605
 	if !config.AttachStdout && !config.AttachStderr {
1608 1606
 		// Make this asynchrone in order to let the client write to stdin before having to read the ID
... ...
@@ -1621,7 +1619,6 @@ func (cli *DockerCli) CmdRun(args ...string) error {
1621 1621
 		}
1622 1622
 
1623 1623
 		v := url.Values{}
1624
-		v.Set("logs", "1")
1625 1624
 		v.Set("stream", "1")
1626 1625
 		var out, stderr io.Writer
1627 1626
 
... ...
@@ -1641,18 +1638,18 @@ func (cli *DockerCli) CmdRun(args ...string) error {
1641 1641
 			}
1642 1642
 		}
1643 1643
 
1644
-		signals := make(chan os.Signal, 1)
1645
-		signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM)
1646
-		go func() {
1647
-			for sig := range signals {
1648
-				fmt.Printf("\nReceived signal: %s; cleaning up\n", sig)
1649
-				if err := cli.CmdStop("-t", "4", runResult.ID); err != nil {
1650
-					fmt.Printf("failed to stop container: %v", err)
1651
-				}
1652
-			}
1653
-		}()
1644
+		errCh = utils.Go(func() error {
1645
+			return cli.hijack("POST", "/containers/"+runResult.ID+"/attach?"+v.Encode(), config.Tty, cli.in, out, stderr)
1646
+		})
1647
+	}
1648
+
1649
+	//start the container
1650
+	if _, _, err = cli.call("POST", "/containers/"+runResult.ID+"/start", hostConfig); err != nil {
1651
+		return err
1652
+	}
1654 1653
 
1655
-		if err := cli.hijack("POST", "/containers/"+runResult.ID+"/attach?"+v.Encode(), config.Tty, cli.in, out, stderr); err != nil {
1654
+	if errCh != nil {
1655
+		if err := <-errCh; err != nil {
1656 1656
 			utils.Debugf("Error hijack: %s", err)
1657 1657
 			return err
1658 1658
 		}
... ...
@@ -1667,8 +1664,7 @@ func (cli *DockerCli) CmdRun(args ...string) error {
1667 1667
 			return err
1668 1668
 		}
1669 1669
 		if autoRemove {
1670
-			_, _, err = cli.call("DELETE", "/containers/"+runResult.ID, nil)
1671
-			if err != nil {
1670
+			if _, _, err = cli.call("DELETE", "/containers/"+runResult.ID, nil); err != nil {
1672 1671
 				return err
1673 1672
 			}
1674 1673
 		}
... ...
@@ -1753,6 +1749,7 @@ func (cli *DockerCli) call(method, path string, data interface{}) ([]byte, int,
1753 1753
 		return nil, -1, err
1754 1754
 	}
1755 1755
 	defer resp.Body.Close()
1756
+
1756 1757
 	body, err := ioutil.ReadAll(resp.Body)
1757 1758
 	if err != nil {
1758 1759
 		return nil, -1, err
... ...
@@ -84,7 +84,7 @@ func TestRunHostname(t *testing.T) {
84 84
 		}
85 85
 	})
86 86
 
87
-	setTimeout(t, "CmdRun timed out", 5*time.Second, func() {
87
+	setTimeout(t, "CmdRun timed out", 10*time.Second, func() {
88 88
 		<-c
89 89
 	})
90 90
 
... ...
@@ -115,7 +115,7 @@ func TestRunWorkdir(t *testing.T) {
115 115
 		}
116 116
 	})
117 117
 
118
-	setTimeout(t, "CmdRun timed out", 5*time.Second, func() {
118
+	setTimeout(t, "CmdRun timed out", 10*time.Second, func() {
119 119
 		<-c
120 120
 	})
121 121
 
... ...
@@ -399,6 +399,8 @@ func TestRunDetach(t *testing.T) {
399 399
 		}
400 400
 	})
401 401
 
402
+	closeWrap(stdin, stdinPipe, stdout, stdoutPipe)
403
+
402 404
 	// wait for CmdRun to return
403 405
 	setTimeout(t, "Waiting for CmdRun timed out", 15*time.Second, func() {
404 406
 		<-ch
... ...
@@ -422,30 +424,52 @@ func TestAttachDetach(t *testing.T) {
422 422
 	cli := NewDockerCli(stdin, stdoutPipe, ioutil.Discard, testDaemonProto, testDaemonAddr)
423 423
 	defer cleanup(globalRuntime)
424 424
 
425
-	go stdout.Read(make([]byte, 1024))
426
-	setTimeout(t, "Starting container timed out", 2*time.Second, func() {
425
+	ch := make(chan struct{})
426
+	go func() {
427
+		defer close(ch)
427 428
 		if err := cli.CmdRun("-i", "-t", "-d", unitTestImageID, "cat"); err != nil {
428 429
 			t.Fatal(err)
429 430
 		}
430
-	})
431
+	}()
431 432
 
432
-	container := globalRuntime.List()[0]
433
+	var container *Container
434
+
435
+	setTimeout(t, "Reading container's id timed out", 10*time.Second, func() {
436
+		buf := make([]byte, 1024)
437
+		n, err := stdout.Read(buf)
438
+		if err != nil {
439
+			t.Fatal(err)
440
+		}
441
+
442
+		container = globalRuntime.List()[0]
443
+
444
+		if strings.Trim(string(buf[:n]), " \r\n") != container.ShortID() {
445
+			t.Fatalf("Wrong ID received. Expect %s, received %s", container.ShortID(), buf[:n])
446
+		}
447
+	})
448
+	setTimeout(t, "Starting container timed out", 10*time.Second, func() {
449
+		<-ch
450
+	})
433 451
 
434 452
 	stdin, stdinPipe = io.Pipe()
435 453
 	stdout, stdoutPipe = io.Pipe()
436 454
 	cli = NewDockerCli(stdin, stdoutPipe, ioutil.Discard, testDaemonProto, testDaemonAddr)
437 455
 
438
-	ch := make(chan struct{})
456
+	ch = make(chan struct{})
439 457
 	go func() {
440 458
 		defer close(ch)
441 459
 		if err := cli.CmdAttach(container.ShortID()); err != nil {
442
-			t.Fatal(err)
460
+			if err != io.ErrClosedPipe {
461
+				t.Fatal(err)
462
+			}
443 463
 		}
444 464
 	}()
445 465
 
446 466
 	setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() {
447 467
 		if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil {
448
-			t.Fatal(err)
468
+			if err != io.ErrClosedPipe {
469
+				t.Fatal(err)
470
+			}
449 471
 		}
450 472
 	})
451 473
 
... ...
@@ -455,6 +479,7 @@ func TestAttachDetach(t *testing.T) {
455 455
 			t.Fatal(err)
456 456
 		}
457 457
 	})
458
+	closeWrap(stdin, stdinPipe, stdout, stdoutPipe)
458 459
 
459 460
 	// wait for CmdRun to return
460 461
 	setTimeout(t, "Waiting for CmdAttach timed out", 15*time.Second, func() {
... ...
@@ -568,7 +593,7 @@ func TestRunAutoRemove(t *testing.T) {
568 568
 		}
569 569
 	})
570 570
 
571
-	setTimeout(t, "CmdRun timed out", 5*time.Second, func() {
571
+	setTimeout(t, "CmdRun timed out", 10*time.Second, func() {
572 572
 		<-c
573 573
 	})
574 574
 
... ...
@@ -99,6 +99,8 @@ type BindMap struct {
99 99
 }
100 100
 
101 101
 var (
102
+	ErrContainerStart           = errors.New("The container failed to start. Unkown error")
103
+	ErrContainerStartTimeout    = errors.New("The container failed to start due to timed out.")
102 104
 	ErrInvalidWorikingDirectory = errors.New("The working directory is invalid. It needs to be an absolute path.")
103 105
 	ErrConflictTtySigProxy      = errors.New("TTY mode (-t) already imply signal proxying (-sig-proxy)")
104 106
 	ErrConflictAttachDetach     = errors.New("Conflicting options: -a and -d")
... ...
@@ -838,12 +840,43 @@ func (container *Container) Start(hostConfig *HostConfig) (err error) {
838 838
 	container.ToDisk()
839 839
 	container.SaveHostConfig(hostConfig)
840 840
 	go container.monitor(hostConfig)
841
-	return nil
841
+
842
+	defer utils.Debugf("Container running: %v", container.State.Running)
843
+	// We wait for the container to be fully running.
844
+	// Timeout after 5 seconds. In case of broken pipe, just retry.
845
+	// Note: The container can run and finish correctly before
846
+	//       the end of this loop
847
+	for now := time.Now(); time.Since(now) < 5*time.Second; {
848
+		// If the container dies while waiting for it, just reutrn
849
+		if !container.State.Running {
850
+			return nil
851
+		}
852
+		output, err := exec.Command("lxc-info", "-n", container.ID).CombinedOutput()
853
+		if err != nil {
854
+			utils.Debugf("Error with lxc-info: %s (%s)", err, output)
855
+
856
+			output, err = exec.Command("lxc-info", "-n", container.ID).CombinedOutput()
857
+			if err != nil {
858
+				utils.Debugf("Second Error with lxc-info: %s (%s)", err, output)
859
+				return err
860
+			}
861
+
862
+		}
863
+		if strings.Contains(string(output), "RUNNING") {
864
+			return nil
865
+		}
866
+		utils.Debugf("Waiting for the container to start (running: %v): %s\n", container.State.Running, output)
867
+		time.Sleep(50 * time.Millisecond)
868
+	}
869
+
870
+	if container.State.Running {
871
+		return ErrContainerStartTimeout
872
+	}
873
+	return ErrContainerStart
842 874
 }
843 875
 
844 876
 func (container *Container) Run() error {
845
-	hostConfig := &HostConfig{}
846
-	if err := container.Start(hostConfig); err != nil {
877
+	if err := container.Start(&HostConfig{}); err != nil {
847 878
 		return err
848 879
 	}
849 880
 	container.Wait()
... ...
@@ -212,22 +212,16 @@ func TestRuntimeCreate(t *testing.T) {
212 212
 	}
213 213
 
214 214
 	// Make sure crete with bad parameters returns an error
215
-	_, err = runtime.Create(
216
-		&Config{
217
-			Image: GetTestImage(runtime).ID,
218
-		},
219
-	)
220
-	if err == nil {
215
+	if _, err = runtime.Create(&Config{Image: GetTestImage(runtime).ID}); err == nil {
221 216
 		t.Fatal("Builder.Create should throw an error when Cmd is missing")
222 217
 	}
223 218
 
224
-	_, err = runtime.Create(
219
+	if _, err := runtime.Create(
225 220
 		&Config{
226 221
 			Image: GetTestImage(runtime).ID,
227 222
 			Cmd:   []string{},
228 223
 		},
229
-	)
230
-	if err == nil {
224
+	); err == nil {
231 225
 		t.Fatal("Builder.Create should throw an error when Cmd is empty")
232 226
 	}
233 227
 
... ...
@@ -258,11 +252,11 @@ func TestRuntimeCreate(t *testing.T) {
258 258
 func TestDestroy(t *testing.T) {
259 259
 	runtime := mkRuntime(t)
260 260
 	defer nuke(runtime)
261
+
261 262
 	container, err := runtime.Create(&Config{
262 263
 		Image: GetTestImage(runtime).ID,
263 264
 		Cmd:   []string{"ls", "-al"},
264
-	},
265
-	)
265
+	})
266 266
 	if err != nil {
267 267
 		t.Fatal(err)
268 268
 	}
... ...
@@ -327,11 +321,14 @@ func TestGet(t *testing.T) {
327 327
 }
328 328
 
329 329
 func startEchoServerContainer(t *testing.T, proto string) (*Runtime, *Container, string) {
330
-	var err error
331
-	runtime := mkRuntime(t)
332
-	port := 5554
333
-	var container *Container
334
-	var strPort string
330
+	var (
331
+		err       error
332
+		container *Container
333
+		strPort   string
334
+		runtime   = mkRuntime(t)
335
+		port      = 5554
336
+	)
337
+
335 338
 	for {
336 339
 		port += 1
337 340
 		strPort = strconv.Itoa(port)
... ...
@@ -359,8 +356,7 @@ func startEchoServerContainer(t *testing.T, proto string) (*Runtime, *Container,
359 359
 		t.Logf("Port %v already in use", strPort)
360 360
 	}
361 361
 
362
-	hostConfig := &HostConfig{}
363
-	if err := container.Start(hostConfig); err != nil {
362
+	if err := container.Start(&HostConfig{}); err != nil {
364 363
 		nuke(runtime)
365 364
 		t.Fatal(err)
366 365
 	}
... ...
@@ -192,11 +192,11 @@ func TestCreateStartRestartStopStartKillRm(t *testing.T) {
192 192
 		t.Fatal(err)
193 193
 	}
194 194
 
195
-	if err := srv.ContainerRestart(id, 1); err != nil {
195
+	if err := srv.ContainerRestart(id, 15); err != nil {
196 196
 		t.Fatal(err)
197 197
 	}
198 198
 
199
-	if err := srv.ContainerStop(id, 1); err != nil {
199
+	if err := srv.ContainerStop(id, 15); err != nil {
200 200
 		t.Fatal(err)
201 201
 	}
202 202