Skip to content

Commit

Permalink
Fix error message for errors umounting sysdataset
Browse files Browse the repository at this point in the history
This commit does the following:

1. Fixes a bug whereby query for processes owned by paths would not
   list the paths within the `paths` variable in output
2. Adds ability to pass a parameter to include files opened by
   the middlewared process in the output to assist in troubleshooting
   issues where the middlewared process is what is preventing system
   dataset umount from occuring.
3. Passes new parameter when handling errors during systemdataset
   setup.
4. Parses the recursive umount error message for the dataset that failed
   and pass that to the call to look up processes.
  • Loading branch information
anodos325 committed Mar 4, 2024
1 parent f1479e3 commit 3081a48
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 17 deletions.
55 changes: 40 additions & 15 deletions src/middlewared/middlewared/plugins/pool_/dataset_processes.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,17 @@ async def kill_processes(self, oid, control_services, max_tries=5):
})

@private
def processes_using_paths(self, paths, include_paths=False):
def processes_using_paths(self, paths, include_paths=False, include_middleware=False):
"""
Find processes using paths supplied via `paths`. Path may be an absolute path for
a directory (e.g. /var/db/system) or a path in /dev/zvol or /dev/zd*
`include_paths`: include paths that are open by the process in output. By default
this is not included in output for performance reasons.
`include_middleware`: include files opened by the middlewared process in output.
These are not included by default.
"""
exact_matches = set()
include_devs = []
for path in paths:
Expand All @@ -138,29 +148,44 @@ def processes_using_paths(self, paths, include_paths=False):
result = []
if include_devs or exact_matches:
for pid in os.listdir('/proc'):
if not pid.isdigit() or int(pid) == os.getpid():
if not pid.isdigit() or (not include_middleware and (int(pid) == os.getpid())):
continue

with contextlib.suppress(FileNotFoundError, ProcessLookupError):
# FileNotFoundError for when a process is killed/exits
# while we're iterating
found = False
paths = set()
found_paths = set()
for f in os.listdir(f'/proc/{pid}/fd'):
fd = f'/proc/{pid}/fd/{f}'
is_link = False
realpath = None
if (
(include_devs and os.stat(fd).st_dev in include_devs) or
(
exact_matches and
(is_link := os.path.islink(fd)) and
(realpath := os.path.realpath(fd)) in exact_matches
)
):
found = True
if is_link:
paths.add(realpath)
with contextlib.suppress(FileNotFoundError):
# Have second suppression here so that we don't lose list of files
# if we have TOCTOU issue on one of files.
#
# We want to include file in list of paths in the following
# situations:
#
# 1. File is regular file and has same device id as specified path
# 2. File is a symbolic link and exactly matches a provided /dev/zvol or /dev/zd path
if (
(include_devs and os.stat(fd).st_dev in include_devs) or
(
exact_matches and
(is_link := os.path.islink(fd)) and
(realpath := os.path.realpath(fd)) in exact_matches
)
):
found = True
if include_paths:
if is_link:
# This is a path in `/dev/zvol` or `/dev/zd*`
found_paths.add(realpath)
else:
# We need to readlink to convert `/proc/<pid>/fd/<fd>` to
# the file's path name.
found_paths.add(os.readlink(fd))

if found:
with open(f'/proc/{pid}/comm') as comm:
Expand All @@ -177,7 +202,7 @@ def processes_using_paths(self, paths, include_paths=False):
proc['cmdline'] = cmdline

if include_paths:
proc['paths'] = sorted(paths)
proc['paths'] = sorted(found_paths)

result.append(proc)

Expand Down
6 changes: 4 additions & 2 deletions src/middlewared/middlewared/plugins/sysdataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -598,8 +598,10 @@ def __umount(self, pool, uuid):

error = f'Unable to umount {mp}: {stderr}'
if 'target is busy' in stderr:
processes = self.middleware.call_sync('pool.dataset.processes_using_paths', [mp], True)
error += f'\nThe following processes are using {mp!r}: ' + json.dumps(processes, indent=2)
# error message is of format "umount: <mountpoint>: target is busy"
ds_mp = stderr.split(':')[1].strip()
processes = self.middleware.call_sync('pool.dataset.processes_using_paths', [ds_mp], True, True)
error += f'\nThe following processes are using {ds_mp!r}: ' + json.dumps(processes, indent=2)

raise CallError(error) from None

Expand Down
11 changes: 11 additions & 0 deletions tests/api2/test_pool_dataset_track_processes.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,17 @@ def test__open_path_and_check_proc(request, datasets, file_open_path, arg_path):
result = res[0]
assert result['pid'] == open_pid, f'{result["pid"]!r} does not match {open_pid!r}'
assert result['cmdline'] == cmdline, f'{result["cmdline"]!r} does not match {cmdline!r}'
assert 'paths' not in result

res = call('pool.dataset.processes_using_paths', [arg_path(ssh)], True)
assert len(res) == 1
result = res[0]
assert result['pid'] == open_pid, f'{result["pid"]!r} does not match {open_pid!r}'
assert result['cmdline'] == cmdline, f'{result["cmdline"]!r} does not match {cmdline!r}'
assert 'paths' in result
assert len(result['paths']) == 1
assert result['paths'][0] == test_file if test_file.startswith('/mnt') else '/dev/zd0'

finally:
if opened:
ssh(f'kill -9 {open_pid}', check=False)

0 comments on commit 3081a48

Please sign in to comment.