From 3b758ede4ddf9e812deb1e61a5d2e6103f266788 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 20 Dec 2017 12:40:05 +0100 Subject: [PATCH 1/5] Remove TestEventsLimit() This test was added a long time ago, and over the years has proven to be flaky, and slow. To address those issues, it was modified to; - cleanup containers afterwards - take clock-skew into account - improve performance by parallelizing the container runs - _reduce_ parallelization to address platform issues on Windows (twice..) - adjust the test to take new limits into account - adjust the test to account for more events being generated by containers The last change to this test (made in ddae20c032058a0fd42c34c2e9750ee8f62) actually broke the test, as it's now testing that all events sent by containers (`numContainers*eventPerContainer`) are received, but the number of events that is generated (17 containers * 7 events = 119) is less than the limit (256 events). The limit is already covered by the `TestLogEvents` unit-test, that was added in 8d056423f8c433927089bd7eb6bc97abbc1ed502, and tests that the number of events is limited to `eventsLimit`. This patch removes the test, because it's not needed. Signed-off-by: Sebastiaan van Stijn Upstream-commit: b7ad3e7ea10e285226a0a5b1665e8205b3264128 Component: engine --- .../integration-cli/docker_cli_events_test.go | 44 ------------------- 1 file changed, 44 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_events_test.go b/components/engine/integration-cli/docker_cli_events_test.go index dff54a4463..b75dcc1512 100644 --- a/components/engine/integration-cli/docker_cli_events_test.go +++ b/components/engine/integration-cli/docker_cli_events_test.go @@ -81,50 +81,6 @@ func (s *DockerSuite) TestEventsUntag(c *check.C) { } } -func (s *DockerSuite) TestEventsLimit(c *check.C) { - // Windows: Limit to 4 goroutines creating containers in order to prevent - // timeouts creating so many containers simultaneously. This is a due to - // a bug in the Windows platform. It will be fixed in a Windows Update. - numContainers := 17 - eventPerContainer := 7 // create, attach, network connect, start, die, network disconnect, destroy - numConcurrentContainers := numContainers - if testEnv.DaemonPlatform() == "windows" { - numConcurrentContainers = 4 - } - sem := make(chan bool, numConcurrentContainers) - errChan := make(chan error, numContainers) - - startTime := daemonUnixTime(c) - - args := []string{"run", "--rm", "busybox", "true"} - for i := 0; i < numContainers; i++ { - sem <- true - go func(i int) { - defer func() { <-sem }() - out, err := exec.Command(dockerBinary, args...).CombinedOutput() - if err != nil { - err = fmt.Errorf("%v: %s", err, string(out)) - } - errChan <- err - }(i) - } - - // Wait for all goroutines to finish - for i := 0; i < cap(sem); i++ { - sem <- true - } - close(errChan) - - for err := range errChan { - c.Assert(err, checker.IsNil, check.Commentf("%q failed with error", strings.Join(args, " "))) - } - - out, _ := dockerCmd(c, "events", "--since="+startTime, "--until", daemonUnixTime(c)) - events := strings.Split(out, "\n") - nEvents := len(events) - 1 - c.Assert(nEvents, checker.Equals, numContainers*eventPerContainer, check.Commentf("events should be limited to 256, but received %d", nEvents)) -} - func (s *DockerSuite) TestEventsContainerEvents(c *check.C) { dockerCmd(c, "run", "--rm", "--name", "container-events-test", "busybox", "true") From e5199a0f6ec9e16b0f20aa6e5fad52d6c4455915 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 20 Dec 2017 12:41:51 +0100 Subject: [PATCH 2/5] Fix GoDoc to match actual events-limit Commit 59d45c384a2de7bca73296ce1471646db14cb0c8 changed the `eventsLimit` from 64 to 256, but did not update the GoDoc accordingly. This patch updates the GoDoc for `Subscribe` and `SubscribeTopic` to match the actual limit. Signed-off-by: Sebastiaan van Stijn Upstream-commit: fb3935022dbc160fc1531fa43f0ca2db69184800 Component: engine --- components/engine/daemon/events/events.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/engine/daemon/events/events.go b/components/engine/daemon/events/events.go index d1529e1cea..6a4990fecf 100644 --- a/components/engine/daemon/events/events.go +++ b/components/engine/daemon/events/events.go @@ -28,7 +28,7 @@ func New() *Events { } } -// Subscribe adds new listener to events, returns slice of 64 stored +// Subscribe adds new listener to events, returns slice of 256 stored // last events, a channel in which you can expect new events (in form // of interface{}, so you need type assertion), and a function to call // to stop the stream of events. @@ -46,7 +46,7 @@ func (e *Events) Subscribe() ([]eventtypes.Message, chan interface{}, func()) { return current, l, cancel } -// SubscribeTopic adds new listener to events, returns slice of 64 stored +// SubscribeTopic adds new listener to events, returns slice of 256 stored // last events, a channel in which you can expect new events (in form // of interface{}, so you need type assertion). func (e *Events) SubscribeTopic(since, until time.Time, ef *Filter) ([]eventtypes.Message, chan interface{}) { From 88269f42ba969f694dc2f62207b0e365a42a1ccd Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 20 Dec 2017 12:49:51 +0100 Subject: [PATCH 3/5] Update TestLogEvents to not use deprecated Status field The `Status` field was deprecated in favor of `Action`. This patch updates the test to use the `Action` field, but adds a check that both are set to the same value. Signed-off-by: Sebastiaan van Stijn Upstream-commit: b7d204ef6b1b2d6a3bafb42f844cdc146976e68f Component: engine --- .../engine/daemon/events/events_test.go | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/components/engine/daemon/events/events_test.go b/components/engine/daemon/events/events_test.go index ebb222cfbd..d74f2580b9 100644 --- a/components/engine/daemon/events/events_test.go +++ b/components/engine/daemon/events/events_test.go @@ -135,21 +135,28 @@ func TestLogEvents(t *testing.T) { t.Fatalf("Must be %d events, got %d", eventsLimit, len(current)) } first := current[0] - if first.Status != "action_16" { - t.Fatalf("First action is %s, must be action_16", first.Status) + + // TODO remove this once we removed the deprecated `ID`, `Status`, and `From` fields + if first.Action != first.Status { + // Verify that the (deprecated) Status is set to the expected value + t.Fatalf("Action (%s) does not match Status (%s)", first.Action, first.Status) + } + + if first.Action != "action_16" { + t.Fatalf("First action is %s, must be action_16", first.Action) } last := current[len(current)-1] - if last.Status != "action_271" { - t.Fatalf("Last action is %s, must be action_271", last.Status) + if last.Action != "action_271" { + t.Fatalf("Last action is %s, must be action_271", last.Action) } firstC := msgs[0] - if firstC.Status != "action_272" { - t.Fatalf("First action is %s, must be action_272", firstC.Status) + if firstC.Action != "action_272" { + t.Fatalf("First action is %s, must be action_272", firstC.Action) } lastC := msgs[len(msgs)-1] - if lastC.Status != "action_281" { - t.Fatalf("Last action is %s, must be action_281", lastC.Status) + if lastC.Action != "action_281" { + t.Fatalf("Last action is %s, must be action_281", lastC.Action) } } From d109b596b8c922fad337b702c0b649912662170a Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 20 Dec 2017 11:55:27 -0800 Subject: [PATCH 4/5] integration-cli: clarify multi-stage tests names Signed-off-by: Tonis Tiigi Upstream-commit: e90b7c06b4b08ca2935359667cfac4f05e33819b Component: engine --- .../integration-cli/docker_cli_build_test.go | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index 89e62c14e0..e60f4d5a6e 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/integration-cli/docker_cli_build_test.go @@ -4860,7 +4860,7 @@ func (s *DockerSuite) TestBuildBuildTimeArgDefinitionWithNoEnvInjection(c *check } } -func (s *DockerSuite) TestBuildBuildTimeArgMultipleFrom(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageArg(c *check.C) { imgName := "multifrombldargtest" dockerfile := `FROM busybox ARG foo=abc @@ -4884,7 +4884,7 @@ func (s *DockerSuite) TestBuildBuildTimeArgMultipleFrom(c *check.C) { c.Assert(result.Stdout(), checker.Contains, "bar=def") } -func (s *DockerSuite) TestBuildBuildTimeFromArgMultipleFrom(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageGlobalArg(c *check.C) { imgName := "multifrombldargtest" dockerfile := `ARG tag=nosuchtag FROM busybox:${tag} @@ -4909,7 +4909,7 @@ func (s *DockerSuite) TestBuildBuildTimeFromArgMultipleFrom(c *check.C) { c.Assert(result.Stdout(), checker.Contains, "tag=latest") } -func (s *DockerSuite) TestBuildBuildTimeUnusedArgMultipleFrom(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageUnusedArg(c *check.C) { imgName := "multifromunusedarg" dockerfile := `FROM busybox ARG foo @@ -5727,7 +5727,7 @@ func (s *DockerSuite) TestBuildCacheFrom(c *check.C) { c.Assert(layers1[len(layers1)-1], checker.Not(checker.Equals), layers2[len(layers1)-1]) } -func (s *DockerSuite) TestBuildCacheMultipleFrom(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageCache(c *check.C) { testRequires(c, DaemonIsLinux) // All tests that do save are skipped in windows dockerfile := ` FROM busybox @@ -5888,7 +5888,7 @@ func (s *DockerSuite) TestBuildContChar(c *check.C) { c.Assert(result.Combined(), checker.Contains, "Step 2/2 : RUN echo hi \\\\\n") } -func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageCopyFromSyntax(c *check.C) { dockerfile := ` FROM busybox AS first COPY foo bar @@ -5946,7 +5946,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) { cli.DockerCmd(c, "run", "build4", "cat", "baz").Assert(c, icmd.Expected{Out: "pqr"}) } -func (s *DockerSuite) TestBuildCopyFromPreviousRootFSErrors(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageCopyFromErrors(c *check.C) { testCases := []struct { dockerfile string expectedError string @@ -5993,7 +5993,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFSErrors(c *check.C) { } } -func (s *DockerSuite) TestBuildCopyFromPreviousFrom(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageMultipleBuilds(c *check.C) { dockerfile := ` FROM busybox COPY foo bar` @@ -6026,7 +6026,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousFrom(c *check.C) { c.Assert(strings.TrimSpace(out), check.Equals, "def") } -func (s *DockerSuite) TestBuildCopyFromImplicitFrom(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageImplicitFrom(c *check.C) { dockerfile := ` FROM busybox COPY --from=busybox /etc/passwd /mypasswd @@ -6053,7 +6053,7 @@ func (s *DockerSuite) TestBuildCopyFromImplicitFrom(c *check.C) { } } -func (s *DockerRegistrySuite) TestBuildCopyFromImplicitPullingFrom(c *check.C) { +func (s *DockerRegistrySuite) TestBuildMultiStageImplicitPull(c *check.C) { repoName := fmt.Sprintf("%v/dockercli/testf", privateRegistryURL) dockerfile := ` @@ -6083,7 +6083,7 @@ func (s *DockerRegistrySuite) TestBuildCopyFromImplicitPullingFrom(c *check.C) { cli.Docker(cli.Args("run", "build1", "cat", "baz")).Assert(c, icmd.Expected{Out: "abc"}) } -func (s *DockerSuite) TestBuildFromPreviousBlock(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageNameVariants(c *check.C) { dockerfile := ` FROM busybox as foo COPY foo / @@ -6094,7 +6094,7 @@ func (s *DockerSuite) TestBuildFromPreviousBlock(c *check.C) { FROM foo COPY --from=foo1 foo f1 COPY --from=FOo2 foo f2 - ` // foo2 case also tests that names are canse insensitive + ` // foo2 case also tests that names are case insensitive ctx := fakecontext.New(c, "", fakecontext.WithDockerfile(dockerfile), fakecontext.WithFiles(map[string]string{ @@ -6108,7 +6108,7 @@ func (s *DockerSuite) TestBuildFromPreviousBlock(c *check.C) { cli.Docker(cli.Args("run", "build1", "cat", "f2")).Assert(c, icmd.Expected{Out: "bar2"}) } -func (s *DockerTrustSuite) TestCopyFromTrustedBuild(c *check.C) { +func (s *DockerTrustSuite) TestBuildMultiStageTrusted(c *check.C) { img1 := s.setupTrustedImage(c, "trusted-build1") img2 := s.setupTrustedImage(c, "trusted-build2") dockerFile := fmt.Sprintf(` @@ -6130,7 +6130,7 @@ func (s *DockerTrustSuite) TestCopyFromTrustedBuild(c *check.C) { dockerCmdWithResult("run", name, "cat", "bar").Assert(c, icmd.Expected{Out: "ok"}) } -func (s *DockerSuite) TestBuildCopyFromPreviousFromWindows(c *check.C) { +func (s *DockerSuite) TestBuildMultiStageMultipleBuildsWindows(c *check.C) { testRequires(c, DaemonIsWindows) dockerfile := ` FROM ` + testEnv.MinimalBaseImage() + ` @@ -6218,7 +6218,7 @@ func (s *DockerSuite) TestBuildCopyFromWindowsIsCaseInsensitive(c *check.C) { } // #33176 -func (s *DockerSuite) TestBuildCopyFromResetScratch(c *check.C) { +func (s *DockerSuite) TestBuildMulitStageResetScratch(c *check.C) { testRequires(c, DaemonIsLinux) dockerfile := ` From c61589a0f044503c690a3a60ef65420ccfbf3ce3 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Fri, 1 Dec 2017 10:31:03 -0800 Subject: [PATCH 5/5] Add testcase for onbuild command in multi stage build Signed-off-by: Tonis Tiigi Upstream-commit: fe4ed9d78f496df5cac1eb914877f213f3f1c12c Component: engine --- .../engine/integration/build/build_test.go | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/components/engine/integration/build/build_test.go b/components/engine/integration/build/build_test.go index b447b623cc..1271daea9b 100644 --- a/components/engine/integration/build/build_test.go +++ b/components/engine/integration/build/build_test.go @@ -197,3 +197,73 @@ func TestBuildWithEmptyLayers(t *testing.T) { resp.Body.Close() require.NoError(t, err) } + +// TestBuildMultiStageOnBuild checks that ONBUILD commands are applied to +// multiple subsequent stages +// #35652 +func TestBuildMultiStageOnBuild(t *testing.T) { + defer setupTest(t)() + // test both metadata and layer based commands as they may be implemented differently + dockerfile := `FROM busybox AS stage1 +ONBUILD RUN echo 'foo' >somefile +ONBUILD ENV bar=baz + +FROM stage1 +RUN cat somefile # fails if ONBUILD RUN fails + +FROM stage1 +RUN cat somefile` + + ctx := context.Background() + source := fakecontext.New(t, "", + fakecontext.WithDockerfile(dockerfile)) + defer source.Close() + + apiclient := testEnv.APIClient() + resp, err := apiclient.ImageBuild(ctx, + source.AsTarReader(t), + types.ImageBuildOptions{ + Remove: true, + ForceRemove: true, + }) + + out := bytes.NewBuffer(nil) + require.NoError(t, err) + _, err = io.Copy(out, resp.Body) + resp.Body.Close() + require.NoError(t, err) + + assert.Contains(t, out.String(), "Successfully built") + + imageIDs, err := getImageIDsFromBuild(out.Bytes()) + require.NoError(t, err) + assert.Equal(t, 3, len(imageIDs)) + + image, _, err := apiclient.ImageInspectWithRaw(context.Background(), imageIDs[2]) + require.NoError(t, err) + assert.Contains(t, image.Config.Env, "bar=baz") +} + +type buildLine struct { + Stream string + Aux struct { + ID string + } +} + +func getImageIDsFromBuild(output []byte) ([]string, error) { + ids := []string{} + for _, line := range bytes.Split(output, []byte("\n")) { + if len(line) == 0 { + continue + } + entry := buildLine{} + if err := json.Unmarshal(line, &entry); err != nil { + return nil, err + } + if entry.Aux.ID != "" { + ids = append(ids, entry.Aux.ID) + } + } + return ids, nil +}