DMTF / Redfish-Event-Listener

The Redfish Event Listener is a lightweight HTTPS server that can be deployed to read and record events from Redfish services.
Other
31 stars 15 forks source link

Remove deprecated EventTypes and Expand properties. #22

Closed rgmain closed 2 years ago

rgmain commented 2 years ago

I think these changes would make it easier for anyone just adopting this tool. Both of these properties cause errors when trying to subscribe to Redfish devices, one of which has firmware that is two years old but still doesn't like these properties.

Signed-off-by: Roy Main roy.main@hpe.com

mraineri commented 2 years ago

What's the reason to remove these? There's no requirement to use these properties.

EventTypes is still supported by many implementations, even if it's deprecated. Supporting this allows users working with older implementations to continue to subscribe, otherwise they will lose eventing altogether. Remember, "deprecated" does not mean "removed" or "no longer supported".

Expand is part of the standard. It is used to supply the "IncludeOriginOfCondition" property in the subscription, which was added in 1.8.0 of EventDestination.

rgmain commented 2 years ago

What's the reason to remove these? There's no requirement to use these properties.

EventTypes is still supported by many implementations, even if it's deprecated. Supporting this allows users working with older implementations to continue to subscribe, otherwise they will lose eventing altogether. Remember, "deprecated" does not mean "removed" or "no longer supported".

Expand is part of the standard. It is used to supply the "IncludeOriginOfCondition" property in the subscription, which was added in 1.8.0 of EventDestination.

@mraineri Hey Mike! Good points. My perspective is from the point of view of anyone adopting redfish with newer implementations. I tested two machines - one with AMI BMC that has the 2019 redfish implementation and an HPE iLO BMC with the latest redfish support. Neither of these BMC's like these properties.

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

To adopt use of the script with these properties set by default, the learning curve for new redfish adopters can be rather steep. The script could check the redfish version and advise accordingly, but it feels like that would make the code overly complicated.

Is there some language in the standard that speaks to what should happen when a redfish service receives deprecated schema?

Would you mind pointing me to the definitions/usage of "Expand" in the spec (if you know that readily)? I haven't been able to find it yet.

If you prefer a solution with the least side affects, I can recommend some updates to the README and perhaps some comments in the config.ini.

rgmain commented 2 years ago

@mraineri I do find the mention of expand in the definition for IncludeOriginOfCondition in the EventDestination 1.12 schema, but I don't see anywhere the schema allows for the "Expand" property.

"longDescription": "This property shall indicate whether the event payload sent to the subscription destination will expand the OriginOfCondition property to include the resource or object referenced by the OriginOfCondition property.",

AMI redfish returns:

{ "error": { "@Message.ExtendedInfo": [ { "@odata.type": "#Message.v1_0_8.Message", "Message": "The property Expand is not in the list of valid properties for the resource.", "MessageArgs": [ "Expand" ], "MessageId": "Base.1.5.PropertyUnknown", "RelatedProperties": [ "#/Expand" ], "Resolution": "Remove the unknown property from the request body and resubmit the request if the operation failed.", "Severity": "Warning" } ], "code": "Base.1.5.PropertyUnknown", "message": "The property Expand is not in the list of valid properties for the resource." } }

mraineri commented 2 years ago

@mraineri Hey Mike! Good points. My perspective is from the point of view of anyone adopting redfish with newer implementations. I tested two machines - one with AMI BMC that has the 2019 redfish implementation and an HPE iLO BMC with the latest redfish support. Neither of these BMC's like these properties.

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

The readme calls out everything in SubscriptionDetails is optional with the exception of "Destination".

"The Destination option is required; other options can be omitted or have an empty value."

It's not necessary to specify everything. If a property or configuration is not supported, then it should be removed.

To adopt use of the script with these properties set by default, the learning curve for new redfish adopters can be rather steep. The script could check the redfish version and advise accordingly, but it feels like that would make the code overly complicated.

They were in there as a full example to show each of the configurations possible. If this is confusing and going to hurt usability, then I would recommend removing them from the default configuration in this project. I think having a full example is still good though, so maybe we can just put that directly in the readme. Maybe the default config.ini could simply have the "Destination" option in there with everything else removed.

Is there some language in the standard that speaks to what should happen when a redfish service receives deprecated schema?

There's no language specifically about that aspect; we have general deprecation terminology defined in the Redfish Specification:

"The term "deprecated" in this document is to be interpreted as material that is not recommended for use in new development efforts. Existing and new implementations may use this material, but they should move to the favored approach. Deprecated material may be implemented in order to achieve backwards compatibility. Deprecated material should contain references to the last published version that included the deprecated material as normative material and to a description of the favored approach. Deprecated material may be removed from the next major version of the specification."

In short, if you need to achieve backwards compatibility with existing clients, it's probably good to support the deprecated material. So, while implementations can remove deprecated properties prior to the next major version, they may need to work with their users to ensure they've fully transitioned to the newer approaches.

Would you mind pointing me to the definitions/usage of "Expand" in the spec (if you know that readily)? I haven't been able to find it yet.

Check out "IncludeOriginOfCondition" in this file: https://redfish.dmtf.org/schemas/v1/EventDestination_v1.xml

If you prefer a solution with the least side affects, I would recommend some updates to the README and perhaps some comments in the config.ini.

I think that would be for the best; I don't want users to lose support for things that they might be using today.

rgmain commented 2 years ago

@mraineri Let me have a think on some alternate config.ini and REAMDE language and I will submit a new PR. I have some other changes cooking as well. Thanks for your input!