Remove 'CapacityBytes' from list of required parameters #8956

pyfontan · 2024-10-01T16:15:16Z

SUMMARY

Make CapacityBytes an allowed parameter and not a required one.

Fixes #8955

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

redfish_config

ADDITIONAL INFORMATION

ansibullbot · 2024-10-01T16:20:14Z

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

mraineri · 2024-10-01T16:51:36Z

Why would a user not want to specify the size of the volume? When this was originally discussed, we did not think deferring to the service to dictate the capacity of the new volume is reliable and expect users to know how big of a volume to make in all cases.

mraineri · 2024-10-01T16:54:53Z

Technically "RAIDType" and "Drives" are not required per the schema either, but we had similar concerns with deferring to the service to determine the appropriate RAID type and which physical drives to use for the volume.

pyfontan · 2024-10-01T17:06:51Z

Why would a user not want to specify the size of the volume? When this was originally discussed, we did not think deferring to the service to dictate the capacity of the new volume is reliable and expect users to know how big of a volume to make in all cases.
Technically "RAIDType" and "Drives" are not required per the schema either, but we had similar concerns with deferring to the service to determine the appropriate RAID type and which physical drives to use for the volume.

@mraineri I think i do not totally understand your two comments.
User needs to specify on which drive he wants the raid to be created and which type of RAID. But the capacity can by default be the maximum size possible.
I don't think it's possible to create a volume with redfish with a payload not specifying RAIDType or Drive in it ?? Is it ?

pyfontan · 2024-10-01T17:18:40Z

Creation test with curl and a payload without RAIDType

{
  "error": {
    "code": "Base.1.12.CreateFailedMissingReqProperties",
    "message": "See @Message.ExtendedInfo for more information.",
    "@Message.ExtendedInfo": [
      {
        "MessageId": "Base.1.12.CreateFailedMissingReqProperties",
        "Message": "The create operation failed because the required property %1 was missing from the request.",
        "Severity": "Warning",
        "Resolution": "Provide the missing required parameter.",
        "MessageArgs": [
          "RAIDType"
        ]
      }
    ]
  }
}

When asking for capabilities (extract):

{
    "[email protected]": true,
    "[email protected]": true,
    "Links": {
        "[email protected]": true,
    }
}

RAIDType and Link/Drives are, at least for my adapter, the only required parameters

mraineri · 2024-10-01T17:19:38Z

The issue is there's nothing in the Volume schema that says the service defaults to using the maximum available space. If we need that semantic, we need to provide that feedback to SNIA.

For Drives and RAIDType, today the Ansible module requires the user to provide this, but the schema definitions don't make these mandatory. The same issue applies in that the current schema definition does not dictate what the default behavior can be. The capabilities you've highlighted are provided by the vendor's service. If you look at the schema definitions (https://redfish.dmtf.org/schemas/swordfish/v1/Volume_v1.xml), there's nothing about which properties are required.

Technically, as far as the standard is concerned, this is a valid payload when creating a new Volume:

POST /redfish/v1/Systems/1/Storage/1/Volumes

{}

The resultant volume can have varying characteristics from different vendors. Vendor A can decide that it'll create a RAID0 volume that consumes all drives. Vendor B can decide to not use RAID at all and just allocate space from the first drive.

mraineri · 2024-10-01T17:20:29Z

RAIDType and Link/Drives are, at least for my adapter, the only required parameters

Yes, that's a requirement imposed by the Redfish service you're using; it's not imposed by the standard.

pyfontan · 2024-10-01T17:26:38Z

Yes, that's a requirement imposed by the Redfish service you're using; it's not imposed by the standard.

Ok, so CapacityBytes is a requirement imposed by the module that i don't understand.
And that requirement prevents the creation of volumes on HPE servers here and i think this is also the case for DELL servers.

mraineri · 2024-10-01T17:32:22Z

Ok, so CapacityBytes is a requirement imposed by the module that i don't understand.

It's because the current definition in the Volume schema does not give any interoperable guidance on what the capacity should be if the user does not specify it. We decided to err on the side of caution for Ansible to ensure the user is acknowledging the size of the requested volume.

I can see it being reasonable that a service should create a volume that consumes all available space on the requested drives for the requested RAID type, but I would like others to weigh in on this assumption (perhaps even provide this as feedback to SNIA for adding to the definition of CapacityBytes for clarity).

pyfontan · 2024-10-01T18:20:33Z

Yes, that's a requirement imposed by the Redfish service you're using; it's not imposed by the standard.

Ok, so CapacityBytes is a requirement imposed by the module that i don't understand. And that requirement prevents the creation of volumes on HPE servers here and i think this is also the case for DELL servers.

Confirmed on DELL:
https://www.dell.com/support/manuals/fr-bi/idrac9-lifecycle-controller-v3.2-series/idrac_3.21.21.21_redfishapiguide/volume?guid=guid-30f08e85-68ad-43fa-9e7d-801f2a701780&lang=en-us

pyfontan · 2024-10-01T18:21:24Z

Ok, so CapacityBytes is a requirement imposed by the module that i don't understand.

It's because the current definition in the Volume schema does not give any interoperable guidance on what the capacity should be if the user does not specify it. We decided to err on the side of caution for Ansible to ensure the user is acknowledging the size of the requested volume.

I can see it being reasonable that a service should create a volume that consumes all available space on the requested drives for the requested RAID type, but I would like others to weigh in on this assumption (perhaps even provide this as feedback to SNIA for adding to the definition of CapacityBytes for clarity).

Ok 👍

pyfontan · 2024-10-01T18:30:08Z

The issue is there's nothing in the Volume schema that says the service defaults to using the maximum available space. If we need that semantic, we need to provide that feedback to SNIA.

For Drives and RAIDType, today the Ansible module requires the user to provide this, but the schema definitions don't make these mandatory. The same issue applies in that the current schema definition does not dictate what the default behavior can be. The capabilities you've highlighted are provided by the vendor's service. If you look at the schema definitions (https://redfish.dmtf.org/schemas/swordfish/v1/Volume_v1.xml), there's nothing about which properties are required.

Technically, as far as the standard is concerned, this is a valid payload when creating a new Volume:
POST /redfish/v1/Systems/1/Storage/1/Volumes

{}
The resultant volume can have varying characteristics from different vendors. Vendor A can decide that it'll create a RAID0 volume that consumes all drives. Vendor B can decide to not use RAID at all and just allocate space from the first drive.

I think that current schema definition doesn't make these mandatory because, it is made to create logical volumes over drive arrays.
And what is mandatory for array creation is not for logical volume creation.

pyfontan · 2024-10-01T18:47:53Z

In the description of Redfish Volume 1.9.0
https://www.dmtf.org/sites/default/files/standards/documents/DSP2046_2023.1.pdf

We can read at chapter 6.140.5.11 that RAIDType is a requested property.

pyfontan · 2024-10-02T09:40:42Z

Moreover, perhaps it's just a bug in the HPE redfish implementation but 'CapacityBytes' seems to be a readonly property 😲 ...
And it's not really pertinent for this request , but if i try to create a volume with this payload :

{
    "DisplayName": "SYSTEM",
    "RAIDType": "RAID1",
    "CapacityBytes": "107374182400",
    "Links": {
        "Drives": [
            {
                "@odata.id": "/redfish/v1/Systems/1/Storage/DE07A000/Drives/36"
            },
            {
                "@odata.id": "/redfish/v1/Systems/1/Storage/DE07A000/Drives/37"
            }
        ]
    }
}

The reply is :

{
  "error": {
    "code": "iLO.0.10.ExtendedInfo",
    "message": "See @Message.ExtendedInfo for more information.",
    "@Message.ExtendedInfo": [
      {
        "MessageArgs": [
          "FIXME:NeedProperty"
        ],
        "MessageId": "iLO.2.23.PropertyNotWritableOrUnknown"
      }
    ]
  }
}

I'm a little bit surprised to be the first one to face this kind of problem.

mraineri · 2024-10-04T19:35:32Z

@pyfontan after talking about this with a few others, we did agree that it should be okay from an Ansible perspective to remove CapacityBytes as a required input parameter. We can see that it might be difficult in many cases for a user to determine the appropriate capacity size of the drive.

One thing we'd like to see is at least some description text for the parameter to specify that if the capacity is not specified, the Redfish service will be the one to determine the size to allocate, and depending on the design of the service, this might not always be the maximum available size. Would you be able to add this description text?

changelogs/fragments/8956-remove-capacitybytes-from-the-required-parameters_list.yml

…ed-parameters_list.yml Co-authored-by: Alexei Znamensky <[email protected]>

ansibullbot · 2024-10-07T13:25:04Z

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/modules/redfish_config.py:167:79: W291: trailing whitespace
plugins/modules/redfish_config.py:168:77: W291: trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/modules/redfish_config.py:167:79: W291: trailing whitespace
plugins/modules/redfish_config.py:168:77: W291: trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/modules/redfish_config.py:167:79: W291: trailing whitespace
plugins/modules/redfish_config.py:168:77: W291: trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/modules/redfish_config.py:167:79: W291: trailing whitespace
plugins/modules/redfish_config.py:168:77: W291: trailing whitespace

click here for bot help

ansibullbot · 2024-10-07T13:25:05Z

cc @TSKushal
click here for bot help

plugins/modules/redfish_config.py

Co-authored-by: Mike Raineri <[email protected]>

pyfontan · 2024-10-07T13:57:28Z

Thank you again @mraineri 🙏

mraineri · 2024-10-07T14:09:59Z

Looks good to me!

shipit

plugins/modules/redfish_config.py

patchback · 2024-10-07T21:00:05Z

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/b523d1b1c936cd6f7bacbd5ee6a78c5944cc6296/pr-8956

Backported as #9006

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

* Remove 'CapacityBytes' from list of required parameters * Add CHANGELOG fragment * Fix sanity test failure whitespace before ']' * Update changelogs/fragments/8956-remove-capacitybytes-from-the-required-parameters_list.yml Co-authored-by: Alexei Znamensky <[email protected]> * Add description for the volume_details key CapacityBytes * Update plugins/modules/redfish_config.py Co-authored-by: Mike Raineri <[email protected]> * Adjust description. --------- Co-authored-by: Pierre-yves FONTANIERE <[email protected]> Co-authored-by: Alexei Znamensky <[email protected]> Co-authored-by: Mike Raineri <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit b523d1b)

patchback · 2024-10-07T21:00:14Z

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/b523d1b1c936cd6f7bacbd5ee6a78c5944cc6296/pr-8956

Backported as #9007

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

* Remove 'CapacityBytes' from list of required parameters * Add CHANGELOG fragment * Fix sanity test failure whitespace before ']' * Update changelogs/fragments/8956-remove-capacitybytes-from-the-required-parameters_list.yml Co-authored-by: Alexei Znamensky <[email protected]> * Add description for the volume_details key CapacityBytes * Update plugins/modules/redfish_config.py Co-authored-by: Mike Raineri <[email protected]> * Adjust description. --------- Co-authored-by: Pierre-yves FONTANIERE <[email protected]> Co-authored-by: Alexei Znamensky <[email protected]> Co-authored-by: Mike Raineri <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit b523d1b)

felixfontein · 2024-10-07T21:00:29Z

@pyfontan thanks for your contribution!
@mraineri @russoz thanks for reviewing!

…st of required parameters (#9007) Remove 'CapacityBytes' from list of required parameters (#8956) * Remove 'CapacityBytes' from list of required parameters * Add CHANGELOG fragment * Fix sanity test failure whitespace before ']' * Update changelogs/fragments/8956-remove-capacitybytes-from-the-required-parameters_list.yml Co-authored-by: Alexei Znamensky <[email protected]> * Add description for the volume_details key CapacityBytes * Update plugins/modules/redfish_config.py Co-authored-by: Mike Raineri <[email protected]> * Adjust description. --------- Co-authored-by: Pierre-yves FONTANIERE <[email protected]> Co-authored-by: Alexei Znamensky <[email protected]> Co-authored-by: Mike Raineri <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit b523d1b) Co-authored-by: Pierre-yves Fontaniere <[email protected]>

…st of required parameters (#9006) Remove 'CapacityBytes' from list of required parameters (#8956) * Remove 'CapacityBytes' from list of required parameters * Add CHANGELOG fragment * Fix sanity test failure whitespace before ']' * Update changelogs/fragments/8956-remove-capacitybytes-from-the-required-parameters_list.yml Co-authored-by: Alexei Znamensky <[email protected]> * Add description for the volume_details key CapacityBytes * Update plugins/modules/redfish_config.py Co-authored-by: Mike Raineri <[email protected]> * Adjust description. --------- Co-authored-by: Pierre-yves FONTANIERE <[email protected]> Co-authored-by: Alexei Znamensky <[email protected]> Co-authored-by: Mike Raineri <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit b523d1b) Co-authored-by: Pierre-yves Fontaniere <[email protected]>

Remove 'CapacityBytes' from list of required parameters

4390783

This comment was marked as outdated.

Sign in to view

Pierre-yves FONTANIERE added 2 commits October 1, 2024 18:20

Add CHANGELOG fragment

a8f75b7

Fix sanity test failure whitespace before ']'

a57ef2c

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI small_patch Hopefully easy to review needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 1, 2024

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-8 Automatically create a backport for the stable-8 branch backport-9 Automatically create a backport for the stable-9 branch labels Oct 1, 2024

russoz reviewed Oct 7, 2024

View reviewed changes

changelogs/fragments/8956-remove-capacitybytes-from-the-required-parameters_list.yml Outdated Show resolved Hide resolved

pyfontan and others added 2 commits October 7, 2024 09:43

Update changelogs/fragments/8956-remove-capacitybytes-from-the-requir…

d0bbb91

…ed-parameters_list.yml Co-authored-by: Alexei Znamensky <[email protected]>

Add description for the volume_details key CapacityBytes

2138573

ansibullbot added ci_verified Push fixes to PR branch to re-run CI module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 7, 2024

mraineri reviewed Oct 7, 2024

View reviewed changes

plugins/modules/redfish_config.py Outdated Show resolved Hide resolved

Update plugins/modules/redfish_config.py

2c8f97b

Co-authored-by: Mike Raineri <[email protected]>

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Oct 7, 2024

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 7, 2024

felixfontein reviewed Oct 7, 2024

View reviewed changes

plugins/modules/redfish_config.py Outdated Show resolved Hide resolved

Adjust description.

9fb896a

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 7, 2024

felixfontein merged commit b523d1b into ansible-collections:main Oct 7, 2024
141 checks passed

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Oct 7, 2024

patchback bot mentioned this pull request Oct 7, 2024

[PR #8956/b523d1b1 backport][stable-8] Remove 'CapacityBytes' from list of required parameters #9006

Merged

patchback bot mentioned this pull request Oct 7, 2024

[PR #8956/b523d1b1 backport][stable-9] Remove 'CapacityBytes' from list of required parameters #9007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove 'CapacityBytes' from list of required parameters #8956

Remove 'CapacityBytes' from list of required parameters #8956

pyfontan commented Oct 1, 2024

This comment was marked as outdated.

ansibullbot commented Oct 1, 2024

mraineri commented Oct 1, 2024

mraineri commented Oct 1, 2024

pyfontan commented Oct 1, 2024

pyfontan commented Oct 1, 2024 •

edited

Loading

mraineri commented Oct 1, 2024

mraineri commented Oct 1, 2024

pyfontan commented Oct 1, 2024

mraineri commented Oct 1, 2024

pyfontan commented Oct 1, 2024

pyfontan commented Oct 1, 2024

pyfontan commented Oct 1, 2024 •

edited

Loading

pyfontan commented Oct 1, 2024

pyfontan commented Oct 2, 2024

mraineri commented Oct 4, 2024

ansibullbot commented Oct 7, 2024

ansibullbot commented Oct 7, 2024

pyfontan commented Oct 7, 2024

mraineri commented Oct 7, 2024

patchback bot commented Oct 7, 2024 •

edited

Loading

patchback bot commented Oct 7, 2024 •

edited

Loading

felixfontein commented Oct 7, 2024

Remove 'CapacityBytes' from list of required parameters #8956

Remove 'CapacityBytes' from list of required parameters #8956

Conversation

pyfontan commented Oct 1, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

This comment was marked as outdated.

ansibullbot commented Oct 1, 2024

mraineri commented Oct 1, 2024

mraineri commented Oct 1, 2024

pyfontan commented Oct 1, 2024

pyfontan commented Oct 1, 2024 • edited Loading

mraineri commented Oct 1, 2024

mraineri commented Oct 1, 2024

pyfontan commented Oct 1, 2024

mraineri commented Oct 1, 2024

pyfontan commented Oct 1, 2024

pyfontan commented Oct 1, 2024

pyfontan commented Oct 1, 2024 • edited Loading

pyfontan commented Oct 1, 2024

pyfontan commented Oct 2, 2024

mraineri commented Oct 4, 2024

ansibullbot commented Oct 7, 2024

ansibullbot commented Oct 7, 2024

pyfontan commented Oct 7, 2024

mraineri commented Oct 7, 2024

patchback bot commented Oct 7, 2024 • edited Loading

Backport to stable-8: 💚 backport PR created

patchback bot commented Oct 7, 2024 • edited Loading

Backport to stable-9: 💚 backport PR created

felixfontein commented Oct 7, 2024

pyfontan commented Oct 1, 2024 •

edited

Loading

pyfontan commented Oct 1, 2024 •

edited

Loading

patchback bot commented Oct 7, 2024 •

edited

Loading

patchback bot commented Oct 7, 2024 •

edited

Loading