Skip to content
This repository was archived by the owner on Aug 14, 2020. It is now read-only.

Adds a --no-daemonize option - #91

Open
colinmorelli wants to merge 4 commits into
versatica:masterfrom
colinmorelli:feature/add-no-daemonize
Open

Adds a --no-daemonize option#91
colinmorelli wants to merge 4 commits into
versatica:masterfrom
colinmorelli:feature/add-no-daemonize

Conversation

@colinmorelli

Copy link
Copy Markdown

This PR adds a simple option for no-daemonize.

It also traps SIGINT in the case of running in the foreground, to allow users to simply control-c in a standard terminal to exit the process.

@colinmorelli

Copy link
Copy Markdown
Author

Just to add here - I've gone back and forth on what to do with logging. On the one hand, it's helpful for development purposes to dump logs to stdout. On the other hand, the actual production use case for this feature is more to be used with process monitors and/or docker and the like, at which point the user will likely just pipe stdout to /dev/null and use syslog instead.

Thoughts?

@saghul

saghul commented Jun 15, 2016

Copy link
Copy Markdown

Process monitors (or applications acting as such) which run applications in the foreground such as Docker or systemd typically capture stdout and divert it wherever. So I'd say that when running in foreground mode all logging should go to stdout.

@colinmorelli

Copy link
Copy Markdown
Author

Yes, @saghul - but in the case of docker, for example, it actually just dumps all stdout to a single docker-managed file by default, and doesn't rotate it (unless that has changed recently). This makes it easy to run yourself out of disk space if you don't realize what it's doing.

Do you think existing syslog behavior should be preserved in addition to stdout? Or just stdout? That might become confusing for the user.

@saghul

saghul commented Jun 15, 2016

Copy link
Copy Markdown

You can use the syslog log driver in Docker and send all your container output to a central syslog instance. See: https://docs.docker.com/engine/admin/logging/overview/

IMHO having an application run in the foreground while logging to syslog is an anti-pattern and really unexpected.

@colinmorelli

Copy link
Copy Markdown
Author

I'm aware there are other options. I was just mentioning the default which can lead to strange behavior. In any case, I can easily make a change to disable syslog in favor of using stdout in the foreground.

@ibc

ibc commented Jun 15, 2016

Copy link
Copy Markdown
Member

I also think that, when running in foreground, syslog should not be used.

BTW: If I made OverSIP today, I wouldn't add syslog support at all, nor daemon capability.

@saghul

saghul commented Jun 15, 2016

Copy link
Copy Markdown

I can easily make a change to disable syslog in favor of using stdout in the foreground.

This would be the only acceptable way to log when not in the background IMHO. But it's up to @ibc to decide.

@colinmorelli

colinmorelli commented Jun 15, 2016

Copy link
Copy Markdown
Author

Alright I believe the latest commit should have it only log to syslog if daemonized, preferring stdout otherwise. tbh I don't really like how it's working, though. Partially because the construction of the logger is still based on syslog configuration values (to preserve backwards compatibility). Also partly because it just feels a little ugly.

@mbrodala

mbrodala commented Mar 7, 2018

Copy link
Copy Markdown

What is the current status here? I assume this is essential to get OverSIP running in a Docker container.

@ibc

ibc commented Mar 7, 2018

Copy link
Copy Markdown
Member

The current status is that I'm not actively maintaining OverSIP, but I'd accept a PR that just removes all the daemon and syslog stuff from OverSIP so it becomes a single process that logs to stdout/stderr. Unfortunately this PR is more complex than that and I cannot guarantee that it works fine.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants