add --wait-on-lock-limit and --wait-on-lock-interval configuration options, deprecate --wait-on-lock

boegel · boegel · commit d879cdadc9a9 · 2020-05-01T11:16:46.000+02:00
diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py
@@ -102,6 +102,8 @@
 DEFAULT_PNS = 'EasyBuildPNS'
 DEFAULT_PREFIX = os.path.join(os.path.expanduser('~'), ".local", "easybuild")
 DEFAULT_REPOSITORY = 'FileRepository'
+DEFAULT_WAIT_ON_LOCK_INTERVAL = 60
+DEFAULT_WAIT_ON_LOCK_LIMIT = 0
 
 EBROOT_ENV_VAR_ACTIONS = [ERROR, IGNORE, UNSET, WARN]
 LOADED_MODULES_ACTIONS = [ERROR, IGNORE, PURGE, UNLOAD, WARN]
@@ -211,6 +213,7 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX):
         'subdir_user_modules',
         'test_report_env_filter',
         'testoutput',
+        'wait_on_lock',
         'umask',
         'zip_logs',
     ],
@@ -256,7 +259,7 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX):
         'use_f90cache',
         'use_existing_modules',
         'set_default_module',
-        'wait_on_lock',
+        'wait_on_lock_limit',
     ],
     True: [
         'cleanup_builddir',
@@ -305,6 +308,9 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX):
     DEFAULT_ALLOW_LOADED_MODULES: [
         'allow_loaded_modules',
     ],
+    DEFAULT_WAIT_ON_LOCK_INTERVAL: [
+        'wait_on_lock_interval',
+    ],
 }
 # build option that do not have a perfectly matching command line option
 BUILD_OPTIONS_OTHER = {
diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py
@@ -60,7 +60,7 @@
 from easybuild.tools import run
 # import build_log must stay, to use of EasyBuildLog
 from easybuild.tools.build_log import EasyBuildError, dry_run_msg, print_msg, print_warning
-from easybuild.tools.config import GENERIC_EASYBLOCK_PKG, build_option, install_path
+from easybuild.tools.config import DEFAULT_WAIT_ON_LOCK_INTERVAL, GENERIC_EASYBLOCK_PKG, build_option, install_path
 from easybuild.tools.py2vs3 import std_urllib, string_type
 from easybuild.tools.utilities import nub, remove_unwanted_chars
 
@@ -1531,12 +1531,40 @@ def check_lock(lock_name):
     lock_path = det_lock_path(lock_name)
     if os.path.exists(lock_path):
         _log.info("Lock %s exists!", lock_path)
+
+        wait_interval = build_option('wait_on_lock_interval')
+        wait_limit = build_option('wait_on_lock_limit')
+
+        # --wait-on-lock is deprecated, should use --wait-on-lock-limit and --wait-on-lock-interval instead
         wait_on_lock = build_option('wait_on_lock')
-        if wait_on_lock:
-            while os.path.exists(lock_path):
-                print_msg("lock %s exists, waiting %d seconds..." % (lock_path, wait_on_lock),
+        if wait_on_lock is not None:
+            depr_msg = "Use of --wait-on-lock is deprecated, use --wait-on-lock-limit and --wait-on-lock-interval"
+            _log.deprecated(depr_msg, '5.0')
+
+            # if --wait-on-lock-interval has default value and --wait-on-lock is specified too, the latter wins
+            # (required for backwards compatibility)
+            if wait_interval == DEFAULT_WAIT_ON_LOCK_INTERVAL and wait_on_lock > 0:
+                wait_interval = wait_on_lock
+
+            # if --wait-on-lock-limit is not specified we need to wait indefinitely if --wait-on-lock is specified,
+            # since the original semantics of --wait-on-lock was that it specified the waiting time interval (no limit)
+            if not wait_limit:
+                wait_limit = -1
+
+        # wait limit could be zero (no waiting), -1 (no waiting limit) or non-zero value (waiting limit in seconds)
+        if wait_limit != 0:
+            wait_time = 0
+            while os.path.exists(lock_path) and (wait_limit == -1 or wait_time < wait_limit):
+                print_msg("lock %s exists, waiting %d seconds..." % (lock_path, wait_interval),
                           silent=build_option('silent'))
-                time.sleep(wait_on_lock)
+                time.sleep(wait_interval)
+                wait_time += wait_interval
+
+            if wait_limit != -1 and wait_time >= wait_limit:
+                error_msg = "Maximum wait time for lock %s to be released reached: %s sec >= %s sec"
+                raise EasyBuildError(error_msg, lock_path, wait_time, wait_limit)
+            else:
+                _log.info("Lock %s was released!", lock_path)
         else:
             raise EasyBuildError("Lock %s already exists, aborting!", lock_path)
     else:
diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py
@@ -64,10 +64,10 @@
 from easybuild.tools.config import DEFAULT_JOB_BACKEND, DEFAULT_LOGFILE_FORMAT, DEFAULT_MAX_FAIL_RATIO_PERMS
 from easybuild.tools.config import DEFAULT_MNS, DEFAULT_MODULE_SYNTAX, DEFAULT_MODULES_TOOL, DEFAULT_MODULECLASSES
 from easybuild.tools.config import DEFAULT_PATH_SUBDIRS, DEFAULT_PKG_RELEASE, DEFAULT_PKG_TOOL, DEFAULT_PKG_TYPE
-from easybuild.tools.config import DEFAULT_PNS, DEFAULT_PREFIX, DEFAULT_REPOSITORY, EBROOT_ENV_VAR_ACTIONS, ERROR
-from easybuild.tools.config import FORCE_DOWNLOAD_CHOICES, GENERAL_CLASS, IGNORE, JOB_DEPS_TYPE_ABORT_ON_ERROR
-from easybuild.tools.config import JOB_DEPS_TYPE_ALWAYS_RUN, LOADED_MODULES_ACTIONS, WARN
-from easybuild.tools.config import LOCAL_VAR_NAMING_CHECK_WARN, LOCAL_VAR_NAMING_CHECKS
+from easybuild.tools.config import DEFAULT_PNS, DEFAULT_PREFIX, DEFAULT_REPOSITORY, DEFAULT_WAIT_ON_LOCK_INTERVAL
+from easybuild.tools.config import DEFAULT_WAIT_ON_LOCK_LIMIT, EBROOT_ENV_VAR_ACTIONS, ERROR, FORCE_DOWNLOAD_CHOICES
+from easybuild.tools.config import GENERAL_CLASS, IGNORE, JOB_DEPS_TYPE_ABORT_ON_ERROR, JOB_DEPS_TYPE_ALWAYS_RUN
+from easybuild.tools.config import LOADED_MODULES_ACTIONS, LOCAL_VAR_NAMING_CHECK_WARN, LOCAL_VAR_NAMING_CHECKS, WARN
 from easybuild.tools.config import get_pretend_installpath, init, init_build_options, mk_full_default_path
 from easybuild.tools.configobj import ConfigObj, ConfigObjError
 from easybuild.tools.docs import FORMAT_TXT, FORMAT_RST
@@ -76,9 +76,8 @@
 from easybuild.tools.docs import list_easyblocks, list_toolchains
 from easybuild.tools.environment import restore_env, unset_env_vars
 from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256, CHECKSUM_TYPES, install_fake_vsc, move_file, which
-from easybuild.tools.github import GITHUB_EB_MAIN, GITHUB_EASYCONFIGS_REPO
-from easybuild.tools.github import GITHUB_PR_DIRECTION_DESC, GITHUB_PR_ORDER_CREATED, GITHUB_PR_STATE_OPEN
-from easybuild.tools.github import GITHUB_PR_STATES, GITHUB_PR_ORDERS, GITHUB_PR_DIRECTIONS
+from easybuild.tools.github import GITHUB_EB_MAIN, GITHUB_PR_DIRECTION_DESC, GITHUB_PR_ORDER_CREATED
+from easybuild.tools.github import GITHUB_PR_STATE_OPEN, GITHUB_PR_STATES, GITHUB_PR_ORDERS, GITHUB_PR_DIRECTIONS
 from easybuild.tools.github import HAVE_GITHUB_API, HAVE_KEYRING, VALID_CLOSE_PR_REASONS
 from easybuild.tools.github import fetch_easyblocks_from_pr, fetch_github_token
 from easybuild.tools.hooks import KNOWN_HOOKS
@@ -442,8 +441,15 @@ def override_options(self):
                                      None, 'store_true', False),
             'verify-easyconfig-filenames': ("Verify whether filename of specified easyconfigs matches with contents",
                                             None, 'store_true', False),
-            'wait-on-lock': ("Wait interval (in seconds) to use when waiting for existing lock to be removed "
-                             "(0: implies no waiting, but exiting with an error)", int, 'store', 0),
+            'wait-on-lock': ("Wait for lock to be released; 0 implies no waiting (exit with an error if the lock "
+                             "already exists), non-zero value specified waiting interval [DEPRECATED: "
+                             "use --wait-on-lock-interval and --wait-on-lock-limit instead]",
+                             int, 'store_or_None', None),
+            'wait-on-lock-interval': ("Wait interval (in seconds) to use when waiting for existing lock to be removed",
+                                      int, 'store', DEFAULT_WAIT_ON_LOCK_INTERVAL),
+            'wait-on-lock-limit': ("Maximum amount of time (in seconds) to wait until lock is released (0 means no "
+                                   "waiting at all, exit with error; -1 means no waiting limit, keep waiting)",
+                                   int, 'store', DEFAULT_WAIT_ON_LOCK_LIMIT),
             'zip-logs': ("Zip logs that are copied to install directory, using specified command",
                          None, 'store_or_None', 'gzip'),
 
diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py
@@ -2759,43 +2759,86 @@ def __enter__(self):
             def __exit__(self, type, value, traceback):
                 pass
 
-        # wait for lock to be removed, with 1 second interval of checking
-        extra_args.append('--wait-on-lock=1')
+        # wait for lock to be removed, with 1 second interval of checking;
+        # check with both --wait-on-lock-interval and deprecated --wait-on-lock options
 
         wait_regex = re.compile("^== lock .*_software_toy_0.0.lock exists, waiting 1 seconds", re.M)
         ok_regex = re.compile("^== COMPLETED: Installation ended successfully", re.M)
 
-        self.assertTrue(os.path.exists(toy_lock_path))
+        test_cases = [
+            ['--wait-on-lock=1'],
+            ['--wait-on-lock=1', '--wait-on-lock-interval=60'],
+            ['--wait-on-lock=100', '--wait-on-lock-interval=1'],
+            ['--wait-on-lock-limit=100', '--wait-on-lock=1'],
+            ['--wait-on-lock-limit=100', '--wait-on-lock-interval=1'],
+            ['--wait-on-lock-limit=-1', '--wait-on-lock=1'],
+            ['--wait-on-lock-limit=-1', '--wait-on-lock-interval=1'],
+        ]
 
-        # use context manager to remove lock after 3 seconds
-        with remove_lock_after(3, toy_lock_path):
-            self.mock_stderr(True)
-            self.mock_stdout(True)
-            self.test_toy_build(extra_args=extra_args, verify=False, raise_error=True, testing=False)
-            stderr, stdout = self.get_stderr(), self.get_stdout()
-            self.mock_stderr(False)
-            self.mock_stdout(False)
+        for opts in test_cases:
 
-            self.assertEqual(stderr, '')
+            if any('--wait-on-lock=' in x for x in opts):
+                self.allow_deprecated_behaviour()
+            else:
+                self.disallow_deprecated_behaviour()
 
-            wait_matches = wait_regex.findall(stdout)
-            # we can't rely on an exact number of 'waiting' messages, so let's go with a range...
-            self.assertTrue(len(wait_matches) in range(2, 5))
+            if not os.path.exists(toy_lock_path):
+                mkdir(toy_lock_path)
 
-            self.assertTrue(ok_regex.search(stdout), "Pattern '%s' found in: %s" % (ok_regex.pattern, stdout))
+            self.assertTrue(os.path.exists(toy_lock_path))
+
+            all_args = extra_args + opts
+
+            # use context manager to remove lock after 3 seconds
+            with remove_lock_after(3, toy_lock_path):
+                self.mock_stderr(True)
+                self.mock_stdout(True)
+                self.test_toy_build(extra_args=all_args, verify=False, raise_error=True, testing=False)
+                stderr, stdout = self.get_stderr(), self.get_stdout()
+                self.mock_stderr(False)
+                self.mock_stdout(False)
 
-        # when there is no lock in place, --wait-on-lock has no impact
-        self.assertFalse(os.path.exists(toy_lock_path))
+                if any('--wait-on-lock=' in x for x in all_args):
+                    self.assertTrue("Use of --wait-on-lock is deprecated" in stderr)
+                else:
+                    self.assertEqual(stderr, '')
+
+                wait_matches = wait_regex.findall(stdout)
+                # we can't rely on an exact number of 'waiting' messages, so let's go with a range...
+                self.assertTrue(len(wait_matches) in range(2, 5))
+
+                self.assertTrue(ok_regex.search(stdout), "Pattern '%s' found in: %s" % (ok_regex.pattern, stdout))
+
+        # check use of --wait-on-lock-limit: if lock is never removed, we should give up when limit is reached
+        mkdir(toy_lock_path)
+        all_args = extra_args + ['--wait-on-lock-limit=3', '--wait-on-lock-interval=1']
         self.mock_stderr(True)
         self.mock_stdout(True)
-        self.test_toy_build(extra_args=extra_args, verify=False, raise_error=True, testing=False)
+        error_pattern = r"Maximum wait time for lock /.*toy_0.0.lock to be released reached: [0-9]+ sec >= 3 sec"
+        self.assertErrorRegex(EasyBuildError, error_pattern, self.test_toy_build, extra_args=all_args,
+                              verify=False, raise_error=True, testing=False)
         stderr, stdout = self.get_stderr(), self.get_stdout()
         self.mock_stderr(False)
         self.mock_stdout(False)
 
-        self.assertEqual(stderr, '')
-        self.assertTrue(ok_regex.search(stdout), "Pattern '%s' found in: %s" % (ok_regex.pattern, stdout))
-        self.assertFalse(wait_regex.search(stdout), "Pattern '%s' not found in: %s" % (wait_regex.pattern, stdout))
+        wait_matches = wait_regex.findall(stdout)
+        self.assertTrue(len(wait_matches) in range(2, 5))
+
+        # when there is no lock in place, --wait-on-lock* has no impact
+        remove_dir(toy_lock_path)
+        for opt in ['--wait-on-lock=1', '--wait-on-lock-limit=3', '--wait-on-lock-interval=1']:
+            all_args = extra_args + [opt]
+            self.assertFalse(os.path.exists(toy_lock_path))
+            self.mock_stderr(True)
+            self.mock_stdout(True)
+            self.test_toy_build(extra_args=all_args, verify=False, raise_error=True, testing=False)
+            stderr, stdout = self.get_stderr(), self.get_stdout()
+            self.mock_stderr(False)
+            self.mock_stdout(False)
+
+            self.assertEqual(stderr, '')
+            self.assertTrue(ok_regex.search(stdout), "Pattern '%s' found in: %s" % (ok_regex.pattern, stdout))
+            self.assertFalse(wait_regex.search(stdout), "Pattern '%s' not found in: %s" % (wait_regex.pattern, stdout))
 
         # check for clean error on creation of lock
         extra_args = ['--locks-dir=/']
