Repository: modoboa/modoboa (language: python)

Issue:
Logparser is missing any lock mechanism that prevents multiple instances to be run
# Impacted versions

* OS Type: Debian
* OS Version: 11
* Database Type: PostgreSQL
* Database version: 13.9
* Modoboa: 2.0.*
* installer used: No
* Webserver: Nginx

# Steps to reproduce

When mail.log is huge, processing it may take more then 5 minutes configured in cron... then another instance is started that slows down the system... this goes on and on until all RAM is wasted and OOM killer starts doing it's job...

# Current behavior

Multiple instances of logparser can be run

# Expected behavior

Only one instance of logparser (and maybe other periodic tasks?) should be allowed to run, use pid file lock for this?


# Video/Screenshot link (optional)

Oh now i'm sad i did not take a screenshot of htop with 82 load and like 30 logparsers running lol...




Discussion / hints:
If you don't need current stats but are fine with the day before you could also do this once per day at night and use the rotated log file. We're forced to do this as well as our logfile run takes roughly 45min  due to size.

Using `logtail` in this process would solve this issue imho because it continues on the last line you've left with. That might be a good solution as an enhancement.
@MrGeneration yes, i did that as a first thing! :smiling_face_with_tear: But that is not a FIX that is a HACK, these types of background tasks should have locking mechanism that prevents another task to be run while previous is still running. That means CRON task will "wait" for previous task to finish no matter what CRON period is set to and how long given task actually takes while it prevents spawning more then one task at once...

Why is your solution a hack? It does not prevent the issue, you have no assurance that task will actually finish in "expected" time, anything from huge log file to random deadlock will trigger this issue again... Also this is possible DoS for installations using default settings... filling a mail.log with random crap is not a hard thing to do.

Fix patch:
diff --git a/modoboa/core/commands/templates/settings.py.tpl b/modoboa/core/commands/templates/settings.py.tpl
index 03f4a7ce2..d9455bcbf 100644
--- a/modoboa/core/commands/templates/settings.py.tpl
+++ b/modoboa/core/commands/templates/settings.py.tpl
@@ -230,6 +230,8 @@ MODOBOA_API_URL = 'https://api.modoboa.org/1/'
 
 DISABLE_DASHBOARD_EXTERNAL_QUERIES = False
 
+PID_FILE_STORAGE_PATH = '/var/run'
+
 # REDIS
 
 REDIS_HOST = 'localhost'
diff --git a/modoboa/maillog/management/commands/logparser.py b/modoboa/maillog/management/commands/logparser.py
index db9e9cc70..4a4380ff9 100644
--- a/modoboa/maillog/management/commands/logparser.py
+++ b/modoboa/maillog/management/commands/logparser.py
@@ -27,6 +27,7 @@
 
 import rrdtool
 
+from django.conf import settings
 from django.core.management.base import BaseCommand
 from django.db.models import Q
 from django.utils import timezone
@@ -619,7 +620,27 @@ def add_arguments(self, parser):
             "--debug", default=False, action="store_true",
             help="Set debug mode")
 
+    def can_start(self):
+        """Check if another process is already running or not."""
+        pidfile = f"{settings.PID_FILE_STORAGE_PATH}/modoboa_logparser.pid"
+        if os.path.exists(pidfile):
+            with open(pidfile) as fp:
+                pid = fp.read()
+            pid = int(pid.strip())
+            try:
+                os.kill(pid, 0)
+            except OSError:
+                pass
+            else:
+                return False
+        with open(pidfile, "w") as fp:
+            fp.write(f"{os.getpid()}\n")
+        return True
+
     def handle(self, *args, **options):
+        if not self.can_start():
+            print("Another process is already running, cannot start")
+            sys.exit(2)
         if options["logfile"] is None:
             options["logfile"] = param_tools.get_global_parameter(
                 "logfile", app="maillog")


Test patch:
diff --git a/modoboa/maillog/tests/test_views.py b/modoboa/maillog/tests/test_views.py
index 5b636e12d..1610bb724 100644
--- a/modoboa/maillog/tests/test_views.py
+++ b/modoboa/maillog/tests/test_views.py
@@ -5,6 +5,7 @@
 import shutil
 import tempfile
 
+from django.conf import settings
 from django.core.management import call_command
 from django.urls import reverse
 from django.test import override_settings
@@ -24,6 +25,9 @@ def setUp(self):
 
     def tearDown(self):
         shutil.rmtree(self.workdir)
+        pid_file = f"{settings.PID_FILE_STORAGE_PATH}/modoboa_logparser.pid"
+        if os.path.exists(pid_file):
+            os.remove(pid_file)
 
     def run_logparser(self):
         """Run logparser command."""
@@ -166,3 +170,10 @@ def test_update_statistics(self):
         self.assertTrue(os.path.exists(path))
         self.run_update_statistics(rebuild=True)
         self.assertTrue(os.path.exists(path))
+
+    def test_locking(self):
+        with open(f"{settings.PID_FILE_STORAGE_PATH}/modoboa_logparser.pid", "w") as fp:
+            fp.write(f"{os.getpid()}\n")
+        with self.assertRaises(SystemExit) as inst:
+            self.run_logparser()
+        self.assertEqual(inst.exception.code, 2)
diff --git a/test_project/test_project/settings.py b/test_project/test_project/settings.py
index da414290e..3cbab366e 100644
--- a/test_project/test_project/settings.py
+++ b/test_project/test_project/settings.py
@@ -212,6 +212,8 @@
 
 MODOBOA_API_URL = 'https://api.modoboa.org/1/'
 
+PID_FILE_STORAGE_PATH = '/tmp'
+
 # REDIS
 
 REDIS_HOST = 'localhost'