WIP: Add locking of borgmatic config file #254

Open
drewkett wants to merge 9 commits from drewkett/borgmatic:configlock into master
drewkett commented 2 years ago

This prevents multiple instances of borgmatic from running against
the same config file. This is particularly important when there are
pre-backup scripts with side effects.

It seems to work with initial testing. I just took a guess at where the locking should be done. I also don't know the best name for the option or the best way to describe it.

This prevents multiple instances of borgmatic from running against the same config file. This is particularly important when there are pre-backup scripts with side effects. It seems to work with initial testing. I just took a guess at where the locking should be done. I also don't know the best name for the option or the best way to describe it.
Owner

Thank you for submitting this! I will have a look at it when I get a chance.

Thank you for submitting this! I will have a look at it when I get a chance.
witten requested changes 2 years ago
witten left a comment

Looks like a great start! Thank you for taking the first stab at this.

logger.debug('Ensuring legacy configuration is upgraded')
convert.guard_configuration_upgraded(LEGACY_CONFIG_PATH, config_filenames)
witten commented 2 years ago
Poster
Owner

In terms of where to put this functionality, I'd recommend moving it to the run_configuration() function (same file). The idea is that the function is responsible for running all the actions for a particular borgmatic configuration file, and therefore it can also perform locking of it. That would also let you eliminate the loop, as run_configuration() is already per-config-file!

Alternately, this could could be put in a separate function that's called from run_configuration(), just to factor it out a bit.

One implication of either approach is that you'd be locking configuration files one at a time instead of all at once. That may actually be a good thing, as it's locking a smaller surface area. (Generally best practice, even if it's not strictly needed here.)

In terms of where to put this functionality, I'd recommend moving it to the `run_configuration()` function (same file). The idea is that the function is responsible for running all the actions for a particular borgmatic configuration file, and therefore it can also perform locking of it. That would also let you eliminate the loop, as `run_configuration()` is already per-config-file! Alternately, this could could be put in a separate function that's called from `run_configuration()`, just to factor it out a bit. One implication of either approach is that you'd be locking configuration files one at a time instead of all at once. That may actually be a good thing, as it's locking a smaller surface area. (Generally best practice, even if it's not strictly needed here.)
Poster

I haven’t looked at enough to understand the structure of the code. But for my use case I wanted to make sure it was locked for the entirety of the time before prebackup through postbackup. Would run_configuration accomplish that?

I haven’t looked at enough to understand the structure of the code. But for my use case I wanted to make sure it was locked for the entirety of the time before prebackup through postbackup. Would run_configuration accomplish that?
Poster

Another note is the file handle needs to be open for as long as the lock is held. I’ve done it in the past where I set up a context manager so its a little more pythonic where you use with hold_lock(): and then indent for duration.

Another note is the file handle needs to be open for as long as the lock is held. I’ve done it in the past where I set up a context manager so its a little more pythonic where you use `with hold_lock():` and then indent for duration.
witten commented 2 years ago
Poster
Owner

I haven’t looked at enough to understand the structure of the code. But for my use case I wanted to make sure it was locked > for the entirety of the time before prebackup through postbackup. Would run_configuration accomplish that?

Nope, it wouldn't! It would lock each config file individually. So if you instead want to lock for the entirety of the time before pre-backup through post-backup, then I'd suggest putting the locking near the existing before_everything and after_everything hook invocations (in that same source file). That's because before_everything/after_everything are not configuration-file specific.. They trigger before/after all config files run. Unlike before_backup/after_backup, which are per-config file.

Anyway, if the locking is not per-config file, then have you considered making a separate dedicated lock file instead of having to lock n config files? I suppose by locking config files, you implicitly allow separate simultaneous borgmatic runs if they use disjoint configuration files. Is that a desired property?

> I haven’t looked at enough to understand the structure of the code. But for my use case I wanted to make sure it was locked > for the entirety of the time before prebackup through postbackup. Would run_configuration accomplish that? Nope, it wouldn't! It would lock each config file individually. So if you instead want to lock for the entirety of the time before pre-backup through post-backup, then I'd suggest putting the locking near the existing `before_everything` and `after_everything` hook invocations (in that same source file). That's because `before_everything`/`after_everything` are not configuration-file specific.. They trigger before/after all config files run. Unlike `before_backup`/`after_backup`, which are per-config file. Anyway, if the locking is not per-config file, then have you considered making a separate dedicated lock file instead of having to lock n config files? I suppose by locking config files, you implicitly allow separate simultaneous borgmatic runs if they use disjoint configuration files. Is that a desired property?
Poster

Sorry, I read per config file as per backup destination. That was my confusion. But now I'm not sure whether before_backup or before_everything since I didn't realize you could run borgmatic with multiple config files. I'm honestly not sure which makes more sense. For my use case it would be before_backup but I guess someone could be using before_everything similarly. For before_backup it makes sense to me just to lock the config file, but a single lock file for before_everything seems more logical. I guess it could be configurable, but I'm not sure if that is worth the complexity.

Sorry, I read per config file as per backup destination. That was my confusion. But now I'm not sure whether `before_backup` or `before_everything` since I didn't realize you could run borgmatic with multiple config files. I'm honestly not sure which makes more sense. For my use case it would be `before_backup` but I guess someone could be using `before_everything` similarly. For `before_backup` it makes sense to me just to lock the config file, but a single lock file for `before_everything` seems more logical. I guess it could be configurable, but I'm not sure if that is worth the complexity.
witten commented 2 years ago
Poster
Owner

On the question of locking around before_backup vs. before_everything: Let's start by solving your use case, and then we can always expand the feature later or make it configurable as needed. But I'll also put a note on the original ticket (#250) in case any watchers there want to weigh in.

On the question of locking around `before_backup` vs. `before_everything`: Let's start by solving your use case, and then we can always expand the feature later or make it configurable as needed. But I'll also put a note on the original ticket (#250) in case any watchers there want to weigh in.
# this removes the reference to the open files which python will garbage collect the file objects
# and close them
locks = None
witten commented 2 years ago
Poster
Owner

Is this needed if you're okay with the garbage collector kicking in at the end of the function when the variable just goes out of scope?

Is this needed if you're okay with the garbage collector kicking in at the end of the function when the variable just goes out of scope?
Poster

No preference for this. I mostly put it in as a reminder that there are locks being held in case further changes to the code are made down the line. Though maybe the context manager is the way to go since it accomplishes this as well.

No preference for this. I mostly put it in as a reminder that there are locks being held in case further changes to the code are made down the line. Though maybe the context manager is the way to go since it accomplishes this as well.
witten commented 2 years ago
Poster
Owner

Got it. I don't feel super strongly either way.

Got it. I don't feel super strongly either way.
witten commented 2 years ago
Poster
Owner

Another note is the file handle needs to be open for as long as the lock is held. I’ve done it in the past where I set up a context manager so its a little more pythonic where you use with hold_lock(): and then indent for duration.

I suppose one minor benefit to the context manager is it provides a nice point to factor out the actual locking (e.g. hold_lock()).

> Another note is the file handle needs to be open for as long as the lock is held. I’ve done it in the past where I set up a context manager so its a little more pythonic where you use with hold_lock(): and then indent for duration. I suppose one minor benefit to the context manager is it provides a nice point to factor out the actual locking (e.g. `hold_lock()`).
Poster

An annoyance to the context manager in this case would be if its configurable whether locks are used. Its not a big deal but it makes a weird api since you basically need to pass an argument to the function to make it a noop if its turned off (rather than wrapping the code in an if statement and duplicating whats inside the context). I guess hold_lock(disabled=True) isn't terrible.

An annoyance to the context manager in this case would be if its configurable whether locks are used. Its not a big deal but it makes a weird api since you basically need to pass an argument to the function to make it a noop if its turned off (rather than wrapping the code in an if statement and duplicating whats inside the context). I guess `hold_lock(disabled=True)` isn't terrible.
witten commented 2 years ago
Poster
Owner

True. Although if we punt on the configurability, then that becomes a problem for our future selves to solve. :D

True. Although if we punt on the configurability, then that becomes a problem for our future selves to solve. :D
lock_config:
type: bool
desc: Lock config when running borgmatic to prevent multiple instances from running simultaneously
example: true
witten commented 2 years ago
Poster
Owner

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.
Poster

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

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

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.
Poster

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.
witten commented 2 years ago
Poster
Owner

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).
witten reviewed 2 years ago
witten left a comment

Looks like a great start! Thank you for taking the first stab at this.

witten reviewed 2 years ago
witten commented 2 years ago
Poster
Owner

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.
witten reviewed 2 years ago
witten commented 2 years ago
Poster
Owner

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.
drewkett reviewed 2 years ago
Poster

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 reviewed 2 years ago
witten commented 2 years ago
Poster
Owner

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!
Poster

I’m not sure if you saw but I pushed an update to this that looks okay to me. I’m not sure if there’s any documentation updates or testing that you would want added to this PR.

I’m not sure if you saw but I pushed an update to this that looks okay to me. I’m not sure if there’s any documentation updates or testing that you would want added to this PR.
witten reviewed 2 years ago
witten commented 2 years ago
Poster
Owner

These can be combined into a single if statement.

These can be combined into a single `if` statement.
witten reviewed 2 years ago
witten commented 2 years ago
Poster
Owner

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.
witten reviewed 2 years ago
witten commented 2 years ago
Poster
Owner

tox -e isort will order imports for you.

`tox -e isort` will order imports for you.
witten reviewed 2 years ago
witten commented 2 years ago
Poster
Owner

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.
Owner

Thanks for letting me know. I'm not notified of changeset pushes.. only comments.

Formal documentation probably isn't necessary for this feature given that it's "documented" in the configuration schema. However, adding some basic tests would be welcome if you're up to it. The file to add tests to is tests/unit/commands/test_borgmatic.py, and the test doesn't have to be super involved.. Just getting some basic coverage would be good. I would expect the actual locking to be mocked out!

If adding tests for this is too much, let me know and I can take a crack at it.

Thanks for letting me know. I'm not notified of changeset pushes.. only comments. Formal documentation probably isn't necessary for this feature given that it's "documented" in the configuration schema. However, adding some basic tests would be welcome if you're up to it. The file to add tests to is `tests/unit/commands/test_borgmatic.py`, and the test doesn't have to be super involved.. Just getting some basic coverage would be good. I would expect the actual locking to be mocked out! If adding tests for this is too much, let me know and I can take a crack at it.

Why not using a more pythonic way of opening the file? Currently the file resource is also never closed and is a bad practice.

Instead of lock_f = open(config_filename), use with open(config_filename) as lock_f:.

Why not using a more pythonic way of opening the file? Currently the file resource is also never closed and is a bad practice. Instead of `lock_f = open(config_filename)`, use `with open(config_filename) as lock_f:`.
This pull request has changes conflicting with the target branch.
borgmatic/config/schema.yaml
borgmatic/commands/borgmatic.py
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.