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

[Aruna Balakrishnaiah]

On Thursday 11 September 2014 05:10 PM, Bryn M. Reeves wrote:
> 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.

Bryn,

lsscsi also lists the vendor details for every enclosure, probably it would
be straight forward to compare that instead of going through /sys or /proc
data.

Will address all the comments in my v2.

Regards,
Aruna


>
> 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.
>