borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
15
57
Fork 40
Code Issues 54 Pull Requests 1 Actions Packages Releases 121 Activity

WIP: Add locking of borgmatic config file #254

Closed
drewkett wants to merge 9 commits from (deleted):configlock into main
pull from: (deleted):configlock
merge into: borgmatic-collective:main
Conversation 5 Commits 9 Files Changed 2 +37 -33
1 changed files with 37 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 877b1c440e - Show all commits

70
borgmatic/commands/borgmatic.py
View File

@ -42,56 +42,60 @@ def run_configuration(config_filename, config, arguments):
* JSON output strings from successfully executing any actions that produce JSON
* logging.LogRecord instances containing errors from any actions or backup hooks that fail
'''
(location, storage, retention, consistency, hooks) = (
config.get(section_name, {})
for section_name in ('location', 'storage', 'retention', 'consistency', 'hooks')
)
global_arguments = arguments['global']
if location.get("lock_client",False):
lock_f = open(config_filename)
try:
fcntl.flock(lock_f.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
except IOError:
logger.critical("Failed to acquire lock for {}".format(config_filename))
sys.exit(1)
local_path = location.get('local_path', 'borg')
remote_path = location.get('remote_path')
borg_environment.initialize(storage)
witten commented 2019-12-03 21:22:38 +00:00
Outdated
Review
Copy Link

Forgive me if this is a duplicate comment.. Is there a reason this is non-blocking? Seems like we'd want it to synchronously lock to be absolutely sure that no other borgmatic is running.

Forgive me if this is a duplicate comment.. Is there a reason this is non-blocking? Seems like we'd want it to synchronously lock to be absolutely sure that no other borgmatic is running.
drewkett commented 2019-12-04 03:29:22 +00:00
Outdated
Review
Copy Link

Non blocking here indicates that we don’t want to block the borgmatic and wait for a lock to be freed if the file is currently locked. I think you can do blocking with a timeout but it doesn’t seem that beneficial to me since borgmatic is typically a long running operation so the lock is unlikely to be free in a short amount of time. I think borg uses a timeout so I guess we could replicate that behavior.

Non blocking here indicates that we don’t want to block the borgmatic and wait for a lock to be freed if the file is currently locked. I think you can do blocking with a timeout but it doesn’t seem that beneficial to me since borgmatic is typically a long running operation so the lock is unlikely to be free in a short amount of time. I think borg uses a timeout so I guess we could replicate that behavior.
witten commented 2019-12-04 05:27:24 +00:00
Outdated
Review
Copy Link

Oh, gotcha. I originally misunderstood what LOCK_NB did. I see now that you're using it here to error immediately if a lock is in place, which makes sense. Carry on!

Oh, gotcha. I originally misunderstood what `LOCK_NB` did. I see now that you're using it here to error immediately if a lock is in place, which makes sense. Carry on!
encountered_error = None
error_repository = ''
witten commented 2019-12-03 21:26:29 +00:00
Outdated
Review
Copy Link

Given that this is in run_configuration() now, you can just:

except IOError as error:
    yield from make_error_log_records(
        '{}: Failed to acquire lock'.format(config_filename), error
    )
    return

... and then this would go through the rest of the borgmatic logging/error/summary machinery.

Another option would be this:

except IOError as error:
    encountered_error = error
    yield from make_error_log_records(
        '{}: Failed to acquire lock'.format(config_filename), error
    )

Which would fall through and trigger the on_error hook, if that's what you want to do in this case.

Given that this is in `run_configuration()` now, you can just: ``` except IOError as error: yield from make_error_log_records( '{}: Failed to acquire lock'.format(config_filename), error ) return ``` ... and then this would go through the rest of the borgmatic logging/error/summary machinery. Another option would be this: ``` except IOError as error: encountered_error = error yield from make_error_log_records( '{}: Failed to acquire lock'.format(config_filename), error ) ``` Which would fall through and trigger the `on_error` hook, if that's what you want to do in this case.
if 'create' in arguments:
if location.get("lock_client",False):
witten commented 2019-12-09 18:00:58 +00:00
Outdated
Review
Copy Link

Make sure you run tests with tox, as the code formatter will probably complain about this. To have it reformat things for you: tox -e black is all you need to do.

Make sure you run tests with `tox`, as the code formatter will probably complain about this. To have it reformat things for you: `tox -e black` is all you need to do.
lock_f = open(config_filename)
try:
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.START,
global_arguments.dry_run,
)
command.execute_hook(
hooks.get('before_backup'),
hooks.get('umask'),
config_filename,
'pre-backup',
global_arguments.dry_run,
)
dispatch.call_hooks(
'dump_databases',
hooks,
config_filename,
dump.DATABASE_HOOK_NAMES,
global_arguments.dry_run,
)
except (OSError, CalledProcessError) as error:
fcntl.flock(lock_f.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
except IOError as error:
encountered_error = error
yield from make_error_log_records(
'{}: Error running pre-backup hook'.format(config_filename), error
'{}: Failed to acquire lock'.format(config_filename), error
)
if not encountered_error:
if 'create' in arguments:
witten commented 2019-12-09 18:00:00 +00:00
Outdated
Review
Copy Link

These can be combined into a single if statement.

These can be combined into a single `if` statement.
try:
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.START,
global_arguments.dry_run,
)
command.execute_hook(
hooks.get('before_backup'),
hooks.get('umask'),
config_filename,
'pre-backup',
global_arguments.dry_run,
)
dispatch.call_hooks(
'dump_databases',
hooks,
config_filename,
dump.DATABASE_HOOK_NAMES,
global_arguments.dry_run,
)
except (OSError, CalledProcessError) as error:
encountered_error = error
yield from make_error_log_records(
'{}: Error running pre-backup hook'.format(config_filename), error
)
if not encountered_error:
for repository_path in location['repositories']:
try: