unexpected error when backing up databases without source_directories #655

New Issue

diivi · 2023-03-20T16:39:16Z

diivi commented

2023-03-20 16:39:16 +00:00

What I'm trying to do and why

I tried to dump a database in borgmatic without adding any source directories (didn't want to backup files).

Steps to reproduce (if a bug)

do same as above.

Actual behavior (if a bug)

Borgmatic reports a weird error.

Expected behavior (if a bug)

Borgmatic should not raise an error if source_directories_must_exist is False, and raise one (a better one) if it is True, this is already handled.

Other notes / implementation ideas

Check the discussion here - https://github.com/borgmatic-collective/borgmatic/pull/53

Environment

borgmatic version: [version here]

Use sudo borgmatic --version or sudo pip show borgmatic | grep ^Version

borgmatic installation method: [e.g., Debian package, Docker container, etc.]

Borg version: [version here]

Use sudo borg --version

Python version: [version here]

Use python3 --version

Database version (if applicable): [version here]

Use psql --version or mysql --version on client and server.

operating system and version: [OS here]

#### What I'm trying to do and why I tried to dump a database in borgmatic without adding any source directories (didn't want to backup files). #### Steps to reproduce (if a bug) do same as above. #### Actual behavior (if a bug) ![image](/attachments/0edb3e87-e130-43a3-a48e-4fc8153f3c16) Borgmatic reports a weird error. #### Expected behavior (if a bug) Borgmatic should not raise an error if `source_directories_must_exist` is False, and raise one (a better one) if it is True, this is already handled. #### Other notes / implementation ideas Check the discussion here - https://github.com/borgmatic-collective/borgmatic/pull/53 #### Environment **borgmatic version:** [version here] Use `sudo borgmatic --version` or `sudo pip show borgmatic | grep ^Version` **borgmatic installation method:** [e.g., Debian package, Docker container, etc.] **Borg version:** [version here] Use `sudo borg --version` **Python version:** [version here] Use `python3 --version` **Database version (if applicable):** [version here] Use `psql --version` or `mysql --version` on client and server. **operating system and version:** [OS here]

image.png

74 KiB

diivi changed title from ~~Borgmatic raises unexpected error when backing up databases without source_directories~~ to unexpected error when backing up databases without source_directories

2023-03-21 17:25:39 +00:00

witten added the

bug

label 2023-03-21 21:41:49 +00:00

witten commented

2023-03-21 21:44:15 +00:00

A copy/paste of a relevant comment on the linked GitHub PR:

So I think you found a legitimate bug in the current code! It looks like when database hooks are enabled and a non-existent directory is given in source_directories, then borgmatic gives the weird error message above (in your first screenshot) and exits. That's not intentional; non-existent source directories aren't supposed to cause errors (without your new option being set). When database hooks are not enabled and a non-existent directory is given in source_directories, Borg issues a warning but then continues on without any error, like this:

Processing files ...
/root/noexist: [Errno 2] No such file or directory: 'noexist'

Anyway, I think the bug you've uncovered occurs in collect_special_file_paths() in borgmatic/borg/create.py, which is triggered when a database hook is configured (or read_special is set to true). Specifically, the call to execute_command_and_capture_output() there blows up when one of the directories doesn't exist. At least, I think that's what's going on. If you want to try to fix the bug as part of this ticket it, you are welcome to do so. Otherwise, feel free to file it or just let me know and I'd be happy to have a look.

A copy/paste of a relevant comment on the [linked GitHub PR](https://github.com/borgmatic-collective/borgmatic/pull/53): So I think you found a legitimate bug in the current code! It looks like when database hooks are enabled and a non-existent directory is given in `source_directories`, then borgmatic gives the weird error message above (in your first screenshot) and exits. That's not intentional; non-existent source directories aren't supposed to cause errors (without your new option being set). When database hooks are _not_ enabled and a non-existent directory is given in `source_directories`, Borg issues a warning but then continues on without any error, like this: ``` Processing files ... /root/noexist: [Errno 2] No such file or directory: 'noexist' ``` Anyway, I think the bug you've uncovered occurs in `collect_special_file_paths()` in `borgmatic/borg/create.py`, which is triggered when a database hook is configured (or `read_special` is set to `true`). Specifically, the call to `execute_command_and_capture_output()` there blows up when one of the directories doesn't exist. At least, I think that's what's going on. If you want to try to fix the bug as part of this ticket it, you are welcome to do so. Otherwise, feel free to file it or just let me know and I'd be happy to have a look.

witten commented

2023-03-21 22:45:29 +00:00

@diivi wrote:

I don't understand why Borg raises an error when I have database hooks configured and pass a non-existent directory in this command though:

It's because database hooks being configured implicitly enables the read_special option, and the collect_special_file_paths() code path is only executed when read_special is true.

Also, this (similar) command works (when I don't have database hooks configured):

I think the difference here is how it's executed. Specifically, check out exit_code_indicates_error() in borgmatic/execute.py. What that function does is look at the Borg exit code and decide whether to call it an error or not. So exit status < 2 (e.g., 1 in this case, which is what Borg returns when a source file doesn't exist) is not treated as an error.

And here's the tricky part: exit_code_indicates_error() isn't always used. Lets say you call borgmatic create without any databases configured. That calls execute_command() which calls log_outputs() which calls exit_code_indicates_error() to determine what to do with Borg's exit code.

In contrast though, when you call borgmatic create with databases configured, read_special kicks on and collect_special_file_paths() is called prior to borg create. And collect_special_file_paths() calls execute_command_and_capture_output(), which calls Python's subprocess.check_output(), which raises regardless of the exit code as long as it's non-zero. Meaning the usual Borg exit code of 1 for a non-existent file is treated as an error instead of a warning. And boom, ugly error.

So what's the fix? It might be not using check_output() within execute_command_and_capture_output() and calling exit_code_indicates_error() there to take appropriate action on the result. (Or, continuing to use check_output() but catching the exception and dealing with it as needed based on the exit_code_indicates_error() result.)

The one caveat is that any fix should ideally not break other uses cases. For example, there might be other callers of execute_command_and_capture_output() that implicitly expect an exit code of 1 to be treated as an error.

Also, don't feel obligated to dive into fixing this just because you found the problem. People file borgmatic bugs and then don't fix them themselves all the time. 😄

@diivi [wrote](https://github.com/borgmatic-collective/borgmatic/pull/53#issuecomment-1478630771): > I don't understand why Borg raises an error when I have database hooks configured and pass a non-existent directory in this command though: It's because database hooks being configured implicitly enables the `read_special` option, and the `collect_special_file_paths()` code path is only executed when `read_special` is true. > Also, this (similar) command works (when I don't have database hooks configured): I think the difference here is how it's executed. Specifically, check out `exit_code_indicates_error()` in `borgmatic/execute.py`. What that function does is look at the Borg exit code and decide whether to call it an error or not. So exit status < 2 (e.g., 1 in this case, which is what Borg returns when a source file doesn't exist) is not treated as an error. And here's the tricky part: `exit_code_indicates_error()` isn't always used. Lets say you call `borgmatic create` without any databases configured. That calls `execute_command()` which calls `log_outputs()` which calls `exit_code_indicates_error()` to determine what to do with Borg's exit code. In contrast though, when you call `borgmatic create` with databases configured, `read_special` kicks on and `collect_special_file_paths()` is called prior to `borg create`. And `collect_special_file_paths()` calls `execute_command_and_capture_output()`, which calls Python's `subprocess.check_output()`, which raises regardless of the exit code as long as it's non-zero. Meaning the usual Borg exit code of 1 for a non-existent file is treated as an error instead of a warning. And boom, ugly error. So what's the fix? It *might* be not using `check_output()` within `execute_command_and_capture_output()` and calling `exit_code_indicates_error()` there to take appropriate action on the result. (Or, continuing to use `check_output()` but catching the exception and dealing with it as needed based on the `exit_code_indicates_error()` result.) The one caveat is that any fix should ideally not break other uses cases. For example, there might be other callers of `execute_command_and_capture_output()` that implicitly expect an exit code of 1 to be treated as an error. Also, don't feel obligated to dive into fixing this just because you found the problem. People file borgmatic bugs and then don't fix them themselves all the time. 😄

👍 1

witten referenced this issue from a commit

2023-03-22 20:03:06 +00:00

Fix error when a source directory doesn't exist and databases are configured (#655).

witten referenced this issue from a commit

2023-03-22 20:03:06 +00:00

Add missing source directory error fix to NEWS (#655).

witten commented

2023-03-22 20:05:20 +00:00

Fixed by @diivi here: https://github.com/borgmatic-collective/borgmatic/pull/56!

🚀 1

witten closed this issue