Additional params in methods - create_vd

ulmitov commented 1 year ago

Hi, Regarding controller.create_vd,

For raid1 and raid10 must specify PDperArray=2, otherwise the add vd command will fail with: "operation not possible for current RAID level, Cannot create configuration with 1 span".

Can you please add a parameter to this method so it can get some additional params to append to the add vd command ? Probably the additional params will be needed in other methods also.

ulmitov commented 1 year ago

@ralequi @dojci Hi, please tell me if issues in this project can be fixed

Thanks

ralequi commented 1 year ago

Hi @ulmitov . Absolutely forget about this issues.

Let me check if I can reproduce them and generate the corresponding tests & fixes

ralequi commented 1 year ago

Hi @ulmitov

Please, check develop branch and confirm this issue is fixed

ulmitov commented 1 year ago

posted a comment in the commit, add vd has many additional params

ralequi commented 1 year ago

First of all, You can leave PDperArray to None

Notice that I've added this to the code:

        if raid == '1' or raid == '10':
            if PDperArray is None:
                PDperArray = 2

In other scenarios PDperArray is not required AFAIK.

I'll consider adding the other params you have suggested

ulmitov commented 1 year ago

for raid1 i've rechecked, don't need pdperarray. for raid60 it should be 4 for raid00 should be 1

ralequi commented 1 year ago

For raid0 it is not required:

{
"Controllers":[
{
    "Command Status" : {
            "CLI Version" : "007.1704.0000.0000 Jan 16, 2021",
            "Operating system" : "Linux 5.15.0-58-generic",
            "Controller" : 0,
            "Status" : "Success",
            "Description" : "Add VD Succeeded."
    }
}
]
}

agree raid1 does not requires it

for raid6 it is not required:

{
"Controllers":[
{
    "Command Status" : {
            "CLI Version" : "007.1704.0000.0000 Jan 16, 2021",
            "Operating system" : "Linux 5.15.0-58-generic",
            "Controller" : 0,
            "Status" : "Success",
            "Description" : "Add VD Succeeded."
    }
}
]
}

As far as I'm checking, it is not required for any raid that does not have an underlying raid

I think this code should work fine:

        if int(raid) >= 10 and PDperArray is None:
            # Try to count the number of drives in the array
            # The format of the drives argument is e:s|e:s-x|e:s-x,y;e:s-x,y,z
            # The number of drives is the number of commas plus the dashes intervals

            numDrives = 0
            drives2 = drives.split(':')
            drives2 = drives2[1]
            numDrives += drives2.count(',')+1
            for interval in drives2.split(','):
                if '-' in interval:
                    left = int(interval.split('-')[0])
                    right = int(interval.split('-')[1])
                    numDrives += right - left

            PDperArray = numDrives//2

        if raid == '00' and PDperArray is None:
            PDperArray = 1

I don't know what raid "00" stands for, but at least it is not documented on LSI's storcli doc: https://docs.broadcom.com/doc/12352476

ulmitov commented 1 year ago

for now i see it's required only for 00, 10, 60. '00' is based on '0', some controllers have it: \"Capabilities\" : { \"Supported Drives\" : \"SAS, SATA\", \"RAID Level Supported\" : \"RAID0, RAID1(2 or more drives), RAID5, RAID6, RAID00, RAID10(2 or more drives per span), RAID50, RAID60\",

ralequi commented 1 year ago

Do you think that fix would be enough or do you propose something else?

ulmitov commented 1 year ago

it looks alright, but does pdperarray for sure should be half amount of drives? for now i'm running with this and it works (total amount of dirves per raid level is different each time): if 'raid10' in args: args = args.replace('strip=', 'pdperarray=2 strip=') if 'raid00' in args: args = args.replace('strip=', 'pdperarray=1 strip=') if 'raid60' in args: args = args.replace('strip=', 'pdperarray=4 strip=')

ralequi commented 1 year ago

Unless I've been missundertanding the meaning of PdPerArray, means how many disks should be on the "mirror" side of the array.

Check this document for reference: https://www.broadcom.com/support/knowledgebase/1211161503234/how-to-create-a-raid-10-50-or-60

In the examples:

C:\>storcli64 /C0 add vd type=raid10 drives=83:5,6,7,8 pdperarray=2
C:\>storcli64 /C0 add vd type=raid50 drives=83:5,6,7,8,9,10 pdperarray=3
C:\>storcli64 /C0 add vd type=raid60 drives=83:5,6,7,8,9,10,11,12 pdperarray=4

pdperarray is the half of the number of disks, but, in the r50/r60 is also the minimal number of disks for a r5/r6

Just in case I've tested it out (notice each drive has 4TB/3,637TB):

after run storcli /c0 add vd r50 name=raid50a 'drives=35:12-13,15-22' strip=512 pdperarray=5 This appeared: sdg 8:96 0 29.1T 0 disk -> There were 8/10 drives available as space ~ 2 drives as mirror/sum
after run storcli /c0 add vd r50 name=raid50a 'drives=35:12-13,15-20' strip=512 pdperarray=4 This appeared: sdg 8:96 0 21.8T 0 disk -> There were 6/8 available disks ~ 2 drives as mirror/sum
after run storcli /c0 add vd r50 name=raid50a 'drives=35:12-13,15-21' strip=512 pdperarray=3 This appeared: sdg 8:96 0 21.8T 0 disk -> There were 6/9 available disks ~ 3 drives as mirror/sum

To be honest. I don't know exactly what's going on but halving the number of disks sounds OK to me. At least as a "default valid value". Even we wish to discuss if the half is the best number (don't know, probably not), at least is the simplest that should always work as a valid value for any combination of #disks and raid type. You can always overwrite it as a param

ulmitov commented 1 year ago

I think you're right, at this point I don't have additional info here, the fix is good for me. Just one comment, i think on some rare cases this line can be problematic:

if int(raid) >= 10

since some vendors that support storcli for their products have some exotic raid levels with chars (raid5t2): https://www.ibm.com/docs/en/power9/0009-ESS?topic=arrays-supported-raid-levels

also oracle have raid50e, but i don't know if they support storcli

ulmitov commented 1 year ago

but those are corner cases, not critical at this point

ralequi commented 1 year ago

I've added a simple check. Thanks for notice that!

ulmitov commented 1 year ago

Hit a minor issue in this code:

In [1]: drives='177:18,177:19,177:20,177:21,177:22,177:23'

In [2]: numDrives = 0
   ...: drives2 = drives.split(':')
   ...: drives2 = drives2[1]
   ...: numDrives += drives2.count(',')+1
   ...: for interval in drives2.split(','):
   ...:     if '-' in interval:
   ...:         left = int(interval.split('-')[0])
   ...:         right = int(interval.split('-')[1])
   ...:         numDrives += right - left
   ...: 

In [3]: numDrives
Out[3]: 2

ralequi commented 1 year ago

Added a couple of funcs on common that should fix this:

def expand_drive_ids(drives: str) -> str:
    """Expand drive ids to range if needed

    Args:
        drives (str): storcli drives expression (e:s|e:s-x|e:s-x,y;e:s-x,y,z)

    Returns:
        (str): expanded drives expression (without dashes)
    """
    drive_list = drives.split(',')
    output = ""

    for i, drive in enumerate(drive_list):
        drive = drive.strip()
        encl, slot = drive.split(':')
        new_output = drive

        encl = encl.strip()
        slot = slot.strip()

        if '-' in slot:
            begin, end = slot.split('-')

            begin = begin.strip()
            end = end.strip()

            new_output = ','.join(['{0}:{1}'.format(encl, i)
                                   for i in range(int(begin), int(end)+1)])

        if i > 0:
            output += ',' + new_output
        else:
            output += new_output

    return output

def count_drives(drives: str) -> int:
    """Count number of drives in drives expression

    Args:
        drives (str): storcli drives expression (e:s|e:s-x|e:s-x,y;e:s-x,y,z)

    Returns:
        (int): number of drives
    """

    expanded_drives = expand_drive_ids(drives)
    return len(expanded_drives.split(','))

Also some tests to ensure it should work:


    def test_expand_drive_ids(self):
        assert common.expand_drive_ids(
            '0:0') == '0:0'
        assert common.expand_drive_ids(
            '0:0-1') == '0:0,0:1'
        assert common.expand_drive_ids(
            '0:0-2') == '0:0,0:1,0:2'
        assert common.expand_drive_ids(
            '0:0-1,0:3-4') == '0:0,0:1,0:3,0:4'
        assert common.expand_drive_ids(
            '0:0-1,0:3-4,1:0-1') == '0:0,0:1,0:3,0:4,1:0,1:1'
        assert common.expand_drive_ids(
            '0:0-1,0:3-4,1:0-1,5:3-4') == '0:0,0:1,0:3,0:4,1:0,1:1,5:3,5:4'
        assert common.expand_drive_ids(
            '0:0-1 ,0:3-4, 1:0-1 , 5 : 3 - 4  ') == '0:0,0:1,0:3,0:4,1:0,1:1,5:3,5:4'
        assert common.expand_drive_ids(
            '177:18,177:19,177:20,177:21,177:22,177:23') == '177:18,177:19,177:20,177:21,177:22,177:23'

    def test_count_drives(self):
        assert common.count_drives(
            '0:0') == 1
        assert common.count_drives(
            '0:0-1') == 2
        assert common.count_drives(
            '0:0-2') == 3
        assert common.count_drives(
            '0:0-1,0:3-4') == 4
        assert common.count_drives(
            '0:0-1,0:3-4,1:0-1') == 6
        assert common.count_drives(
            '0:0-1,0:3-4,1:0-1,5:3-4') == 8
        assert common.count_drives(
            '0:0-1 ,0:3-4, 1:0-1 , 5 : 3 - 4  ') == 8
        assert common.count_drives(
            '177:18,177:19,177:20,177:21,177:22,177:23') == 6

ralequi commented 1 year ago

For now it's on develop, tell me if you found any other counting issue & thanks!

ulmitov commented 1 year ago

Ok, i will check it in couple of weeks. Got another issue with raid60: when num of drives is 9, pdperarray 4 fails, with 3 it works:

storcli64 /c0 add vd r60 name=raid60 drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9,252:11 strip=64 PDperArray=4
CLI Version = 007.2309.0000.0000 Sep 16, 2022
Operating system = Linux 5.4.0-99-generic
Controller = 0
Status = Failure
Description = PD per array does not match with the specified arrays

storcli64 /c0 add vd r60 name=raid60 drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9,252:11 strip=64 PDperArray=3
CLI Version = 007.2309.0000.0000 Sep 16, 2022
Operating system = Linux 5.4.0-99-generic
Controller = 0
Status = Success
Description = Add VD Succeeded.

can add a check if it is dividable by 3 then that's the pd

ralequi commented 1 year ago

Change pushed to develop... Let's see if there are any new corner (& unexpected) case

ralequi commented 1 year ago

I've pushed 0.6.1.dev6 into pypi: https://pypi.org/project/PyStorCLI2/0.6.1.dev6/

It's a develop release, so you can only download it if you really ask for dev versions.

Tell how does it works, and if you don't need anything else I'll push a true-release

ulmitov commented 1 year ago

@ralequi fixes checked, works great

ulmitov commented 1 year ago

A new note on this, for raid level 00, when num of drives is less than 8 then pdperarray=1 works ok, but when it is more than 8 it fails. setting pdperrray=2 works for num drives % 2 = 0 and setting pdperrray=3 works for num drives % 3 = 0

Up to 8 drives:

# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9 strip=64 PDperArray=1
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Success
Description = Add VD Succeeded.

9 drives:

# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9,252:11 strip=64 PDperArray=1
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Failure
Description = Invalid PDs specified

root@ubuntu22-cuda:~# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9,252:11 strip=64 PDperArray=2
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Failure
Description = PD per array does not match with the specified arrays

root@ubuntu22-cuda:~# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9,252:11 strip=64 PDperArray=3
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Success
Description = Add VD Succeeded.

10 drives:

# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9,252:11,252:12 strip=64 PDperArray=2
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Success
Description = Add VD Succeeded.

11 drives: For 11 drives any pdperarray number is not working, looks like can't create raid 00 with 11 drives.

# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:4,252:5,252:6,252:7,252:8,252:9,252:11,252:12,252:13 strip=64 PDperArray=2
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Failure
Description = PD per array does not match with the specified arrays

12 dirves:

# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:2,252:4,252:5,252:6,252:7,252:8,252:9,252:11,252:12,252:13 strip=64 PDperArray=1
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Failure
Description = Invalid PDs specified

# storcli64 /c0 add vd r00 name=R00_HDD_4K_SAS drives=252:15,252:0,252:2,252:4,252:5,252:6,252:7,252:8,252:9,252:11,252:12,252:13 strip=64 PDperArray=2
CLI Version = 007.2408.0000.0000 Nov 15, 2022
Operating system = Linux 5.15.0-25-generic
Controller = 0
Status = Success
Description = Add VD Succeeded.

ralequi commented 1 year ago

Let's have a look on this...

ralequi commented 1 year ago

Issue confirmed. My storcli version recommends for r0, to use pdArray = #disks Have to refresh the differences between r0 and r00....

ulmitov commented 1 year ago

i tried to set pdArray = #disks but it also failed (in the examples above)

ralequi commented 1 year ago

Sure, I've tested too and also fails. I'm doing some performance tests.

It seems that r0 has no major logical differences with r00 just the "line" in the r0 is shared across all disks but in r00 it is share across "pdArray" disks. If I'm not wrong, that mean, that the value on pdArray for r00 disks would be any number (different from 1 and the number of disks) that could divide the number of disks. I would share here my insights soon

Naudit / pystorcli2

Additional params in methods - create_vd #2