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

WIP: make it so that journald logs are ascribed to containers #247

Open
wants to merge 3 commits into
base: docker-1.13.1
Choose a base branch
from

Conversation

nalind
Copy link

@nalind nalind commented May 10, 2017

Looking for feedback on this.

Currently, when we log container output to the journal, dockerd is doing the work of forwarding each piece of output to journald. For every piece of data that journald receives over its native socket, it looks up the unit of the process that sent that piece of data, and applies rate limiting logic to all of the data it receives from that unit. As a result, one container spewing massive numbers of log messages can effectively cause another container's output to be squelched due to rate limiting.

This PR modifies dockerd's journald logging driver to add a new process to each container's scope. That process's sole job is to receive messages over a pipe and forward them to the journal. The dockerd journald logging driver starts that process along with the container, feeds it each message, and stops it along with the container.

In my limited testing with journalctl -o export after this is applied, the journal is attributing log messages to the container's scope, so journalctl -u docker-$CONTAINERID.scope should also produce the container's logs.

When we start a container, stash a copy of the runtime spec that we
generated for it in the container object.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Make logger.Context into an object that also carries the cgroup path
of the container for which it's logging, and initialize the field when
we start a container.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
@nalind
Copy link
Author

nalind commented May 10, 2017

@rhatdan
Copy link
Member

rhatdan commented May 11, 2017

@vbatts @runcom @mrunalp PTAL

@nalind
Copy link
Author

nalind commented May 11, 2017

Hmm, we don't have the "unified" controller on RHEL 7, and I suppose it's better to try to continue after failing to join the container's cgroup than to just exit and let the daemon log pipe-closed errors each time it tries to log some output.

@nalind
Copy link
Author

nalind commented May 11, 2017

Factored the logging of error messages in the helper, and we're now joining the container's cgroup in all available controllers.

@rh-atomic-bot
Copy link

@rh-atomic-bot
Copy link

@nalind
Copy link
Author

nalind commented May 11, 2017

Hmm, for some reason I'm seeing device-busy errors after exiting the container, which suggests that the log helper is keeping the container's filesystem busy, which is rather unexpected.

@rh-atomic-bot
Copy link

@nalind
Copy link
Author

nalind commented May 11, 2017

Scratch that, I'm seeing that with json-file as well, so it's probably not a bug that's being introduced here.

Previously, dockerd just relayed messages by itself from containers to
the journal, which caused journald to apply rate limiting to messages
across all containers as a single group.

Here, we add another process to each container's cgroup, and have
dockerd forward messages to that process over a pipe.  That process,
named "journal-logger", receives the messages and sends them on to the
journal.

As part of the container's cgroup, it's killed when the main process
exits, so we only need to close the pipe and read its exit status when
closing the logger.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
@rh-atomic-bot
Copy link

// to assume that we know how to compute the same path that runc is
// going to use, based on a value of the form "parent:docker:ID", where
// the "docker" is literal.
parts := strings.Split(scope, ":")
Copy link

Choose a reason for hiding this comment

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

This will only work when cgroupdriver is set to systemd. We also need to handle cgroupfs (which is the upstream default) if we plan to try and upstream this.

Copy link

Choose a reason for hiding this comment

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

You could also look into using libcontainer cgroups library for doing this as it handles both the drivers.

Copy link
Author

Choose a reason for hiding this comment

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

Doing the derivation right using libcontainer makes sense, since it's already a dependency. I'll have another look at that. Though that does leave open a question - how often do we see a running journald when systemd isn't managing cgroups?

@cgwalters
Copy link
Member

I didn't read the code, but the concept of this change sounds good to me.

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

Successfully merging this pull request may close these issues.

5 participants