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

Add --until flag for docker logs; closes #32807 #32914

Merged
merged 1 commit into from Nov 3, 2017

Conversation

@jamiehannaford
Contributor

jamiehannaford commented Apr 28, 2017

- What I did

Added an --until flag for docker logs in line with #32807.

- How I did it

Adapted API, CLI, Client and Daemon pkgs.

- How to verify it

Run unit/integration tests. One caveat is that we don't have any integration tests for any of the logger adaptors at the moment (e.g. loggerd), so I was not able to verify the changes to this.

- Description for the changelog

docker logs now supports --until

@GordonTheTurtle GordonTheTurtle removed the dco/no label Apr 28, 2017

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Apr 28, 2017

Contributor

@dperny This is RFR :)

BTW I couldn't find integration tests for any of the other logging adapters, such as loggerd. I assume they need different OS environments than debian, which doesn't really jive with the current test setup. Is there any way to reconfigure so I can specify an ubuntu/coreos image for specific tests?

Contributor

jamiehannaford commented Apr 28, 2017

@dperny This is RFR :)

BTW I couldn't find integration tests for any of the other logging adapters, such as loggerd. I assume they need different OS environments than debian, which doesn't really jive with the current test setup. Is there any way to reconfigure so I can specify an ubuntu/coreos image for specific tests?

dperny added a commit to dperny/swarmkit-1 that referenced this pull request Apr 28, 2017

Support until option on logs
Starting with PR moby/moby#32914, container logs supports an until
option, which allows fetching logs up to a certain point in time and not
after. This API change allows that log option to be passed.

Signed-off-by: Drew Erny <drew.erny@docker.com>
@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Apr 28, 2017

Contributor

@jamiehannaford Everything above daemon/logger LGTM, but I am not a maintainer.

The API change for swarmkit would look roughly like this.

Are you willing to do that change as a PR on swarmkit and carry the corresponding update to service logs in this repo? If not, let me know; I can do it.

Contributor

dperny commented Apr 28, 2017

@jamiehannaford Everything above daemon/logger LGTM, but I am not a maintainer.

The API change for swarmkit would look roughly like this.

Are you willing to do that change as a PR on swarmkit and carry the corresponding update to service logs in this repo? If not, let me know; I can do it.

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Apr 28, 2017

Contributor

Also, @jamiehannaford don't forget to update your commits with a --signoff. Gordon's instructions are roughly accurate; just go to your branch and git commit --amend --signoff and force push.

Contributor

dperny commented Apr 28, 2017

Also, @jamiehannaford don't forget to update your commits with a --signoff. Gordon's instructions are roughly accurate; just go to your branch and git commit --amend --signoff and force push.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 1, 2017

Contributor

Design OK for me.

Contributor

cpuguy83 commented May 1, 2017

Design OK for me.

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 2, 2017

Contributor

@dperny Sure, I can do swarmkit. I was thinking of getting this merged first, then adding the protobuf stuff, then adding the logic to this repo.

Contributor

jamiehannaford commented May 2, 2017

@dperny Sure, I can do swarmkit. I was thinking of getting this merged first, then adding the protobuf stuff, then adding the logic to this repo.

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 2, 2017

Contributor

Not sure what the cause is for the Windows error:

Error response from daemon: write \?\d:\temp\docker-builder519745529\vendor\github.com\gogo\protobuf\proto\decode.go: There is not enough space on the disk.

Maybe just a flaky test? Might pass if Jenkins is rekicked.

Contributor

jamiehannaford commented May 2, 2017

Not sure what the cause is for the Windows error:

Error response from daemon: write \?\d:\temp\docker-builder519745529\vendor\github.com\gogo\protobuf\proto\decode.go: There is not enough space on the disk.

Maybe just a flaky test? Might pass if Jenkins is rekicked.

@tophj-ibm

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm May 2, 2017

Contributor

@jamiehannaford the machine the tests ran on was out of space, I'll restart it.

Contributor

tophj-ibm commented May 2, 2017

@jamiehannaford the machine the tests ran on was out of space, I'll restart it.

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 3, 2017

Contributor

Hmm, I'm routing running into flaky tests 🤔 The windows build is complaining about docker_api_exec_test.go:159 and powerpc check_test.go:355: DockerSwarmSuite.TearDownTest.

Is there any way to get around these apart from re-pushing and getting lucky?

Contributor

jamiehannaford commented May 3, 2017

Hmm, I'm routing running into flaky tests 🤔 The windows build is complaining about docker_api_exec_test.go:159 and powerpc check_test.go:355: DockerSwarmSuite.TearDownTest.

Is there any way to get around these apart from re-pushing and getting lucky?

@tophj-ibm

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm May 3, 2017

Contributor

No, but anyone with merge access should be familiar with the flaky tests.

I'll restart them again. It's a pain I know :( (flaky tests are hard)

Contributor

tophj-ibm commented May 3, 2017

No, but anyone with merge access should be familiar with the flaky tests.

I'll restart them again. It's a pain I know :( (flaky tests are hard)

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 3, 2017

Contributor

Looks like everything's passing 🎉

Contributor

jamiehannaford commented May 3, 2017

Looks like everything's passing 🎉

@thaJeztah thaJeztah added the impact/cli label May 5, 2017

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 11, 2017

Contributor

I've removed the changes in the cli folder and opened up a new PR to docker/cli: docker/cli#74

Contributor

jamiehannaford commented May 11, 2017

I've removed the changes in the cli folder and opened up a new PR to docker/cli: docker/cli#74

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester May 15, 2017

Member

Design LGTM, let's move this to code review

Member

vdemeester commented May 15, 2017

Design LGTM, let's move this to code review

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 16, 2017

Contributor

@vdemeester I've added in some API tests, but the Windows build seems to be having some trouble. Would you mind relaunching that test box?

Contributor

jamiehannaford commented May 16, 2017

@vdemeester I've added in some API tests, but the Windows build seems to be having some trouble. Would you mind relaunching that test box?

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester May 16, 2017

Member

@jamiehannaford that's weird indeed. Could you rebase against master to see if it fixes it ? 👼

Member

vdemeester commented May 16, 2017

@jamiehannaford that's weird indeed. Could you rebase against master to see if it fixes it ? 👼

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 17, 2017

Contributor

@vdemeester Hmm, seems to be a known issue. I'm disabling this test on Windows for now and have created an issue to address it.

Contributor

jamiehannaford commented May 17, 2017

@vdemeester Hmm, seems to be a known issue. I'm disabling this test on Windows for now and have created an issue to address it.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2017

Member

@jamiehannaford looks like there's a failure in the test;

12:46:19 ./docker_api_logs_test.go:113: cannot use s (type *DockerSuite) as type *check.C in argument to testRequires
12:46:19 ---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-integration-cli)
Member

thaJeztah commented May 17, 2017

@jamiehannaford looks like there's a failure in the test;

12:46:19 ./docker_api_logs_test.go:113: cannot use s (type *DockerSuite) as type *check.C in argument to testRequires
12:46:19 ---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-integration-cli)
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2017

Member

Tests are green now, @vdemeester @cpuguy83 @dperny PTAL

Member

thaJeztah commented May 17, 2017

Tests are green now, @vdemeester @cpuguy83 @dperny PTAL

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Sep 15, 2017

Contributor

Is it normal to have this many flaky tests? They fail around 80-90% of the time. I'm seeing this recently:

16:08:24 
	Error:      	Received unexpected error:
16:08:24 
	            	Cannot connect to the Docker daemon at unix:///go/src/github.com/docker/docker/bundles/test-integration/docker.sock. Is the docker daemon running?
16:08:24 
	Messages:   	failed to list images
``
Contributor

jamiehannaford commented Sep 15, 2017

