Add Support for Service Task Logs by dperny · Pull Request #32015 · moby/moby

dperny · 2017-03-22T20:11:16Z

This PR probably needs a docs update too. I'm gonna wait until it passes design review before I update the docs

- What I did
Refactored the API to more easily accept new endpoints. Added REST, client, and CLI endpoints for getting logs from a specific task. All that is needed after this commit to enable arbitrary service log selectors is a REST endpoint.

~~Added a --task flag to docker service logs to get logs for a specific task.~~

Added search by task if a service wasn't found.

- How I did it
Refactored a bunch of code, added a couple of new endpoints.

- How to verify it
Added a new integration test. You can also try running docker service logs --task [taskid].

- Description for the changelog
docker service logs command now also gets task logs. Added /task/{id}/logs REST endpoint.

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

/cc @aluzzardi

AkihiroSuda · 2017-03-23T01:49:14Z

related: #29307

cc @stevvooe @aluzzardi @dnephin

dnephin

Will task ids and service ids ever overlap?

Could we just accept all the ids without adding a new flag?

dnephin · 2017-03-23T19:26:46Z

api/server/router/swarm/cluster_routes.go

Could this be passed to getSwarmLogs() with a LogSelector{} param instead of using forms?

if we leave getSwarmLogs as is, with no LogSelector parameter, then later on a route for arbitrary combinations of service and task logs (for example, maybe stack logs?) can be easily constructed. in fact, on local, i had such a route, that looked somewhat like /swarm/logs?service=foo&service=bar&task=wombo and returned a log stream for whatever selectors.

(what i mean is getSwarmLogs() can be used verbatim as a handler)

I think it should still be pretty easy to support such a handler, without having to modify the request parameters in the current implementation.

Ok, then my second question (for you and anyone else) is where is the best place to put the getSwarmLogs function, if it does not conform to the Handler function type? Can I leave it in the cluster_routes.go file? I believe explicitly should NOT go in the daemon, I think the pattern of having the daemon construct the logs byte stream response is Bad and i'm addressing that in a separate PR.

dnephin · 2017-03-23T19:42:23Z

integration-cli/docker_cli_service_logs_experimental_test.go

Please use github.com/docker/docker/pkg/testutil/cmd to run commands in tests. It helps ensure that we have consistent (and sufficiently detailed) error messages when things fail, and it should be a lot less verbose.

Will do, chief.

dperny · 2017-03-23T21:18:38Z

I'm not sure the right options for the CLI UI. What I've presented is my best effort as to what I think is correct. Ideally I'd like to support any arbitrary combination of task/service/node logs to reflect what the swarmkit GRPC can do with log selectors. However, that concept DOES NOT map to the existing REST API paradigm, and, by extension, to the CLI.

Someone is going to come in here, like the last PR about this, and suggest that we do like filters or whatever and I agree with you but I'm really boxed in by the REST paradigm, which is not conducive to arbitrary selectors like GRPC API is. We'd need an extra API endpoint that does have the format /service/{id}/logs, like maybe /swarm/logs?service=wombo&task=combo which DOES map to the LogSelector. That's part of what I've enabled by making that getSwarmLogs() handler like it is; we can just add a new route when we decide what we want that route should be named.

I'm going to alter the CLI to have the pattern docker service logs SERVICE|TASK, no flags. If you want something better, fight me with your PRs. Both fox no items final destination.

dperny · 2017-03-23T23:09:01Z

Fixed the CLI UI. Fixed the tests.

cowlicks · 2017-03-25T04:43:26Z

LGTM

dperny · 2017-03-30T21:30:51Z

@dnephin Moved service log handling to a selector as suggested.

aluzzardi · 2017-03-30T21:35:01Z

daemon/cluster/services.go

nit: This should be tty

aluzzardi · 2017-03-30T21:35:20Z

daemon/cluster/services.go

Yes, we do! Good catch

aluzzardi · 2017-03-30T21:35:43Z

daemon/cluster/services.go

Same as above as well - we need this check

aluzzardi · 2017-03-30T21:36:33Z

Minor comments, overall LGTM

dperny · 2017-03-30T23:57:20Z

@aluzzardi fixed nits

vdemeester

SGTM (design wise)
/CC @cpuguy83 @thaJeztah @mlaventure

vdemeester · 2017-03-31T13:15:16Z

cli/command/service/logs.go

This comment can go 👼 (and I agree with the change 😛)

vdemeester · 2017-03-31T13:16:40Z

cli/command/service/logs.go

Isn't this err un-useful at all ? (and thus the call as well)
@dperny missing an error check here ?

Yeah I totally dropped an error check, good catch.

This doesn't look addressed but it has been below. I moved the logs call to after the error check.

dperny · 2017-03-31T20:19:56Z

@vdemeester fixed.

vieux · 2017-03-31T23:08:54Z

@dperny can you update the documentation ? (at least the API doc to add the endpoint)

dperny · 2017-03-31T23:17:38Z

I would like to do so but I am unclear on how to go about it.

vieux · 2017-03-31T23:19:42Z

you need to edit api/swagger.yaml you can look at service logs for inspiration :P

dperny · 2017-03-31T23:32:37Z

updated api/swagger.yml to reflect task logs. The updated API documentation may not be totally correct (it is copied from service logs, which is also not totally correct for pre-existing reasons) and will need to be updated again before service logs are moved out of experimental.

vieux · 2017-03-31T23:33:24Z

api/swagger.yaml

service > task

vieux · 2017-03-31T23:33:35Z

api/swagger.yaml

service > task

vieux · 2017-03-31T23:34:11Z

api/swagger.yaml

should probably be task ?

i'm correcting it but it's also wrong in the docs for service. i need to, in a separate PR, fix the problems with all the API documentation.

vdemeester

LGTM 🐸
/cc @thaJeztah @dnephin

dnephin · 2017-04-03T16:21:01Z

daemon/cluster/services.go

Could this be a new function convertSelector() ? The existing function is already way too long, so if we could avoid adding more code to it, that would be awesome.

The refactoring in #32154 substantially shortens this function as well, but I'm going to make this change right now anyway.

dnephin · 2017-04-03T16:24:09Z

integration-cli/docker_cli_service_logs_experimental_test.go

result := d.Command(...) result.Assert(c, icmd.Expected{ Out: "<something you expect to see in stdout>", })

will give much better error messages when there are failures here.

Doesn't exactly work in this place b/c I'm expecting a service ID, which I can't do a simple string match on. But I understand the gist of what you're saying, I'll fix it.

dnephin · 2017-04-03T16:28:53Z

integration-cli/docker_cli_service_logs_experimental_test.go

I think this could easily pass when you don't expect it to. out is actually Combined(), so any error message would allow this to pass. Using icmd.Expected{Out: ...} is a much stricter assertion.

dnephin · 2017-04-03T16:29:20Z

integration-cli/docker_cli_service_logs_experimental_test.go

same as above (d.Command(...))

dnephin · 2017-04-03T16:29:38Z

integration-cli/docker_cli_service_logs_experimental_test.go

same as above (d.Command(...))

dnephin · 2017-04-03T16:41:20Z

cli/command/service/logs.go

I believe this will panic in the formatter if passed a task id where the task slot > 9 because strings.Repeat() will be called with a negative number. padding needs to be set to something reasonable for the tasks case.

padding is a bad name for this variable. It should probably be changed to something like maxLength.

You're right, it does panic. This whole part of the CLI is kind of a mess and I'm going to refactor it later. For now, I'm going to add a check as a bit of a kludge.

Since you have task in the branch above (returned from TaskInspectWithRaw()), couldn't you set padding to

padding = len(strconv.FormatInt(int64(task.Slot), 10))

which would now be duplicated with the formatting below. but could be extracted to a function.

You're right, I've fixed it correctly.

dperny · 2017-04-03T19:51:00Z

@dnephin fixed concerns.

dperny · 2017-04-03T21:33:40Z

@dnephin actually fixed all the concerns the right way this time.

dnephin

LGTM!

vieux · 2017-04-04T00:47:54Z

@dperny I think the error is legit

Refactored the API to more easily accept new endpoints. Added REST, client, and CLI endpoints for getting logs from a specific task. All that is needed after this commit to enable arbitrary service log selectors is a REST endpoint and handler. Task logs can be retrieved by putting in a task ID at the CLI instead of a service ID. Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny · 2017-04-04T01:42:07Z

Forgot to strip a trailing newline, it's fixed now.

vieux · 2017-04-04T07:15:10Z

LGTM 👍

thaJeztah · 2017-04-04T09:56:00Z

ping @dperny erm;

Added docker service logs --task command and /task/{id}/logs REST endpoint.

Looks like the --task flag is not part of this PR, correct?

Also, can you open a pull request to add a mention of this endpoint to the API changelog; https://github.com/docker/docker/blob/master/docs/api/version-history.md#v129-api-changes ?

thaJeztah · 2017-04-04T10:13:06Z

cli/command/service/logs.go

+			return err
+		}
+		maxLength = getMaxLength(task.Slot)
+		responseBody, err = cli.TaskLogs(ctx, opts.target, options)


Do we have to make this API-version dependent? Or is it ok to have this fail when talking to an older daemon (given that it was "experimental" so far?

Oh hmmmm I hadn't thought about that...

I may be mistaken but I BELIEVE if you talk to an older daemon, it's going to fail in the same way it would if I patched this, just with a less helpful error message.

Well, it's still experimental, so "ok" to break, but what happens with a 17.03 / 17.04 client talking to a 17.05 daemon?

dperny · 2017-04-04T16:24:30Z

@thaJeztah Yes, the --task part of this was dropped. Sorry about that. I'll edit the original PR message.

thaJeztah · 2017-04-04T16:28:32Z

Yes, the --task part of this was dropped. Sorry about that. I'll edit the original PR message.

Thanks! I was confused for a bit, couldn't find the flag LOL

Add Support for Service Task Logs

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

dperny force-pushed the service-logs-support-task-logs branch 2 times, most recently from 7fc1b96 to 7264087 Compare March 22, 2017 20:32

vdemeester added status/1-design-review and removed status/0-triage labels Mar 22, 2017

dnephin reviewed Mar 23, 2017

View reviewed changes

dperny force-pushed the service-logs-support-task-logs branch from 7264087 to c2b5057 Compare March 23, 2017 23:08

dperny mentioned this pull request Mar 28, 2017

Refactor logs and support service logs with TTY #32154

Merged

dperny force-pushed the service-logs-support-task-logs branch from c2b5057 to 0635a57 Compare March 30, 2017 21:08

aluzzardi reviewed Mar 30, 2017

View reviewed changes

daemon/cluster/services.go Outdated

Copy link

Member

aluzzardi Mar 30, 2017

Choose a reason for hiding this comment

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

nit: This should be tty

aluzzardi reviewed Mar 30, 2017

View reviewed changes

daemon/cluster/services.go Outdated

Copy link

Member

aluzzardi Mar 30, 2017

Choose a reason for hiding this comment

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

Yes, we do! Good catch

aluzzardi reviewed Mar 30, 2017

View reviewed changes

daemon/cluster/services.go Outdated

Copy link

Member

aluzzardi Mar 30, 2017

Choose a reason for hiding this comment

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

Same as above as well - we need this check

dperny force-pushed the service-logs-support-task-logs branch from 0635a57 to d82ae95 Compare March 30, 2017 23:48

vdemeester suggested changes Mar 31, 2017

View reviewed changes

dperny force-pushed the service-logs-support-task-logs branch from d82ae95 to ea442cf Compare March 31, 2017 20:19

dperny force-pushed the service-logs-support-task-logs branch from ea442cf to 0765e98 Compare March 31, 2017 23:31

vieux reviewed Mar 31, 2017

View reviewed changes

dperny force-pushed the service-logs-support-task-logs branch from 0765e98 to c5d1179 Compare March 31, 2017 23:41

vdemeester approved these changes Apr 3, 2017

View reviewed changes

vdemeester added status/2-code-review rebuild/janky and removed status/1-design-review labels Apr 3, 2017

GordonTheTurtle removed the rebuild/janky label Apr 3, 2017

dnephin reviewed Apr 3, 2017

View reviewed changes

dperny force-pushed the service-logs-support-task-logs branch 2 times, most recently from a01f028 to 0ea9867 Compare April 3, 2017 19:50

dperny force-pushed the service-logs-support-task-logs branch from 0ea9867 to 0131057 Compare April 3, 2017 21:32

dnephin approved these changes Apr 3, 2017

View reviewed changes

dperny force-pushed the service-logs-support-task-logs branch from 0131057 to d330dc3 Compare April 4, 2017 01:41

vieux merged commit 170be9c into moby:master Apr 4, 2017

GordonTheTurtle added this to the 17.05.0 milestone Apr 4, 2017

thaJeztah added impact/api impact/changelog impact/cli kind/experimental labels Apr 4, 2017

thaJeztah reviewed Apr 4, 2017

View reviewed changes

thaJeztah removed the impact/cli label Apr 4, 2017

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

Merge pull request moby#32015 from dperny/service-logs-support-task-logs

4284efd

Add Support for Service Task Logs

thaJeztah mentioned this pull request May 2, 2017

service logs: Support for individual tasks #29223

Closed

Conversation

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

Uh oh!

Uh oh!

AkihiroSuda commented Mar 23, 2017

Uh oh!

dnephin left a comment

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!

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!

dperny commented Mar 23, 2017

Uh oh!

dperny commented Mar 23, 2017

Uh oh!

cowlicks commented Mar 25, 2017

Uh oh!

dperny commented Mar 30, 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!

aluzzardi commented Mar 30, 2017

Uh oh!

dperny commented Mar 30, 2017

Uh oh!

vdemeester left a comment

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!

dperny commented Mar 31, 2017

Uh oh!

vieux commented Mar 31, 2017

Uh oh!

dperny commented Mar 31, 2017

Uh oh!

vieux commented Mar 31, 2017

Uh oh!

dperny commented Mar 31, 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!

vdemeester left a comment

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

dperny commented Mar 22, 2017 •

edited

Loading