Ticket #313 (new enhancement)

Opened 5 months ago

Last modified 3 months ago

improve docs: Adding exclusive access mode to Locks

Reported by: albertHofkamp Assigned to:
Priority: major Milestone: 0.8.0
Component: buildprocess Version: 0.7.7
Keywords: devtree review Cc: albertHofkamp, dustin

Description (Last modified by warner)

Hereby I submit an extension for Buildbot to allow exclusive claiming of a lock. The patch consists of two parts. The first part, harmless2.patch contains some rather harmless changes that you may want to do even if the extension is not accepted. The second part lockaccess2.patch builds on the first part and contains the actual extension.

The fundamental extension is that with using a lock you now need to specify how (in which mode) you want access. The extension allows two modes, namely 'counting' and 'exclusive'. The former is what Buildbot 0.7.7 is currently supporting, namely allowing up-to 'maxCount' users. The latter claims the lock exclusively (with a MasterLock one user globally, with a SlaveLock, one user per slave). For backwards compability, using a lock without specifying an access mode defaults to 'counting' mode.

Example:

In the next example, a limit of at most two builds is enforced. The cleanup process however needs exclusive access to prevent interference with the build processes. 

    from buildbot import locks
    from buildbot.steps import source, shell
    from buildbot.process import factory

    build_lock = locks.MasterLock("builds", maxCount=2)
    fb = factory.BuildFactory()
    fb.addStep(source.SVN(svnurl="http://example.org/svn/Trunk"))
    fb.addStep(shell.ShellCommand(command=["make", "all"]))
    b1 = {'name': 'b1', 'slavename': 'bot-1', 'builddir' : 'b1',
          'factory': fb, 'locks': [build_lock.access('counting')] }
    b2 = {'name': 'b2', 'slavename': 'bot-2', 'builddir' : 'b2',
          'factory': fb, 'locks': [build_lock.access('counting')] }
    b3 = {'name': 'b3', 'slavename': 'bot-3', 'builddir' : 'b3',
          'factory': fb, 'locks': [build_lock.access('counting')] }
    fc = factory.BuildFactory()
    fc.addStep(shell.ShellCommand(command=["cleanup"]))
    cl = {'name': 'clean', 'slavename': 'bot-1', 'builddir' :clean',
          'factory': fc, 'locks': [build_lock.access('exclusive')] }
    c['builders'] = [b1, b2, b3, cl]

Note the addition of ".access(MODE)" while using the lock.

harmless2.patch:

  • Added several doc-strings in locks.BaseLock class
  • Added a locks.BaseLockId abstract base class on top of the MasterLock and SlaveLock classes
  • Added some assertion checking in master.py. Needed to import 'locks'. That name was already in use, so renamed a few variables to prevent clashes.
  • util.ComparableMixin class: Computed comparisons more than once. Stored the result in an intermediate variable
  • Several buildbot.texinfo fixes (a typo, fixing of "builddir='XXX'" to "'builddir': 'XXX'" which works better in a dictionary, and replaced deprecated html.WaterFall() use with html.WebStatus() call).

lockaccess2.patch:

  • Added a locks.LockAccess class, which represents in which a lock should be accessed. It contains the mode, and a lockid (a reference to MasterLock or SlaveLock instance).
  • Almost complete rewrite of the locks.BaseLock class. The methods associated with use now have an additional 'access' parameter for the LockAccess instance. This data is also stored in the 'waiting' and 'owners' lists.
  • Extension of the locks.BaseLockId class with an 'access(mode)' method, so the user can specify the access mode upon use. There is also a 'defaultAccess()' method that is used when the user doesn't specify the mode.
  • In master.py, the LockAccess instance is removed during checking of locks.
  • In process/base.py, the self.locks for step-locks now contains a tuple (real-lock, LockAccess instance). This combination is also used when querying/getting/releasing the locks.
  • Same in process/buildstep.py for builder locks.
  • Fixed unit-tests to also use a LockAccess instance. Some tests are duplicated so both modes are tested. The names of the tests have been extended to state more precisely what is tested. In some cases, only the counting mode is tested (exclusive tests with sharing locks makes no sense).
  • Added 2 tests to verify that an exclsuive lock and a counting lock cannot exist together.
  • Extended buildbot.texinfo with the new functionality. I didn't re-format the lines to make more clear what I have changed. You may want to do this after reviewing my additions.

Attachments

harmless2.patch (9.5 kB) - added by albertHofkamp on 07/07/08 00:49:37.
several small changes, see ticket for details
lockaccess2.patch (29.2 kB) - added by albertHofkamp on 07/07/08 00:50:23.
the lock access extension
bblock.txt (6.3 kB) - added by albertHofkamp on 11/10/08 05:54:22.
New description of current implementation. I left out some examples, as they don't seem to add much.

Change History

07/07/08 00:49:37 changed by albertHofkamp

  • attachment harmless2.patch added.

several small changes, see ticket for details

07/07/08 00:50:23 changed by albertHofkamp

  • attachment lockaccess2.patch added.

the lock access extension

08/31/08 08:19:15 changed by dustin

  • cc changed from albertHofkamp to albertHofkamp, dusin.
  • keywords set to devtree review.

http://repo.or.cz/w/buildbot.git?a=shortlog;h=refs/remotes/dustin/bug313

All tests pass.

08/31/08 08:19:24 changed by dustin

  • milestone changed from undecided to 0.7.9.

(follow-up: ↓ 5 ) 09/03/08 15:06:11 changed by warner

  • description changed.

I read through your docs patch, but I don't understand what the difference is between "exclusive mode" and simply setting maxCount=1 . Is it possible to use exclusive mode with maxCount=2 ? What would that mean? If the parameters are not orthogonal, it suggests that perhaps there should not be two separate parameters.

(in reply to: ↑ 4 ) 09/04/08 00:18:54 changed by albertHofkamp

Replying to warner:

I read through your docs patch, but I don't understand what the difference is between "exclusive mode" and simply setting maxCount=1 .

There is none, since allowing at most 1 client implies exclusive access to the resource.

Is it possible to use exclusive mode with maxCount=2 ? What would that mean?

Yes.

A resource with maxCount=2 can be in the following states:

  • free
  • claimed exclusively ('exclusive mode') by 1 slave
  • claimed shared ('counting mode') by 1 slave
  • claimed shared ('counting mode') by 2 slaves

Increasing maxCount allows for more slaves using the resource in counting mode at the same time.

A use case for this addition is management of a shared directory tree.

  • When used as source for retrieving information it may be shared between slaves, but up to some limit.
  • When pruning the tree, nobody should be retrieving information from the tree (to prevent information from disappearing 'magically' while building).

(the former access uses counting mode, the latter uses exclusive mode)

If the parameters are not orthogonal, it suggests that perhaps there should not be two separate parameters.

The above tree is basically accessed in two different ways, one that allows sharing, and one that does not. I don't understand how you'd express this nicely with one parameter.

An alternative solution that I came up with was adding a 'weight' to claiming. Normal access is with a weight of 1 (this covers current implementation), if a slave uses the resource more heavily, it can claim a resource saying 'count me for N users'.

Exclusive access is of course in the latter solution 'count me for maxCount users'.

Advantage of the latter solution is that it allows more subtle specification of resource use (set maxCount=5, and you can have slaves that weigh 1, 2, 3, 4, or 5).

Resons for throwing out the latter solution:

  • Whether the more subtle specification of weights is beneficial is not clear to me. It may be that users just get more confused about what weight to apply for each slave ("hmm, should this slave weigh as 2 or 3?, I need a weight of 2.5!").
  • It is more ambigious (maxCount=2 and always using weight=1 is the same as maxCount=4 and always using weight=2).
  • Exclusive access is specified by defining the same upper limit twice (namely once as maxCount, and once as weight). That may cause inconsistencies (eg when upgrading to a faster machine and only changing maxCount). We could have some default value for expressing weight=maxCount, but we'd still have two ways of specifying exclusive access, thus at most only reduce the inconsistency problem.

Maybe the wording in the documentation is not optimal and should be changed to a less confusing one. I did consider to use 'shared mode' instead of 'counting mode', but shared mode together with maxCount didn't really feel right to me, also since current users are all used to the counting idea. On the other hand, I am not a native English speaker, so my idea of 'feeling right' may be incorrect.

09/15/08 22:05:49 changed by warner

  • cc changed from albertHofkamp, dusin to albertHofkamp, dustin.
  • summary changed from Adding exclusive access mode to Locks to improve docs: Adding exclusive access mode to Locks.
  • milestone changed from 0.7.9 to 0.8.0.

Hm. Ok, this is going into 0.7.9 unchanged, but I can't say I'm completely happy with the feature: I worry that it is going to be confusing for users. I think we need to find a more strongly-motivating use case, both to justify this feature and to explain it in the user's manual. A build process that allows a 'clean' step to modify a directory that is also being used by a 'compile' step sounds fragile to me.. using locks in this mixed counting/exclusive form might be a way to prevent problems, but maybe we should be discouraging folks from doing such things in the first place.

When you get a chance (post-0.7.9), please see if you can improve the documentation somehow. Thanks!

09/16/08 12:18:10 changed by dustin

OK, guys, these are read-write locks:

http://en.wikipedia.org/wiki/Read/write_lock

Can we just rename them?

09/18/08 08:31:09 changed by albertHofkamp

From a technical point of view, yes imho (I have extended to read/write locks).

The current use of locks to limit the number of concurrent slaves at a machine would correspond with 'read'. As a result, one would write in the manual" "to limit the number of slaves at work at the same time, use slavelock.access('read')". That sounds quite confusing to me, especially when my slave is not reading but generating and copying a .tar.gz file or so.

shared/exclusive instead of read/write would solve the above confusion (for me at least). Your point of having a better use case still stands, but I have no idea's at all atm.

09/18/08 08:52:33 changed by dustin

I'm fine with calling them shared and exclusive, as that is more germane to the use in Buildbot. However, I think that r/w locks are well-understood, or at least easy to google for, so I think it would be good to mention them in the docs. I know I was baffled by the shared/exclusive thing until I made this mental link.

11/10/08 05:54:22 changed by albertHofkamp

  • attachment bblock.txt added.

New description of current implementation. I left out some examples, as they don't seem to add much.