Adding CreateVolume functionality

[TSKushal]

SUMMARY
Adding functionality to create volumes on Gen11 servers.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME
redfish_config.py
redfish_utils.py

[ansibullbot]

cc @Bhavya06 @jyundt @mraineri @rajeevkallur @renxulei @tomasg2012 @xmadsen
click here for bot help

[TSKushal]

@mraineri any further comments on this?

[mraineri]

I think we should add CapacityBytes to the required list; I can't think of a practical reason as to why we'd let the Redfish service decide the capacity on behalf of the user.

[TSKushal]

Since CapacityBytes parameter is not needed from the API side, should we be restricting it from a code perspective. Also, since the drive link is a mandatory parameter from the API, users have to check the capacity before providing the drives as input.

[mraineri]

Technically none of these parameters are needed from the API side. Redfish has no required properties in the standard when a user is trying to create a volume. If you look at the Volume schema (https://redfish.dmtf.org/schemas/swordfish/v1/Volume_v1.xml), there are no properties marked as "Redfish.RequiredOnCreate".

A request like this is perfectly acceptable per the specification:

POST /redfish/v1/Systems/1/Storage/1/Volumes
Content-Type: application/json

{}

With an empty request like this, the client is deferring everything to the service (including the drive selection, RAID type, and size of the volume).

However, is it practical from an Ansible perspective to let a user attempt to create a new Volume without specifying the size? I'm not sure I'm willing to make that leap yet (maybe @felixfontein has an opinion about this).

[felixfontein]

I'm curious what use-cases are there for creating a volume without a specified size. If not specifying the size would mean "use the maximum available" that would make sense to me, but if not specifying it means "pick something for me, I don't really care" (which as far as I understand is the case here?) and there's no other option which implicitly defines the size (like a boolean flag "use the maximum available"), then it feels strange.

One thing to remember is that it's easier to remove restrictions (like initially require the size, and later drop that restriction when someone provides a good argument for it) than adding them (which is usually a breaking change).

[TSKushal]

Sorry for the late reply.
When we check the API /redfish/v1/Systems/1/Storage/DE00C000/Volumes/Capabilities
We see the following response.

{
    "@odata.etag": "\"6ed17d03\"",
    "@odata.id": "/redfish/v1/Systems/1/Storage/DE00C000/Volumes/Capabilities",
    "@odata.type": "#Volume.v1_6_2.Volume",
    "Id": "Capabilities",
    "Name": "Capabilities for VolumeCollection",
    "CapacityBytes@Redfish.OptionalOnCreate": true, --> Optional
    "DisplayName@Redfish.OptionalOnCreate": true,
    "DisplayName@Redfish.UpdatableAfterCreate": true,
    "InitializeMethod@Redfish.OptionalOnCreate": true,
    "InitializeMethod@Redfish.AllowableValues": [
        "Foreground",
        "Background"
    ],
    "Links@Redfish.RequiredOnCreate": true, --> Required
    "Links": {
        "Drives@Redfish.RequiredOnCreate": true,
        "DedicatedSpareDrives@Redfish.OptionalOnCreate": true,
        "DedicatedSpareDrives@Redfish.UpdatableAfterCreate": true
    },
    "MediaSpanCount@Redfish.OptionalOnCreate": true,
    "Name@Redfish.OptionalOnCreate": true, --> Required
    "RAIDType@Redfish.RequiredOnCreate": true,  --> Required
    "RAIDType@Redfish.AllowableValues": [
        "None",
        "RAID0",
        "RAID1",
        "RAID10",
        "RAID5",
        "RAID50",
        "RAID6",
        "RAID60"
    ],
    "ReadCachePolicy@Redfish.OptionalOnCreate": true,
    "ReadCachePolicy@Redfish.UpdatableAfterCreate": true,
    "ReadCachePolicy@Redfish.AllowableValues": [
        "Off",
        "ReadAhead"
    ],
    "StripSizeBytes@Redfish.OptionalOnCreate": true,
    "VolumeUsage@Redfish.OptionalOnCreate": true,
    "VolumeUsage@Redfish.AllowableValues": [
        "Data"
    ],
    "WriteCachePolicy@Redfish.OptionalOnCreate": true,
    "WriteCachePolicy@Redfish.UpdatableAfterCreate": true,
    "WriteCachePolicy@Redfish.AllowableValues": [
        "ProtectedWriteBack",
        "UnprotectedWriteBack",
        "WriteThrough"
    ]
}

Our reasoning was based on this response.

When we do a POST request with just Name, RAIDType and Links, the request goes through, so at least from the API end CapacityBytes is not required always.

[mraineri]

I understand that your API allows for this; the standard definition technically allows for a completely empty request.

However, from an Ansible perspective, it's not clear what the use case for not specifying the volume size would be. This is not about what the API allows, but rather is it realistic to let an Ansible user create a volume without specifying its size. I can't think of a scenario where a user would not want to specify the size of the volume.

[TSKushal]

Added CapacityBytes as a mandatory parameter.

[mraineri]

Why do we need this check up front? Isn't the failure to POST to the VolumeCollection sufficient? Doing version checks for capabilities is not a good practice; the interface itself is self-descriptive

[TSKushal]

I do see your point, but we may benefit from explicitly pointing out that it doesn't work for certain server. Changed the implementation a little, please a have look.

[mraineri]

I still don't think this is really in our best interest to add... We don't perform additional version checks in failure paths anywhere else, and this seems like it'll be nearly impossible to maintain across the board.

[TSKushal]

Actually, this version specific code only comes into picture for iLO6 RAID/Volume related operations. All other functionalities that we maintain do not have any version dependencies.

[mraineri]

That's not the issue; the issue is establishing a pattern of:

if FAIL:
    if vendor == "X":
        # Add version checks and modify error message for vendor X
    elif vendor == "Y":
        # Add version checks and modify error message for vendor Y
    elif vendor == "Z":
        # Add version checks and modify error message for vendor Z
    elif ...

I don't think this pattern scales well or is very maintainable. The service should already be returning the proper information to convey that it doesn't support the given request, such as a "method not allowed" message, which is a clear indication the service does not support POST to the VolumeCollection.

[TSKushal]

Hmm, will defer to your expertise on the matter. Updated the code to remove this.

[mraineri]

Looks good to me now; thanks!

[patchback]

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/55893f27c99282028069728e2b1e2f44478c0455/pr-6813

Backported as #7359

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@TSKushal thanks for your contribution!
@mraineri thanks for reviewing!