Skip to content
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

service logs #28089

Merged
merged 4 commits into from Nov 10, 2016
Merged

service logs #28089

merged 4 commits into from Nov 10, 2016

Conversation

@aluzzardi
Copy link
Member

@aluzzardi aluzzardi commented Nov 5, 2016

This PR adds docker service logs support by wiring up the SwarmKit logs plumbing.

This includes:

  • Container Adapter & Controller: To push up the logs from the agent using the internal engine APIs
  • API integration: Exposing logs using a similar API than the regular docker logs
  • CLI integration: docker service logs

I want to validate the API before documenting and adding integration tests, so this PR does not include them and is not in a mergeable state.

/cc @aaronlehmann @stevvooe

if err != nil {
return 0, err
}
if n != len(output) {

This comment has been minimized.

@aaronlehmann

aaronlehmann Nov 5, 2016
Contributor

Write must return a non-nil error if it returns n < len(p)

If lw.w.Write follows this requirement, this check should not be necessary.

return 0, err
}
if n != len(output) {
return 0, err

This comment has been minimized.

@aaronlehmann

aaronlehmann Nov 5, 2016
Contributor

As noted above, this if doesn't seem necessary. But if it's kept, it should return n and a non-nil error.

}
query.Set("tail", options.Tail)

resp, err := cli.get(ctx, "/services/"+serviceID+"/logs", query, nil)

This comment has been minimized.

@aaronlehmann

aaronlehmann Nov 5, 2016
Contributor

Wondering if we should validate client-side that user input only contains valid characters. Passing in a service ID with a ? in it will cause some pretty weird behavior. But this should probably be done in a different PR.

msg.Context.NodeID,
msg.Context.ServiceID,
msg.Context.TaskID,
)), data...)

This comment has been minimized.

@aaronlehmann

aaronlehmann Nov 5, 2016
Contributor

I would reorder these appends to append rather than prepend. This is more intuitive and also slightly more efficient (prepending has to copy the old contents, appending might not if cap(x) > len(x)).

This comment has been minimized.

@stevvooe

stevvooe Nov 8, 2016
Contributor

Actually, it might be more efficient to just flush to the stream, rather than assembling the entire data buffer.

This comment has been minimized.

@aluzzardi

aluzzardi Nov 8, 2016
Author Member

Followed the same pattern as docker logs (reverse append) - I can change that


serviceName := vars["id"]
logsConfig := &backend.ContainerLogsConfig{
ContainerLogsOptions: basictypes.ContainerLogsOptions{

This comment has been minimized.

@stevvooe

stevvooe Nov 8, 2016
Contributor

Do we rename this to LogsOptions?

This comment has been minimized.

@aluzzardi

aluzzardi Nov 10, 2016
Author Member

In a follow up

This comment has been minimized.

@stevvooe

stevvooe Nov 10, 2016
Contributor

Can we add a TODO somewhere?

@aluzzardi
Copy link
Member Author

@aluzzardi aluzzardi commented Nov 8, 2016

@aaronlehmann @stevvooe Actually, the biggest thing I need to solve is the format.

Here's docker logs:

[optional timestamp] [optional details] [line]

Details (docker logs --details) are key values (k1=v1,k2=v2) which I believe are set by the log driver?

In service logs, we have a context (node_id, task_id, etc).

  • Should we support "details" in service logs?
  • Should we inject the context into the details or keep it separate?

Having the context in details makes the service logs protocol 100% compliant with docker logs, although it gets a little messier for parsing: depending whether --timestamp and or --details was passed or not, we have to split the log stream differently. Details contains the context (which we need to resolve to actual service names) and the actual details (which we need to passthrough).

Thoughts?

/cc @tiborvass @icecrime @vieux

Original:
[optional timestamp] [optional details] [line]

Service logs proposals:
1) [context] [optional timestamp] [optional details] [line]
2) [context] [optional timestamp] [line]
3) [optional timestamp] [optional details + context] [line]
4) [optional timestamp] [optional context] [line]
@vieux
Copy link
Contributor

