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

Remove the logging while building the docker image #727

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lfoppiano
Copy link
Collaborator

@lfoppiano lfoppiano commented Mar 8, 2021

This PR removes the log on a file of the application. It's performed at runtime while building so that the log file can be kept for other usages.

It requires just that the two marks in the config.yml are not removed or modified

CC @superdude264

@coveralls
Copy link

coveralls commented Mar 8, 2021

Coverage Status

Coverage decreased (-0.003%) to 39.964% when pulling 1b8eaf9 on improvement/remove-log-on-file-docker into 1e6903e on master.

@superdude264
Copy link

@lfoppiano Looks good. Will definitely help users avoid some surprises when they use the image from DockerHub.

@lfoppiano lfoppiano added this to the 0.6.2 milestone Mar 15, 2021
@kermitt2
Copy link
Owner

I am not sure it is a good idea:

  • we should reasonably expect logging when using the docker image, because it's typically a production usage
  • I think it's confusing for the user to have logging settings specified for the service and then not using it
  • all the config will change in a unique yaml file with New yaml config #687 and I plan to merge to this version (which is changing a lot of things) immediately after releasing 0.6.2, so this removal of the log will have to be changed

If we want to avoid logs in the container, why don't we simply use a volume which will be managed on the host:

VOLUME ["/opt/grobid/logs"]

Nothing else to do I think. This is also a good choice for persistent data and it easier than bind mount which requires something in the command line.

This would allow logging and avoid automatically modifying the config files, which is a bit confusing for the user who sets the settings.

@superdude264
Copy link

The abbreviated idea around Docker logging is that the Docker runtime captures the logs using a log driver. In the simplest (and I believe default) case, Docker captures standard out & standard error and stores them, so the console logs will still be available.

Additionally, putting the VOLUME keyword in the Dockerfile is not a guarantee that it will be created by the Docker runtime on the host. That decision depends on how the operator sets it up. If they don't create a volume and mount it, then the container will just write the log data into itself and it will be removed when the container is terminated. In a production setting, this would likely mean the runtime would kill the container for taking up too much space (because of all the log data). This is what happened in our production setup.

@kermitt2 kermitt2 modified the milestones: 0.6.2, 0.7.0 Mar 19, 2021
@lfoppiano lfoppiano added this to the 0.7.0 milestone Mar 26, 2021
@kermitt2 kermitt2 modified the milestones: 0.7.0, 0.7.1 Jul 9, 2021
@kermitt2 kermitt2 removed this from the 0.7.1 milestone Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants