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 +45 -29
drewkett commented 2019-11-27 05:31:00 +00:00
First-time contributor
Copy Link

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.
witten commented 2019-11-27 18:05:14 +00:00
Owner
Copy Link

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 2019-12-03 01:03:40 +00:00
witten left a comment
Owner
Copy Link

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

Looks like a great start! Thank you for taking the first stab at this.
borgmatic/commands/borgmatic.py Outdated
@ -566,9 +567,25 @@ def main(): # pragma: no cover
logger.debug('Ensuring legacy configuration is upgraded')
convert.guard_configuration_upgraded(LEGACY_CONFIG_PATH, config_filenames)
witten commented 2019-12-03 00:31:29 +00:00
Owner
Copy Link

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.)
drewkett commented 2019-12-03 06:51:46 +00:00
Author
First-time contributor
Copy Link

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?
drewkett commented 2019-12-03 06:54:12 +00:00
Author
First-time contributor
Copy Link

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 2019-12-03 17:32:11 +00:00
Owner
Copy Link

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?
drewkett commented 2019-12-03 17:42:22 +00:00
Author
First-time contributor
Copy Link

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 2019-12-03 17:58:47 +00:00
Owner
Copy Link

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.
borgmatic/commands/borgmatic.py Outdated
@ -571,1 +584,4 @@
# this removes the reference to the open files which python will garbage collect the file objects
# and close them
locks = None
witten commented 2019-12-03 01:03:15 +00:00
Owner
Copy Link

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?
drewkett commented 2019-12-03 06:55:16 +00:00
Author
First-time contributor
Copy Link

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 2019-12-03 17:33:19 +00:00
Owner
Copy Link

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

Got it. I don't feel super strongly either way.
witten commented 2019-12-03 17:35:10 +00:00
Owner
Copy Link

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()`).
drewkett commented 2019-12-03 17:46:29 +00:00
Author
First-time contributor
Copy Link

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 2019-12-03 18:00:55 +00:00
Owner
Copy Link

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
borgmatic/config/schema.yaml
@ -35,0 +35,4 @@
lock_config:
type: bool
desc: Lock config when running borgmatic to prevent multiple instances from running simultaneously
example: true
witten commented 2019-12-03 01:01:31 +00:00
Owner
Copy Link

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.
drewkett commented 2019-12-03 06:52:39 +00:00
Author
First-time contributor
Copy Link

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

Those sound better. I’ll fix it when I get the chance
drewkett commented 2019-12-03 19:28:14 +00:00
Author
First-time contributor
Copy Link

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.
drewkett commented 2019-12-03 19:29:27 +00:00
Author
First-time contributor
Copy Link

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 2019-12-03 21:19:54 +00:00
Owner
Copy Link

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 2019-12-03 01:03:51 +00:00
witten left a comment
Owner
Copy Link

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

Looks like a great start! Thank you for taking the first stab at this.
witten reviewed 2019-12-03 21:22:38 +00:00
borgmatic/commands/borgmatic.py Outdated
witten commented 2019-12-03 21:22:38 +00:00
Owner
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.
witten reviewed 2019-12-03 21:26:29 +00:00
borgmatic/commands/borgmatic.py Outdated
witten commented 2019-12-03 21:26:29 +00:00
Owner
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.
drewkett reviewed 2019-12-04 03:29:22 +00:00
borgmatic/commands/borgmatic.py Outdated
drewkett commented 2019-12-04 03:29:22 +00:00
Author
First-time contributor
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 reviewed 2019-12-04 05:27:24 +00:00
borgmatic/commands/borgmatic.py Outdated
witten commented 2019-12-04 05:27:24 +00:00
Owner
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!
drewkett commented 2019-12-07 19:18:49 +00:00
Author
First-time contributor
Copy Link

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 2019-12-09 18:00:00 +00:00
borgmatic/commands/borgmatic.py Outdated
witten commented 2019-12-09 18:00:00 +00:00
Owner
Copy Link

These can be combined into a single if statement.

These can be combined into a single `if` statement.
witten reviewed 2019-12-09 18:00:58 +00:00
borgmatic/commands/borgmatic.py Outdated
witten commented 2019-12-09 18:00:58 +00:00
Owner
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.
witten reviewed 2019-12-09 18:01:26 +00:00
borgmatic/commands/borgmatic.py Outdated
witten commented 2019-12-09 18:01:26 +00:00
Owner
Copy Link

tox -e isort will order imports for you.

`tox -e isort` will order imports for you.
witten reviewed 2019-12-09 18:04:46 +00:00
borgmatic/config/schema.yaml Outdated
witten commented 2019-12-09 18:04:46 +00:00
Owner
Copy Link

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.
witten commented 2019-12-09 18:07:57 +00:00
Owner
Copy Link

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.
aardbol commented 2020-12-06 14:42:00 +00:00
First-time contributor
Copy Link

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:`.
witten commented 2021-08-31 23:41:33 +00:00
Owner
Copy Link

Closing due to inactivity. However, please feel free to resurrect if you're still interested in this!

Closing due to inactivity. However, please feel free to resurrect if you're still interested in this!
witten closed this pull request 2021-08-31 23:41:33 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#254
No description provided.
Delete Branch "(deleted):configlock"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?