Drive state property is missing states

ulmitov commented 1 year ago

Hi, Please add "JBOD'" state to this method:

    @property
    def state(self):
        """Get/Set drive state

        One of the following states can be set (str):
            online - changes the drive state to online
            offline - changes the drive state to offline
            missing - marks a drive as missing
            good - changes the drive state to unconfigured good
            jbod - sets the drive state to JBOD

        Returns:
            (str):
                dhs - dedicated hotspare to some virtual drive
                ghs - global hotspare
                bad - bad drive
                good - unconfigured good
                online - already in virtual drive with good state
                offline - already in virtual drive with bad state
        """
        args = [
            'show'
        ]

        state = self._response_properties(self._run(args))['State']
        if state == 'DHS':
            return 'dhs'
        elif state == 'UBad':
            return 'bad'
        elif state == 'Onln':
            return 'online'
        elif state == 'Offln':
            return 'offline'
        elif state == 'GHS':
            return 'ghs'
        return 'good'

ulmitov commented 1 year ago

Actually more states are missing. Although in the documentation they do not show all of them in the drive states table, they are mentioned in different places and anyway the cli shows them:

root@ubuntu20-cuda:~# storcli64 /c0/eall/sall show
CLI Version = 007.2309.0000.0000 Sep 16, 2022
Operating system = Linux 5.4.0-99-generic
Controller = 0
Status = Success
Description = Show Drive Information Succeeded.

Drive Information :
=================

----------------------------------------------------------------------------------
EID:Slt DID State  DG       Size Intf Med SED PI SeSz Model               Sp Type 
----------------------------------------------------------------------------------
252:0    43 Onln   -   10.914 TB SAS  HDD N   N  4 KB HUH721212AL4204     U  JBOD 
252:1    51 Onln   -  447.130 GB SATA SSD N   N  512B INTEL SSDSC2KB480G8 U  JBOD 
252:2    48 UBUnsp -        0 KB SAS  HDD N   N  512B HUH721212AL4204     U  -    
252:3    23 Onln   -  447.130 GB SATA SSD N   N  512B INTEL SSDSC2KB480G8 U  JBOD 
----------------------------------------------------------------------------------

EID=Enclosure Device ID|Slt=Slot No|DID=Device ID|DG=DriveGroup
DHS=Dedicated Hot Spare|UGood=Unconfigured Good|GHS=Global Hotspare
UBad=Unconfigured Bad|Sntze=Sanitize|Onln=Online|Offln=Offline|Intf=Interface
Med=Media Type|SED=Self Encryptive Drive|PI=Protection Info
SeSz=Sector Size|Sp=Spun|U=Up|D=Down|T=Transition|F=Foreign
UGUnsp=UGood Unsupported|UGShld=UGood shielded|HSPShld=Hotspare shielded
CFShld=Configured shielded|Cpybck=CopyBack|CBShld=Copyback Shielded
UBUnsp=UBad Unsupported|Rbld=Rebuild

So the full list is as follows:

plus the JBOD state which is present on HBA cards.

If you allow me to say my opinion, i think it would be more correct to remove the "if" block from the drive state property method and just return the state string as it appears in storcli, since each time there is a need to check up how the state is being translated in pystorcli, it might be confusing considering all these states, and who knows what they might add in future raid releases

ulmitov commented 1 year ago

Also please add to the state setter a 'force' option:

storcli /cx[/ex]/sx set good [force]

or maybe always append force to good, broadcom blocks setting good state if the drive is in jbod mode.

root@ubuntu20-cuda:~# storcli64 /c0/e252/s1 set good
CLI Version = 007.2309.0000.0000 Sep 16, 2022
Operating system = Linux 5.4.0-99-generic
Controller = 0
Status = Failure
Description = Set Drive Good Failed.

Detailed Status :
===============

------------------------------------------------------------------------
Drive       Status  ErrCd ErrMsg                                        
------------------------------------------------------------------------
/c0/e252/s1 Failure   255 Drive is JBOD! Use 'force' option to confirm. 
------------------------------------------------------------------------

root@ubuntu20-cuda:~# storcli64 /c0/e252/s1 set good force
CLI Version = 007.2309.0000.0000 Sep 16, 2022
Operating system = Linux 5.4.0-99-generic
Controller = 0
Status = Success
Description = Set Drive Good Succeeded.

ralequi commented 1 year ago

Hey @ulmitov !

I've added a new class and method on develop branch to better address this. Check it and tell me your thoughts ! :-)

ulmitov commented 1 year ago

Checked, all good. Will wait for master release with all fixes

ralequi commented 1 year ago

@ulmitov new release have been published

Thank you !

ulmitov commented 1 year ago

Hi @ralequi After those changes there is a bug in controller metrics, physical_drives_non_optimal now is a dict of objects

>>> import pystorcli
>>> c = pystorcli.Controllers()
>>> c.ids
[0]
>>> c=c.get_ctl(0)
>>> c.metrics.all
{'ctl_temperature': 'unknown', 'drive_groups': '3', 'memory_correctable_error': '0', 'memory_uncorrectable_error': '0', 'physical_drives': '16', 'physical_drives_non_optimal': {<DriveState.Onln: 'Online'>: '15', <DriveState.UGood: 'Unconfigured Good'>: '1'}, 'roc_temperature': '79', 'state': 'optimal', 'virtual_drives': '3', 'virtual_drives_non_optimal': {}}

ralequi commented 1 year ago

Hi @ralequi After those changes there is a bug in controller metrics, physical_drives_non_optimal now is a dict of objects

>>> import pystorcli
>>> c = pystorcli.Controllers()
>>> c.ids
[0]
>>> c=c.get_ctl(0)
>>> c.metrics.all
{'ctl_temperature': 'unknown', 'drive_groups': '3', 'memory_correctable_error': '0', 'memory_uncorrectable_error': '0', 'physical_drives': '16', 'physical_drives_non_optimal': {<DriveState.Onln: 'Online'>: '15', <DriveState.UGood: 'Unconfigured Good'>: '1'}, 'roc_temperature': '79', 'state': 'optimal', 'virtual_drives': '3', 'virtual_drives_non_optimal': {}}

Hi @ulmitov

Sure, there is an issue there, since the drive state is not correctly handled

In the other hand, I didn't noticed that everything there is considered "string" but I don't think that is a good behavior at all. FOR NOW I've kept that as string and fixed that

ulmitov commented 1 year ago

Thanks!

Naudit / pystorcli2

Drive state property is missing states #7