Finer-grained locking by tgamblin · Pull Request #1562 · spack/spack

tgamblin · 2016-08-19T16:18:02Z

Fixes #1266, #1966.

Rebased #866 on develop. See discussion there.

To recap, from @alalazo:

Modifications

removed global db locks from install, uninstall and diy
added lock for stage directory (an exclusive lock is taken as part of the Stage context manager)
added lock for prefix directory (an exclusive lock is taken during Package.do_install)
debug output shows locks acquire and release
Use single lock file instead of per-prefix locks.

Install and uninstall are not mutually exclusive

This means that if :

spack uninstall ...

is run concurrently with

spack install ...

the former command may cause the latter to fail (the same way an administrator can sudo rm ... something that another admin is about to use)

trws · 2016-08-19T19:39:11Z

This looks good, I will admit though that I'm a bit wary of it without any tests.

tgamblin · 2016-08-21T08:13:49Z

@alalazo @trws: I played around with this a bit and I think a few things are needed. I'm worried that the lock files:

tend to pile up in the install and stage directories (there are a lot of them), and
They're all in one directory.

Depending on the parallel FS, having all the locks in one directory can be good or bad, but when it's bad it can be really bad. So I would like to somehow get the locks inside the stage and prefix directories.

Given those requirements, I played around with Spack's lock file a bit. It uses readers-writer locks because it has heretofore only had to deal with locking single files (like the DB) that stick around. I think that is the right way to lock those files. However, the install and stage directories are transient, and they'll always need an exclusive lock (not reader locks).

I think the best thing to do would be to use simple exclusive file locks (open with O_EXCL) to control these directories. They have the advantage that the files only exist when in use, and I think we could implement them such that they live inside the install prefix and/or stage. I can't do that with reader-writer locks. RW locks don't play nicely with being deleted -- you get races between open and the first call to fcntl that I don't think are possible to get rid of when the lock files are deleted on release.

So, if you guys aren't opposed, I would like to take a stab at introducing a new exclusive file lock (pretty simple) and using that inside the stage and prefix directories where @alalazo currently uses the current exclusive fcntl locks.

Thoughts?

alalazo · 2016-08-21T09:50:33Z

So, if you guys aren't opposed, I would like to take a stab at introducing a new exclusive file lock (pretty simple) and using that inside the stage and prefix directories where @alalazo currently uses the current exclusive fcntl locks.

👍 This will surely be an improvement over the current implementation

trws · 2016-08-22T17:13:11Z

That sounds like a good idea to me, as long as we can count on an outer lock to prevent creation of multiple versions of an otherwise redundant stage directory.

tgamblin · 2016-10-06T08:54:45Z

@alalazo: this is reworked. Can you test it out?

The approach is slightly different (but better IMHO) than what I suggested above. I figured out how we can have a single lock file for all the prefixes and a single file for all the stages and ALSO get readers-writer semantics.

How it works for prefixes:

The lock file is in opt/spack/.spack-db/prefix_lock and it lives alongside the install DB.
Each lock is an fcntl byte-range lock on the nth byte of a file. Byte range locks can extend past the end of a file w/o taking up space, so we can have up to 2^63-1 of these on 64-bit machines.
n is the sys.maxsize-bit prefix of the DAG hash.

Stages are similar -- they use a prefix of the sha1 of the stage name instead of the DAG hash prefix.

With 100 concurrent builds, the likelihood of a lock collision (false positive) is ~5.36e-16, so this scheme should retain more than sufficient parallelism (with no chance of false negatives), and get us reader-writer lock semantics with a single file, so no need to clean up lots of lock files.

tgamblin · 2016-10-06T08:56:58Z

Also this fixes #1266.

tgamblin · 2016-10-06T09:56:16Z

Also, if anyone is interested, both of these are good reads:

TL; DR: fcntl locks (lockf in Python... sigh) work on NFS v2.something and up, and most sites deploy NFS v3 or v4, where they work now. They also work on GPFS and Lustre. I believe that covers most filesystems we care about. These locks should let us do parallel builds, and the readers-writer semantics should be useful for things like taking read locks out on dependencies while packages are being installed (preventing others from uninstalling them while that happens and before the new package that depends on them is in the DB).

alalazo · 2016-10-06T10:36:48Z

@tgamblin Thanks for the references above!

alalazo · 2016-10-06T10:50:10Z

Each lock is an fcntl byte-range lock on the nth byte of a file. Byte range locks can extend past the end of a file w/o taking up space, so we can have up to 2^63-1 of these on 64-bit machines.

Quite clever! I didn't know you could do something like this.

tgamblin · 2016-10-06T15:14:01Z

@alalazo: I still wish there was zero chance of taking a lock unnecessarily but sha1 collision likelihood is very low even for a prefix.

If this works for you I'll merge. Note that this also fixes a bug with the backported read only lock fix.

tgamblin · 2016-10-06T18:53:51Z

@adamjstewart @davydden: I need someone besides me to test this -- if either of you have time can you verify it fixes #1266?

adamjstewart · 2016-10-06T18:56:12Z

Will do.

adamjstewart · 2016-10-06T19:02:54Z

I'm seeing some problems on my end:

[01:59:45] ajstewart@blogin4:/soft/spack-0.9.1 (develop)
$ git co features/db-locking
Branch features/db-locking set up to track remote branch features/db-locking from upstream.
Switched to a new branch 'features/db-locking'
[01:59:52] ajstewart@blogin4:/soft/spack-0.9.1 (features/db-locking)
$ spack edit m4
[02:00:14] ajstewart@blogin4:/soft/spack-0.9.1 (features/db-locking *)
$ spack install gmp
==> gmp is already installed in /blues/gpfs/home/software/spack-0.9.1/opt/spack/linux-centos6-x86_64/gcc-6.1.0/gmp-6.1.1-e3x7lcgg4qizghb53izi72aep6ybswvp
Traceback (most recent call last):
  File "/soft/spack-0.9.1/bin/spack", line 192, in <module>
    main()
  File "/soft/spack-0.9.1/bin/spack", line 169, in main
    return_val = command(parser, args)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/cmd/install.py", line 97, in install
    explicit=True)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/package.py", line 981, in do_install
    rec = spack.installed_db.get_record(self.spec)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 79, in converter
    return function(self, spec_like, *args, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 527, in get_record
    key = self._get_matching_spec_key(spec, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 519, in _get_matching_spec_key
    match = self.query_one(spec, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 653, in query_one
    assert len(concrete_specs) <= 1
AssertionError
[02:00:24] ajstewart@blogin4:/soft/spack-0.9.1 (features/db-locking *)
$ spack reindex
[02:01:28] ajstewart@blogin4:/soft/spack-0.9.1 (features/db-locking *)
$ spack install gmp
==> gmp is already installed in /blues/gpfs/home/software/spack-0.9.1/opt/spack/linux-centos6-x86_64/gcc-6.1.0/gmp-6.1.1-e3x7lcgg4qizghb53izi72aep6ybswvp
Traceback (most recent call last):
  File "/soft/spack-0.9.1/bin/spack", line 192, in <module>
    main()
  File "/soft/spack-0.9.1/bin/spack", line 169, in main
    return_val = command(parser, args)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/cmd/install.py", line 97, in install
    explicit=True)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/package.py", line 981, in do_install
    rec = spack.installed_db.get_record(self.spec)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 79, in converter
    return function(self, spec_like, *args, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 527, in get_record
    key = self._get_matching_spec_key(spec, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 519, in _get_matching_spec_key
    match = self.query_one(spec, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 653, in query_one
    assert len(concrete_specs) <= 1
AssertionError

alalazo · 2016-10-06T20:50:42Z

@tgamblin I'll have a chance to test this tomorrow...

tgamblin · 2016-10-06T21:00:16Z

@adamjstewart: can you attach output of spack debug create-db-tarball?

adamjstewart · 2016-10-06T21:11:36Z

Um, I'm not sure if I'm doing this right...

$ spack debug create-db-tarball > create-db-tarball.txt
tar (child): /blues/gpfs/home/software/spack-0.9.1/spack-db.features/db-locking.ca5f5ee.2016-10-06-160940.tar.gz: Cannot open: No such file or directory
tar (child): Error is not recoverable: exiting now
/bin/tar: /blues/gpfs/home/software/spack-0.9.1/spack-db.features/db-locking.ca5f5ee.2016-10-06-160940.tar.gz: Wrote only 4096 of 10240 bytes
/bin/tar: Error is not recoverable: exiting now
==> Error: Command exited with status 2:

alalazo · 2016-10-07T08:37:07Z

@tgamblin I just installed in parallel :

spack install -j 1 hdf5+mpi ^ mpich
spack install -j 1 cgal
spack install -j 1 [email protected]+icu+python+graph

on a Ubuntu 14.04 laptop with [email protected] : everything went well and couldn't see the error @adamjstewart reported.

tgamblin · 2016-10-10T22:42:58Z

But, I tried following my steps to reproduce the bug and it didn't occur this time, so that's promising!

That is promising. I think your error might be coming from something else -- I just want to confirm with the DB test data.

adamjstewart · 2016-10-10T23:02:31Z

@tgamblin Good catch! spack debug create-db-tarball works for me again.

Although my steps to reproduce the bug can no longer reproduce the bug on my laptop (macOS), I'm still seeing a problem on our CentOS 6 cluster. Here is the output:

$ spack install gmp %pgi
==> Installing gmp
==> Installing m4
==> libsigsegv is already installed in /blues/gpfs/home/software/spack-0.9.1/opt/spack/linux-centos6-x86_64/pgi-16.5-0/libsigsegv-2.10-3zrk2bhqrgknreumylyvff2tzpqlrt2d
Traceback (most recent call last):
  File "/soft/spack-0.9.1/bin/spack", line 192, in <module>
    main()
  File "/soft/spack-0.9.1/bin/spack", line 169, in main
    return_val = command(parser, args)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/cmd/install.py", line 97, in install
    explicit=True)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/package.py", line 1007, in do_install
    dirty=dirty)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/package.py", line 1007, in do_install
    dirty=dirty)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/package.py", line 985, in do_install
    rec = spack.installed_db.get_record(self.spec)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 79, in converter
    return function(self, spec_like, *args, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 527, in get_record
    key = self._get_matching_spec_key(spec, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 519, in _get_matching_spec_key
    match = self.query_one(spec, **kwargs)
  File "/blues/gpfs/home/software/spack-0.9.1/lib/spack/spack/database.py", line 653, in query_one
    assert len(concrete_specs) <= 1
AssertionError

And here is my debug tarball:
spack-db.features-db-locking.842c36c.2016-10-10-175746.tar.gz

adamjstewart · 2016-10-10T23:05:38Z

Aside from the obvious OS difference, the biggest difference is that on my laptop, I didn't have anything installed before trying out your branch, whereas on our cluster, I had 460 packages already installed (mostly duplicates). That assert statement ought to shed some light on the problem though.

…e about A use case where the previous approach was failing is : - more than one spack process running on compute nodes - stage directory is a link to fast LOCAL storage In this case the processes may try to unlink something that is "dead" for them, but actually used by other processes on storage they cannot see.

- Make sure we write, truncate, flush when setting PID and owning host in the file.

- Locks will now create enclosing directories and touch the lock file automatically.

- Closing and re-opening to upgrade to write will lose all existing read locks on this process. - If we didn't allow ranges, sleeping until no reads would work. - With ranges, we may never be able to take some legal write locks without invalidating all reads. e.g., if a write lock has distinct range from all reads, it should just work, but we'd have to close the file, reopen, and re-take reads. - It's easier to just check whether the file is writable in the first place and open for writing from the start. - Lock now only opens files read-only if we *can't* write them.

- Locks now use fcntl range locks on a single file. How it works for prefixes: - Each lock is a byte range lock on the nth byte of a file. - The lock file is ``spack.installed_db.prefix_lock`` -- the DB tells us what to call it and it lives alongside the install DB. n is the sys.maxsize-bit prefix of the DAG hash. For stages, we take the sha1 of the stage name and use that to select a byte to lock. With 100 concurrent builds, the likelihood of a false lock collision is ~5.36e-16, so this scheme should retain more than sufficient paralellism (with no chance of false negatives), and get us reader-writer lock semantics with a single file, so no need to clean up lots of lock files.

- Fix a bug handling '/' characters in branch names. - Make tarballs use a descriptive name for the top-level directory, not just `opt`.

tgamblin · 2016-10-11T09:04:38Z

@adamjstewart: I looked into this. The cause for your issue is separate from what this issue is trying to fix so I made a separate issue for it (#1992) and threw it in the 0.10 release.

tgamblin mentioned this pull request Aug 19, 2016

feature : finer locking of the db #866

Closed

4 tasks

tgamblin force-pushed the features/db-locking branch from 7fa91aa to 537e9cc Compare August 19, 2016 16:32

tgamblin changed the title ~~Finer-grained locking~~ [WIP] Finer-grained locking Aug 29, 2016

tgamblin mentioned this pull request Aug 29, 2016

Sandbox install() from the Internet #1655

Closed

tgamblin added WIP pkg-database locking and removed pkg-database labels Sep 21, 2016

tgamblin force-pushed the features/db-locking branch 2 times, most recently from 5dd56e8 to 784944b Compare October 6, 2016 08:47

tgamblin removed the WIP label Oct 6, 2016

tgamblin changed the title ~~[WIP] Finer-grained locking~~ Finer-grained locking Oct 6, 2016

tgamblin force-pushed the features/db-locking branch from 872e5a0 to ca5f5ee Compare October 6, 2016 10:00

alalazo and others added 15 commits October 11, 2016 01:38

install : finer graned locking for install command

34fe51a

unit tests : fixed failing tests

b3a6e30

uninstall : removed global lock

74fb102

diy : removed global write lock on the db

a3fc492

lockfiles : creates directory for locks if not already present

f47dcdc

Make llnl.util.lock use file objects instead of low-level OS fds.

907fe91

- Make sure we write, truncate, flush when setting PID and owning host in the file.

Remove need to touch lock files before using.

ea10e3b

- Locks will now create enclosing directories and touch the lock file automatically.

Add byte-range parameters to llnl.util.lock

da6bbfb

Add base32_prefix_bits function to get prefix of DAG hash as an int.

a024c6d

Add tests for locks with byte ranges.

080a786

Roll my my own bit_length function for Python 2.6 compatibility.

f0edfa6

Fix bug in spack debug create-db-tarball

9c5c8b2

- Fix a bug handling '/' characters in branch names. - Make tarballs use a descriptive name for the top-level directory, not just `opt`.

tgamblin force-pushed the features/db-locking branch from 842c36c to 9c5c8b2 Compare October 11, 2016 09:05

tgamblin merged commit f9d8325 into develop Oct 11, 2016

This was referenced Oct 11, 2016

OS X: "spack find" with new spack clone gives "Bad file descriptor" #1966

Closed

spack use fails when database is locked #218

Closed

mathstuf deleted the features/db-locking branch October 11, 2016 15:33

This was referenced Oct 12, 2016

Use cYAML if it is available #2010

Merged

InterProcessLock files left behind harlowja/fasteners#26

Open

Spack gets confused about which packages are installed #186

Closed

adamjstewart mentioned this pull request Oct 20, 2016

Fix satisfaction rules for concrete specs #1992

Closed

tgamblin mentioned this pull request Oct 25, 2016

Spack install AssertionError assert len(concrete_specs) <= 1 #2123

Closed

tgamblin mentioned this pull request Nov 7, 2016

Document Locking in Spack #2267

Closed

Conversation

tgamblin commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Modifications

Install and uninstall are not mutually exclusive

Uh oh!

trws commented Aug 19, 2016

Uh oh!

tgamblin commented Aug 21, 2016

Uh oh!

alalazo commented Aug 21, 2016

Uh oh!

trws commented Aug 22, 2016

Uh oh!

tgamblin commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

tgamblin commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Oct 6, 2016

Uh oh!

alalazo commented Oct 6, 2016

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

adamjstewart commented Oct 6, 2016

Uh oh!

adamjstewart commented Oct 6, 2016

Uh oh!

alalazo commented Oct 6, 2016

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

adamjstewart commented Oct 6, 2016

Uh oh!

alalazo commented Oct 7, 2016

Uh oh!

tgamblin commented Oct 10, 2016

Uh oh!

adamjstewart commented Oct 10, 2016

Uh oh!

adamjstewart commented Oct 10, 2016

Uh oh!

tgamblin commented Oct 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgamblin commented Aug 19, 2016 •

edited

Loading

tgamblin commented Oct 6, 2016 •

edited

Loading

tgamblin commented Oct 6, 2016 •

edited

Loading