WIP: Add locking of borgmatic config file #254

Closed
drewkett wants to merge 9 commits from (deleted):configlock into main
2 changed files with 45 additions and 29 deletions

View File

@ -1,4 +1,5 @@
import collections
import fcntl
import json
import logging
import os
@ -54,37 +55,48 @@ def run_configuration(config_filename, config, arguments):
error_repository = ''
prune_create_or_check = {'prune', 'create', 'check'}.intersection(arguments)

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.
try:
if prune_create_or_check:
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.START,
global_arguments.dry_run,
if location.get("lock_client", False):

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:
fcntl.flock(lock_f.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
except IOError as error:
encountered_error = error
yield from make_error_log_records(
'{}: Failed to acquire lock'.format(config_filename), error
)
if 'create' in arguments:
command.execute_hook(
hooks.get('before_backup'),
hooks.get('umask'),
config_filename,
'pre-backup',
global_arguments.dry_run,
if not encountered_error:
try:

These can be combined into a single if statement.

These can be combined into a single `if` statement.
if prune_create_or_check:
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.START,
global_arguments.dry_run,
)
if 'create' in arguments:
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,
location,
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
)
dispatch.call_hooks(
'dump_databases',
hooks,
config_filename,
dump.DATABASE_HOOK_NAMES,
location,
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']:

View File

@ -32,6 +32,10 @@ map:
type: bool
desc: Stay in same file system (do not cross mount points). Defaults to false.
example: true
lock_client:
type: bool
desc: Lock config when running borgmatic to prevent multiple instances from running simultaneously. Defaults to false.

You could add: ". Defaults to false." here. See other descriptions for an example.

You could add: ". Defaults to false." here. See other descriptions for an example.
example: true
Review

The fact that it's locking the configuration file might just be an implementation detail as far as the user is concerned. What about calling this something like lock_client, lock_borgmatic, etc? I don't feel strongly about this in any case.

The fact that it's locking the configuration file might just be an implementation detail as far as the user is concerned. What about calling this something like `lock_client`, `lock_borgmatic`, etc? I don't feel strongly about this in any case.
Review

Those sound better. I’ll fix it when I get the chance

Those sound better. I’ll fix it when I get the chance
Review

I renamed it to lock_client and left it as a bool for now. I didn't see immediately the best way to do it with the config (and validation), but if you want to keep options open switching this to something like lock: "config" so that then you could add a lock: "everything option later would be an option.

I renamed it to `lock_client` and left it as a bool for now. I didn't see immediately the best way to do it with the config (and validation), but if you want to keep options open switching this to something like `lock: "config"` so that then you could add a `lock: "everything` option later would be an option.
Review

I also wasn't sure if the example line is supposed to be the default value or not.

I also wasn't sure if the example line is supposed to be the default value or not.
Review

A bool is fine for now, as far as I'm concerned.

example: is not usually the default value. In fact it's often the opposite of the default value in that it's demonstrating how you would override the default if you are so inclined. So example: true sounds right to me here, given that I'm assuming the default is false (don't lock).

A bool is fine for now, as far as I'm concerned. `example:` is not usually the default value. In fact it's often the *opposite* of the default value in that it's demonstrating how you would override the default if you are so inclined. So `example: true` sounds right to me here, given that I'm assuming the default is `false` (don't lock).
numeric_owner:
type: bool
desc: Only store/extract numeric user and group identifiers. Defaults to false.