borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
12
63
Fork 16
Code Issues 49 Pull Requests 1 Actions Packages Releases 133 Activity

Update the logic that probes for the borgmatic streaming database dump, bootstrap metadata, and check state directories (#934). #937

Merged
witten merged 11 commits from update-runtime-directory-logic into main 2024-11-17 19:57:10 +00:00
Conversation 5 Commits 11 Files Changed 28 +1147 -601
witten commented 2024-11-16 02:22:37 +00:00
Owner
Copy Link

This is an implementation of #934. More specifically, these are the changes:

  • Update the logic that probes for the borgmatic streaming database dump, bootstrap metadata, and check state directories to support more platforms and use cases.
  • To reduce the chance of possible directory collisions (unintended or otherwise), also add a random subdirectory when using a potentially shared temporary directory like /tmp for the runtime (database dump + bootstrap metadata) directory.
  • Add the "RuntimeDirectory" and "StateDirectory" options to the sample systemd service file to support the new runtime and state directory logic.

See #934 for details.

Implementation notes

The broad strokes changes here are:

  • Implement a new context manager for the borgmatic runtime directory containing the new logic. A context manager was necessary because if a temporary subdirectory in say /tmp gets created as part of this, borgmatic needs to actually delete it before exiting! So the context manager makes the cleanup step easier and more assured to happen.
  • Make use of this context manager in several borgmatic actions that need runtime directory access.
  • Change a ton of code to pass around the borgmatic_runtime_directory to various code that needs it, since we can no longer calculate it on the fly wherever it's needed (due to the aforementioned temporary subdirectory thing).
  • It wasn't mentioned in #934, but I also added STATE_DIRECTORY to one of the probed-for environment variables for the state directory in particular. (This is distinct from the runtime directory.) That way the systemd StateDirectory= option can be used to set borgmatic's state directory from a systemd service.

Still to do

  • manual testing of all the affected actions
  • manual testing of all the various permutations of the runtime and state directory logic
  • documentation updates
  • audit changes for any missing test coverage
  • With Borg 1.2, both database restoration and config bootstrapping are broken now when using a temporary directory for the runtime directory. That's because Borg 1.2 doesn't support the "/./" hack, which means full runtime directory paths get stored into the archives including the randomly generated temporary directory portion. And because that temporary directory changes on every run (by design), any database dumps / bootstrap metadata cannot be found in the archive at the expected path.
  • run end-to-end tests
This is an implementation of #934. More specifically, these are the changes: * Update the logic that probes for the borgmatic streaming database dump, bootstrap metadata, and check state directories to support more platforms and use cases. * To reduce the chance of possible directory collisions (unintended or otherwise), also add a random subdirectory when using a potentially shared temporary directory like `/tmp` for the runtime (database dump + bootstrap metadata) directory. * Add the "RuntimeDirectory" and "StateDirectory" options to the sample systemd service file to support the new runtime and state directory logic. See #934 for details. ### Implementation notes The broad strokes changes here are: * Implement a new context manager for the borgmatic runtime directory containing the new logic. A context manager was necessary because if a temporary subdirectory in say `/tmp` gets created as part of this, borgmatic needs to actually delete it before exiting! So the context manager makes the cleanup step easier and more assured to happen. * Make use of this context manager in several borgmatic actions that need runtime directory access. * Change a ton of code to pass around the `borgmatic_runtime_directory` to various code that needs it, since we can no longer calculate it on the fly wherever it's needed (due to the aforementioned temporary subdirectory thing). * It wasn't mentioned in #934, but I also added `STATE_DIRECTORY` to one of the probed-for environment variables for the state directory in particular. (This is distinct from the runtime directory.) That way the systemd `StateDirectory=` option can be used to set borgmatic's state directory from a systemd service. ### Still to do * [x] manual testing of all the affected actions * [x] manual testing of all the various permutations of the runtime and state directory logic * [x] documentation updates * [x] audit changes for any missing test coverage * [x] With Borg 1.2, both database restoration and config bootstrapping are broken now when using a temporary directory for the runtime directory. That's because Borg 1.2 doesn't support the "/./" hack, which means full runtime directory paths get stored into the archives *including* the randomly generated temporary directory portion. And because that temporary directory changes on every run (by design), any database dumps / bootstrap metadata cannot be found in the archive at the expected path. * [x] run end-to-end tests
witten added 1 commit 2024-11-16 02:22:37 +00:00
witten added 1 commit 2024-11-16 06:24:16 +00:00
witten added 1 commit 2024-11-16 15:26:51 +00:00
SimonPilkington commented 2024-11-16 16:25:38 +00:00
First-time contributor
Copy Link

My Python isn't good enough to give this a proper review but thumbs up for supporting STATE_DIRECTORY. I didn't think to make a fuss about it but strictly speaking system service state should be under /var/lib like this makes it. Thanks for your work.

My Python isn't good enough to give this a proper review but thumbs up for supporting `STATE_DIRECTORY`. I didn't think to make a fuss about it but strictly speaking system service state should be under `/var/lib` like this makes it. Thanks for your work.
witten commented 2024-11-16 19:08:01 +00:00
Author
Owner
Copy Link

Sure thing! And thanks for commenting.. I'm glad to hear STATE_DIRECTORY would be useful. If you do want to review something that doesn't require a degree in Python, you can have a gander at the docs changes and make sure at least the documented logic (if not the actual code it's documenting) looks right:

Sure thing! And thanks for commenting.. I'm glad to hear `STATE_DIRECTORY` would be useful. If you do want to review something that doesn't require a degree in Python, you can have a gander at the docs changes and make sure at least the documented logic (if not the actual code it's documenting) looks right: - https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/937/files#diff-e9bb9548c4a770285bc0c6c1fbc780c4a49e5cac (scroll down to "Runtime directory") - https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/937/files#diff-483dc501d282da42473caeea603a3a246c735ce9
witten added 1 commit 2024-11-16 19:27:11 +00:00
SimonPilkington commented 2024-11-16 20:11:03 +00:00
First-time contributor
Copy Link

Sure thing! And thanks for commenting.. I'm glad to hear STATE_DIRECTORY would be useful. If you do want to review something that doesn't require a degree in Python, you can have a gander at the docs changes and make sure at least the documented logic (if not the actual code it's documenting) looks right:

The text itself LGTM but I have some unrelated comments.

It doesn't seem to be documented anywhere that borgmatic uses the runtime dir to place the bootstrap manifest into the archive and not just for the database backup feature. This means the runtime dir must be available and writable even if database backups aren't used.

Semi-relatedly it seems borgmatic will write to the state dir even if the check frequency feature isn't used. This may also be worth documenting or changing.

Both are nits because with StateDirectory and RuntimeDirectory now configured systemd will make sure these exist and are writable but problems with the runtime dir not being writable is how I ended up filing #934.

Edit: Perhaps consider documenting all files/dirs borgmatic uses like how borg does. (See "directories and files".)

> Sure thing! And thanks for commenting.. I'm glad to hear `STATE_DIRECTORY` would be useful. If you do want to review something that doesn't require a degree in Python, you can have a gander at the docs changes and make sure at least the documented logic (if not the actual code it's documenting) looks right: > > - https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/937/files#diff-e9bb9548c4a770285bc0c6c1fbc780c4a49e5cac (scroll down to "Runtime directory") > - https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/937/files#diff-483dc501d282da42473caeea603a3a246c735ce9 The text itself LGTM but I have some unrelated comments. It doesn't seem to be documented anywhere that borgmatic uses the runtime dir to place the bootstrap manifest into the archive and not just for the database backup feature. This means the runtime dir must be available and writable even if database backups aren't used. Semi-relatedly it seems borgmatic will write to the state dir even if the check frequency feature isn't used. This may also be worth documenting or changing. Both are nits because with `StateDirectory` and `RuntimeDirectory` now configured systemd will make sure these exist and are writable but problems with the runtime dir not being writable is how I ended up filing #934. Edit: Perhaps consider documenting all files/dirs borgmatic uses like how [borg does](https://borgbackup.readthedocs.io/en/1.4.0/usage/general.html). (See "directories and files".)
witten added 1 commit 2024-11-16 20:19:56 +00:00
witten added 1 commit 2024-11-17 00:03:55 +00:00
witten added 1 commit 2024-11-17 00:46:55 +00:00
witten commented 2024-11-17 02:42:33 +00:00
Author
Owner
Copy Link

It doesn't seem to be documented anywhere that borgmatic uses the runtime dir to place the bootstrap manifest into the archive and not just for the database backup feature. This means the runtime dir must be available and writable even if database backups aren't used.

Good point! It's now documented, such as it is, here: 57decfa4db

At some point, the docs could probably use some reorganization to factor more stuff out into reference docs.

Semi-relatedly it seems borgmatic will write to the state dir even if the check frequency feature isn't used. This may also be worth documenting or changing.

There is a (documented) case where frequency isn't specified but borgmatic still applies a default frequency, which is when no checks are declared at all. But one approach might be to skip writing the check time to the state directory when it's not specified for that check. Filed here: #938.

Edit: Perhaps consider documenting all files/dirs borgmatic uses like how borg does. (See "directories and files".)

I'll defer that to any forthcoming reference docs refactoring. (Which I probably need to file.)

Thanks for pointing out these issues!

> It doesn't seem to be documented anywhere that borgmatic uses the runtime dir to place the bootstrap manifest into the archive and not just for the database backup feature. This means the runtime dir must be available and writable even if database backups aren't used. Good point! It's now documented, such as it is, here: 57decfa4db425b29a172e6e0a1e4b1b60570500a At some point, the docs could probably use some reorganization to factor more stuff out into reference docs. > Semi-relatedly it seems borgmatic will write to the state dir even if the check frequency feature isn't used. This may also be worth documenting or changing. There is a (documented) case where `frequency` isn't specified but borgmatic still applies a default frequency, which is when no checks are declared at all. But one approach might be to skip writing the check time to the state directory when it's not specified for that check. Filed here: #938. > Edit: Perhaps consider documenting all files/dirs borgmatic uses like how borg does. (See "directories and files".) I'll defer that to any forthcoming reference docs refactoring. (Which I probably need to file.) Thanks for pointing out these issues!
👍 1
witten added 1 commit 2024-11-17 17:02:29 +00:00
witten added 1 commit 2024-11-17 18:16:50 +00:00
witten added 1 commit 2024-11-17 18:30:47 +00:00
witten added 1 commit 2024-11-17 19:56:12 +00:00
witten changed title from WIP: Update the logic that probes for the borgmatic streaming database dump, bootstrap metadata, and check state directories (#934). to Update the logic that probes for the borgmatic streaming database dump, bootstrap metadata, and check state directories (#934). 2024-11-17 19:56:35 +00:00
witten merged commit 76cfeda290 into main 2024-11-17 19:57:10 +00:00
witten commented 2024-11-18 23:25:34 +00:00
Author
Owner
Copy Link

Edit: Perhaps consider documenting all files/dirs borgmatic uses like how borg does. (See "directories and files".)

I'll defer that to any forthcoming reference docs refactoring. (Which I probably need to file.)

#942.

> > Edit: Perhaps consider documenting all files/dirs borgmatic uses like how borg does. (See "directories and files".) > > I'll defer that to any forthcoming reference docs refactoring. (Which I probably need to file.) #942.
witten deleted branch update-runtime-directory-logic 2024-11-18 23:25:43 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#937
No description provided.
Delete Branch "update-runtime-directory-logic"

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?