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

Option to create directory of specified log files #120

Open
felipemachado-sambatech opened this issue May 24, 2012 · 76 comments · May be fixed by #1400
Open

Option to create directory of specified log files #120

felipemachado-sambatech opened this issue May 24, 2012 · 76 comments · May be fixed by #1400
Labels

Comments

@felipemachado-sambatech

I tried to set the logfile to a file in a directory that doesn't exist. Shouldn't there be an option to automatically create these directories?

@mnaberez
Copy link
Member

I'd rather not add this option, sorry. The config file is already quite verbose and this option may not provide much benefit over mkdir. It would probably be less effort to just run mkdirthan to shut down supervisord, edit the configuration file to enable the automagic directory option, and start supervisord again.

@sherbert
Copy link

For the record, we would also appreciate this feature. We keep our log files in a path that is periodically purged of files and directories that haven't been touched in some amount of time (and there's nothing I can do about this). This means that if we shut down a process for a month, its log directory will be gone when we try to restart it, and so it would be nice for that directory to be automatically recreated.

@RobWin
Copy link

RobWin commented Jun 23, 2014

+1 for this option

@jaanauati
Copy link

+1

@felipou
Copy link
Contributor

felipou commented Oct 13, 2014

As a related note, this diminishes the reliability of supervisor. I don't know if that's been solved somehow in the master branch, but in version 3.1.1, someone accidentally removed a log directory in our server, and supervisor simply stopped working, taking all other programs with it, not just the one with the log directory missing.

I think the scalability of supervisor is also compromised because of this, since one problem in one of the programs can end up taking all others down.

I'd be happy to contribute with a pull request, I just need some pointers as to where I should start in the code.

Also, I don't know if I should create another issue for this...

@coderlol
Copy link

coderlol commented Jul 3, 2015

+1...very reasonable request, why not honoring it?

@rb2nem
Copy link

rb2nem commented Jan 3, 2016

Missing this too....

@paambaati
Copy link

Is there a workaround that will at least let us create the directory from within the config file? Maybe run mkdir inside?

@kujiy
Copy link

kujiy commented Apr 13, 2016

+1

We docker users should do this

  1. ENTRYPOINT["sh", "/entrypoint.sh"] in Dockerfile
  2. In /entrypoint.sh, create a log directory and log files before calling supervisord

This is a shell script for /entrypoint.sh to create them from supervisord.conf automatically.

# Create log dirs and files
mkdir -p $( dirname $(cat /etc/supervisord.conf  | grep logfile= | grep "\.log" | sed s/.*logfile=// ) )
touch $( cat /etc/supervisord.conf  | grep logfile= | grep "\.log" | sed s/.*logfile=// )

# Then run supervisord
/usr/bin/supervisord

Is there any good idea?

@timmyomahony
Copy link

+1

supervisord doesn't start on boot for Arch Linux as the /var/log/supervisord.log doesn't exist when booting.

@rb2nem
Copy link

rb2nem commented Feb 25, 2017

Here are more details about why this would be useful in my case.
I use supervisord in a docker to run some software for which I want to persist the logs, so people are mounting a volume. But with supervisord not creating the needed subdirectories, it requires users to make the directories on the host and set owner and permission themselves. And it is not possible to put it in the command directive, as supervisord complains and stops even before looking at the command.

@mnaberez why sticking to your refusal? I have seen more solid arguments in favor of this than in your rejection.... (but I may be missing something, so more explanations welcome ;-)

@aujang
Copy link

aujang commented Jun 15, 2017

This continues to be a problem, specifically for users deploying supervisor within Docker in an ElasticBeanstalk environment. If you specify logging in Dockerrun, the service wipes the specified directories during deployment, deleting anything created while Docker is spinning up. This in turn breaks supervisor, because it expects files where none exist. If supervisor simply created directories and logs when none existed, it would completely solve the issue. As it stands, I had to disable discrete log files (and use syslog instead) in order to get my app to deploy.

@Niyojan
Copy link

Niyojan commented Aug 10, 2017

same here, I used the following workaround :
docker run -v /mnt/my_logs/celery:/var/log/celery -v /mnt/my_logs/supervisor:/var/log/supervisor -v /mnt/my_logs/supervisor:/var/log/supervisor -v /mnt/my_logs/nginx:/var/log/nginx -v /mnt/my_media:/home/project/media -p 80:80 -p 5555:5555 -d my_project

@adamreisnz
Copy link

adamreisnz commented Aug 28, 2017

I am also running into this now. I have a Raspberry Pi which has /var/log mounted on tmpfs to prevent writes to the SD card. After a reboot of the device, the /var/log/supervisor folder is gone and supervisor won't start. Auto creating this directory would be a much welcome feature.

@mnaberez would you mind reconsidering? There have been offers of PR's for this feature as well.

For context, I've spent about 4 hours debugging this issue today, and could not find any useful information or error messages informing me that a missing log directory causes supervisor to not start at all. That should really not be happening, and I don't know of any other software that doesn't know how to create it's own log folders, let alone refuses to run if they don't exist 😢

@corck
Copy link

corck commented Sep 13, 2017

👍 I just ran into this as well.

@adamatan
Copy link

adamatan commented Oct 3, 2017

+1 👍 Would be very helpful for me as well.

@Nidhoggur1993
Copy link

A kindly remind here.
If supervisor was installed by default and do not change it's log path out of /tmp/supervisord.log, in certain system like CentOS 7.3 or higher, the systemd-tmpfiles-clean.timer will do a cron to delete everything under /tmp everyday.
We have suffer this a lot...

@mavci
Copy link

mavci commented Nov 1, 2017

I just edited /etc/supervisor/supervisord.conf file and changed these lines;

logfile=/var/log/supervisor/supervisord.log to logfile=/var/log/supervisord.log
childlogdir=/var/log/supervisor to childlogdir=/var/log/

and fixed for me. And I was the excatly the same situation with @adamreisnz :)

@adililhan
Copy link

adililhan commented Nov 24, 2017

docker wasn't around when this issue created. so, we use supervisord in docker nowadays. thanks it is nice tool.

however, if we want to redirect the outputs then we have to add new line (and it creates new layer) to dockerfile such as mkdir /var/log/supervisord/"

if supervisord creates the directory then it would be good. thanks anyway.

@Destroy666x
Copy link

I don't know of any other software that doesn't know how to create it's own log folders, let alone refuses to run if they don't exist

Bump

@derekargueta
Copy link

+1

@mikelibg
Copy link

@mnaberez First of all, supervisor is a great tool. Due to the fact that supervisor is the first one to run (before the log directory can be created by the program it is starting) it is intuitive that it can create the log directory by itself, if it does not exists yet. Is there any other logic explanation not to add this option besides the comment "...the config file is already quite verbose..."?

@mnaberez

This comment has been minimized.

@injaon
Copy link

injaon commented Mar 28, 2018

👍 +1 to this request

@rzaharenkov
Copy link

rzaharenkov commented Sep 5, 2018

In my opinion supervisor daemon itself should be super reliable and start even if there is any issue with configuration of particular programs.

In my case I am deploying my application in 2 steps:

  1. Bootstrap server using chef (install packages and create configuration files).
  2. Deploy application using capistrano (fetches application from github and restart particular services using supervisorctl restart my_group:*.

I run 1st step first time when I am bootstrapping new machine or when I do any changes to software or system configuration. 2nd step is running in order to release new version of application code.

So after very first running of chef I am ending up with broken supervisor (service is dead) because it can't find directory. Then run capistrano in order to deploy application 1st time (it creates all the required directories) and tries to restart services using supervisorctl but fails. So I need to run chef 1 more time which is inconvenience.

I do believe if something is wrong with single program configuration then supervisor daemon itself should start but program should be in failed state.

@shankar-moeng
Copy link

+1

@ghost
Copy link

ghost commented Oct 8, 2018

If I need some wrapper scripts for the supervisor itself, it kind of defeats the very purpose of the program. I think it should automatically create the whole directory branch by default. There could be an option to disable this behavior, not the other way around.

@ghost
Copy link

ghost commented Oct 8, 2018

There are actually two major problems with the behavior of the supervisor right now:

  1. Not creating the directory branch automatically.
  2. Shutting down completely just because some log files could not have been created. This is even worse than the first problem. (This is addressed in Supervisor daemon fails to start if log directory does not exist #1143, which I think is not a duplicate.)

@ghost
Copy link

ghost commented Oct 8, 2018

@mnaberez wrote:

The current behavior prevents configuration mistakes. Currently, if the system has a directory /var/log/ but you accidentally enter /var/logs/foo.txt in the config file, supervisord will halt with the error above. I think that's better than ending up with both /var/log/ and /var/logs/ and maybe not noticing that for a long time.

The hypothetical problem of accidentally creating log files in a slightly wrong location seems like a much lesser trouble to me than the very real and frequent (it should be) problem of bringing down the entire server just because the administrator forgot to create the folder for a log file.

And what if you mix up the name of the file and not the folder? What if you create a log in a wrong location that nevertheless exists? What if you specify a completely nonsensical name in an existing folder? After all, you know, garbage in, garbage out.

Another aspect of this question is that one can conceptually see full file paths as fully qualified file names. That is, one can think of the file system as of a collection of files with names that happen to have slashes in them and not as of a tree of folders. Yet this collection can also be viewed as a tree of folders in another representation. Which brings us to:

What does automatically creating the directory mean, exactly? Does it mean mkdir or mkdir -p?

mkdir -p, of course. Purely from a practical standpoint of the user not having to deal with some wrapper scripts or dependent command sequences.

What should the chown and chmod of the automatically created directories be?

How about they would correspond to the chown and chmod of the created log file? You create the log file itself, don't you? Why not also create the directory?

Currently, the user sets up all these things outside of supervisord with the shell.

...which they don't do at first, but they find out about this problem after their server is brought down unexpectedly.

If supervisord requires a wrapper script or dependent command sequences to deal with these things, it greatly reduces its usefulness as an application, as what the supervisor does is not particularly rocket science, so when you have to fix these kinds of problems yourself, you start thinking about the necessity of using the supervisord at all. Also makes you think about the general approach of the authors and what other traps they might have prepared for you.

Is it just logs or should this apply to other paths as well, e.g. pidfiles? Why or why not?

Well, this sounds like it is out of scope for the current issue.

If not, wouldn't it be better to be consistent and not create any directory than to create some and not others?

No. Different situations require different handling. Logs are different from e.g. critical files whose consistency is vital to the system. Logs are important, but aren't critical or vital. If some logs cannot be created, you could issue a warning, and the user would later fix them. It is much better than to shutdown the entire system just because one log file was not created. Edit: I lost my chain of thought here. The argument was about creating the folder structure for log files automatically, so the problem from which we are protecting ourselves here is that the name of the folder for some log file may be wrong. Thus, given that log files and their locations are usually not critical or vital, I would gladly exchange the risk of placing a log file in a wrong folder with the risk of unexpected outages of the server. For the (supposedly rare) folk for whom the location of the log files is mission-critical I would make an opt-in possibility to ensure that the folder for a log file exists.

@mnaberez
Copy link
Member

mnaberez commented Oct 8, 2018

Logs are different from e.g. critical files whose consistency is vital to the system. Logs are important, but aren't critical or vital. If some logs cannot be created, you could issue a warning, and the user would later fix them. It is much better than to shutdown the entire system just because one log file was not created.

This opinion is application-specific. We will not guess about how important log files might be for someone else's application. If the user provides a supervisord.conf file that the supervisord daemon cannot fulfill, it will fail-fast so the problem is corrected sooner than later.

@ghost
Copy link

ghost commented Oct 9, 2018

@mnaberez

Logs are different from e.g. critical files whose consistency is vital to the system. Logs are important, but aren't critical or vital. ...

This opinion is application-specific.

Well, sure, I am certain there are people out there who have designed a system in which log files and their locations are vital for the functioning of that system, and they would prefer an outage of the entire server to a possibly wrong name of a folder for a single log file. But are those the majority of use cases? How typical is that situation? Do you generally prefer to optimize for the most common use case or the most uncommon use case?

We will not guess about how important log files might be for someone else's application.

But you did guess that an outage of the entire system is less critical than a possibility that the name of the folder for a single log file might be wrong. Essentially, if we have to choose between these two evils (service outage vs. possibly wrong folder name for a log file), some guessing seems unavoidable.

@mnaberez
Copy link
Member

mnaberez commented Oct 9, 2018

But you did guess that an outage of the entire system is less critical than a possibility that the name of the folder for a single log file might be wrong. Essentially, if we have to choose between these two evils (service outage vs. possibly wrong folder name for a log file), some guessing seems unavoidable.

The supervisord daemon doesn't accept an invalid configuration file. If you change the configuration file to one it cannot fulfill (including log files it can't write), it will refuse to (re)start and will give you an error message why. People who configure other daemons regularly should find this behavior typical.

While it's entirely possible that some option to create log directories could be added at some point as discussed here, it's unlikely that the fail-fast behavior will change. It is unlikely that we are going to add special cases to ignore only some configuration mistakes as you are requesting. It would mean that users could no longer have confidence that a running supervisord instance is in compliance with its configuration file.

@aujang
Copy link

aujang commented Oct 9, 2018

As I mentioned in my original comment, there are circumstances where the directory literally cannot pre-exist (namely, docker within Elastic Beanstalk) because the deploying service wipes the master log directory during any new deployment. This behavior causes supervisord to fail every time, without any fault on the user's behalf. I don't find the counter-arguments against automatic directory creation terribly compelling. For example:

The current behavior prevents configuration mistakes. Currently, if the system has a directory /var/log/ but you accidentally enter /var/logs/foo.txt in the config file, supervisord will halt with the error above. I think that's better than ending up with both /var/log/ and /var/logs/ and maybe not noticing that for a long time.

OK, let's assume there is a config mistake. What is the ultimate consequence of said mistake? Completely valid logs are generated and stored in the wrong place. When someone goes looking for the logs, they don't find them where they expected, so they go to the config file, see that the log destination has been incorrectly configured... and correct it. Meanwhile, they have all of the previous logs, and nothing has broken in the interim. If something did break (let's say there was an aggregator running and it wasn't collecting the logs because they weren't in the right place), then only one of two things could occur:

  1. The aggregating service fails with an error that the expected logs were not found (which would prompt an investigation that would lead back to the config file), or:
  2. The aggregating service does not include the supervisord logs, in which case anyone examining the aggregator notes the lack of logs, and then goes to find them manually, discovering the discrepancy. Meanwhile, all the services continue to run, and logs have been generated, so they can be manually uploaded into aggregator without any real consequence.

I don't see the pidfiles argument as analogous because logs aren't mission-critical, but PIDs are. You can't run processes at all without generating a PID, but you can run any application without generating logs.

I don't think anyone here is arguing against the general fail-fast behavior here: instead, we're pointing out that the current fail-fast behavior for log files is a bad design pattern. But it strikes me that there's a simple solution: include a configuration parameter called "Automatically create log directory if absent?" and, when set to true, have it execute mkdir -p (as @dmitry-akimov-dXgjvQjNiMuVYecJ suggests) with the same permissions/ownership as the created logfile, and move on. You can have the config param default to "false" so that the error procs the first time a non-existent log directory is passed, so the user is forced to take a closer look at log configuration before making a decision.

@mnaberez
Copy link
Member

mnaberez commented Oct 9, 2018

I don't find the counter-arguments against automatic directory creation terribly compelling.

Most of the discussion of the last couple of days, including my own replies, has been a rehash of things already stated in this thread. I don't think it's worth going around again because I do not think new points are being made.

But it strikes me that there's a simple solution: include a configuration parameter called "Automatically create log directory if absent?"

This issue is currently open. I reopened it because I think it is worth reconsidering. It is possible that we may add some sort of optional feature to create directories. I stated this above also. I am currently working on other issues and I am not able to work on this particular issue right now. The other committers on this project may be willing to work on it, or it can stay open.

@sdanbury

This comment has been minimized.

@mnaberez

This comment has been minimized.

@danmihai89
Copy link

+1

1 similar comment
@bbankowski
Copy link

+1

@renaudcerrato
Copy link

Such a stupid bug and yet unfixed. Our /var/log directory is a tmpfs, so totally wiped between reboots.

@beasteers
Copy link

I really don't understand why there is so much pushback on this idea. Tbh I think this behavior should be available and enabled by default for logs (but I understand the concern about changing default behavior). I don't see any argument about having it available and disabled by default however.

If I ever get a minute I might look into submitting a pull request, but in case someone else has a desire to figure it out:

Here's where they throw an error if the directory doesn't exist

def logfile_name(val):
if hasattr(val, 'lower'):
coerced = val.lower()
else:
coerced = val
if coerced in LOGFILE_NONES:
return None
elif coerced in LOGFILE_AUTOS:
return Automatic
else:
return existing_dirpath(val)

def existing_dirpath(v):
nv = os.path.expanduser(v)
dir = os.path.dirname(nv)
if not dir:
# relative pathname with no directory component
return nv
if os.path.isdir(dir):
return nv
raise ValueError('The directory named as part of the path %s '
'does not exist' % v)

So a start of a fix could be:

# https://github.com/Supervisor/supervisor/blob/3e7c082a71fef3860ecf06727edd091ddb3cc282/supervisor/options.py#L635
self.create_log_directories = boolean(
    get('supervisord', 'create_log_directories', 'true')) # +++
...
section.logfile = existing_dirpath(
    get('logfile', 'supervisord.log'), 
    create=self.create_log_directories) # +-

# https://github.com/Supervisor/supervisor/blob/b6b762d809e8c5966208b0836a9403294c3294c4/supervisor/datatypes.py#L342
def existing_dirpath(v, create=False):
    nv = os.path.expanduser(v)
    dir = os.path.dirname(nv)
    if not dir:
        # relative pathname with no directory component
        return nv
    if create: # +++
        os.makedirs(dir, exist_ok=True)
    if os.path.isdir(dir):
        return nv
    raise ValueError('The directory named as part of the path %s '
                     'does not exist' % v)

# https://github.com/Supervisor/supervisor/blob/b6b762d809e8c5966208b0836a9403294c3294c4/supervisor/datatypes.py#L94
def logfile_name(val, create=False):
    coerced = val.lower() if hasattr(val, 'lower') else val
    
    if coerced in LOGFILE_NONES:
        return None
    elif coerced in LOGFILE_AUTOS:
        return Automatic
    return existing_dirpath(val, create=create) # +-

# https://github.com/Supervisor/supervisor/blob/3e7c082a71fef3860ecf06727edd091ddb3cc282/supervisor/options.py#L970
lf_val = logfile_name(lf_val, create=self.create_log_directories) # +-

# https://github.com/Supervisor/supervisor/blob/3e7c082a71fef3860ecf06727edd091ddb3cc282/supervisor/options.py#L426
self.add("logfile", "supervisord.logfile", "l:", "logfile=",
         lambda v: existing_dirpath(v, create=self.create_log_directories),
         default="supervisord.log")

@xiandong79
Copy link

I have created the log path myself
but supervisord do not creat the log file

~~ help

@mnaberez
Copy link
Member

Tbh I think this behavior should be available and enabled by default for logs (but I understand the concern about changing default behavior). I don't see any argument about having it available and disabled by default however.

We probably would not change the default (at least not without bumping the major version). This issue has been open for a long time and there have been a lot of comments asking for it, so at this point we would consider adding it as an option (this was stated above) but nobody is currently working on it. On this issue tracker, issues that are rejected are closed. Issues that are still open are considered unresolved.

@bob-smetana
Copy link

bob-smetana commented Jan 16, 2021

This thread has been silent for a minute, but as the issue is still open I assume comments are still welcome.

I agree with @mnaberez and others who have said that changing default behavior is not the thing to do. And the fail-fast approach is a good one - highly unambiguous - but situations demonstrably exist where it is a problem for non-existent directories specifically, as there is occasionally no way around it. Docker environments using supervisor have been mentioned as an example, and that situation is what brought me here today.

The suggestion @willmcgugan made is a good one in my opinion; it doesn't change any existing or default behaviors so is compatible with any config files that may exist today. Not sure whether I think there needs to be a recursive option or if it should simply assume mkdir -p if it's creating directories. I'm leaning toward the latter.
Ownership could be set to the user:group the logging process runs as, whether that be supervisor itself or a child. I think documenting that created directories will have 775 or 755 permissions would be sufficient.

As for questions about whether supervisor should also then create directories for pids and the like - I think it could be thought of as a separate issue, although whatever decision is made here certainly could be applied elsewhere. Perhaps even it simply becomes a global option; will supervisor be creating directories where needed, or not?

All that to say - another late vote in favor of this feature.

@pixtim
Copy link

pixtim commented Jun 11, 2021

I support this feature request as well.

Having an opt-in option ( #1400 ) will allow the user to decide if:

  • Missing log directories is unacceptable and the supervisor should shut down, or
  • Automatic log directory creation is acceptable and the supervisor should gracefully create the missing directories and continue running.

The existing behavior can be kept as the default. Having the option would allow for some flexibility. Currently, there is no flexibility.

@pompushko
Copy link

pompushko commented May 9, 2024

+1

10+ years of stability to do not create log directory if not exists :D
whats the point of that? why? for what :D

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

Successfully merging a pull request may close this issue.