[scsi] add scsi tapes cmd function

[jcastill]

This commit enables the capture of SCSI tape
information via two commands:

udevadm info -a
udevadm info

It relies on two commits. The first one adds the required
commands in the scsi plugin, the second one adds the
function get_tape_devs() and add_blockdev_cmd() so
plugins can iterate over them. It's based on get_block_devs().

Resolves: #2463

Signed-off-by: Jose Castillo [email protected]

---

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

• Is the commit message split over multiple lines and hard-wrapped at 72 characters?
• Is the subject and message clear and concise?
• Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
• Does the commit contain a Signed-off-by: First Lastname [email protected]?
• If this commit closes an existing issue, is the line Closes: #ISSUENUMBER included in an independent line?
• If this commit resolves an existing pull request, is the line Resolves: #PRNUMBER included in an independent line?

[pmoravec]

Can't be add_blockdev_cmd enhanced accordingly? As we basically duplicate its code here.

[TurboTurtle]

That was my thought too, we can add tape as a known keyword for devices. Otherwise, this looks good and get_tape_devs() is a nice addition.

[bmr-cymru]

Ack to the direction but there seem to be a few stray hunks here?

[bmr-cymru]

This hunk of this patch seems to be fixing up some whitespace errors in the previous patch? It's better to just re-do the patches to avoid this kind of cross-patch dependency (git rebase -i).

[jcastill]

Sorry, I jumped between the RHEL 7 server where I have the tapes, to test and code this, and the usual server where I send the PRs, and I noticed too late the mix

[bmr-cymru]

It's usually better to put clean ups (like non functional whitespace changes) into their own patches rather than tucking them away in another functional commit.

[bmr-cymru]

Seems reasonable - one nit is that tape devices are typically character devices that support sequential access modes, rather than block devices that support random access. From the sos code side it's a minor detail, but it could be confusing for other readers of the code down the line if we don't clear this up.

[bmr-cymru]

Tape devices are normally character devices not block devices.

[jcastill]

I missed this one when copying from the other block, sorry!

[TurboTurtle]

Yeah - it'd be a bit of a misnomer (add_blockdev_cmd('foo', devices='tape')), but I think that's preferable to having another wrapper here specifically for tape devices. My thought is that in the future we'll have an add_netdev_cmd() that similar supports keywords for e.g. eth devices, bridge devices, etc... without having separate add_ethdev_cmd() wrappers. We just need to document properly.

But agreed, the tag should be changed from block to chardev or similar.

[bmr-cymru]

but I think that's preferable to having another wrapper here specifically for tape devices.

Sure, but that isn't our only option - imho, if we are broadening the scope of add_blockdev_cmd to non-block devices, it's worth renaming it something like add_device_cmd, which I think is a much more apt name for the work it'd be taking on following such a change. Even more so if future network device collection interfaces can share some of the wiring.

[jcastill]

OK, let me see if I can summarise my understanding. _get_hardware_devices() that calls one of these:

get_fibre_devs()
get_block_devs()
get_tape_devs()

get_tape_devs() could become get_char_devs() to cover more chardevs in the future, and 'tape' be a part of it.

Then:

add_blockdev_cmd() that covers block devices, and deals with 'block' and 'fibre'
add_tape_cmd() that perhaps should be named add_chardev_cmd(), to cover more character devices in the future
add_netdev_cmd() that should cover network devices in the future

And all these are wrappers into _add_device_cmd(), but I'm repeating code in add_tape_cmd()/add_chardev_cmd(). So, what Bryn proposes could look like this, right?

def add_device_cmd(self, cmds, devices='block', timeout=300,
 sizelimit=None, chroot=True, runat=None, env=None,
 binary=False, prepend_path=None, whitelist=[],
 blacklist=[], tags=[]):

We only need to change 6 calls to add_blockdev_cmd(), and I think we cover them being by default block with the argument "devices='block'", right?

sos/report/plugins/block.py: self.add_blockdev_cmd(cmds, blacklist='ram.')
sos/report/plugins/fibrechannel.py: self.add_blockdev_cmd("udevadm info -a %(dev)s", devices='fibre')
sos/report/plugins/init.py: def add_blockdev_cmd(self, cmds, devices='block', timeout=300,
sos/report/plugins/md.py: self.add_blockdev_cmd("mdadm -E %(dev)s",
sos/report/plugins/nvme.py: self.add_blockdev_cmd(cmds, whitelist='nvme.')
sos/report/plugins/scsi.py: self.add_blockdev_cmd("udevadm info -a %(dev)s", devices=scsi_hosts,

And I add add_tapedev_cmd() code into add_device_cmd(), like this:

    def add_device_cmd(self, cmds, devices='block', timeout=300,
                         sizelimit=None, chroot=True, runat=None, env=None,
                         binary=False, prepend_path=None, whitelist=[],
                         blacklist=[], tags=[]):
   [...]
        _dev_tags = []
        if isinstance(tags, str):
            tags = [tags]
        if devices == 'block':
            prepend_path = prepend_path or '/dev/'
            devices = self.devices['block']
            _dev_tags.append('block')
        if devices == 'fibre':
            devices = self.devices['fibre']
            _dev_tags.append('fibre')
        if devices == 'tape':
            devices = self.devices['tape']
            _dev_tags.append('tape')
        _dev_tags.extend(tags)
        self._add_device_cmd(cmds, devices, timeout=timeout,
                             sizelimit=sizelimit, chroot=chroot, runat=runat,
                             env=env, binary=binary, prepend_path=prepend_path,
                             whitelist=whitelist, blacklist=blacklist,
                             tags=_dev_tags)

So there's no code duplication, the function covers more kind of devices, and we open the door to being able to add just three lines in this function to support network devices (and obviously more code in other functions as well).

[TurboTurtle]

@jcastill generally yes I believe we're on the same page. To summarize:

• rename add_blockdev_cmd() to add_device_cmd()
• remove add_tape_cmd(), add logic to add_device_cmd() to make the appropriate substitution when devices='tape'
• update necessary plugins for blockdev -> device

Personally I don't mind get_tape_devs() but if you want to name it get_char_devs() then that's perfectly fine too.

[jcastill]

@TurboTurtle ok, shall I use this PR for all these changes, or do you prefer if I group them in another way?

About get_tape_devs() vs get_char_devs(), perhaps for now tape is better, and more descriptive. If in the future we need to add other char devices, we can check if renaming is better than actually having another function.

[TurboTurtle]

This PR is fine, just logically separate the changes in different commits. E.G. one for the blockdev -> device rename and current plugin changes, another for the tape device additions, etc..

[jcastill]

This is now obsolete, with the changes to add_device_cmd() and other new improved functions. I'll send a new PR with the logic for tape devices soon.