Update Events #549

johnflavin · 2016-12-02T19:10:01Z

Add event fields from docker updates
Document the version each had been added
Deprecate older fields
Update event tests to assert on new fields
Refactor event tests (now explicitly test event streaming and polling modes)

codecov-io · 2016-12-22T17:54:33Z

Current coverage is 47.79% (diff: 72.97%)

No coverage report found for master at 05446c8.

Powered by Codecov. Last update 05446c8...5e6a201

davidxia · 2016-12-23T20:17:02Z

Looks like two event tests still fail on Docker 1.9.1. https://travis-ci.org/spotify/docker-client/jobs/186413456

johnflavin · 2016-12-27T18:18:00Z

I fixed the tests for 1.9. Current build fails on 1.10, but it looks like a random, transient failure. Unrelated to my updates.

davidxia · 2016-12-29T15:51:36Z

@johnflavin Thanks for the PR. I think the 1.10.3 build keeps failing for some reason though.

johnflavin · 2016-12-29T16:34:27Z

Yes, that does look like a real failure. I spun up a 1.10.3 docker-machine vm, but I wasn't able to reproduce this.

I added assertions to the test that the stream does have events so that, at the very least, the test can FAIL with a message rather than just ERROR with an exception.

If this continues to happen on the travis machines, I do have a suspicion about the cause. In the past I have had issues with docker events not behaving like I expected; the cause turned out to be an mismatch between the clocks on the docker vm and the host machine (see moby/moby#25579 for more). I'm not sure if that is happening on the travis machines, but that is where I would investigate.

johnflavin · 2016-12-29T16:39:36Z

Hey, all green! ✅

davidxia · 2016-12-29T15:54:06Z

src/main/java/com/spotify/docker/client/DockerClient.java

      return new EventsFilterParam(name, value);
    }

    /**
     * Show only certain events. For example, "event=pull" for image pull events.
     * @param event Type of event to show
     * @return EventsParam
+     * @since API 1.21


The docs here imply this was available since 1.18? https://docs.docker.com/engine/reference/api/docker_remote_api_v1.18/#/monitor-dockers-events

Yes, I think you are correct. Can fix.

davidxia · 2016-12-29T16:04:40Z

src/main/java/com/spotify/docker/client/DockerClient.java

+    /**
+     * Valid event types for EventsParam.type
+     */
+    public enum EventType {


Can we also test this?

Let me think this through. I'm not sure exactly what you’re asking to test.

Do you mean we should test whether the filter=type=<> param works correctly, and that if we ask for, say, only image events, then that's all we get back? That's doable. A test could do a bunch of stuff—pull an image, create a container, create a volume, etc.—then ask docker for the different types of events one-by-one, and verify that we only get back what we expect.

Or do you mean to test the validity of the enum itself, that it has all the values that it needs to have? I don't know how that would be tested. I'm kind of trusting that the API fully documents all the valid values.

Sorry, I meant the former.

Ok. That might take a little time, but it shouldn't be too complicated.

davidxia · 2016-12-29T16:07:46Z

src/main/java/com/spotify/docker/client/messages/Event.java

+
+  public static class Actor {
+    @JsonProperty("ID") private String id;
+    @JsonProperty("Attributes") private ImmutableMap<String, Object> attributes;


Could this just be ImmutableMap<String, String> or do we expect nested JSON objects?

It looks like it can be <String, String>. I'm not sure why I thought it was possible to get an Object there, but their swagger.yaml says Attributes will always have String values.

I'll fix that.

davidxia · 2016-12-29T16:40:34Z

Nice. What changed to make it pass?

johnflavin · 2016-12-29T16:45:59Z

Short answer: nothing. I'm pretty sure that event errors like that, where for some reason docker just doesn't give back events, are machine-dependent and not always reproducible. Like I said in my comment above it could very well be that the clock on the particular docker vm that they were using is out of sync with the host clock. Docker re-syncs occasionally, but in my experience it doesn't happen reliably and I have to manually sync to make the errors stop.

davidxia

@johnflavin Thanks for adding the extra test. I had a few questions.

davidxia · 2017-01-01T19:44:37Z

src/test/java/com/spotify/docker/client/DefaultDockerClientTest.java

+            .build();
+
+    // Wait once to clean our event "palette" of events from other tests
+    Thread.sleep(1000);


This is probably fine for now, but is there a way to check there are no leftover events? Perhaps we can use awaitility in the future here.

I think I could do that. There is a case to avoid: when you are streaming events, if you ask for the next one and there is none, the thread will hang while it waits for more to come over the stream. But I can careful in my tests to only assert that there are no events when I have polled for events and the stream is already closed. Also, setting timeouts on the tests will help.

davidxia · 2017-01-01T19:45:31Z

src/test/java/com/spotify/docker/client/DefaultDockerClientTest.java

+    final ContainerCreation container = sut.createContainer(config, containerName);
+    final String containerId = container.id();
+    sut.startContainer(containerId);
+    Thread.sleep(1000);


Is this to wait for the container to exit?

That is correct. Writing these tests, I got into problems when I tried to remove a container that was still running. I also tried to stop the container first, but sometimes it was stopped already so that caused its own problems.

davidxia · 2017-01-01T19:52:31Z

src/test/java/com/spotify/docker/client/DefaultDockerClientTest.java

    assertThat(pullEvent.time(), notNullValue());
    if (dockerApiVersionAtLeast("1.22")) {
-      assertEquals("image", pullEvent.type());
+      assertEquals(IMAGE.getName(), pullEvent.type());


Perhaps make this method take more parameters like containerEventAssertions() below so we don't have to hard-code IMAGE and BUSYBOX_LATEST?

I left the IMAGE.getName() hard-coded. The method is a helper for assertions about image events only, so they will always have that event.type(). But I generalized the image name and the type of action.

johnflavin · 2017-01-02T20:41:35Z

I just had a thought based on your comment. Currently the event type enum is in DockerClient; it is used as a parameter to filter events to those of certain types. However, the actual type member of Event just returns a String.

It seems it would make more sense to move enum Type from DockerClient into Event, and change String type to Type type.

Maybe I'll give that a go and see if it works.

johnflavin · 2017-01-02T20:51:49Z

👆 ✅

johnflavin · 2017-01-02T20:55:44Z

Ugh. I just realized that the best version of the Events model object is not just one class with enumerated types. It is a different class for each type, with enumerated actions.

johnflavin · 2017-01-03T15:02:20Z

Test failure appears to be a travis hiccup, unrelated to PR.

davidxia · 2017-01-03T17:58:51Z

@johnflavin I've seen similar spurious pull failures. I usually click the "restart job" button for the individual failed jobs to fix them.

johnflavin · 2017-01-03T18:08:39Z

I don't have that button. Must be a Travis feature that only repo owners get.

davidxia

cc @mattnworb if he wants to take a look.

* Explicitly enumerate event types * Deprecate old fields * Update: Add event filters to query params like all other filter types

johnflavin · 2017-01-03T18:39:24Z

Oops. Didn't see the approval before I rebased.

davidxia · 2017-01-03T18:41:36Z

That's OK. Your last commit is straight forward.

mattnworb · 2017-01-03T18:42:25Z

lgtm 👍 thanks!

johnflavin mentioned this pull request Dec 22, 2016

docker events #555

Closed

davidxia reviewed Dec 29, 2016

View reviewed changes

davidxia reviewed Jan 1, 2017

View reviewed changes

davidxia approved these changes Jan 3, 2017

View reviewed changes

johnflavin added 11 commits January 3, 2017 12:37

Update Events

0d1fa34

* Explicitly enumerate event types * Deprecate old fields * Update: Add event filters to query params like all other filter types

Add assertions on hasNext to event stream test

6f0489c

Fix JavaDoc: 'event' filter available since 1.18

1e0f6cf

Fix: Event.Actor.attributes has String values

70d5839

Add more testing for event types

17cd2a2

Fix: volume mount propagation default is system-dependent, apparently

2ee2b09

Generalize imagePullEventAssertions -> imageEventAssertions

a51b1a6

Assert no more events in stream/list

3a9c47d

Add note about testEventStream

261395d

Add note about two sleep statements

bc11d95

Move Event Type enum from DockerClient to Event

06ef7c8

davidxia merged commit 2164907 into spotify:master Jan 4, 2017

johnflavin deleted the events branch January 4, 2017 21:40

johnflavin mentioned this pull request May 22, 2017

Adds support for RegistryAuthSupplier #749

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Events #549

Update Events #549

johnflavin commented Dec 2, 2016

codecov-io commented Dec 22, 2016 •

edited

Loading

davidxia commented Dec 23, 2016

johnflavin commented Dec 27, 2016

davidxia commented Dec 29, 2016

johnflavin commented Dec 29, 2016

johnflavin commented Dec 29, 2016

davidxia Dec 29, 2016

johnflavin Dec 29, 2016

davidxia Dec 29, 2016

johnflavin Dec 29, 2016

davidxia Dec 29, 2016

johnflavin Dec 29, 2016

davidxia Dec 29, 2016

johnflavin Dec 29, 2016

davidxia commented Dec 29, 2016

johnflavin commented Dec 29, 2016

davidxia left a comment

davidxia Jan 1, 2017

johnflavin Jan 2, 2017

davidxia Jan 1, 2017

johnflavin Jan 2, 2017

davidxia Jan 1, 2017

johnflavin Jan 2, 2017

johnflavin Jan 2, 2017

johnflavin commented Jan 2, 2017

johnflavin commented Jan 2, 2017

johnflavin commented Jan 2, 2017

johnflavin commented Jan 3, 2017

davidxia commented Jan 3, 2017

johnflavin commented Jan 3, 2017

davidxia left a comment

johnflavin commented Jan 3, 2017

davidxia commented Jan 3, 2017

mattnworb commented Jan 3, 2017

Update Events #549

Update Events #549

Conversation

johnflavin commented Dec 2, 2016

codecov-io commented Dec 22, 2016 • edited Loading

Current coverage is 47.79% (diff: 72.97%)

davidxia commented Dec 23, 2016

johnflavin commented Dec 27, 2016

davidxia commented Dec 29, 2016

johnflavin commented Dec 29, 2016

johnflavin commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidxia commented Dec 29, 2016

johnflavin commented Dec 29, 2016

davidxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnflavin commented Jan 2, 2017

johnflavin commented Jan 2, 2017

johnflavin commented Jan 2, 2017

johnflavin commented Jan 3, 2017

davidxia commented Jan 3, 2017

johnflavin commented Jan 3, 2017

davidxia left a comment

Choose a reason for hiding this comment

johnflavin commented Jan 3, 2017

davidxia commented Jan 3, 2017

mattnworb commented Jan 3, 2017

codecov-io commented Dec 22, 2016 •

edited

Loading