config and command-line options to disable locking by tgamblin · Pull Request #7692 · spack/spack

tgamblin · 2018-04-08T08:13:19Z

Depends on #7774. Merge once that's in.

This adds an option to config.yaml that enables and disables locking. This is intended to allow people on filesystems that do not support locking to use Spack more easily.

  # When set to true, concurrent instances of Spack will use locks to
  # avoid modifying the install tree, database file, etc. If false, Spack
  # will disable all locking, but you must NOT run concurrent instances
  # of Spack.  For filesystems that don't support locking, you should set
  # this to false and run one Spack at a time, but otherwise we recommend
  # enabling locks.
  locks: true

You can also now run spack -L ... to disable locking:

$ spack help -a
...
optional arguments:
...
  -l, --enable-locks    force spack to use filesystem locking
  -L, --disable-locks   force spack to disable filesystem locking

This isn't recommended, but sometimes it's the only option.

You can ignore the first commit, which simplifies some import madness to make disabling locks possible. The second two add the actual feature.

@cyrush @junghans @ianfoster

adamjstewart · 2018-04-08T12:15:20Z

lib/spack/spack/__init__.py

 #-----------------------------------------------------------------------------
-# When packages call 'from spack import *', this extra stuff is brought in.
+# When packages call 'from spack import *', we import a set of things that
+# shoudl be useful for builds.


Typo: shoudl

alalazo

The main concern I have is if we should allow users to disable locking from the command line, because if we do we give them a way to work around a global configuration setting.

alalazo · 2018-04-08T13:25:42Z

etc/spack/defaults/config.yaml

+  # of Spack.  For filesystems that don't support locking, you should set
+  # this to false and run one Spack at a time, but otherwise we recommend
+  # enabling locks.
+  locks: true


I wish we could find a simple way to detect concurrent usage when:

locks: false

and inform users that another instance of Spack is running - but right now I can't think of a solution that is 100% correct...

I mean, this is what the locks are for 😄 If there was a good way to do this, we wouldn't need locks.

Well, right now we are (cleverly, and thanks to your lock implementation) using flock features to permit concurrent use of Spack from different users. I was wondering if we can have a simpler global lock that doesn't expose users to potential races, and doesn't use flock. The idea is to exit with a message like:

Spack is currently being used by somebody else. Retry later.

if the lock is already taken, instead of saying to users that they are on their own.

EDIT: this is based on my understanding that the real issue here is missing flock support, not locking in itself

Although it's probably not applicable here: last time we encountered a FS without lock capabilitities (Piz Daint), I just changed the logic to created locks for pseudo-files on another shared filesystem of the machine...

For Spack locks: false in the SPACK_ROOT, one could also think about setting a path for locks of shared pseudo-files... not sure it it's worth the effort (I did not do this with spack but with some post-processing).

simpler global lock

just a simple file with an identifier, say an uuid created by the first process to win, would probably work. you only get into trouble if this process dies in an unclean state and all follow-up commands are deadlocking on the global lock :)

alalazo · 2018-04-08T13:35:25Z

lib/spack/docs/basic_usage.rst

-filesystems are mounted with ``mount -p``.  The output for a Lustre
+versions of NFS support this, but parallel filesystems or NFS volumes may
+be configured without ``flock`` support enabled.  You can determine how
+your filesystems are mounted with ``mount -p``.  The output for a Lustre


Is -p the right option? Asking because I don't use mount very often, but reading the manual on my Ubuntu 14.04:

-p, --pass-fd num In case of a loop mount with encryption, read the passphrase from file descriptor num instead of from the terminal.

Good question. -p isn't an option on my mac and doesn't seem to work on our clusters either. I'm not sure where I got that. Just mount seems to work so I'll revise this.

alalazo · 2018-04-08T14:06:24Z

lib/spack/llnl/util/lock/dummy.py

+in your code.
+"""
+import llnl.util.lock
+from llnl.util.lock import *  # noqa


An alternative way of structuring the code could be:

lock/ __init__.py # almost empty, selects the implementation based on user's request and exports classes _flock.py # current implementation _dummy.py # dummy implementation

Both implementations are fine with me, just want to point out this different layout if you didn't already consider it.

I think I like the current one b/c you explicitly import dummy, and the details of selecting an implementation are left up to spack.util.lock. How would the importer control which version is imported in the layout above?

If you want to keep the selection logic in spack.util.lock an alternative is:

lock/ __init__.py # empty flock.py # current implementation dummy.py # dummy implementation

and the importer can select among llnl.util.lock.flock and llnl.util.lock.dummy. As I said, no strong preference here, just wanted to point out another option in case it wasn't weighed already.

alalazo · 2018-04-08T14:07:49Z

lib/spack/spack/__init__.py


 # The spack script itself
-spack_file = join_path(spack_root, "bin", "spack")
+spack_file = os.path.join(spack_root, "bin", "spack")


👍 to this and similar changes below

alalazo · 2018-04-08T14:17:44Z

lib/spack/spack/main.py

+                        help="force spack to use filesystem locking")
+    parser.add_argument('-L', '--disable-locks', action='store_true',
+                        dest='locks',
+                        help="force spack to disable filesystem locking")


I think there's an error here. Shouldn't the second argument store_false, i.e. :

parser.add_argument('-L', '--disable-locks', action='store_false', dest='locks', help="force spack to disable filesystem locking")

?

Well that's an important catch. Thanks!

alalazo · 2018-04-08T14:21:55Z

lib/spack/spack/main.py


+    # override lock configuration if passed on command line
+    if args.locks is not None:
+        spack.locks = args.locks


I wonder if we should expose this option to the command line at all. If we enable/disable locks, it should be for all the users of a particular Spack instance (and we should leave them no way to work around this setting without modifying the configuration file).

tgamblin · 2018-04-08T15:48:46Z

I wonder if we should expose this option to the command line at all. If we enable/disable locks, it should be for all the users of a particular Spack instance (and we should leave them no way to work around this setting without modifying the configuration file).

I thought about this. I don't think it makes much difference safety-wise -- users can override this setting in their own user config already, so they can get around any system-wide lock setting. Personally I think that's good. I like the user control, and I think users should be able to be unsafe if they want.

I also think the users are only going to find this after reading documentation or at least spack help -a, so they will know the risks. I suspect most people will find this after something goes wrong -- most won't even know it exists. I added a (unsafe) after the -L option to make it clear that this is dangerous.

One thing we could do is either fail or warn if the spack instance the user is running is group-writeable and the group isn't the owner's group or the owner isn't the calling user. Those are signs that the installation might be intended for use by a group. Thoughts? Personally I think this is overkill and maybe something for a later PR.

tgamblin · 2018-04-08T15:52:03Z

Ok I think this addresses the feedback @alalazo?

alalazo · 2018-04-08T16:03:18Z

One thing we could do is either fail or warn if the spack instance the user is running is group-writeable and the group isn't the owner's group or the owner isn't the calling user. Those are signs that the installation might be intended for use by a group.

That's exactly my concern. I agree with you that users are well informed of the risks by docs - but the people I know in HPC don't read them 😆

Ok I think this addresses the feedback

👍 I don't have additional comments

tgamblin · 2018-04-08T17:21:25Z

I think I'll add the check anyway. Trying to do so has revealed some other races at init time. There's a race, and the -L preference can't currently be set before spack.util.lock is imported.

I think I need to do some more surgery on config, main, and __init__.py, so hold off on merging until I fix this and add more tests.

tgamblin · 2018-05-14T07:21:45Z

@alalazo: this is ready for another look. It's just the last commit now.

cyrush

Not qualified to audit all of the changes in the guts, but the conf.yaml and the command line options look great!

…ocks - spack.util.lock behaves the same as llnl.util.lock, but Lock._lock and Lock._unlock do nothing. - can be disabled with a control variable. - configuration options can enable/disable locking: - `locks` option in spack configuration controls whether Spack will use filesystem locks or not. - `-l` and `-L` command-line options can force-disable or force-enable locking. - Spack will check for group- and world-writability before disabling locks, and it will not allow a group- or world-writable instance to have locks disabled. - update documentation

tgamblin added configuration locking labels Apr 8, 2018

tgamblin requested review from alalazo and cyrush April 8, 2018 08:13

tgamblin changed the title ~~Add config and command-line options to enable/disable locking~~ Add config and command-line options to disable locking Apr 8, 2018

tgamblin changed the title ~~Add config and command-line options to disable locking~~ config and command-line options to disable locking Apr 8, 2018

tgamblin force-pushed the features/lock-config branch from 4482cc6 to 53cc1ac Compare April 8, 2018 08:19

adamjstewart reviewed Apr 8, 2018

View reviewed changes

alalazo reviewed Apr 8, 2018

View reviewed changes

tgamblin force-pushed the features/lock-config branch from 53cc1ac to cb1afc9 Compare April 8, 2018 15:51

tgamblin added the don't-merge-yet label Apr 8, 2018

tgamblin mentioned this pull request Apr 16, 2018

Refactor initialization and configuration to remove globals #7774

Merged

6 tasks

tgamblin force-pushed the features/lock-config branch 3 times, most recently from 22fd709 to a41b255 Compare May 14, 2018 07:21

cyrush approved these changes May 15, 2018

View reviewed changes

tgamblin force-pushed the features/lock-config branch from a41b255 to 7e6897f Compare May 17, 2018 23:15

tgamblin removed the don't-merge-yet label May 17, 2018

tgamblin merged commit 54201e3 into develop May 18, 2018

alalazo mentioned this pull request May 21, 2018

Lock unit test failing on Travis MacOS builder #8225

Closed

junghans deleted the features/lock-config branch June 16, 2018 18:22

Conversation

tgamblin commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Apr 8, 2018

Uh oh!

tgamblin commented Apr 8, 2018

Uh oh!

alalazo commented Apr 8, 2018

Uh oh!

tgamblin commented Apr 8, 2018

Uh oh!

tgamblin commented May 14, 2018

Uh oh!

cyrush left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tgamblin commented Apr 8, 2018 •

edited

Loading

alalazo Apr 8, 2018 •

edited

Loading

ax3l Apr 10, 2018 •

edited

Loading