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

Use stdout/stderr instead of logs #29

Open
frafra opened this issue Apr 23, 2021 · 13 comments
Open

Use stdout/stderr instead of logs #29

frafra opened this issue Apr 23, 2021 · 13 comments
Labels
review Issues with improvements proposals or fix a bug... but need to be checked first.

Comments

@frafra
Copy link

frafra commented Apr 23, 2021

Using stdout/stderr in containers instead of using volumes is a good practice as it simplifies monitoring, especially across multiple services.

@marcbria
Copy link
Collaborator

Sorry for the delay with this answer. We have been rethinking some parts of this project and we missed to keep the track of the issues.

I'm not sure I agree.

It's true that docker recommends it and I see benefits in stdout/stderr (basically for devOps getting "docker/docker-compose logs" command), but setting bind volumes for logs is not a bad practice: allows you more control over the space they consume (or where are placed), is ready for complex architectures (share between multiple containers or NFS) and (probably more important to me) keep logs segmented (instead of getting a mess of info when things are quite verbose)...

With our existing structure, you can easily check the log you like.
For instance, to read php log you only need: "$ tail -f ./volumes/logs/app/error.log"

Do you mind to argue a little more your petition.

Cheers,
m.

@frafra
Copy link
Author

frafra commented May 18, 2021

Hi,
no problem :)
I will try to reply to your points and explaining why using volumes is considered to be a bad practice when using containers.

allows you more control over the space they consume (or where are placed),

This is something that your logging system should take care of. It does not really matter if the application is running in a container or not.

is ready for complex architectures

In complex architectures you do not want to setup a volume for each container just to do logging and configure your logging software to harvest all the files, trying to parse information about the timestamp (which can be missing or printed in a different format, or with the wrong time zone), copy everything and repeat the harvesting regularly.

Think about doing that for some hundreds of containers and then setting up another service which should read all the volumes to do harvesting in different ways.

(share between multiple containers

You do not usually want to share volumes containers with logs across containers for security reasons.

or NFS)

You often do not control the underlying operating system when deploying containers (and you do not even want to usually).

and (probably more important to me) keep logs segmented (instead of getting a mess of info when things are quite verbose)...

I am not sure if you mean keeping messages belonging to different processes in different places or if you refer to log rotation.

If it is the first, that is something that should not happen, as each container should run a single process (or a group of homogeneous processes) in general, as you want to decide what to do when a task fails, which is set by a policy on the container, instead of having an init system inside the container which makes everything opaque and has its own policies. I know that there are Dockerfiles around where people pack together a web server, a database and other things, but that is unmaintainable, and you do not deploy in a production system usually.

If you were referring about log rotation, that is again something that logging system should take care of.

What is usually done, is to choose an appropriate logging driver for Docker, toallowing tools like systemd-journald or graylog or fluentd or your preferred solution, to handle the logs.

With our existing structure, you can easily check the log you like.
For instance, to read php log you only need: "$ tail -f ./volumes/logs/app/error.log"

That's valid also valid for Docker (no need to specify any path): https://docs.docker.com/engine/reference/commandline/logs/#options

@marcbria
Copy link
Collaborator

marcbria commented Sep 7, 2021

Writing to let you know that I didn't forget about this. :-)
Your proposal looks solid to me... it's only I still didn't find the time to do the work.

Cheers,
m.

@marcbria marcbria added the review Issues with improvements proposals or fix a bug... but need to be checked first. label Sep 15, 2021
@marcbria
Copy link
Collaborator

@frafra probably I will have some time during the xmas vacations to take a look to this.

But once I start working, I had doubts: What logs need to be redirected: error, access or both?

I'm full convinced about error logs, but not as sure about access ones as far as I don't know if OJS will be able to reach them (need to check how OJS capture it) and they could be so big (and docker is usually installed in root so it could lead to issues).

Happy new year,
m.

@frafra
Copy link
Author

frafra commented Dec 28, 2021

But once I start working, I had doubts: What logs need to be redirected: error, access or both?

Both.

I'm full convinced about error logs, but not as sure about access ones as far as I don't know if OJS will be able to reach them (need to check how OJS capture it) and they could be so big (and docker is usually installed in root so it could lead to issues).

What about using a couple of environmental variables to set where stdout and stderr should be written to? Users could then decide to save them to a file or to /dev/stdout and /dev/stderr for example, or a mix of the two.

Happy new year, m.

Happy new year!

@frafra
Copy link
Author

frafra commented Feb 1, 2023

I can try to make a PR based on my last message, if no one is willing to take this :)

@frafra
Copy link
Author

frafra commented Feb 1, 2023

I am just using these lines in ojs.conf:

    ErrorLog  /dev/stderr  
    CustomLog /dev/stdout combined

I execute the container using /usr/sbin/httpd -f /etc/apache2/httpd.conf -DFOREGROUND.

@marcbria
Copy link
Collaborator

marcbria commented Feb 1, 2023

Hi @frafra

We are moving to multi-stage and adding extra features this repository is kind are frozen.

Some months ago we started to test gitLab ci/cd pipelines and right now images are generated from there.
So, if you have a PR, please commit in gitLab:
https://gitlab.com/pkp-org/docker

BTW, good timing for a PR as far 3.4 will be released soon.


For your information, as far as you participated in the RFC post, let me say that we are actively working in making better images for OJS, OMP and OPS.

We sort all the feedback we got in this document that aims to be the guide for the new oxs images:
https://docs.google.com/document/d/1hl3c6PYQgOZWWtwHk2siBTUj3WC6fzrv9hCp7F1jDGQ/edit?usp=sharing

This "frame" document will be published soon to the community, and your comments are always welcome.

Take care,
m.

@frafra
Copy link
Author

frafra commented Feb 2, 2023

Thanks for sharing such information.
I would be more than happy of supporting the refactoring efforts on the GitHub repository, but I wonder it a draft PR here wouldn't be better, as discussion can be made over the code.

@marcbria
Copy link
Collaborator

marcbria commented Feb 3, 2023

I would be more than happy of supporting the refactoring efforts on the GitHub repository
I'm so happy to hear this. Your expertise will be really appreciated.

I wonder it a draft PR here wouldn't be better, as discussion can be made over the code.
Sure, but before start walking we need to be sure we agreed in the direction. ;-)
Hopefully we will be able to start coding next month or so.

@marcbria
Copy link
Collaborator

marcbria commented Feb 9, 2023

Hi @frafra

New 3.3.0-14 image created and pushed to dockerHub right now with your PR proposal.
I also moved to alpine 3.17 that requires php 8.1 (that is one of the new ojs 3.3.0-14 improvements).

I don't have time right now to test this new image (will don next week or so)... but if you did and find any trouble, plesae let me know.

@marcbria
Copy link
Collaborator

marcbria commented Feb 9, 2023

Hummm... too much changes and build failed:
https://gitlab.com/pkp-org/docker/ojs/-/jobs/3744339913

Will need to review it in detail.
I will advice you as soon as the image is ready for testing.

@marcbria
Copy link
Collaborator

Ok... there was an issue when running "node run build".
Fixed now. Testing and pushing to dockerHub.

Please, let me know if you find any issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Issues with improvements proposals or fix a bug... but need to be checked first.
Projects
None yet
Development

No branches or pull requests

2 participants