search

ibm-openbmc / dev

Product Development Project Mgmt and Tracking
16 stars 2 forks source link

1050: bmcweb: ThermalSubsystem has 2 Fan resources per fan #3621

Closed gtmills closed 1 year ago

gtmills commented 1 year ago

@lxwinspur FYI 1050:

{
  "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans",
  "@odata.type": "#FanCollection.FanCollection",
  "Description": "The collection of Fan resource instances chassis",
  "Members": [
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan3_1"
    },
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan3_0"
    },
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan2_1"
    },
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan2_0"
    },
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan1_1"
    },
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan1_0"
    },
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan0_1"
    },
    {
      "@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan0_0"
    }
  ],
  "Members@odata.count": 8,
  "Name": "Fan Collection"
}

1030:

{
"@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/",
"@odata.type": "#FanCollection.FanCollection",
"Description": "The collection of Fan resource instances chassis",
"Members": [
{
"@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan0/"
},
{
"@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan1/"
},
{
"@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan2/"
},
{
"@odata.id": "/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan3/"
}
],
"Members@odata.count": 4,
"Name": "Fan Collection"
}
lxwinspur commented 1 year ago

@gtmills Which one is correct? 1030 or 1050? (I think 1050 is correct) If Fan0 is dual rotor, then it should be fan0_0, fan0_1.

inline void fanCollection(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
                          const std::string& fanPath,
                          const std::string& chassisId)
{
    dbus::utility::getAssociationEndPoints(
        fanPath + "/sensors",
        [asyncResp, fanPath,
         chassisId](const boost::system::error_code& ec,
                    const dbus::utility::MapperEndPoints& endpoints) {
        if (ec)
        {
            if (ec.value() != EBADR)
            {
                messages::internalError(asyncResp->res);
                return;
            }
        }

        if (endpoints.empty())
        {
            updateFanList(asyncResp, chassisId, fanPath);
            return;
        }

        for (const auto& endpoint : endpoints)
        {
            updateFanList(asyncResp, chassisId, endpoint);
        }
        });
}
lxwinspur commented 1 year ago 
/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan0

This is a single rotor

/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan0_0
/redfish/v1/Chassis/chassis/ThermalSubsystem/Fans/fan0_1

This is a dual rotor

gtmills commented 1 year ago

@lxwinspur Can we go with what we have for 1030 for 1050 so behavior doesn't change for the user Upstream we can do whatever we think

lxwinspur commented 1 year ago

@gtmills I double checked the codes of 1030 and 1050, the logic they implement is basically the same It’s just that in 1050, cooled_by is used to associate and find the Fan belonging to chassisId, but this association has been realized in 1050’s ibm, rainier-2u_associations.json, So I think the results of 1030 and 1050 are the same. For Mex Fan (chassisIdxxx), if the association of cooled_by is not implemented, return directly, logically there should be no problem. If you have a problem with the test, please show the log, thank you.

spinler commented 1 year ago

@lxwinspur There was issue https://github.com/ibm-openbmc/dev/issues/3513 from last year where we first requested the ThermalSubsystem info not be based on the tach sensors but rather the FRUs.

In that associations file, I can see the cooling/cooled_by association isn't pointing at the sensors, so why are they showing up in the output?

lxwinspur commented 1 year ago

@spinler Ah, got you. Also I had a discussion with @zhanghaodi that we shouldn't show the tachometer sensor in ThermalSubsystem

I will fix this issue this week, thanks.

lxwinspur commented 1 year ago

@gtmills @spinler review by: https://github.com/ibm-openbmc/bmcweb/pull/684

ChicagoDuan commented 1 year ago

Merged: https://github.com/ibm-openbmc/bmcweb/pull/684