[sos-devel] [PATCH] sosreport: Capture IBM Power RAID storage adapter configuration information

[Bryn M. Reeves]

Hi Aruna,

Thanks for the submission - see in-line for review comments.

On Thu, Sep 11, 2014 at 04:47:17PM +0530, Aruna Balakrishnaiah wrote:
> Capture information which helps in better understanding of
> IBM Power RAID storage adapter configuration. Since iprconfig
> is specific to power adding the relevant commands in powerpc plugin.
> 
> Signed-off-by: Aruna Balakrishnaiah <aruna at linux.vnet.ibm.com>
> ---
>  sos/plugins/powerpc.py |   23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

I think it may be better to put this into its own plugin. This is very
much a storage-specific addition and we've been moving toward more fine
grained plugins for a while.

One reaon this will soon become even more important is Issue #247:

  https://github.com/sosreport/sos/issues/247

The current proposal is to tag plugins with 'profiles' to allow a user to
select e.g. all storage-relevant data with:

  # sosreport --batch --profile=storage

Having this in its own plugin makes that type of arrangement simpler.

> diff --git a/sos/plugins/powerpc.py b/sos/plugins/powerpc.py
> index c829a5f..4495ea5 100644
> --- a/sos/plugins/powerpc.py
> +++ b/sos/plugins/powerpc.py
> @@ -16,6 +16,7 @@
>  # specific logs for Pseries, PowerNV platforms.
>  
>  import os
> +import re
>  from sos.plugins import Plugin, RedHatPlugin, UbuntuPlugin, DebianPlugin
>  
>  
> @@ -58,9 +59,29 @@ class PowerPC(Plugin, RedHatPlugin, UbuntuPlugin, DebianPlugin):
>                  "ppc64_cpu --dscr",
>                  "lscfg -vp",
>                  "lsmcode -A",
> -                "lsvpd --debug"
> +                "lsvpd --debug",
> +                "iprconfig -c show-config",
> +                "iprconfig -c show-alt-config",
> +                "iprconfig -c show-arrays",
> +                "iprconfig -c show-jbod-disks",
> +                "iprconfig -c show-ioas",
> +                "lsscsi -g"
>              ])
>  
> +            devices = []
> +            for line in os.popen("iprconfig -c show-ioas").readlines():

Need to use a Plugin API for this. E.g. Plugin::call_ext_prog(). Plugins should
never call os.popen or subprocess APIs directly.

> +                temp = line.split(' ')
> +                p = re.compile('sg')
> +                if p.search(temp[0]):
> +                     devices.append(temp[0])
> +
> +            for device in devices:
> +                self.add_cmd_output("iprconfig -c show-details %s" % (device,))
> +
> +            for line in os.popen("lsscsi -g | grep enclo").readlines():

Ditto for popen use here.

It would be good to avoid lsscsi here if possible as it's already exec'ed several
times in the scsi plugin.

It also looks like the plugin will run iprconfig on every SCSI enclosure device;
that may not be desirable and could be confusing for users with other types of
enclosure attached.

I think we can use data from /proc and /sys to identify the devices and restrict
collection to just the intended IBM Power RAID devices.

> +                temp = re.split('\s+', line)
> +                self.add_cmd_output("iprconfig -c query-ses-mode %s" % (temp[7],))

Generally it would be better to document the format of what's being parsed here
via comments; right now there's quite a bit of 'magic' here. E.g. temp[0], temp[7].

An example of the output being processed or an explanation of its structure is helpful
as it gives people something to work with if something changes in the future and
this code breaks.

Regards,
Bryn.