Is it normal to have this many flaky tests? They fail around 80-90% of the time. I'm seeing this recently:

16:08:24 
	Error:      	Received unexpected error:
16:08:24 
	            	Cannot connect to the Docker daemon at unix:///go/src/github.com/docker/docker/bundles/test-integration/docker.sock. Is the docker daemon running?
16:08:24 
	Messages:   	failed to list images
``
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 15, 2017

Member

that's not a flaky test , the daemon that was being tested actually died. You can take a look at the daemon.log in https://jenkins.dockerproject.org/job/Docker-PRs/45506/artifact/bundles.tar.gz to see if it has a stack trace or something

Member

dnephin commented Sep 15, 2017

that's not a flaky test , the daemon that was being tested actually died. You can take a look at the daemon.log in https://jenkins.dockerproject.org/job/Docker-PRs/45506/artifact/bundles.tar.gz to see if it has a stack trace or something

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Sep 15, 2017

Contributor

@dnephin This was the stack trace:

time="2017-09-15T08:56:21.806611232Z" level=debug msg="Calling GET /v1.30/containers/parent/logs?stderr=1&stdout=1&tail=all"
time="2017-09-15T08:56:21.806744565Z" level=debug msg="begin logs" container=parent method="(*Daemon).ContainerLogs" module=daemon
time="2017-09-15T08:56:21.807085463Z" level=debug msg="end logs" container=parent method="(*Daemon).ContainerLogs" module=daemon
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x50474c]

goroutine 46288 [running]:
encoding/json.(*Decoder).refill(0xc421a18e00, 0x457ed0, 0xc420faa820)
	/usr/local/go/src/encoding/json/stream.go:152 +0xec
encoding/json.(*Decoder).readValue(0xc421a18e00, 0x0, 0x0, 0xc4216214a0)
	/usr/local/go/src/encoding/json/stream.go:128 +0x1ca
encoding/json.(*Decoder).Decode(0xc421a18e00, 0x1962920, 0xc427f8f340, 0xc4211f3ba8, 0x442fdb)
	/usr/local/go/src/encoding/json/stream.go:57 +0x85
github.com/docker/docker/daemon/logger/jsonfilelog.decodeLogLine(0xc421a18e00, 0xc427f8f340, 0xc4207fd380, 0x200000003, 0xc4207fd380)
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:28 +0xcc
github.com/docker/docker/daemon/logger/jsonfilelog.tailFile(0x282a8a0, 0xc4208ee750, 0xc4208ee690, 0xffffffffffffffff, 0x0, 0xc400000000, 0x0, 0x0, 0x0, 0x0)
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:152 +0xd1
github.com/docker/docker/daemon/logger/jsonfilelog.(*JSONFileLogger).readLogs(0xc421243770, 0xc4208ee690, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xffffffffffffffff, 0x0)
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:101 +0x5ba
created by github.com/docker/docker/daemon/logger/jsonfilelog.(*JSONFileLogger).ReadLogs
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:52 +0x146

Not sure why this has suddenly stopped working. Maybe my latest changeset?

Contributor

jamiehannaford commented Sep 15, 2017

@dnephin This was the stack trace:

time="2017-09-15T08:56:21.806611232Z" level=debug msg="Calling GET /v1.30/containers/parent/logs?stderr=1&stdout=1&tail=all"
time="2017-09-15T08:56:21.806744565Z" level=debug msg="begin logs" container=parent method="(*Daemon).ContainerLogs" module=daemon
time="2017-09-15T08:56:21.807085463Z" level=debug msg="end logs" container=parent method="(*Daemon).ContainerLogs" module=daemon
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x50474c]

goroutine 46288 [running]:
encoding/json.(*Decoder).refill(0xc421a18e00, 0x457ed0, 0xc420faa820)
	/usr/local/go/src/encoding/json/stream.go:152 +0xec
encoding/json.(*Decoder).readValue(0xc421a18e00, 0x0, 0x0, 0xc4216214a0)
	/usr/local/go/src/encoding/json/stream.go:128 +0x1ca
encoding/json.(*Decoder).Decode(0xc421a18e00, 0x1962920, 0xc427f8f340, 0xc4211f3ba8, 0x442fdb)
	/usr/local/go/src/encoding/json/stream.go:57 +0x85
github.com/docker/docker/daemon/logger/jsonfilelog.decodeLogLine(0xc421a18e00, 0xc427f8f340, 0xc4207fd380, 0x200000003, 0xc4207fd380)
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:28 +0xcc
github.com/docker/docker/daemon/logger/jsonfilelog.tailFile(0x282a8a0, 0xc4208ee750, 0xc4208ee690, 0xffffffffffffffff, 0x0, 0xc400000000, 0x0, 0x0, 0x0, 0x0)
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:152 +0xd1
github.com/docker/docker/daemon/logger/jsonfilelog.(*JSONFileLogger).readLogs(0xc421243770, 0xc4208ee690, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xffffffffffffffff, 0x0)
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:101 +0x5ba
created by github.com/docker/docker/daemon/logger/jsonfilelog.(*JSONFileLogger).ReadLogs
	/go/src/github.com/docker/docker/daemon/logger/jsonfilelog/read.go:52 +0x146

Not sure why this has suddenly stopped working. Maybe my latest changeset?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 15, 2017

Member

Yes, this definitely looks related to the change. I would suggest trying to reproduce by writing a unit test for one of the docker/daemon/logger/jsonfilelog functions in the test, which will make it easier to fix

Member

dnephin commented Sep 15, 2017

Yes, this definitely looks related to the change. I would suggest trying to reproduce by writing a unit test for one of the docker/daemon/logger/jsonfilelog functions in the test, which will make it easier to fix

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 21, 2017

Member

ping @jamiehannaford have you had a chance to look at the issue?

Member

thaJeztah commented Sep 21, 2017

ping @jamiehannaford have you had a chance to look at the issue?

@thaJeztah thaJeztah removed this from backlog in maintainers-session Sep 21, 2017

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Sep 21, 2017

Contributor

@thaJeztah Not yet, I'm swamped with other things. I might have some free time next week. Not sure what suddenly caused it to stop working 😐

Contributor

jamiehannaford commented Sep 21, 2017

@thaJeztah Not yet, I'm swamped with other things. I might have some free time next week. Not sure what suddenly caused it to stop working 😐

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 21, 2017

Member

No worries; just checking in 🤗

Member

thaJeztah commented Sep 21, 2017

No worries; just checking in 🤗

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Oct 5, 2017

Contributor

@thaJeztah @cpuguy83 this is finally RFR again 😄 Sorry about the wait!

Contributor

jamiehannaford commented Oct 5, 2017

@thaJeztah @cpuguy83 this is finally RFR again 😄 Sorry about the wait!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 9, 2017

Member

@jamiehannaford thanks! Looking at review comments, this one looks to be unaddressed; #32914 (comment)

@cpuguy83 ^^ that one still need addressing?

Member

thaJeztah commented Oct 9, 2017

@jamiehannaford thanks! Looking at review comments, this one looks to be unaddressed; #32914 (comment)

@cpuguy83 ^^ that one still need addressing?

@cpuguy83

One nit in the code still.
It'd be nice to move this to integration/ instead of integration-cli (which is now frozen), but this PR is pretty old, so I won't be a stickler about it...

Show outdated Hide outdated integration-cli/docker_api_logs_test.go
@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Oct 19, 2017

Contributor

Ping, this is RFR again. I know a lot of folks have been at DockerCon so no rush 😄

Contributor

jamiehannaford commented Oct 19, 2017

Ping, this is RFR again. I know a lot of folks have been at DockerCon so no rush 😄

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford
Contributor

jamiehannaford commented Oct 26, 2017

Show outdated Hide outdated integration-cli/docker_api_logs_test.go
case l := <-chLog:
c.Assert(l.err, checker.IsNil)
i, err := strconv.ParseInt(strings.Split(l.out, " ")[1], 10, 64)
c.Assert(err, checker.IsNil)

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 26, 2017

Contributor

Wouldn't the reader return an EOF at some point?

@cpuguy83

cpuguy83 Oct 26, 2017

Contributor

Wouldn't the reader return an EOF at some point?

This comment has been minimized.

@jamiehannaford

jamiehannaford Oct 26, 2017

Contributor

I assumed not because the for loop would cease before the EOF, but now I'm not too sure. What do you recommend? Explicitly permitting the EOF error?

@jamiehannaford

jamiehannaford Oct 26, 2017

Contributor

I assumed not because the for loop would cease before the EOF, but now I'm not too sure. What do you recommend? Explicitly permitting the EOF error?

This comment has been minimized.

@jamiehannaford

jamiehannaford Oct 26, 2017

Contributor

Also do we need a time.Sleep(1 * time.Second) after L130?

@jamiehannaford

jamiehannaford Oct 26, 2017

Contributor

Also do we need a time.Sleep(1 * time.Second) after L130?

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 26, 2017

Contributor

It may be best to check for EOF in the reader goroutine and just return instead of sending.

We should not need to a sleep.
The buffered reader should block unless there is an EOF in the underlying reader.

@cpuguy83

cpuguy83 Oct 26, 2017

Contributor

It may be best to check for EOF in the reader goroutine and just return instead of sending.

We should not need to a sleep.
The buffered reader should block unless there is an EOF in the underlying reader.

Add --until flag for docker logs; closes #32807
Signed-off-by: Jamie Hannaford <jamie.hannaford@rackspace.com>
@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Nov 1, 2017

Contributor

@cpuguy83 @thaJeztah tests are passing now, PTAL. Does everything look good?

Contributor

jamiehannaford commented Nov 1, 2017

@cpuguy83 @thaJeztah tests are passing now, PTAL. Does everything look good?

@cpuguy83

LGTM

@thaJeztah

LGTM! Sorry, I thought I changed my review already 😄

@thaJeztah thaJeztah merged commit 68a4552 into moby:master Nov 3, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37627 has succeeded
Details
janky Jenkins build Docker-PRs 46327 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6735 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17897 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6543 has succeeded
Details
@@ -23,6 +23,7 @@ keywords: "API, Docker, rcli, REST, documentation"
If `Error` is `null`, container removal has succeeded, otherwise
the test of an error message indicating why container removal has failed
is available from `Error.Message` field.
* `GET /containers/(name)/logs` now supports an additional query parameter: `until`, which returns log lines that occurred before the specified timestamp.

This comment has been minimized.

@thaJeztah

thaJeztah Nov 3, 2017

Member

DOH! Forgot about this again; this should now be under v1.35; I'll do a quick follow up 😅

@thaJeztah

thaJeztah Nov 3, 2017

Member

DOH! Forgot about this again; this should now be under v1.35; I'll do a quick follow up 😅

This comment has been minimized.

@thaJeztah

thaJeztah Nov 3, 2017

Member

opened #35398 for that

@thaJeztah

thaJeztah Nov 3, 2017

Member

opened #35398 for that

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford Nov 11, 2017

Contributor

@thaJeztah Any chance this can make it in for the v17.11.0-ce docker-ce release?

Contributor

jamiehannaford commented Nov 11, 2017

@thaJeztah Any chance this can make it in for the v17.11.0-ce docker-ce release?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 14, 2017

Member

@jamiehannaford unfortunately, no, the 17.11 release was cut-off already, and release-candidates are already released. 17.12 should not be far out though

Member

thaJeztah commented Nov 14, 2017

@jamiehannaford unfortunately, no, the 17.11 release was cut-off already, and release-candidates are already released. 17.12 should not be far out though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment