New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Graylog2/go-gelf vendoring. Fixes #35613 #35765

Merged
merged 2 commits into from Dec 12, 2017

Conversation

Projects
None yet
6 participants
@ghislainbourgeois
Contributor

ghislainbourgeois commented Dec 11, 2017

- What I did

Updated the vendoring of Graylog2/go-gelf to get the latest bug fix. Fixes #35613.

- How I did it

Simply fetch the changes with vndr.

- How to verify it

  1. start something to imitate log server ex: "nc -l -p 12000"
  2. start something to imitate log server ex: "nc -l -p 12000"
  3. start container "docker run --rm -it --log-driver gelf --log-opt gelf-address=tcp://127.0.0.1:12000 debian bash"
  4. type something in the container's terminal (generate some logs)
  5. stop the nc process
  6. type something again in the container's terminal (generate some more logs)

Validate that the docker daemon does not crash.

- Description for the changelog

Fixed daemon crash when using the GELF log driver over TCP when the GELF server goes down.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 11, 2017

Member

We just noticed that the previous pull request updated the vendoring to use a branch, not a tag; #34758 (comment)

Given that the vendor.conf expects the vendoring to be reproducible, can you;

  • add a commit that updates the vendor.conf to refer to the commit that was previously used (i.e., matching what was vendored in #34758 (comment))
  • then in your current commit, update the vendor.conf to 4143646226541087117ff2f83334ea48b3201841 (current tip of the v2 branch)
Member

thaJeztah commented Dec 11, 2017

We just noticed that the previous pull request updated the vendoring to use a branch, not a tag; #34758 (comment)

Given that the vendor.conf expects the vendoring to be reproducible, can you;

  • add a commit that updates the vendor.conf to refer to the commit that was previously used (i.e., matching what was vendored in #34758 (comment))
  • then in your current commit, update the vendor.conf to 4143646226541087117ff2f83334ea48b3201841 (current tip of the v2 branch)
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 11, 2017

Member

We'll then cherry-pick/backport the first commit to the currently supported Docker releases (17.09.x, 17.12)

Member

thaJeztah commented Dec 11, 2017

We'll then cherry-pick/backport the first commit to the currently supported Docker releases (17.09.x, 17.12)

ghislainbourgeois added some commits Dec 11, 2017

Fix vendoring of go-gelf to point to specific commit ID
Signed-off-by: Ghislain Bourgeois <ghislain.bourgeois@gmail.com>
Update Graylog2/go-gelf vendoring. Fixes #35613
Signed-off-by: Ghislain Bourgeois <ghislain.bourgeois@gmail.com>
@ghislainbourgeois

This comment has been minimized.

Show comment
Hide comment
@ghislainbourgeois

ghislainbourgeois Dec 11, 2017

Contributor

Sure, just fixed it.

Contributor

ghislainbourgeois commented Dec 11, 2017

Sure, just fixed it.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 11, 2017

Member

Thanks!

ping @anusha-ragunathan @vieux PTAL

Member

thaJeztah commented Dec 11, 2017

Thanks!

ping @anusha-ragunathan @vieux PTAL

@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan Dec 11, 2017

Contributor

LGTM

Contributor

anusha-ragunathan commented Dec 11, 2017

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 12, 2017

Member

Hm, possibly flaky test on Windows;

22:02:25 ----------------------------------------------------------------------
22:02:25 FAIL: docker_api_logs_test.go:152: DockerSuite.TestLogsAPIUntil
22:02:25 
22:02:25 docker_api_logs_test.go:184:
22:02:25     c.Assert(logsString, checker.Not(checker.Contains), "log3", check.Commentf("unexpected log message returned, until=%v", until))
22:02:25 ... obtained string = "" +
22:02:25 ...     "2017-12-11T22:02:27.571445400Z log1\n" +
22:02:25 ...     "2017-12-11T22:02:27.571657700Z log2\n" +
22:02:25 ...     "2017-12-11T22:02:27.571657700Z log3\n"
22:02:25 ... substring string = "log3"
22:02:25 ... unexpected log message returned, until=2017-12-11T22:02:27.5716577Z
22:02:25 
Member

thaJeztah commented Dec 12, 2017

Hm, possibly flaky test on Windows;

22:02:25 ----------------------------------------------------------------------
22:02:25 FAIL: docker_api_logs_test.go:152: DockerSuite.TestLogsAPIUntil
22:02:25 
22:02:25 docker_api_logs_test.go:184:
22:02:25     c.Assert(logsString, checker.Not(checker.Contains), "log3", check.Commentf("unexpected log message returned, until=%v", until))
22:02:25 ... obtained string = "" +
22:02:25 ...     "2017-12-11T22:02:27.571445400Z log1\n" +
22:02:25 ...     "2017-12-11T22:02:27.571657700Z log2\n" +
22:02:25 ...     "2017-12-11T22:02:27.571657700Z log3\n"
22:02:25 ... substring string = "log3"
22:02:25 ... unexpected log message returned, until=2017-12-11T22:02:27.5716577Z
22:02:25 
@vieux

vieux approved these changes Dec 12, 2017

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 12, 2017

Member

Same test is failing again, so there may be an actual issue;

02:20:01 ----------------------------------------------------------------------
02:20:01 FAIL: docker_api_logs_test.go:152: DockerSuite.TestLogsAPIUntil
02:20:01 
02:20:01 docker_api_logs_test.go:184:
02:20:01     c.Assert(logsString, checker.Not(checker.Contains), "log3", check.Commentf("unexpected log message returned, until=%v", until))
02:20:01 ... obtained string = "" +
02:20:01 ...     "2017-12-12T02:20:00.938221900Z log1\n" +
02:20:01 ...     "2017-12-12T02:20:00.938221900Z log2\n" +
02:20:01 ...     "2017-12-12T02:20:00.938221900Z log3\n"
02:20:01 ... substring string = "log3"
02:20:01 ... unexpected log message returned, until=2017-12-12T02:20:00.9382219Z
02:20:01 
Member

thaJeztah commented Dec 12, 2017

Same test is failing again, so there may be an actual issue;

02:20:01 ----------------------------------------------------------------------
02:20:01 FAIL: docker_api_logs_test.go:152: DockerSuite.TestLogsAPIUntil
02:20:01 
02:20:01 docker_api_logs_test.go:184:
02:20:01     c.Assert(logsString, checker.Not(checker.Contains), "log3", check.Commentf("unexpected log message returned, until=%v", until))
02:20:01 ... obtained string = "" +
02:20:01 ...     "2017-12-12T02:20:00.938221900Z log1\n" +
02:20:01 ...     "2017-12-12T02:20:00.938221900Z log2\n" +
02:20:01 ...     "2017-12-12T02:20:00.938221900Z log3\n"
02:20:01 ... substring string = "log3"
02:20:01 ... unexpected log message returned, until=2017-12-12T02:20:00.9382219Z
02:20:01 
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 12, 2017

Member

I think the failing test may be introduced by / related to ee594dc (#35745); looking at the output for the failing test, I see that all three log-entries have exactly the same timestamp, so because of that, the --until probably doesn't filter

Member

thaJeztah commented Dec 12, 2017

I think the failing test may be introduced by / related to ee594dc (#35745); looking at the output for the failing test, I see that all three log-entries have exactly the same timestamp, so because of that, the --until probably doesn't filter

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 12, 2017

Member

Opened #35771 to fix the test

Member

thaJeztah commented Dec 12, 2017

Opened #35771 to fix the test

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 12, 2017

Member

All green now 👍

Member

thaJeztah commented Dec 12, 2017

All green now 👍

@thaJeztah thaJeztah merged commit e48e938 into moby:master Dec 12, 2017

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38346 has succeeded
Details
janky Jenkins build Docker-PRs 47085 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7462 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 4028 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18605 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7287 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment