Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separating Ext2/3/4 from filesys and using lsblk -rpo instead /proc/mounts #3502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Separating Ext2/3/4 from filesys and using lsblk -rpo instead /proc/mounts #3502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions sos/report/plugins/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1616,6 +1616,19 @@ def get_tags_for_file(self, fname):
tags.extend(val)
return tags

def get_devices_by_fstype(self, fstype):
"""Get list of devices based on fstype and returns empty array in case
of failure.
"""
dev = []
all_devs = self.exec_cmd("lsblk -rpo NAME,FSTYPE")
if (all_devs['status'] == 0 and not all_devs['truncated']):
if all_devs['status'] == 0:
for line in all_devs['output'].splitlines():
if (fstype in line) and (line.split()[0] not in dev):
dev.append(line.split()[0])
return dev
Comment on lines +1619 to +1630
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm torn about having this move from Plugin to SoSReport, and have it run alongside our device introspection, like for block devs. Granted, these aren't hardware devices, but it's in the same vein as what we do for storage and network interfaces.

That way we run this once, when sos first initializes, and then in Plugin this function would instead simply filter through an already-made list/dict/whatever for those matching the requested fstype.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I will into it, the main idea here is to reuse the info, not just for FS but also md.py can benefit from it as the FSTYPE in limited to FS as per blkid -k

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm torn about having this move from Plugin to SoSReport, and have it run alongside our device introspection, like for block devs. Granted, these aren't hardware devices, but it's in the same vein as what we do for storage and network interfaces.

That way we run this once, when sos first initializes, and then in Plugin this function would instead simply filter through an already-made list/dict/whatever for those matching the requested fstype.

So I have the work almost ready to have this merged into _get_hardware_devices(), but there is thing. If I create nested dict like:

 {'storage': {'block': ['data0', 'data1'], 'fstype': {'unknown': ['block0','block1'],
                                 'vfat': ['block2']}}}

The function recursive_dict_values_by_key() will not correctly return the device list, only if I put it into same level as storage and network dict are. So the function does not seems to recursive properly. Any pointers what could be there wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have been clearer - my intent was to suggest that this be at the same level as storage, network, or similar. So plugins would access it via something like for fs in self.devices['filesystems']['xfs']: ...blah....

Again, this is a bit of a misnomer placing it under devices, so if you prefer we can also make a separate attribute to hold these, so that plugins could do something like for fs in self.filesystems['xfs']:


def generate_copyspec_tags(self):
"""After file collections have completed, retroactively generate
manifest entries to apply tags to files copied by generic copyspecs
Expand Down
57 changes: 57 additions & 0 deletions sos/report/plugins/ext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# This file is part of the sos project: https://github.com/sosreport/sos
#
# This copyrighted material is made available to anyone wishing to use,
# modify, copy, or redistribute it subject to the terms and conditions of
# version 2 of the GNU General Public License.
#
# See the LICENSE file in the source distribution for further information.

from sos.report.plugins import Plugin, IndependentPlugin, PluginOpt


class Ext(Plugin, IndependentPlugin):
"""This plugin collects information on mounted Ext2/3/4 filessystems on
the local system.

Users should expect `dumpe2fs -h` or `dumpe2fs` collections by this
plugin for each Ext2/3/4 filesystem that is recognized by lsblk.
"""

short_desc = 'Ext2/3/4 filesystem'

plugin_name = 'ext'
profiles = ('storage',)
files = ('/sys/fs/ext4/', '/proc/fs/ext4/', '/proc/fs/jbd2/')

option_list = [
PluginOpt('dumpe2fs', default=False, desc='dump filesystem info'),
PluginOpt('frag', default=False,
desc='collect filesystem fragmentation status')
]

def setup(self):
dumpe2fs_opts = '-h'
if self.get_option('dumpe2fs'):
lherbolt marked this conversation as resolved.
Show resolved Hide resolved
dumpe2fs_opts = ''
allfs = self.get_devices_by_fstype('ext')
if allfs:
for fs in allfs:
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional is superfluous, if allfs is an empty list, python just continues on, skipping whatever is in the for-loop.

Copy link
Contributor Author

@lherbolt lherbolt Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting something like:

    def setup(self):
        allfs = self.get_devices_by_fstype('xfs')
        for fs in allfs:
            self.add_cmd_output(f"xfs_info {fs}", tags="xfs_info")
            self.add_cmd_output(f"xfs_admin -l -u {fs}")

        if not allfs:
            mounts = '/proc/mounts'
            ext_fs_regex = r"^(/dev/.+).+xfs\s+"
            for dev in zip(self.do_regex_find_all(ext_fs_regex, mounts)):
                for e in dev:
                    parts = e.split()
                    self.add_cmd_output(f"xfs_info {parts[1]}",
                                        tags="xfs_info")
                    self.add_cmd_output(f"xfs_admin -l -u {parts[0]}")

        self.add_copy_spec(self.files)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's correct. We could even button it up a little more by for fs in self.get_devices_by_fstype('xfs'): provided there is nothing else using allfs later on.

self.add_cmd_output(f"dumpe2fs {dumpe2fs_opts} {fs}",
tags="dumpe2fs_h")

if self.get_option('frag'):
self.add_cmd_output(f"e2freefrag {fs}", priority=100)

else:
mounts = '/proc/mounts'
ext_fs_regex = r"^(/dev/\S+).+ext[234]\s+"
for dev in self.do_regex_find_all(ext_fs_regex, mounts):
self.add_cmd_output(f"dumpe2fs {dumpe2fs_opts} {fs}",
tags="dumpe2fs_h")

if self.get_option('frag'):
self.add_cmd_output(f"e2freefrag {fs}", priority=100)

self.add_copy_spec(self.files)

# vim: set et ts=4 sw=4 :
33 changes: 24 additions & 9 deletions sos/report/plugins/filesys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@ class Filesys(Plugin, DebianPlugin, UbuntuPlugin, CosPlugin):
option_list = [
PluginOpt('lsof', default=False,
desc='collect information on all open files'),
PluginOpt('dumpe2fs', default=False, desc='dump filesystem info'),
PluginOpt('frag', default=False,
desc='collect filesystem fragmentation status')
]

def setup(self):
Expand Down Expand Up @@ -75,18 +72,36 @@ def setup(self):
if self.get_option('lsof'):
self.add_cmd_output("lsof -b +M -n -l -P", root_symlink="lsof",
priority=50)
option_list = [
PluginOpt('dumpe2fs', default=False, desc='dump filesystem info'),
PluginOpt('frag', default=False,
desc='collect filesystem fragmentation status')
]

def setup(self):
Comment on lines +75 to +81
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, missed this on my first review. This seems like a copy/paste error. The options_list shouldn't go within setup(), and we've got a duplicate definition of the setup() method, which would break several things. If I'm reading everything right on the github webui, I think you just need to remove these lines entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah copy&paste error

dumpe2fs_opts = '-h'
if self.get_option('dumpe2fs'):
dumpe2fs_opts = ''
mounts = '/proc/mounts'
ext_fs_regex = r"^(/dev/\S+).+ext[234]\s+"
for dev in self.do_regex_find_all(ext_fs_regex, mounts):
self.add_cmd_output("dumpe2fs %s %s" % (dumpe2fs_opts, dev),
tags="dumpe2fs_h")
allfs = self.get_devices_by_fstype('ext')
if allfs:
for fs in allfs:
Comment on lines +86 to +87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we can drop the conditional and unnest the entire block by one level here.

self.add_cmd_output(f"dumpe2fs {dumpe2fs_opts} {fs}",
tags="dumpe2fs_h")

if self.get_option('frag'):
self.add_cmd_output(f"e2freefrag {fs}", priority=100)

else:
mounts = '/proc/mounts'
ext_fs_regex = r"^(/dev/\S+).+ext[234]\s+"
for dev in self.do_regex_find_all(ext_fs_regex, mounts):
self.add_cmd_output(f"dumpe2fs {dumpe2fs_opts} {fs}",
tags="dumpe2fs_h")

if self.get_option('frag'):
self.add_cmd_output("e2freefrag %s" % (dev), priority=100)
self.add_cmd_output(f"e2freefrag {fs}", priority=100)

self.add_copy_spec(self.files)

def postproc(self):
self.do_file_sub(
Expand Down
37 changes: 21 additions & 16 deletions sos/report/plugins/xfs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@


class Xfs(Plugin, IndependentPlugin):
"""This plugin collects information on mounted XFS filessystems on the
local system.
"""This plugin collects information on mounted XFS filessystems on
the local system.

Users should expect `xfs_info` and `xfs_admin` collections by this plugin
for each XFS filesystem that is locally mounted.
Users should expect `xfs_info` and `xfs_admin` collections by this
plugin for each XFS filesystem that is recognized by lsblk.
"""

short_desc = 'XFS filesystem'
Expand All @@ -25,18 +25,23 @@ class Xfs(Plugin, IndependentPlugin):
kernel_mods = ('xfs',)

def setup(self):
mounts = '/proc/mounts'
ext_fs_regex = r"^(/dev/.+).+xfs\s+"
for dev in zip(self.do_regex_find_all(ext_fs_regex, mounts)):
for e in dev:
parts = e.split(' ')
self.add_cmd_output("xfs_info %s" % (parts[1]),
allfs = self.get_devices_by_fstype('xfs')
if allfs:
for fs in allfs:
Comment on lines +29 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here.

self.add_cmd_output(f"xfs_info {fs}",
tags="xfs_info")
self.add_cmd_output("xfs_admin -l -u %s" % (parts[0]))

self.add_copy_spec([
'/proc/fs/xfs',
'/sys/fs/xfs'
])
self.add_cmd_output(f"xfs_admin -l -u {fs}")

else:
mounts = '/proc/mounts'
ext_fs_regex = r"^(/dev/.+).+xfs\s+"
for dev in zip(self.do_regex_find_all(ext_fs_regex, mounts)):
for e in dev:
parts = e.split()
self.add_cmd_output(f"xfs_info {parts[1]}",
tags="xfs_info")
self.add_cmd_output(f"xfs_admin -l -u {parts[0]}")

self.add_copy_spec(self.files)

# vim: set et ts=4 sw=4 :