@vieux vieux commented Nov 9, 2016

@aluzzardi I would start by the timestamp, so I vote for 3) or 4) details is nice to have but can come later.

@stevvooe
Copy link
Contributor

@stevvooe stevvooe commented Nov 9, 2016

  1. [optional timestamp] [optional details + context] [line]

I vote for 3.

@aluzzardi
Copy link
Member Author

@aluzzardi aluzzardi commented Nov 9, 2016

Which one though:

[optional(details) + context]
OR
[optional(details + context)]

@dnephin @aanand Any input? The output of service logs basically looks a lot like compose logs.

@stevvooe @vieux The weird part is that context has a special meaning. The CLI will parse out the context to resolve it (map service ID to names and so on)

@stevvooe
Copy link
Contributor

@stevvooe stevvooe commented Nov 9, 2016

[optional(details) + context]

I'm in favor of this, since you need the context to disambiguate the streams.

@vieux
Copy link
Contributor

@vieux vieux commented Nov 9, 2016

@aluzzardi #28088 was merged

@@ -17,6 +19,7 @@ type Backend interface {
CreateService(types.ServiceSpec, string) (string, error)
UpdateService(string, uint64, types.ServiceSpec, string, string) error
RemoveService(string) error
ServiceLogs(context.Context, string, *backend.ContainerLogsConfig, chan struct{}) error

This comment has been minimized.

@dongluochen

dongluochen Nov 9, 2016
Contributor

It'd be better to follow Verb + Noun naming convention. ServiceLogs -> GetServiceLogs?

This comment has been minimized.

@aluzzardi

aluzzardi Nov 10, 2016
Author Member

Staying consistent with ContainerLogs

This comment has been minimized.

@stevvooe

stevvooe Nov 10, 2016
Contributor

It'd be better to follow Verb + Noun naming convention.

This is not a convention in Go.

}

func (lw *logWriter) Write(buf []byte) (int, error) {
parts := bytes.SplitN(buf, []byte(" "), 2)

This comment has been minimized.

@dongluochen

dongluochen Nov 9, 2016
Contributor

Does this mean context cannot have any space?

noResolve bool
follow bool
since string
timestamps bool

This comment has been minimized.

@dongluochen

dongluochen Nov 9, 2016
Contributor

Does logs from different nodes ordered by timestamps by default? As an option?

This comment has been minimized.

@stevvooe

stevvooe Nov 10, 2016
Contributor

No. The resource requirements for doing so are too intensive. In practice, they tend to come in with the correct ordering. To guarantee it, you have to buffer all logs, sort them and merge on client-side. Empty signals also need to be sent to preserve ordering.

@aluzzardi
Copy link
Member Author

@aluzzardi aluzzardi commented Nov 9, 2016

[optional(details) + context]

I'm in favor of this, since you need the context to disambiguate the streams.

Keep in mind that this means:

  1. Logs format is incompatible with service logs (because there's an extra column even if you don't provide --details)
  2. CLI parsing would require to: Extract the "thing" in the middle, parse out the relevant part (node_id and so on) to resolve and display it, and if --details was provided, take what's remaining of that "thing" (minus node_id and so on) and display it

Are we cool with that, @stevvooe @aaronlehmann @vieux @icecrime ?

@dnephin
Copy link
Member

@dnephin dnephin commented Nov 9, 2016

I think the logs are going to be pretty confusing without the task id. I would opt for always including a task id by default (maybe a flag to disable it).

@aluzzardi aluzzardi changed the title [wip] service logs service logs Nov 10, 2016
@aluzzardi
Copy link
Member Author

@aluzzardi aluzzardi commented Nov 10, 2016

PTAL @stevvooe @vieux @stevvooe

There are a few things far from perfect (e.g. the reference docs are mostly copy paste from container logs) - could you please distinguish your comments between fix now and fix later? I'll make a follow-up with all less important fixes

@aluzzardi
Copy link
Member Author

@aluzzardi aluzzardi commented Nov 10, 2016

Stop gaps: non-follow is not supported, details is not supported

@vieux
Copy link
Contributor

@vieux vieux commented Nov 10, 2016

@aluzzardi I get

$ docker service logs pbf1duhxowx0
Error response from daemon: Bad parameters: Only follow mode is currently supported
context[parts[0]] = parts[1]
}

