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/3] 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/3] 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/3] 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) } }