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

Log to files instead of docker logs #201

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
2 participants
@silviot
Copy link
Contributor

commented Apr 18, 2019

This PR changes handling of logs by lms, cms, mysql and nginx so that they log to ${TUTOR_ROOT}/data/logs instead of sending the logs to docker.

It includes an opinionated refactoring that should reduce code duplication. Should that part need further discussion I can revert it and PR it separately.

@regisb

This comment has been minimized.

Copy link
Owner

commented Apr 18, 2019

There are a lot of things in this PR, and I'm sure I will have comments and ask you for a couple changes, but overall it's a great move forward. Thanks a lot for this!

@regisb

This comment has been minimized.

Copy link
Owner

commented Apr 19, 2019

On second thought, I think we may have an issue here. I understand what you are trying to achieve here, and in some cases it's a considerable improvement: it helps debug issues, makes logs persistent, etc. But in some other cases, there is the possibility that it degrades the user experience. I'm thinking in particular about Kubernetes: I heard there are solutions that automatically collect logs from stdout and aggregate them nicely, per container/pod/node, make them searchable, etc.

Also, with the chosen approach the developer is responsible for selecting destination file names, how these files will be rotated, etc.

These reasons are example that backup the 12 factor app principle according to which apps should not attempt to manage their own logs: https://12factor.net/logs

So, a perfect solution would consist of a nice log collector that would centralize all the logs, distribute them in files, take care of file rotation, deletion, etc. I think it would be great to have such a tool, but it's probably very much out of the scope of this PR...

Can we make a compromise? You are probably most interested in tracking logs, right? I suggest we only redirect tracking logs to files: they would be both stored in files and output to stdout, with two different handlers. Maybe use a TimedRotatingFileHandler?

What do you think?

EDIT

  1. I forgot to mention that I find the prod/dev settings refactoring really good. I tried to think of a way to simplify the settings for a long time but didn't think of using execfile. Let's keep it in this PR!
  2. Forget my comment about TimedRotatingFileHandler, as WatchedFileLogger is probably safer when there are multiple web workers.

@regisb regisb force-pushed the regisb:master branch 7 times, most recently from b528123 to 5558454 Apr 22, 2019

@silviot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Thanks for the pointer to https://12factor.net: I was not aware of those guidelines.
While I agree with many of the points there, the one about logging doesn't fully convince me.

The part that I find less convincing is that every app should have a single event stream.

I don't agree.

I think it's perfectly OK for a component to have more than one logical log stream.

A good example from this PR are the tracking logs: it's better not to mix them with python tracebacks.

I agree that ideally there should be a Logplex, Fluentd, Logstash, Graylog or similar to collect all logs.
But I also think it would be good for tutor to support more basic usage patterns, when the
operator does not have the time/knowledge/resources to deploy such a solution.

My personal favourite is the Filebeat approach: apps still write to log files, and the
logging infrastructure reads from these. Going this way one can incrementally grow the complexity,
having log files initially and switching to a more solid loggin infrastructure later on.

I propose to have tutor configured in a similar fashion to the nginx image: the daemon
logs to two files on the disk. These files are the only ones in their directory.
The files shipped with the image are symbolic links to stdout/stderr.
If a user wants to have those files on disk they can mount that directory as a volume.

I'm still of the opinion that tutor would better serve its users defaulting to logging to
files instead of docker logs.

I'll amend this PR to make sure all logs (except the tracking logs) are sent to stdout/stderr,
with the option for users to mount the log directory and have them persisted as files,
in a similar fashion to how the nginx docker image is set up.

@regisb

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

I think it's perfectly OK for a component to have more than one logical log stream.

Yes, I think it's ok, too, on the condition that one of those log streams is stdout.

A good example from this PR are the tracking logs: it's better not to mix them with python tracebacks.

Well... tracking logs are helpful in debugging, too, right?

I propose to have tutor configured in a similar fashion to the nginx image: the daemon logs to two files on the disk.

This approach makes sense -- again, as long as there is both a filestream and a console handler, so that we don't break the tutor local logs command.

Also, you may want to use a file-rotating handler; otherwise, logs are going to use more and more space until the containers are restarted.

However, I don't quite understand the comparison with the nginx image. AFAIU, the nginx daemon logs only to stdout/stderr, and there is no way to configure nginx to log to 2 different destinations at the same time (say: stdout and /var/log/nginx/access.log), right?

@silviot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I was a bit too terse wrt nginx. Let me elaborate.

The daemon is instructed to log to /var/log/nginx/access.log and /var/log/nginx/error.log.

These two files are symlinked in the image to /dev/stdout and /dev/stderr.

The effect is that, as a user of that image, I can get the default behaviour (logs go to docker) by leaving it as is.

But I also have the option to mount /var/log/nginx as a volume, and have logs as files in any desired directory.

My proposal is to follow the same pattern in tutor.

@regisb

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

My proposal is to follow the same pattern in tutor.

Do you have an idea how you would go about doing that? I suspect it would involve additional configuration options, right?

Let's take a step back. Currently, the docker containers use the default json-file logging driver: https://docs.docker.com/config/containers/logging/configure/
That means that logs are already stored in log files (in ./containers/<containerid>/<containerid>-json.log). In particular, the tracking logs are available by running something similar to tutor local logs -f | grep "\[tracking\]". (that's not a viable solution in itself, but please bear with me)

Now, let's say I'm a regular Tutor user, and I'm interested in collecting logs. Maybe I'm interested both in nginx and tracking logs; maybe I want to investigate mysql logs for performance reasons; maybe I want to store my logs in an existing ELK cluster, or in Splunk, Loggly, Datadog or my data lake. The following solution addresses all those needs at once:

  • run tutor local logs -f >> all.log as a supervised service on the host
  • run filebeat/fluentd/logstash on the host to collect the logs from all.log and forward them to logstash/fluentd/Splunk/HDFS
  • If there is an intermediate parser, such as Logstash, then tracking logs can be separated from the rest of the logs and sent to a specific destination.
  • (optional) run a file-rotating service on all.log to make sure it does not grow beyond a certain size.

With this approach, not only do we address the needs from all users (including yours, right?), but we do so without touching the tutor code.

I agree that the proposed approach requires some engineering, and maybe we can provide some documentation on the best way to tackle this. But we address all problems at once.

On the other hand, if the docker containers are in charge of logging to dedicated files, then:

  1. we need to setup ad-hoc configuration variables (and thus make the codebase more complex)
  2. users are still responsible for log rotation
  3. we lose logging on stdout

I'll add an exception to this for the tracking logs: often, users don't realise they will need access to the tracking logs until, well, they do. So it makes sense to keep an archive for them, and thus configure the tracking logger to use both the console and the filestream handlers. Archives for other sources of logs are not as important, IMHO.

Maybe we could find an agreement if you described your exact needs? I guess you want to better debug production instances?

EDIT: I tried to bring more people in the conversation

@silviot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I'll take a step back to better describe the issue I'm having.

Currently tutor can be used to deploy a production Open edX instance using docker.

When using the vanilla docker and the vanilla tutor configurations, logs will be entirely managed by docker, and live in /var/lib/docker/containers/${CONTAINER_ID}/${CONTAINER_ID}-json.log.
Incidentally, they're not rotated at all by the default docker config.

When an update is made (for instance because of a code change), a new image is pulled, and a new container is created. The new container logs and the old container logs now live in two different locations, and docker logs can only access the newer ones.
In addition to this, if the previous container is removed (the default action if one considers containers disposable) the old log is lost forever.

Some people (like myself) might prefer having log files stored outside the containers in the same format the service/application produces.

The approach nginx takes allows to switch between docker logs and file logs extremely easily: you can leave it as is, and the nginx process will write to files (acess.log and error.log) in /var/log/nginx, effectively writing to stdout and stderr by virtue of the symbolic links bundled with the image.

But you can also mount /var/log/nginx as a volume, and have log files persisted on the filesystem,
outside the container, in a location of your choice.

I understand the proposal to have an always running tutor local logs -f process, but it does not sound reliable to me; for instance, if for any reason the process goes awry, some logs might be lost.

It feels to me like the change I propose would not make any difference at all to users who are fine with
using docker logs as they are now defined in tutor. The only change would be that daemons would write to some files instead of directly to stdout/stderr, but these files would exist in the shiped image, and
they would be symbolic links pointing to /dev/stdout end /dev/stderr.

But users who want their log files living a regular logfile life on the filesystem outside of containers
would have an easy way to achieve this, by specifying an additional volume to mount.

Now that your concerns are clearer to me I'll open a different PR with the refactoring of common logic
into a common.py file, since I understand you have no objections there and it could go in.

On top of that, I'll work on a separate PR to change logging as I described above: use intermediate files
that by default points to std* to give users the option to persist log files by mounting an additional volume.

@silviot silviot closed this Apr 23, 2019

@regisb

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

@silviot your use case is 100% valid. It makes sense to persist app logs on disk in production. I'll try to think of an approach that makes us both happy. I suspect it's possible to achieve the best of both worlds with a custom docker logging driver: https://docs.docker.com/config/containers/logging/configure/
Would you be satisfied with a syslog configuration (on the host) that would dispatch all logs to /var/log/tutor/appname/stdout.log//var/log/tutor/appname/stderr.log, where appname would be nginx/lms/lms_worker/... ? (with log rotation)

@silviot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@regisb I agree one possible way to achieve log-in-files-outside-the-container is to change the docker configuration, but what I'm proposing is way simpler, and would only require changes "inside" tutor, not "outside" of it. IMHO the simplest way is to make sure daemons write to files without the additional step of a log collection process, which brings complexity, along with the power it provides.

@regisb

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

Hmmm ok so that's the core of the problem: we don't agree on what's "simple" 😅

@regisb

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

What if I told you that you could store logs for all your apps by running simply:

docker run -d -p 127.0.0.1:24224:24224 --restart=unless-stopped  -v /var/log/openedx/:/var/log/openedx/ regis/openedx-logging
tutor config save -y --set LOCAL_LOGGING_DRIVER=fluentd --set LOCAL_LOGGING_OPTS='{"fluentd-address": "localhost:24224"}'

The regis/openedx-logging would be running a fluentd daemon (in a docker container, bien sûr) that would centralise and neatly organise all logs.

The future will have plugins and so that could be as simple as running:

tutor plugin install openedx-logging
@silviot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

The fluentd solution would be super-cool!

But I still think there's also room for something like #205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.