-
Notifications
You must be signed in to change notification settings - Fork 71
util: Add pretty name for resources (#918) #1201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi @deepakala-k Can you also add to Power Supply, Fan, Dimm and squash to 1 commit like you have done? |
|
Also don't you need it for Fabric Adapters #869 |
|
And assemblies #858 ? |
| if (services.size() != 1) | ||
| { | ||
| BMCWEB_LOG_ERROR("Invalid Service Size {}", services.size()); | ||
| if (asyncResp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In current 1110 code we are also adding all of the services to the debug log. That was added by commit 74a0b78
Can that one be included here and squashed to put all the pretty name changes together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. I will pull it
redfish-core/lib/chassis.hpp
Outdated
| asyncResp->res.jsonValue["Name"] = "Chassis Collection"; | ||
| name_util::getPrettyName(asyncResp, path, connectionNames, | ||
| "/Name"_json_pointer); | ||
| asyncResp->res.jsonValue["ChassisType"] = "RackMount"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using the enum for setting ChassisType: chassis::ChassisType::RackMount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this line from the commit? As it is not part of prettyName. I believe subsequent commits will get that code in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please remove the line of this.
I think ChassisType is added to each chassis (not chassis collection) as in upstream, and also it is not a part of this prettyName.
DEL> asyncResp->res.jsonValue["ChassisType"] = "RackMount";
redfish-core/lib/storage.hpp
Outdated
| BMCWEB_REDFISH_SYSTEM_URI_NAME, driveId); | ||
| asyncResp->res.jsonValue["Name"] = driveId; | ||
| asyncResp->res.jsonValue["@odata.id"] = boost::urls::format( | ||
| "/redfish/v1/Systems/system/Storage/1/Drives/{}", driveId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using the BMCWEB_REDFISH_SYSTEM_URI_NAME setting to match upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will revert the odata.id change. Should the pretty name be used for the name property or should that also be reverted to
asyncResp->res.jsonValue["Name"] = driveId; I see that the "Id" is anyways populated with driveId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @jeaaustx pointed out, this would be -- as in https://github.com/openbmc/bmcweb/blob/193582b6c4acc1eb60eb8ca1f445e3af4f3fe6d8/redfish-core/lib/storage.hpp#L664
asyncResp->res.jsonValue["@odata.id"] =
boost::urls::format("/redfish/v1/Systems/{}/Storage/1/Drives/{}",
BMCWEB_REDFISH_SYSTEM_URI_NAME, driveId);
I could not get that as the assembly.hpp file is not yet in 1120 |
I am unable to get this as redfish-core/lib/fabric_ports.hpp is not present in 1120. |
c3f0607 to
b3b433c
Compare
I think we can make this PR as WIP and wait for a while (so that the others can be rebased). For example,
|
I am okay either way. I am okay with this going first then being added to assembly and ports later |
I can wait, so it becomes easier to handle in the future |
All dependent PRs are merged. @deepakala-k |
d99d419 to
115e43e
Compare
The current BTW, is this PrettyName replacing bmcweb/redfish-core/lib/fabric_ports.hpp Line 159 in fc1dcb8
|
baemyung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to a comment about fabric_ports.hpp & comparing with 1110, fabric_adapters.hpp seems not done for 1120.
So, should i wait for all of those commits to be in? |
The dependent codes are currently ready. |
8d49bdb to
30c2213
Compare
948e4c7 to
a47eb3f
Compare
baemyung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also a few piece.
- pcie.hpp.
In fact, 1110 code also seems incorrect (similar processor.hpp). I think getPrettyName() should be moved intoaddPCIeDeviceCommonProperties
redfish-core/lib/memory.hpp
Outdated
| dimmInterface = true; | ||
| found = true; | ||
| name_util::getPrettyName(asyncResp, objectPath, serviceMap, | ||
| "/Name"_json_pointer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this be here?
Because getPrettyName() expects only one service, I don't think it needs to be in a loop.
Should be this moved to L412?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing it here as compared to Ln412 is almost the same. If we called at Ln.412, we got to pass the service name as assembleDimmProperties does not have service name. That can be done. But otherwise I do not see it getting called in a loop.
afterGetDimmData -> getDimmDataByService -> assembleDimmProperties ->Ln412
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant -- more put it inside getDimmDataByService(service) or more like inside ` assembleDimmProperties() by passing 'service', as Name is one of properties...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it. I didnt have to call getPrettyName as all the properties are anyways read and so I added pretty name to it. And tested the same. It works as expected.
redfish-core/lib/processor.hpp
Outdated
|
|
||
| callback(objectPath, serviceMap); | ||
| name_util::getPrettyName(asyncResp, objectPath, serviceMap, | ||
| "/Name"_json_pointer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think this is not a right place.
This function handleProcessorSubtree() is an intermediate callback of getProcessorObject() (which is not a direct GET function, and it is also used by PATCH).
getPrettyName() should be done for GET - i.e. replacing "Name".
So, the right place to do is L297 - in getCpuDataByService() which is a part of getProcessorData().
I also think 1110 code also needs to be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem everywhere is that we need to find the service name, which we get in one of these methods. I know we could do the right way. But just trying to ensure that we do not break something that is already working. Will it affect if we update the pretty name in the patch function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried PATCH for Processor and see what's returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, this PATCH is issued, it goes thru getPrettyName() which is not needed.
- Enable debug level
root@p10bmc:~# bmcweb loglevel debug
<6>[webserver_cli.cpp:88] logging level changed to: DEBUG
root@p10bmc:~# journalctl -f
- Run PATCH
curl -k -H "Content-Type: application/json" -X PATCH https://${bmc}/redfish/v1/Systems/system/Processors/dcm0-cpu0 -d '{"LocationIndicatorActive":true}'
Then we can see
Apr 05 13:32:48 p10bmc bmcwebd[920]: [routing.hpp:652] Matched rule '/redfish/v1/Systems/<str>/Processors/<str>/' PATCH / 16
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_connection.hpp:906] 0x2708bc0 Timer canceled
Apr 05 13:32:48 p10bmc bmcwebd[920]: [dbus_privileges.hpp:59] userName = service userRole =
Apr 05 13:32:48 p10bmc bmcwebd[920]: [query.hpp:125] setup redfish route
Apr 05 13:32:48 p10bmc bmcwebd[920]: [processor.hpp:845] Get available system processor resources.
Apr 05 13:32:48 p10bmc bmcwebd[920]: [led.hpp:427] Set LocationIndicatorActive for /xyz/openbmc_project/inventory/system/chassis/motherboard/dcm0/cpu0
Apr 05 13:32:48 p10bmc bmcwebd[920]: [name_utils.hpp:73] Get PrettyName with MapperServiceMap for: /xyz/openbmc_project/inventory/system/chassis/motherboard/dcm0/cpu0
Apr 05 13:32:48 p10bmc bmcwebd[920]: [name_utils.hpp:34] Get PrettyName for: /xyz/openbmc_project/inventory/system/chassis/motherboard/dcm0/cpu0
Apr 05 13:32:48 p10bmc bmcwebd[920]: [name_utils.hpp:51] Pretty Name: System processor module 0
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_response.hpp:259] 0x26d3620 calling completion handler
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_response.hpp:262] 0x26d3620 completion handler was valid
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_response.hpp:97] Moving response containers; this: 0x270ae08; other: 0x26d3620
Apr 05 13:32:48 p10bmc bmcwebd[920]: [audit_events.cpp:250] auditEvent: bufLeft=152 opPathLen=57 detailLen=33 userLen=14
Apr 05 13:32:48 p10bmc bmcwebd[920]: [complete_response_fields.hpp:25] Response: 204
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_connection.hpp:851] 0x2708bc0 doWrite
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_response.hpp:213] 0x270ae08 Response content provided but code was no-content or not_modified, which aren't allowed to have a body
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_connection.hpp:934] 0x2708bc0 timer started
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_body.hpp:198] Returning 41 bytes more=false
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_response.hpp:269] 0x270ae08 setting completion handler
Apr 05 13:32:48 p10bmc bmcwebd[920]: [http_connection.hpp:799] 0x2708bc0 async_write wrote 352 bytes, ec=Success [system:0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks fir checking. I will try to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix seems good for 1120.
Would you also create a PR for 1110?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create a PR for 1110 processor.hpp shortly, by the eod
|
This PR also handles #992 |
The problem is the same. We do not have the service name and probably the best way would be move those into a function and call it both from get and patch. I guess handle was created with that intent. But we need to create another level of method may be! |
For 1110 pcie.hpp, I may misread the code. It is done for GET only. |
27036d7 to
577d3a0
Compare
redfish-core/lib/memory.hpp
Outdated
| if (prettyName != nullptr) | ||
| { | ||
| asyncResp->res.jsonValue[jsonPtr]["Name"] = *prettyName; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, for the consistency, would it be better to use getPrettyName() function by either passing serviceName to assembleDimmProperties(), or doing it like this?
inline void getDimmDataByService(
std::shared_ptr<bmcweb::AsyncResp> asyncResp, const std::string& dimmId,
const std::string& service, const std::string& objPath)
{
...
name_util::getPrettyName(asyncResp, objPath, service, "/Name"_json_pointer);
...
dbus::utility::getAllProperties(
....
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will better do it here, because in the future if people look into assembleDimmProperties and start questioning why we are again making separate call for pretty name, it might be confusing.
576332d to
804f5f1
Compare
redfish-core/lib/assembly.hpp
Outdated
| #include <nlohmann/json.hpp> | ||
| #include <sdbusplus/asio/property.hpp> | ||
| #include <sdbusplus/unpack_properties.hpp> | ||
| #include <utils/name_utils.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be at L16
#include "utils/name_utils.hpp"
redfish-core/lib/fabric_adapters.hpp
Outdated
| #include <boost/url/format.hpp> | ||
| #include <sdbusplus/message/native_types.hpp> | ||
| #include <sdbusplus/unpack_properties.hpp> | ||
| #include <utils/name_utils.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to L20
#include "utils/name_utils.hpp"
redfish-core/lib/fabric_ports.hpp
Outdated
| #include <boost/system/error_code.hpp> | ||
| #include <boost/url/format.hpp> | ||
| #include <sdbusplus/asio/property.hpp> | ||
| #include <utils/name_utils.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to L20
#include "utils/name_utils.hpp"
| const std::string& service, const std::string& objPath) | ||
| { | ||
| BMCWEB_LOG_DEBUG("Get available system components."); | ||
| name_util::getPrettyName(asyncResp, objPath, service, "/Name"_json_pointer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing
#include "utils/name_utils.hpp"
804f5f1 to
35df10f
Compare
| #include "utils/dbus_utils.hpp" | ||
| #include "utils/hex_utils.hpp" | ||
| #include "utils/name_utils.hpp" | ||
| #include "utils/json_utils.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rearrange this
#include "utils/json_utils.hpp"
#include "utils/name_utils.hpp"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, my docker does not work. And I have to use the CI to look for the formatting changes. Thanks a lot! I will fix my docker soon!
0e7f0a8 to
27c9fde
Compare
baemyung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment. I think it is getting close.
redfish-core/lib/storage.hpp
Outdated
| boost::urls::format("/redfish/v1/Systems/{}/Storage/1/Drives/{}", | ||
| BMCWEB_REDFISH_SYSTEM_URI_NAME, driveId); | ||
| asyncResp->res.jsonValue["Name"] = driveId; | ||
| name_util::getPrettyName(asyncResp, path, drive->second, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L663 sets connectionNames for drive->second.
So, this may be better to be like
name_util::getPrettyName(asyncResp, path, connectionNames,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
Created a helper function to fetch pretty name and added for some
resources.
- Chassis
- FirmwareInventory
- Storage
- StorageController
- System
- Processor
- Fan
- Fabric Adaptor
- Memory
- Assembly
Implementation Details:
The helper function makes a call to ObjectMapper to get the services
that support the object path with the interfaces we want. It expects
only one service is exporting the object path.
Then it will try to get the `PrettyName` property. If it is found and is
valid, it will set the `Name` to the value. If not, it will use the
default value that is set before the function call.
This will affect all systems and replace the Name with the PrettyName if
it exist within the resource's object path.
Tested:
Passed Redfish Validator
The output names release the existing `Name` property only if the
PrettyName property exists. If not, it will use the default value.
Output with PrettyName
```
$ curl -u root:0penBmc -X GET http://${bmc}/redfish/v1/Chassis/chassis0
{
"@odata.id": "/redfish/v1/Chassis/chassis0",
"@odata.type": "#Chassis.v1_14_0.Chassis",
"ChassisType": "RackMount",
"Id": "chassis0",
"Links": {
"ComputerSystems": [
{
"@odata.id": "/redfish/v1/Systems/system"
}
],
"ManagedBy": [
{
"@odata.id": "/redfish/v1/Managers/bmc"
}
]
},
"Name": "Test Chassis",
...
}
```
Without PrettyName
```
$ curl -u root:0penBmc -X GET http://${bmc}/redfish/v1/Chassis/chassis0
{
"@odata.id": "/redfish/v1/Chassis/chassis0",
"@odata.type": "#Chassis.v1_14_0.Chassis",
"ChassisType": "RackMount",
"Id": "chassis0",
"Links": {
"ComputerSystems": [
{
"@odata.id": "/redfish/v1/Systems/system"
}
],
"ManagedBy": [
{
"@odata.id": "/redfish/v1/Managers/bmc"
}
]
},
"Name": "chassis0",
...
}
```
Squashed following commits:
Logging invalid service name for debug
Util api to fetch pretty name for assemblies
Dimm: Look at Pretty Name (#973) (#1111)
Add pretty name for fabric adapters (#869)
The method getPrettyName() may be called more than one time.
This will generate journal logging messages for the required Service name
if there are unexpected service names.
In case prettyName is blank for that FRU, then last part of the
object path will be used to populate assembly name.
Signed-off-by: Willy Tu <[email protected]>
Co-authored-by: Willy Tu <[email protected]>
Co-authored-by: Sunny Srivastava <[email protected]>
Co-authored-by: Gunnar Mills <[email protected]>
27c9fde to
6f8a4dc
Compare
|
I will get the 1110 PR by tomorrow. |
|
Just started it. #1234 |
baemyung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
I thought getPrettyName for pcie.hpp is handled by this PR....but it turns out it is missed. |
It looks like another PR #1201 is trying to handle for pcie.hpp. |
Created a helper function to fetch pretty name and added for some resources.
Implementation Details:
The helper function makes a call to ObjectMapper to get the services that support the object path with the interfaces we want. It expects only one service is exporting the object path.
Then it will try to get the
PrettyNameproperty. If it is found and is valid, it will set theNameto the value. If not, it will use the default value that is set before the function call.This will affect all systems and replace the Name with the PrettyName if it exist within the resource's object path.
Tested:
Passed Redfish Validator
The output names release the existing
Nameproperty only if the PrettyName property exists. If not, it will use the default value.Output with PrettyName
Without PrettyName
Performance Diff: