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 Support for Service Task Logs #32015
Conversation
7fc1b96
to
7264087
Compare
related: #29307 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will task ids and service ids ever overlap?
Could we just accept all the ids without adding a new flag?
r.Form.Add("tasks", vars["id"]) | ||
// TODO(dperny) i might should delete the id from vars before handling | ||
// this request? but i don't think it matters | ||
return sr.getSwarmLogs(ctx, w, r, vars) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be passed to getSwarmLogs()
with a LogSelector{}
param instead of using forms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(what i mean is getSwarmLogs()
can be used verbatim as a handler)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should still be pretty easy to support such a handler, without having to modify the request parameters in the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
r, w := io.Pipe() | ||
cmd.Stdout = w | ||
cmd.Stderr = w | ||
c.Assert(cmd.Start(), checker.IsNil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, chief.
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 I'm going to alter the CLI to have the pattern |
7264087
to
c2b5057
Compare
Fixed the CLI UI. Fixed the tests. |
LGTM |
c2b5057
to
0635a57
Compare
@dnephin Moved service log handling to a selector as suggested. |
daemon/cluster/services.go
Outdated
c.mu.RUnlock() | ||
return err | ||
// if ANY tasks use a TTY, don't mux streams | ||
var TTY bool |
There was a problem hiding this comment.
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
daemon/cluster/services.go
Outdated
if err != nil { | ||
return err | ||
} | ||
// TODO(dperny) not sure we need this check? |
There was a problem hiding this comment.
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
daemon/cluster/services.go
Outdated
if err != nil { | ||
return err | ||
} | ||
// TODO(dperny) same as above |
There was a problem hiding this comment.
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
Minor comments, overall LGTM |
0635a57
to
d82ae95
Compare
@aluzzardi fixed nits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM (design wise)
/CC @cpuguy83 @thaJeztah @mlaventure
cli/command/service/logs.go
Outdated
@@ -70,28 +75,45 @@ func runLogs(dockerCli *command.DockerCli, opts *logsOptions) error { | |||
Tail: opts.tail, | |||
} | |||
|
|||
client := dockerCli.Client() | |||
// this variable was original called "client" which conflicts with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can go 👼 (and I agree with the change 😛)
cli/command/service/logs.go
Outdated
if !client.IsErrServiceNotFound(err) { | ||
return err | ||
} | ||
_, _, err := cli.TaskInspectWithRaw(ctx, opts.target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this err un-useful at all ? (and thus the call as well)
@dperny missing an error check here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I totally dropped an error check, good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look addressed but it has been below. I moved the logs call to after the error check.
d82ae95
to
ea442cf
Compare
@vdemeester fixed. |
@dperny can you update the documentation ? (at least the API doc to add the endpoint) |
I would like to do so but I am unclear on how to go about it. |
you need to edit |
ea442cf
to
0765e98
Compare
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. |
api/swagger.yaml
Outdated
get: | ||
summary: "Get task logs" | ||
description: | | ||
Get `stdout` and `stderr` logs from a service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service > task
api/swagger.yaml
Outdated
description: | | ||
Get `stdout` and `stderr` logs from a service. | ||
|
||
**Note**: This endpoint works only for services with the `json-file` or `journald` logging drivers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service > task
api/swagger.yaml
Outdated
schema: | ||
type: "string" | ||
404: | ||
description: "no such container" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably be task ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
0765e98
to
c5d1179
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐸
/cc @thaJeztah @dnephin
daemon/cluster/services.go
Outdated
return errors.New("logs only supported on container tasks") | ||
} | ||
tty = tty || c.TTY | ||
swarmSelector.TaskIDs = append(swarmSelector.TaskIDs, task.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactoring in #32154 substantially shortens this function as well, but I'm going to make this change right now anyway.
name := "TestServicelogsTaskLogs" | ||
replicas := 2 | ||
|
||
out, err := d.Cmd( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"busybox", "sh", "-c", "for line in $(seq 0 5); do echo $TASK log test $line; done; sleep 100000", | ||
) | ||
c.Assert(err, checker.IsNil) | ||
c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// get the task ids | ||
out, err = d.Cmd("service", "ps", "-q", name) | ||
c.Assert(err, checker.IsNil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above (d.Command(...)
)
for _, taskID := range taskIDs { | ||
c.Logf("checking task %v", taskID) | ||
out, err := d.Cmd("service", "logs", taskID) | ||
c.Assert(err, checker.IsNil, check.Commentf("output: %v", out)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above (d.Command(...)
)
cli/command/service/logs.go
Outdated
} | ||
|
||
taskFormatter := newTaskFormatter(client, opts, padding) | ||
taskFormatter := newTaskFormatter(cli, opts, padding) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I've fixed it correctly.
a01f028
to
0ea9867
Compare
@dnephin fixed concerns. |
0ea9867
to
0131057
Compare
@dnephin actually fixed all the concerns the right way this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@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>
0131057
to
d330dc3
Compare
Forgot to strip a trailing newline, it's fixed now. |
LGTM 👍 |
ping @dperny erm;
Looks like the 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 ? |
return err | ||
} | ||
maxLength = getMaxLength(task.Slot) | ||
responseBody, err = cli.TaskLogs(ctx, opts.target, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hmmmm I hadn't thought about that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@thaJeztah Yes, the |
Thanks! I was confused for a bit, couldn't find the flag LOL |
Add Support for Service Task Logs
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 todocker 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