taskID, ok := context["task_id"]

This comment has been minimized.

@stevvooe

stevvooe Nov 10, 2016
Contributor

We should use the convention of task.id we've used in labels. It might also be a good idea to namespace these com.docker.swarm.

@aluzzardi
Copy link
Member Author

@aluzzardi aluzzardi commented Nov 10, 2016

@vieux Yeah, unfortunately, only follow mode (-f) is supported

@vieux
Copy link
Contributor

@vieux vieux commented Nov 10, 2016

my bad @aluzzardi I read non-follow is supported, details is not supported

@stevvooe
Copy link
Contributor

@stevvooe stevvooe commented Nov 10, 2016

LGTM

I think we can tweak the output quite a bit but let's do that in a follow up.

@@ -1121,6 +1130,82 @@ func (c *Cluster) RemoveService(input string) error {
return nil
}

// ServiceLogs collects service logs and writes them back to `config.OutStream`
func (c *Cluster) ServiceLogs(ctx context.Context, input string, config *backend.ContainerLogsConfig, started chan struct{}) error {
if !c.isActiveManager() {

This comment has been minimized.

@tonistiigi

tonistiigi Nov 10, 2016
Member

Need to get a readlock for this. You can release it before the long running operations.

This comment has been minimized.

@aluzzardi

aluzzardi Nov 10, 2016
Author Member

Done. PTAL

This comment has been minimized.

@vieux
Copy link
Contributor

@vieux vieux commented Nov 10, 2016

It would be nice to get it for 1.13, but since it's still missing a few feature and after some discussion among the engine maintainers, we think it's best if you can put this in experimental only @al

aluzzardi added 4 commits Oct 26, 2016
Plumbed the executor to the container logs backend.

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>
Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>
Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>
Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>
@aluzzardi
Copy link
Member Author

@aluzzardi aluzzardi commented Nov 10, 2016

Moved to experimental

@vieux PTAL

@vieux
Copy link
Contributor

@vieux vieux commented Nov 10, 2016

LGTM

@aluzzardi aluzzardi merged commit 5f9fe54 into moby:master Nov 10, 2016
4 checks passed
4 checks passed
@GordonTheTurtle
dco-signed All commits are signed
@GordonTheTurtle
experimental Jenkins build Docker-PRs-experimental 26614 has succeeded
Details
@GordonTheTurtle
janky Jenkins build Docker-PRs 35185 has succeeded
Details
@GordonTheTurtle
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 6098 has succeeded
Details
@aluzzardi aluzzardi deleted the aluzzardi:service-logs branch Nov 10, 2016
@bvis
Copy link

@bvis bvis commented Nov 25, 2016

Hi,

I tried this feature in my staging swarm cluster and it didn't worked as expected. I reproduced the error here: http://play-with-docker.com/p/f0020045-d8e8-4bf0-a86c-55f3a25cd4dd

I left some messages related with this in the swam slack room.

If you want me to create an issue just tell it to me.

@aaronlehmann
Copy link
Contributor

@aaronlehmann aaronlehmann commented Nov 28, 2016

@bvis: I wasn't able to load your link. I get the error message "session not found".

Opening an issue with some details sounds like a good next step.

@bvis
Copy link

@bvis bvis commented Nov 29, 2016

@aaronlehmann Here you are: #28915

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet