Skip to content

Conversation

@mwieczorek
Copy link
Contributor

This is a proposal of how to handle '$expand' redfish odata queries. Related issue: #340

Note: creating PR to get early feedback if general approach is valid, things to be added to this PR:

  • add tests for queries with expand option
  • godoc comments
  • handle all Memory 'subresources', not only Metrics

Assumptions, I've made:

  • I don't want to break existing clients of the pkg
  • I don't want to change default behavior (in case someone update the pkg)

Because of above, I'd like to introduce the 'expand feature' with 'functional options' approach.
There's a new common/odata.go file with 2 'options' functions responsible for controlling the expand value and level (ref: redfish spec).

Draft of this PR supports 'expand' only for system.Memory() and 'expands' only Metrics. It's possible to us it as it was (system.Memory()) or with expand option (system.Memory(common.WithExpand(common.ExpandOptionPeriod))). By default, no 'expand' is used, so we won't change the default behavior.
Memory struct was changed, and metrics var type is changed from common.Link to MemoryMetrics. In case the expand is not used for system.Memory(), memory.Metrics() call will make a call to redfish API.

Tested with Redfish mockup server and real device.

Please let me know if above approach is ok or if you have any suggestions how to do it differently.


Something I'm not sure how to tackle right now...
Following my use case (get system's memory and it's metrics):

system
  -> memory collection
    -> memory item
      -> metrics

system.Memory() encapsulate '2 levels' above: it send request to API to get 'memory collection' and then 'memory item'
Both of the requests can handle expand parameter. Let's assume Redfish API supports only 1 level for expand.
If we do system.Memory(common.WithExpand(common.ExpandOptionPeriod)), should gofish pass $expand param:

  • only to 'memory collection'
  • only to 'memory item'
  • both?
    Or should it be possible to decide when invoking system.Memory()

I assumed, when we use system.Memory() and get in response []Memory we're interested in Memory resource and that's what expanded in this 'draft' PR. (tbh: that's what I'm interested in my use case - getting metrics for all 'memory items')
With this approach we don't change anything in GetCollectionObjects (except 'optional' list of QueryOption in the definition)

If we'd like to support that at 'memory collection' request, it will get more dificult and complex and would need more changes to those 'generic' functions (GetCollectionObjects -> CollectList -> GetCollection etc).

Let me know what you think and how you suggest to approach that.

@stmcginnis
Copy link
Owner

Awesome, thanks for working on this. I'm probably tied up for a few days, but will try to dig into this as soon as I can.

@mwieczorek mwieczorek changed the title Add initial support for in system.Memory() Add initial support for $expand in system.Memory() Mar 11, 2025
@mwieczorek
Copy link
Contributor Author

mwieczorek commented Mar 11, 2025

I was thinking about $expand support in collections, and came to something like #412

I created a separate PR in case it would be easier to compare both version for you.

In #412 it's possible to explicitly decide if we want to expand collection or objects in collection. It makes sense in case of only one level of expand is supported.

My use case:
I want to get 'memory metrics' and 'memory environment metrics' and reduce number of Redfish API calls.
Let's say we have 1 system, 1 memory 'item.
If we 'expand' collection (1 API request), we would still need to get 'metrics' and 'environmentMetrics' (so additional 2 API requests)
I we decide to expand only 'objects' (memory 'item'), then we will have 1 API request for collection and 1 API request for memory item.

Anyway, please take a look when you find time.

PS:
#412 makes changes in 'public' collections types, unfortunately I didn't find a way how to do it without such changes. The only thing is I guess those structs/funcs are used only 'internally' in the gofish so hopefully it wouldn't break someone's code.

BTW:
In case you agree to the approach, I'll add more tests proving how the expand works with different options. For now I tested system.Memory() calls with different options and against 'official' redfish mock server

@stmcginnis
Copy link
Owner

Hey! Can you address the lint job errors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants