Using LVM snapshots results in extra output

apollo13 commented

2025-04-11 20:02:35 +00:00

Contributor

What I'm trying to do and why

I am running with LVM snapshots enabled and am seeing quite a few messages about leaked file descriptors, for instance:

ssh://yyyy/./paas_fox.borg: Looking for snapshots to remove in /home/borgmatic/lvm_snapshots/*
ssh://yyy/./paas_fox.borg: lvs --report-format json --options lv_name,lv_path --select lv_attr =~ ^s
File descriptor 3 (pipe:[14943]) leaked on lvs invocation. Parent PID 162: /usr/local/bin/python3

This would be fixed if execute_command_and_capture_output would set close_fds=True which it cannot due to the command saying:

# Necessary for passing credentials via anonymous pipe.

So one option would be to allow the caller to pass through close_fds, I don't think credentials are needed for lvm commands or accept the leak and pass LVM_SUPPRESS_FD_WARNINGS=1 as env. Another option would be to figure out what those file descriptors are and manually set close_on_exec on them (if that is possible).

What would you prefer?

Steps to reproduce

No response

Actual behavior

No response

Expected behavior

No response

Other notes / implementation ideas

No response

borgmatic version

No response

borgmatic installation method

No response

Borg version

No response

Python version

No response

Database version (if applicable)

No response

Operating system and version

No response

### What I'm trying to do and why I am running with LVM snapshots enabled and am seeing quite a few messages about leaked file descriptors, for instance: ``` ssh://yyyy/./paas_fox.borg: Looking for snapshots to remove in /home/borgmatic/lvm_snapshots/* ssh://yyy/./paas_fox.borg: lvs --report-format json --options lv_name,lv_path --select lv_attr =~ ^s File descriptor 3 (pipe:[14943]) leaked on lvs invocation. Parent PID 162: /usr/local/bin/python3 ``` This would be fixed if `execute_command_and_capture_output` would set `close_fds=True` which it cannot due to the command saying: ``` # Necessary for passing credentials via anonymous pipe. ``` So one option would be to allow the caller to pass through `close_fds`, I don't think credentials are needed for lvm commands or accept the leak and pass `LVM_SUPPRESS_FD_WARNINGS=1` as env. Another option would be to figure out what those file descriptors are and manually set close_on_exec on them (if that is possible). What would you prefer? ### Steps to reproduce _No response_ ### Actual behavior _No response_ ### Expected behavior _No response_ ### Other notes / implementation ideas _No response_ ### borgmatic version _No response_ ### borgmatic installation method _No response_ ### Borg version _No response_ ### Python version _No response_ ### Database version (if applicable) _No response_ ### Operating system and version _No response_

witten commented

2025-04-11 21:18:35 +00:00

Owner

Ah yes, I took the lazy approach with close_fds=False, and I guess this is the consequence. You are correct that the LVM hook doesn't need/use credentials. Probably adding a close_fds argument (defaulting to True) to the various execute_* functions makes the most sense. And then passing close_fds=False from any callers that make use of this functionality. Off the top of my head, that's at least: mariadb.py, mysql.py, and mongodb.py in hooks/data_source/ and also borg/environment.py.

The latter is probably the reason I made the lazy choice here; make_environment() is responsible for setting the credential, but it's called by pretty much everything in borg/*.py and also actions/check.py, so execute_* function calls in those files would have to be updated.

Let me know if this is something you're interested in taking on or if instead you're running screaming once you see the scope. 😄

(I'd also be fine with an even more surgical solution that only passes the file descriptors used. That seems like even more work though.)

Ah yes, I took the lazy approach with `close_fds=False`, and I guess this is the consequence. You are correct that the LVM hook doesn't need/use credentials. Probably adding a `close_fds` argument (defaulting to `True`) to the various `execute_*` functions makes the most sense. And then passing `close_fds=False` from any callers that make use of this functionality. Off the top of my head, that's at least: `mariadb.py`, `mysql.py`, and `mongodb.py` in `hooks/data_source/` and also `borg/environment.py`. The latter is probably the reason I made the lazy choice here; `make_environment()` is responsible for setting the credential, but it's called by pretty much everything in `borg/*.py` and also `actions/check.py`, so `execute_*` function calls in those files would have to be updated. Let me know if this is something you're interested in taking on or if instead you're running screaming once you see the scope. 😄 (I'd also be fine with an even more surgical solution that only passes the file descriptors used. That seems like even more work though.)

apollo13 commented

2025-04-15 06:07:58 +00:00

Author

Contributor

Sorry for the late response, apparently I did not get an email notification :/

I'll try to take a look at the surgical solution, might need some time though since I will be at DjangoCon Europe next week…

Sorry for the late response, apparently I did not get an email notification :/ I'll try to take a look at the surgical solution, might need some time though since I will be at DjangoCon Europe next week…

witten commented

2025-04-15 06:27:43 +00:00

Owner

Okay, no worries. Enjoy the convention! And let me know if it turns out you can't get to this. Thanks!

apollo13 referenced this issue from a pull request that will close it,

2025-04-15 17:05:45 +00:00

Explicitly close filedescriptors when invoking lvm commands. Closes #1068 #1074

apollo13 commented

2025-04-15 17:08:25 +00:00

Author

Contributor

I have added a simple PR to borgmatic-collective/borgmatic#1074 -- worked fine locally. This approach is the "safe" approach, it is really hard to determine if and when file descriptors get passed in, so before breaking something I opted least invasive patch. If over time all calls get converted to close_fds=True or pass_fds gets added to the other calls, then we could flip the default at some point.

While playing with my backup I ran into borgmatic-collective/borgmatic#1074 where I am not sure how to fix yet ;)

I have added a simple PR to borgmatic-collective/borgmatic#1074 -- worked fine locally. This approach is the "safe" approach, it is really hard to determine if and when file descriptors get passed in, so before breaking something I opted least invasive patch. If over time all calls get converted to `close_fds=True` or `pass_fds` gets added to the other calls, then we could flip the default at some point. While playing with my backup I ran into borgmatic-collective/borgmatic#1074 where I am not sure how to fix yet ;)

👍 1

witten closed this issue

2025-04-30 15:23:44 +00:00

witten referenced this issue from a commit

2025-04-30 15:23:45 +00:00

Explicitly close filedescriptors when invoking lvm commands. Closes #1068

witten referenced this issue from a commit

2025-04-30 15:23:45 +00:00

Explicitly close file descriptors when invoking LVM commands (#1068).

witten referenced this issue from a commit

2025-04-30 15:30:39 +00:00

Add LVM file descriptor warning fix to NEWS (#1068).