Separating Ext2/3/4 from filesys and using lsblk -rpo instead /proc/mounts #3502
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
There was a problem hiding this comment.
With this refactor, we are now collecting potentially the same set of items from 2 separate plugins, and we are typically not keen on that.
We'll also need to double check to see if any automation that our downstream have that interrogates files collected in filesys in order for them to be removed from there.
I am sure other core reviewers may have other things on this
I'm not sure if this should go in the same commit or a separated one. My feeling is that it should be a second commit, but lets hear other's opinions. We are still using /proc/mounts in filesys.py: So it may be worth removing that code completely from filesys now, since you'll be getting it via this new plugin. |
Yeah sorry about that forgot to push filesys where the Ext2/3/4 are removed. I will update the pull request. |
Same on our side, but I see a lot of benefits on these changes, specially separating ext from filesys like xfs is -that is, with my storage/filesystem engineer hat, I can find it really really useful. We need to find a way to make sure these changes are properly propagated to users using automation. Some kind of policy for coexistence and maybe duplication for some time, in terms of minor releases. Other options, but I imagine these are less agreeable, are:
I don't think we've ever done these kind of things in sos, I guess to avoid interlinking plugins. |
It can happen the FS of interest is not actually mounted and then we have no info about it. This is revorks of how we should get list of devices. It adds a new __init__.py fuction to decrease code duplication get_dev_by_fstype(self,fstype). It is using lsblk instead of /proc/mounts. Last change is separation of Ext2/3/4 info into separate module from filesys. Signed-off-by: Lukas Herbolt <[email protected]>
|
I hope I add all the changes. About the duplicate info, we can append to the filesys dumpe2fs some note about being deprecated in next X.Y releases. |
There was a problem hiding this comment.
Mostly a couple minor points below, however I think it warrants fleshing out moving the discovery of filesystems to SoSReport, as part of our _get_hardware_devices() bits, so that we generate this information once, and have plugins (and potentially any other part of sos) reference that collection.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm torn about having this move from
PlugintoSoSReport, 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
Pluginthis function would instead simply filter through an already-made list/dict/whatever for those matching the requestedfstype.
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?
There was a problem hiding this comment.
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']:
| if allfs: | ||
| for fs in allfs: |
There was a problem hiding this comment.
The conditional is superfluous, if allfs is an empty list, python just continues on, skipping whatever is in the for-loop.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| if allfs: | ||
| for fs in allfs: |
There was a problem hiding this comment.
Same here, we can drop the conditional and unnest the entire block by one level here.
| if allfs: | ||
| for fs in allfs: |
Automation should be using This isn't really that different from other plugins we've broken up in the past, like |
| option_list = [ | ||
| PluginOpt('dumpe2fs', default=False, desc='dump filesystem info'), | ||
| PluginOpt('frag', default=False, | ||
| desc='collect filesystem fragmentation status') | ||
| ] | ||
|
|
||
| def setup(self): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah copy&paste error
|
I will finalize this one after ACK/NACK/CHANGE of #3538 |
|
uups went out of my radar, will look at it asap |
[xfs,filesys,ext] Get list of devices by lsblk -lpo NAME,FSTYPE.
It can happen the FS of interest is not actually mounted and then we have no info about it. This is revorks of how we should get list of devices. It adds a new init.py fuction to decrease code duplication get_dev_by_fstype(self,fstype). It is using lsblk instead of /proc/mounts. Last change is separation of Ext2/3/4 info into separate module from filesys.
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines