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.

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

[block] Refine the '/sys/block' disks query #1897

Closed
slashdd opened this issue Dec 18, 2019 · 7 comments
Closed

[block] Refine the '/sys/block' disks query #1897

slashdd opened this issue Dec 18, 2019 · 7 comments

Comments

@slashdd
Copy link

slashdd commented Dec 18, 2019

The [block] plugin is instruct to look inside /sys/block and perform some query cmd against every disk(s) found.

https://github.com/sosreport/sos/blob/master/sos/plugins/block.py#L42-L54

I think sosreport should be smarter and query only valid (in-used/mounted) devices, and not everything. Inside /sys/block one can find unused loop devices for instance pre-created for future needs.

Using the above loop device example, sosreport should only be querying what its in used, and don't bother querying unused loop device, but only list them, as it may or may not generate unwanted error for nothing.

@slashdd
Copy link
Author

slashdd commented Dec 18, 2019

It currently does generate error in Ubuntu, and we suspect systemd-udevd to be the cause, but still while it is harmless at sosreport point of view, I don't think sosreport should query unused disk anyway.

@slashdd
Copy link
Author

slashdd commented Dec 18, 2019

For reference about the Ubuntu bug:
https://bugs.launchpad.net/bugs/1856871

@slashdd slashdd changed the title Refine the '/sys/block' disks query [block] Refine the '/sys/block' disks query Dec 18, 2019
@slashdd
Copy link
Author

slashdd commented Dec 18, 2019

I'm also wondering if querying parted on loop devices is helpful at all.

Because we could simply do this instead:

        if os.path.isdir("/sys/block"):
            for disk in os.listdir("/sys/block"):
-              if disk.startswith("ram")
+              if disk.startswith("ram") or disk.startswith("loop"):
                    continue
                disk_path = os.path.join('/dev/', disk)
                self.add_udev_info(disk_path)
                self.add_udev_info(disk_path, attrs=True)
                self.add_cmd_output([
                    "parted -s %s unit s print" % (disk_path),
                    "fdisk -l %s" % disk_path
                ])

@slashdd
Copy link
Author

slashdd commented Dec 18, 2019

@BryanQuigley brought to my attention the following PR: #1508
This may possibly fix the situation I'm trying to solve, I'll give it a try during the holidays or when I get back in Jan 2020.

@bmr-cymru
Copy link
Member

What are the errors you are seeing on Ubuntu? Unfortunately there are cases where these unused devices are relevant to support engineers, so I think excluding all loop devices isn't the right answer here.

I haven't taken a close look at the block device iterators work for a while but I'll try to go through it again before the new year.

@bmr-cymru
Copy link
Member

I'm also wondering if querying parted on loop devices is helpful at all.

Somewhat unfortunately it has been possible to configure partitioned loop devices for some time now (since 2.6ish) - and that can create problems when users don't fully understand the configuration or its implications. I'm generally not a fan of parted for diagnostics but there is at least occasional need to understand if loop devices contain partition metadata or not (or have been configured as partitionable loop devices).

@TurboTurtle
Copy link
Member

Looking at the LP report, I think it's safe to say the error being generated is not something sos can address, and as Bryn notes there are times the unused loop device info is relevant to support cases. With that said, I'm closing this as the block plugin uses the (now) standardized device list via add_blockdev_cmd().

If there are further concerns with capturing unused loop device info, or if there should be further restrictions on the block plugin's device iteration, feel free to re-open this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants