Add tail and since to service logs by dperny · Pull Request #31500 · moby/moby

dperny · 2017-03-02T22:24:41Z

- What I did
This change adds the ability to do --tail and --since on docker service logs.

Should also fix #31307.

Part of a series of changes in #31399.

- How I did it
It wires up the API endpoints to each other and fixes some older bugs.

- How to verify it
Make a service, note how passing --tail and --since

- Description for the changelog

docker service logs now supports since and tail options.

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

aaronlehmann · 2017-03-02T22:35:51Z

daemon/cluster/services.go

Yeah, there should be a different message for parse errors and negative values.

I submit a CFB: Call For Bikeshedding.

(I'm sorry, I mean it is legitimately a bad error message and I do need help getting it better, and it would benefit from some light bikeshedding to get right)

My suggestion:

if err != nil { return errors.New(`Tail must be a non-negative integer, or "all"`) } if t < 0 { return errors.New("negative Tail values are not supported") }

(I don't actually think the current log message is bad for the second case, but I have a slight preference for talking about the domain of the parameter, instead of what a negative parameter would do if it was supported.)

aaronlehmann · 2017-03-02T22:42:14Z

Can you please add tests for these features?

dperny · 2017-03-02T22:52:40Z

I'm finally remembering to run make test before I push but now the hurdle is remembering to actually write new tests.

I'll get right on it @aaronlehmann .

dperny · 2017-03-03T23:49:05Z

pushed up new changes. tests are gonna fail, this requires a revendor from swarmkit master, which someone mentioned is currently in a different PR

aaronlehmann · 2017-03-04T00:19:53Z

integration-cli/docker_cli_service_logs_experimental_test.go

nit: how about $(seq 1 6)

fixed on local

aaronlehmann · 2017-03-04T00:20:01Z

integration-cli/docker_cli_service_logs_experimental_test.go

nit: how about $(seq 1 6)

aaronlehmann · 2017-03-04T00:22:49Z

integration-cli/docker_cli_service_logs_experimental_test.go

Non-blocking: Seems like the sleep could be a lot shorter, since we're parsing the timestamps later on as an argument to --since, instead of fudging it.

fixed this sleep to be shorter.

aaronlehmann · 2017-03-04T00:23:22Z

integration-cli/docker_cli_service_logs_experimental_test.go

Non-blocking: might be better to use waitAndAssert to wait for docker service logs to return the full set of logs.

waitAndAssert is undocumented, headache-causing voodoo magic and I was trying to avoid it if at all possible.

aaronlehmann · 2017-03-04T00:28:16Z

integration-cli/docker_cli_service_logs_experimental_test.go

I think this could be racy. If service logs runs before the container finishes its output, the test might fail. Should we wait for the task to finish running instead of waiting for it to start running?

you're almost certainly correct, and the possibility of a race also exists in the since test as well, as I've just realized (sleeping does not guarantee that anything else has happened on the system in the meantime.)

aaronlehmann · 2017-03-04T00:28:40Z

integration-cli/docker_cli_service_logs_experimental_test.go

Same comment as https://github.com/docker/docker/pull/31500/files#r104269245

thaJeztah · 2017-03-05T22:58:32Z

This is not introducing API changes (i.e. the API already supported this, and is documented?)

dperny · 2017-03-06T18:03:52Z

@thaJeztah I'm not sure if this is documented or not, but the API endpoints and CLI flags all existed, they just weren't wired up.

thaJeztah · 2017-03-06T20:34:30Z

It's probably here; https://github.com/docker/docker/blob/f9bd8ec8b268581f93095c5a80679f0a8ff498bf/api/swagger.yaml#L7646-L7726, but check if it's all documented.

Given that it's been an experimental feature, if we expect this feature to leave experimental in docker 17.04, it should be mentioned in the API changes for API 1.27 https://github.com/docker/docker/blob/3a88a24d23e6eb1ca521cd9ab6e306d4ba1c1464/docs/api/version-history.md#v127-api-changes (or 1.28 if it's gonna leave experimental in 17.05

dperny · 2017-03-07T19:45:58Z

Right now there is a flaky test in my local branch that I'm trying to sort out before I finish this PR.

dperny · 2017-03-07T21:54:54Z

Fixed the flaky integration test.

dperny · 2017-03-08T00:05:22Z

There probably needs to be a revendoring for the integration test to pass

dperny · 2017-03-08T00:10:01Z

This definitely need revendoring for the integration tests to pass. There is a separate open PR for revendoring swarmkit.

thaJeztah · 2017-03-08T18:45:14Z

@dperny #31535 (comment) was merged

dperny · 2017-03-08T21:18:01Z

Rebasing, running tests, and pushing now.

aluzzardi · 2017-03-10T01:12:50Z

Tentatively targeting 17.04.0

aluzzardi · 2017-03-10T22:23:36Z

@dperny I see a few of @aaronlehmann's comments not being addressed - could you double check?

dperny · 2017-03-10T22:32:54Z

His comments don't appear addressed because the diff for those particular lines is the same; the comments have been addressed after those lines.

aaronlehmann · 2017-03-10T22:35:59Z

daemon/cluster/services.go

non-blocking: maybe use errors.Wrap to add some context to the error.

aaronlehmann · 2017-03-10T22:36:02Z

daemon/cluster/services.go

non-blocking: maybe use errors.Wrap to add some context to the error.

aaronlehmann · 2017-03-10T22:39:26Z

integration-cli/docker_cli_service_logs_experimental_test.go

As far as I understand, yes. There's a stream from the engine to the manager, and a stream from the manager to the client. While lines from different nodes could get interleaved, I can't think of any way that log lines from a single node would change their order.

Someone, I cannot remember who, has mentioned that they have seen engine-side logs come back in the wrong order. I don't have proof, this is something like a third-hand account, i'm not sure if it's a bug or a consequence of some technical limitation, and i'm not even sure it's real. would it be alright for me to leave these TODOs in, in case the tests fail later, so someone might know why?

i've reworded the TODOs into regular comments.

aaronlehmann · 2017-03-10T22:40:21Z

LGTM

aaronlehmann · 2017-03-10T22:48:05Z

integration-cli/docker_cli_service_logs_experimental_test.go

Should be TestServiceLogsCompleteness

Comment there looks good, though

This change adds the ability to do --tail and --since on docker service logs. It wires up the API endpoints to each other and fixes some older bugs. It adds integration tests for these new features. Signed-off-by: Drew Erny <drew.erny@docker.com>

thaJeztah

LGTM

vdemeester

LGTM 🐸

Add tail and since to service logs

GordonTheTurtle added the status/0-triage label Mar 2, 2017

dperny mentioned this pull request Mar 2, 2017

Fix service logs since option moby/swarmkit#2004

Merged

aaronlehmann reviewed Mar 2, 2017

View reviewed changes

dperny force-pushed the fix-service-logs-cli branch from ce43475 to c35a984 Compare March 3, 2017 23:48

aaronlehmann reviewed Mar 4, 2017

View reviewed changes

integration-cli/docker_cli_service_logs_experimental_test.go Outdated

Copy link

aaronlehmann Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: how about $(seq 1 6)

aaronlehmann reviewed Mar 4, 2017

View reviewed changes

thaJeztah added impact/changelog impact/cli status/2-code-review and removed status/0-triage labels Mar 5, 2017

dperny force-pushed the fix-service-logs-cli branch from c35a984 to 4481bba Compare March 7, 2017 21:54

dperny mentioned this pull request Mar 7, 2017

Make Docker Service Logs GA #31399

Closed

12 tasks

dperny mentioned this pull request Mar 8, 2017

Vendor swarmkit d60ccf3 #31535

Merged

dperny force-pushed the fix-service-logs-cli branch from 4481bba to fe9e67d Compare March 8, 2017 21:50

aaronlehmann added the rebuild/windowsRS1 label Mar 9, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Mar 9, 2017

aluzzardi added the priority/P1 Important: P1 issues are a top priority and a must-have for the next release. label Mar 10, 2017

aluzzardi added this to the 17.04.0 milestone Mar 10, 2017

aaronlehmann reviewed Mar 10, 2017

View reviewed changes

daemon/cluster/services.go Outdated

Copy link

aaronlehmann Mar 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking: maybe use errors.Wrap to add some context to the error.

aaronlehmann reviewed Mar 10, 2017

View reviewed changes

daemon/cluster/services.go Outdated

Copy link

aaronlehmann Mar 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking: maybe use errors.Wrap to add some context to the error.

aaronlehmann reviewed Mar 10, 2017

View reviewed changes

dperny force-pushed the fix-service-logs-cli branch from fe9e67d to 6860fe5 Compare March 10, 2017 22:46

aaronlehmann reviewed Mar 10, 2017

View reviewed changes

Add tail and since to service logs

8dc437b

This change adds the ability to do --tail and --since on docker service logs. It wires up the API endpoints to each other and fixes some older bugs. It adds integration tests for these new features. Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny force-pushed the fix-service-logs-cli branch from 6860fe5 to 8dc437b Compare March 10, 2017 22:59

thaJeztah approved these changes Mar 13, 2017

View reviewed changes

thaJeztah added status/4-merge rebuild/experimental and removed status/2-code-review labels Mar 13, 2017

GordonTheTurtle removed the rebuild/experimental label Mar 13, 2017

thaJeztah added rebuild/powerpc labels Mar 13, 2017

GordonTheTurtle removed rebuild/powerpc labels Mar 13, 2017

vdemeester approved these changes Mar 14, 2017

View reviewed changes

vdemeester added the rebuild/experimental label Mar 14, 2017

GordonTheTurtle removed the rebuild/experimental label Mar 14, 2017

vdemeester merged commit 1d46080 into moby:master Mar 14, 2017

thaJeztah mentioned this pull request Apr 4, 2017

Docker Service Logs --tail and --since Not Working as Expected #32331

Open

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#31500 from dperny/fix-service-logs-cli

7ce0cb0

Add tail and since to service logs

Conversation

dperny commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 2, 2017

Uh oh!

dperny commented Mar 2, 2017

Uh oh!

dperny commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 5, 2017

Uh oh!

dperny commented Mar 6, 2017

Uh oh!

thaJeztah commented Mar 6, 2017

Uh oh!

dperny commented Mar 7, 2017

Uh oh!

dperny commented Mar 7, 2017

Uh oh!

dperny commented Mar 8, 2017

Uh oh!

dperny commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 8, 2017

Uh oh!

dperny commented Mar 8, 2017

Uh oh!

aluzzardi commented Mar 10, 2017

Uh oh!

aluzzardi commented Mar 10, 2017

Uh oh!

dperny commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

aaronlehmann Mar 4, 2017 •

edited

Loading

dperny commented Mar 8, 2017 •

edited

Loading

dperny commented Mar 10, 2017 •

edited

Loading