Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow borg with-lock children to grab locks on same repository. #4120

Closed
wants to merge 3 commits into from

Conversation

vctgross
Copy link

This PR adds a --shared-lock option to borg with-lock, and allows borg children of borg with-lock --shared-lock to grab an exclusive or shared lock.

I have a server that hosts all my borg repositories, and the only access allowed is SSH with borg server --append-only. I want to run a daily cron on this server that runs borg prune only if no archive has been deleted since the last borg prune; in other words, the borg prune cron is the only way to delete archives. It could be easy enough to do :

export BORG_REPO
borg with-lock $BORG_REPO /bin/bash -c 'is_subset "$(< $REPO_ARCHIVE_REFFILE)" "$(borg list)" && borg prune && borg list > $REPO_ARCHIVE_REFFILE'

except that borg with-lock takes an exclusive lock on the repository.

This PR address this issue by allowing to downgrade borg with-lock to a shared lock, and by allowing a borg process to take an exclusive lock on top of a shared one. This is subject to restrictions : there can be at most one process holding the shared lock on the repository, and this process' PID must be equal to our PGID.
Because forked process inherit their parent PGID, and because shells with job control (set -m) enabled spawns jobs ("a set of processes, comprising a shell pipline" [Open Group Base Definitions 3.202]) in their own process groups, this means that, assuming the shared lock holder PGID equals its PID, without additional actions (i.e. setpgid(2)) only children of the shared lock holder will meet the conditions.

This only works if the process seeking the exclusive lock has its pgid
equal to the shared lock holder's pid, and there is only one shared lock holder.
With this it is possible to run borg commands from within borg with-lock.
@codecov-io
Copy link

codecov-io commented Oct 23, 2018

Codecov Report

Merging #4120 into master will decrease coverage by 0.01%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4120      +/-   ##
==========================================
- Coverage   84.44%   84.43%   -0.02%     
==========================================
  Files          37       37              
  Lines        9382     9391       +9     
  Branches     1556     1557       +1     
==========================================
+ Hits         7923     7929       +6     
- Misses       1015     1019       +4     
+ Partials      444      443       -1
Impacted Files Coverage Δ
src/borg/locking.py 86.56% <100%> (+0.42%) ⬆️
src/borg/platform/base.py 82.3% <100%> (+0.15%) ⬆️
src/borg/platform/__init__.py 90.47% <100%> (ø) ⬆️
src/borg/archiver.py 81.62% <40%> (-0.26%) ⬇️
src/borg/archive.py 84.14% <0%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62de891...d50eb3c. Read the comment docs.

@textshell
Copy link
Member

I think the restriction to local repositories is strange and likely an artifact of your specific requirements.
For determining which processes may access i would prefer a environment variable with some kind of token so more complex usage scenarios can be supported.
Should the borg commands that share the lock require a "--with-shared-lock" option or is that overkill?

@ThomasWaldmann
Copy link
Member

There were quite big changes in #8332 to the repository and locking code, so this does conflict a lot and in any case does not apply any more, because the locking code is now completely different.

A lot of borg commands do not take an exclusive lock anymore, exceptions are check and compact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants