Uploaded image for project: 'ejabberd development'
  1. ejabberd development
  2. EJAB-1243

Improve reopen-log to not rename the files, and add new rotate-log

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: ejabberd 14.07
    • Fix Version/s: ejabberd 16.01
    • Component/s: Command-line tools
    • Labels:
      None

      Description

      Konstantin Khomoutov found two problems in ejabberd reopen-log command, and wrote a preliminary patch:

      Now I rewrite his explanations:

      1. erlang.log for some reason only gets reopened each second call to `ejabberdcl reopen-log`

      2. After reopening the same files (that is, when the files were not renamed before reopening, by logrotate or whoever), erlang.log gets truncated, and ejabberd.log gets renamed to "ejabberd-old.log". Both behaviours break the "reopening of log files" contract common in Unix.

      In standard rotation systems: reopen != rename

      In ejabberd, when you do `ejabberdctl reopen-log`, your log files (at least ejabberd.log) end up being renamed to ejabberd-old.log. This is definitely not the expected behaviour of "reopening log files", which must be idempotent on Unix. ejabberd must not mess with log names in any way.

      Reopening should work this way:
      i) Remember the name of log file.
      ii) Close it.
      iii) Open it by name.

      And who takes care to rename the old log file? The system log rotation software (typically Logrotate). This program usually rotates the log file of a program this way:
      1) Renames the log file. ejabberd continues to write to that file, as it uses file descriptor which doesn't care about renames/deletions.
      2) Call ejabberd to reopen its logs.
      3) ejabberd performs the steps i, ii, iii mentioned before: ejabberd reopens its log files by name and starts to write to its "usually named" files
      4) The system log rotation script stores the old closed log files.

      First proposed patch

      The patch fixes these problems in the following way:

      • The SASL application is started with its built-in file logger disabled. After this a custom module is inserted to handle SASL messages; this module does not truncate its log file when reopening it.
      • Built-in ejabberd's "log rotation" (renaming original log files by appending the "-old" suffix to their base names) is disabled.
      • Both ejabberd's logging module and the custom SASL logging module now respond to a custom event which instructs them to reload their respective log files; this event is synchronously sent to these modules when `ejabberdctl reopen-log` is being run.

      The Windows problem

      The scheme previously mentioned does not work on Windows because in 99% of cases the program which opens a file for writing locks it at least for writing and sometimes it locks it for reading as well.

      A solution is to implement an ejabberd command to reopen the logs and also rename them: rotate-log.

      This is useful not only for Windows, but also for Unix machines where Logrotate isn't avaiable, or the admin doesn't know to setup it, or the ejabberd Binary Installer can't configure it.

        Activity

        Hide
        cromain@process-one.net Christophe Romain (Inactive) added a comment -

        we can avoid glitches by using 'nocreate' option to logrotate and let logrotate trigger the reopen-log

        ejabberd.log {
                daily
                missingok
                rotate 5
                delaycompress
                compress
                ifempty
                nocreate
                postrotate
                        su - ejabberd -c "ejabberdctl reopen-log"
                endscript
        }
        

        or we can also let ejabberd rotate by itself keeping only one archive, and let logrotate handle archives only without touching main log

        ejabberd.log.0 {
          daily
          missingok
          rotate 5
          compress
          notifempty
          nocreate
          start 1
        }
        

        but indeed, having two functions: reopen-log and rotate-log can be less confusing for packagers/users

        Show
        cromain@process-one.net Christophe Romain (Inactive) added a comment - we can avoid glitches by using 'nocreate' option to logrotate and let logrotate trigger the reopen-log ejabberd.log { daily missingok rotate 5 delaycompress compress ifempty nocreate postrotate su - ejabberd -c "ejabberdctl reopen-log" endscript } or we can also let ejabberd rotate by itself keeping only one archive, and let logrotate handle archives only without touching main log ejabberd.log.0 { daily missingok rotate 5 compress notifempty nocreate start 1 } but indeed, having two functions: reopen-log and rotate-log can be less confusing for packagers/users
        Hide
        cromain@process-one.net Christophe Romain (Inactive) added a comment -

        this has been obsoleted by our internal log rotation feature included in 15.07

        Show
        cromain@process-one.net Christophe Romain (Inactive) added a comment - this has been obsoleted by our internal log rotation feature included in 15.07
        Hide
        cromain@process-one.net Christophe Romain (Inactive) added a comment -

        debian still needs better reopen-log. Holger provides following patch to make sure reopen log don't rotate files also.
        http://git.deb.at/w/pkg/ejabberd.git/blob/HEAD:/debian/patches/src.ejabberd_logger.erl.patch

        Show
        cromain@process-one.net Christophe Romain (Inactive) added a comment - debian still needs better reopen-log. Holger provides following patch to make sure reopen log don't rotate files also. http://git.deb.at/w/pkg/ejabberd.git/blob/HEAD:/debian/patches/src.ejabberd_logger.erl.patch
        Hide
        badlop Badlop added a comment -

        But that patch avoids calling lager at all, is that good only for Debian, or for all? I mean, is it safe to apply as it is in git?

        Show
        badlop Badlop added a comment - But that patch avoids calling lager at all, is that good only for Debian, or for all? I mean, is it safe to apply as it is in git?
        Hide
        cromain@process-one.net Christophe Romain (Inactive) added a comment -

        good catch indeed. i guess we need to test if this breaks things or maybe add a dedicated "rotate-logs" command
        Holger suggests: «another option might be to add a new "really-just-reopen-logs" command. Many users don't need a "really-just-reopen-logs" command these days, because Lager automatically notices when log files were rotated and it reopens them in that case. But some contrib modules (mod_logsession, mod_message_log, mod_s2s_log) use the 'reopen_log_hook' as well, so for those modules such a "reopen-log" command makes sense.»

        Show
        cromain@process-one.net Christophe Romain (Inactive) added a comment - good catch indeed. i guess we need to test if this breaks things or maybe add a dedicated "rotate-logs" command Holger suggests: «another option might be to add a new "really-just-reopen-logs" command. Many users don't need a "really-just-reopen-logs" command these days, because Lager automatically notices when log files were rotated and it reopens them in that case. But some contrib modules (mod_logsession, mod_message_log, mod_s2s_log) use the 'reopen_log_hook' as well, so for those modules such a "reopen-log" command makes sense.»
        Hide
        holger Holger Weiß added a comment - - edited

        Yes, it should be safe to apply as-is.

        The full story is that Lager actually notices external log rotation automatically, so many users don't need a reopen-log command at all these days. However, mod_http_fileserver and some contrib modules (mod_logsession, mod_message_log, mod_s2s_log) use the reopen_log_hook as well. So, for those modules, a reopen-log command (that really just reopens logs) makes sense.

        Apart from that, there may be users who actually want the current reopen-log behavior, i.e., they might need a command that tells Lager to rotate logs (instead of just reopening them). I'd suggest to add a separate rotate-log command for them. (If you guys are happy with that suggestion, I can do that myself, of course.)

        Update: Sorry, I hadn't seen Christophe's reply before sending mine.

        Show
        holger Holger Weiß added a comment - - edited Yes, it should be safe to apply as-is. The full story is that Lager actually notices external log rotation automatically , so many users don't need a reopen-log command at all these days. However, mod_http_fileserver and some contrib modules (mod_logsession, mod_message_log, mod_s2s_log) use the reopen_log_hook as well. So, for those modules, a reopen-log command (that really just reopens logs) makes sense. Apart from that, there may be users who actually want the current reopen-log behavior, i.e., they might need a command that tells Lager to rotate logs (instead of just reopening them). I'd suggest to add a separate rotate-log command for them. (If you guys are happy with that suggestion, I can do that myself, of course.) Update: Sorry, I hadn't seen Christophe's reply before sending mine.
        Hide
        badlop Badlop added a comment -

        It's ok for me to make those changes. Remember to mention in the commit log any change relevant to old admins, so such advice gets added to the release announcement.

        Show
        badlop Badlop added a comment - It's ok for me to make those changes. Remember to mention in the commit log any change relevant to old admins, so such advice gets added to the release announcement.
        Hide
        holger Holger Weiß added a comment -

        It's ok for me to make those changes. Remember to mention in the commit log any change relevant to old admins, so such advice gets added to the release announcement.

        Okay, thanks. Will do.

        Show
        holger Holger Weiß added a comment - It's ok for me to make those changes. Remember to mention in the commit log any change relevant to old admins, so such advice gets added to the release announcement. Okay, thanks. Will do.
        Hide
        cromain@process-one.net Christophe Romain (Inactive) added a comment -

        fixed in 07baf2d9

        Show
        cromain@process-one.net Christophe Romain (Inactive) added a comment - fixed in 07baf2d9

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development