From a9ec9974820d983101ba3ac20291d2b2c8ca77f4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?=
 <psuarezhernandez@suse.com>
Date: Mon, 22 Jan 2024 15:46:09 +0000
Subject: [PATCH] Prevent parallel executions of sync tasks via file lock

Fix issue raised by pre-commit

Fix issues raised by pyright

Add docstring for locking functions

Wrap file lock and release functions inside contextmanager

Take care of possible exceptions and reduce complexity

Update cobbler/utils/__init__.py

Co-authored-by: Alexander Graul <agraul@suse.com>
---
 cobbler/api.py         | 28 ++++++++++++++++++++++------
 cobbler/utils.py       | 22 ++++++++++++++++++++++
 tests/api/sync_test.py |  9 +++++++++
 tests/utils_test.py    | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+), 6 deletions(-)
#L3PD: bsc#1218764

diff --git a/cobbler/api.py b/cobbler/api.py
index 896bc9547f..421d3fcd79 100644
--- a/cobbler/api.py
+++ b/cobbler/api.py
@@ -1565,8 +1565,12 @@ def sync_systems(self, systems: List[str], verbose: bool = False):
                 self.logger.debug("sync_systems needs at least one system to do something. Bailing out early.")
                 return
             raise TypeError('Systems must be a list of one or more strings.')
-        sync_obj = self.get_sync(verbose=verbose)
-        sync_obj.run_sync_systems(systems)
+        self.logger.info(
+            "Waiting sync_lock to be available to perform the sync action (this might take some time)"
+        )
+        with utils.filelock("/var/lib/cobbler/sync_lock"):
+            sync_obj = self.get_sync(verbose=verbose)
+            sync_obj.run_sync_systems(systems)
 
     # ==========================================================================
 
@@ -1582,8 +1586,12 @@ def sync(self, verbose: bool = False, what: Optional[list] = None):
         # Empty what: Full sync
         if not what:
             self.logger.info("syncing all")
-            sync_obj = self.get_sync(verbose=verbose)
-            sync_obj.run()
+            self.logger.info(
+                "Waiting sync_lock to be available to perform the sync action (this might take some time)"
+            )
+            with utils.filelock("/var/lib/cobbler/sync_lock"):
+                sync_obj = self.get_sync(verbose=verbose)
+                sync_obj.run()
             return
         # Non empty what: Specific sync
         if not isinstance(what, list):
@@ -1609,7 +1617,11 @@ def sync_dns(self):
             "managers.bind"
         )
         dns = dns_module.get_manager(self)
-        dns.sync()
+        self.logger.info(
+            "Waiting sync_lock to be available to perform the sync action (this might take some time)"
+        )
+        with utils.filelock("/var/lib/cobbler/sync_lock"):
+            dns.sync()
 
     # ==========================================================================
 
@@ -1627,7 +1639,11 @@ def sync_dhcp(self):
             "managers.isc"
         )
         dhcp = dhcp_module.get_manager(self)
-        dhcp.sync()
+        self.logger.info(
+            "Waiting sync_lock to be available to perform the sync action (this might take some time)"
+        )
+        with utils.filelock("/var/lib/cobbler/sync_lock"):
+            dhcp.sync()
 
     # ==========================================================================
 
diff --git a/cobbler/utils.py b/cobbler/utils.py
index ba82821296..53cb7d4bbd 100644
--- a/cobbler/utils.py
+++ b/cobbler/utils.py
@@ -20,8 +20,10 @@
 02110-1301  USA
 """
 
+import contextlib
 import enum
 import errno
+import fcntl
 import glob
 import json
 import logging
@@ -2131,3 +2133,23 @@ def is_str_float(value: str) -> bool:
     except ValueError:
         pass
     return False
+
+
+@contextlib.contextmanager
+def filelock(lock_file: str):
+    """
+    Context manager to acquire a file lock and release it afterwards
+
+    :param lock_file: Path to the file lock to acquire
+    :raises CX: Raised in case of unexpect error acquiring file lock.
+    """
+    fd = None
+    try:
+        fd = os.open(lock_file, os.O_RDWR | os.O_CREAT | os.O_TRUNC, 0o660)
+        fcntl.flock(fd, fcntl.LOCK_EX)
+        yield
+    finally:
+        if fd:
+            with contextlib.suppress(OSError):
+                fcntl.flock(fd, fcntl.LOCK_UN)
+            os.close(fd)
diff --git a/tests/api/sync_test.py b/tests/api/sync_test.py
index e7e3b0d3d2..905ea825a3 100644
--- a/tests/api/sync_test.py
+++ b/tests/api/sync_test.py
@@ -1,3 +1,4 @@
+# type: ignore
 import pytest
 from unittest.mock import Mock, MagicMock, create_autospec, PropertyMock
 
@@ -24,12 +25,14 @@ def test_sync(cobbler_api, input_verbose, input_what, expected_exception, mocker
     mocker.patch.object(cobbler_api, "get_sync", return_value=stub)
     mocker.patch.object(cobbler_api, "sync_dhcp", new=stub_dhcp)
     mocker.patch.object(cobbler_api, "sync_dns", new=stub_dns)
+    filelock_mock = mocker.patch("cobbler.utils.filelock")
 
     # Act
     with expected_exception:
         cobbler_api.sync(input_verbose, input_what)
 
     # Assert
+    assert filelock_mock.called_once_with("/var/lib/cobbler/sync_lock")
     if not input_what:
         stub.run.assert_called_once()
     if input_what and "dhcp" in input_what:
@@ -48,6 +51,7 @@ def test_sync_dns(cobbler_api, input_manage_dns, mocker):
     m_property = PropertyMock(return_value=input_manage_dns)
     type(mock).manage_dns = m_property
     mocker.patch.object(cobbler_api, "settings", return_value=mock)
+    filelock_mock = mocker.patch("cobbler.utils.filelock")
 
     # mock get_manager() and ensure mock object has the same api as the object it is replacing.
     # see https://docs.python.org/3/library/unittest.mock.html#unittest.mock.create_autospec
@@ -58,6 +62,7 @@ def test_sync_dns(cobbler_api, input_manage_dns, mocker):
     cobbler_api.sync_dns()
 
     # Assert
+    assert filelock_mock.called_once_with("/var/lib/cobbler/sync_lock")
     m_property.assert_called_once()
     assert stub.sync.called == input_manage_dns
 
@@ -72,6 +77,7 @@ def test_sync_dhcp(cobbler_api, input_manager_dhcp, mocker):
     m_property = PropertyMock(return_value=input_manager_dhcp)
     type(mock).manage_dhcp = m_property
     mocker.patch.object(cobbler_api, "settings", return_value=mock)
+    filelock_mock = mocker.patch("cobbler.utils.filelock")
 
     stub = create_autospec(spec=cobbler.modules.managers.isc._IscManager)
     mocker.patch("cobbler.modules.managers.isc.get_manager", return_value=stub)
@@ -80,6 +86,7 @@ def test_sync_dhcp(cobbler_api, input_manager_dhcp, mocker):
     cobbler_api.sync_dhcp()
 
     # Assert
+    assert filelock_mock.called_once_with("/var/lib/cobbler/sync_lock")
     m_property.assert_called_once()
     assert stub.sync.called == input_manager_dhcp
 
@@ -111,8 +118,10 @@ def test_sync_systems(cobbler_api, input_systems, input_verbose, expected_except
     # Arrange
     stub = create_autospec(spec=cobbler.actions.sync.CobblerSync)
     mocker.patch.object(cobbler_api, "get_sync", return_value=stub)
+    filelock_mock = mocker.patch("cobbler.utils.filelock")
 
     # Act
+    assert filelock_mock.called_once_with("/var/lib/cobbler/sync_lock")
     with expected_exception:
         cobbler_api.sync_systems(input_systems, input_verbose)
 
diff --git a/tests/utils_test.py b/tests/utils_test.py
index 352d7b894c..8d746ae0ae 100644
--- a/tests/utils_test.py
+++ b/tests/utils_test.py
@@ -1,9 +1,11 @@
+import fcntl
 import os
 import re
 import shutil
 from pathlib import Path
 
 import pytest
+from pytest_mock import MockerFixture
 from netaddr.ip import IPAddress
 
 from cobbler import utils
@@ -1033,3 +1035,35 @@ def test_service_restart_service(mocker):
     # Assert
     assert result == 0
     utils.subprocess_call.assert_called_with(["service", "testservice", "restart"], shell=False)
+
+
+@pytest.mark.parametrize(
+    "flock_side_effect,os_open_mock_calls,os_close_mock_calls,expected_exception",
+    [
+        (None, 1, 0, does_not_raise()),
+        (Exception, 2, 1, pytest.raises(CX)),
+    ],
+)
+def test_filelock(
+    mocker: MockerFixture,
+    flock_side_effect,
+    os_open_mock_calls,
+    os_close_mock_calls,
+    expected_exception,
+):
+    # Arrange
+    os_open_mock = mocker.patch("os.open", return_value=1234)
+    os_close_mock = mocker.patch("os.close")
+    flock_mock = mocker.patch("fcntl.flock", side_effect=flock_side_effect)
+
+    # Act
+    with expected_exception:
+        with utils.filelock("foobar"):
+
+            # Assert
+            assert flock_mock.called_once_with(1234, fcntl.LOCK_EX)
+            assert os_open_mock.call_count == os_open_mock_calls
+            assert os_close_mock.call_count == os_close_mock_calls
+
+        assert flock_mock.called_once_with(1234, fcntl.LOCK_UN)
+        assert os_close_mock.called_once_with(1234)
