From 62cbd3ab19bc5f59945d7ad16f328c5f9feb164f Mon Sep 17 00:00:00 2001
From: Lukas Abelt <github@login.luabelt.de>
Date: Tue, 23 Apr 2024 09:21:27 +0200
Subject: [PATCH] Adds lock to FM provider (#882)

When running experiments on slurm, it can happen that multiple nodes fetch the feature model repository at the same time. As nodes may use a shared feature model repository (E.g. on /scratch/) this can lead to issues.

This is no issue if the feature model repository is already up-to-date, however if there are any upstream changes this leads to errors in the fetch on some nodes and subsequent failure of the slurm jobs.


---------

Co-authored-by: Lukas Abelt <git@login.luabelt.de>
Co-authored-by: Florian Sattler <vuld3r@gmail.com>
---
 tests/utils/test_filesystem_util.py           | 48 +++++++++++++++++++
 .../feature/feature_model_provider.py         |  6 ++-
 varats-core/varats/utils/filesystem_util.py   |  3 ++
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 tests/utils/test_filesystem_util.py

diff --git a/tests/utils/test_filesystem_util.py b/tests/utils/test_filesystem_util.py
new file mode 100644
index 000000000..4e444976a
--- /dev/null
+++ b/tests/utils/test_filesystem_util.py
@@ -0,0 +1,48 @@
+"""Test the filesystem utils of VaRA-TS."""
+import errno
+import os
+import unittest
+import uuid
+from fcntl import flock, LOCK_EX, LOCK_NB, LOCK_UN
+
+from varats.utils.filesystem_util import lock_file
+
+
+class TestFileLock(unittest.TestCase):
+    """Tests whether the lock context manager works correctly."""
+
+    def test_file_locking(self):
+        """Test that the file is locked when in a context manager."""
+        tmp_lock_file = "/tmp/lock-test.lock"
+
+        with lock_file(tmp_lock_file):
+            # File should automatically be created
+            self.assertTrue(os.path.exists(tmp_lock_file))
+
+            f = os.open(tmp_lock_file, os.O_RDONLY)
+
+            with self.assertRaises(OSError) as context:
+                # A non-blocking attempt to lock the file again should fail immediately
+                flock(f, LOCK_EX | LOCK_NB)
+            os.close(f)
+            self.assertEqual(context.exception.errno, errno.EWOULDBLOCK)
+
+        # Attempting to lock the file and immediately unlocking should now work
+        f = os.open(tmp_lock_file, os.O_RDONLY)
+        flock(f, LOCK_EX | LOCK_NB)
+        flock(f, LOCK_UN)
+        os.close(f)
+
+    def test_lock_file_new_folder(self):
+        """Test that the lock context manager works correctly when the lock file
+        is in a new folder."""
+        tmp_lock_file = f"/tmp/{uuid.uuid4()}"
+
+        while os.path.isdir(tmp_lock_file):
+            tmp_lock_file = f"/tmp/{uuid.uuid4()}"
+
+        tmp_lock_file += "/lock-test.lock"
+
+        with lock_file(tmp_lock_file):
+            # File should automatically be created
+            self.assertTrue(os.path.exists(tmp_lock_file))
diff --git a/varats-core/varats/provider/feature/feature_model_provider.py b/varats-core/varats/provider/feature/feature_model_provider.py
index 90c9bf4d8..69d2d7cca 100644
--- a/varats-core/varats/provider/feature/feature_model_provider.py
+++ b/varats-core/varats/provider/feature/feature_model_provider.py
@@ -7,6 +7,7 @@
 from benchbuild.source.base import target_prefix
 
 from varats.provider.provider import Provider
+from varats.utils.filesystem_util import lock_file
 
 
 class FeatureModelNotFound(FileNotFoundError):
@@ -90,6 +91,9 @@ def _get_feature_model_repository_path() -> Path:
             refspec="origin/HEAD",
             limit=1,
         )
-        fm_source.fetch()
+
+        lock_path = Path(target_prefix()) / "fm_provider.lock"
+        with lock_file(lock_path):
+            fm_source.fetch()
 
         return Path(Path(target_prefix()) / fm_source.local)
diff --git a/varats-core/varats/utils/filesystem_util.py b/varats-core/varats/utils/filesystem_util.py
index 258fb5e27..4f6ff107d 100644
--- a/varats-core/varats/utils/filesystem_util.py
+++ b/varats-core/varats/utils/filesystem_util.py
@@ -20,6 +20,9 @@ def __init__(self, folder: tp.Union[Path, str]) -> None:
 @contextmanager
 def lock_file(lock_path: Path,
               lock_mode: int = fcntl.LOCK_EX) -> tp.Generator[None, None, None]:
+    # Create directories until lock file if required
+    os.makedirs(os.path.dirname(lock_path), exist_ok=True)
+
     open_mode = os.O_RDWR | os.O_CREAT | os.O_TRUNC
     lock_fd = os.open(lock_path, open_mode)
     try: