Skip to content
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

Ethernet Port: Add LLDP port schema to interfaces #1121

Open
wants to merge 1 commit into
base: 1110
Choose a base branch
from

Conversation

abhilashraju
Copy link
Contributor

@abhilashraju abhilashraju commented Jan 7, 2025

Ethernet Port: Add LLDP port schema to interfaces
Link Layer Discovery Protocol is a Data Link Layer protocol used for
discovering devices on a local network. LLDP advertises information
about themselves to directly connected devices. This information
includes the device's identity, capabilities, and network management
information, which can be used for network monitoring, topology
discovery and network troubleshooting.

This commit will add Bmcweb changes required to enable/disable LLDP
property in ethernet interfaces.

Schema:
https://redfish.dmtf.org/schemas/v1/PortCollection.json
https://redfish.dmtf.org/schemas/v1/Port.v1_14_0.json

Tested by
Get/Patch on
curl https://xxx/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1

{
"@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1",
"@odata.type": "#Port.v1_14_0.Port",
"Ethernet": {
"LLDPEnabled": false
},
"Id": "eth1",
"Links": {
"EthernetInterfaces": [
{
"@odata.id": "/redfish/v1/Managers/bmc/EthernetInterfaces/eth1"
}
]
},
"Name": "Manager Dedicated Network Port"
}

Get on
curl https://xxx/redfish/v1/Managers/bmc/EthernetInterfaces
{
...
"Links": {
"Ports": [
{
"@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1"
}
],
"Ports@odata.count": 1
}
...
}
Get on
curl https://xxx/redfish/v1/Managers/bmc/DedicatedNetworkPorts

{
"@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts",
"@odata.type": "#PortCollection.PortCollection",
"Members": [
{
"@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth0"
},
{
"@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1"
}
],
"Members@odata.count": 2,
"Name": "Port Collection"
}

Upstream PR:
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/76569

asyncResp->res.jsonValue["@odata.id"] = boost::urls::format(
"/redfish/v1/Managers/{}/EthernetInterfaces/Ports/{}",
BMCWEB_REDFISH_MANAGER_URI_NAME, portId);
asyncResp->res.jsonValue["@odata.type"] = "#Port.v1_4_0.Port";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use latest Port.v1_14 schema version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@abhilashraju abhilashraju force-pushed the adding_lldp_feature branch 3 times, most recently from 8770ee8 to 05dd7bd Compare January 13, 2025 05:43
@raviteja-b
Copy link
Contributor

Need to Implement Port Collection as per dmtf mockups

{
    "@odata.type": "#PortCollection.PortCollection",
    "Name": "Port Collection",
    "Description": "Collection of dedicated network ports for this manager",
    "Members@odata.count": 1,
    "Members": [
        {
            "@odata.id": "/redfish/v1/Managers/BMC/DedicatedNetworkPorts/1"
        }
    ],
    "@odata.id": "/redfish/v1/Managers/BMC/DedicatedNetworkPorts",
    "@Redfish.Copyright": "Copyright 2014-2023 DMTF. For the full DMTF copyright policy, see http://www.dmtf.org/about/policies/copyright."
}

@abhilashraju abhilashraju force-pushed the adding_lldp_feature branch 2 times, most recently from 455885a to 06fa6ff Compare January 13, 2025 13:23
@abhilashraju
Copy link
Contributor Author

Need to Implement Port Collection as per dmtf mockups

{
    "@odata.type": "#PortCollection.PortCollection",
    "Name": "Port Collection",
    "Description": "Collection of dedicated network ports for this manager",
    "Members@odata.count": 1,
    "Members": [
        {
            "@odata.id": "/redfish/v1/Managers/BMC/DedicatedNetworkPorts/1"
        }
    ],
    "@odata.id": "/redfish/v1/Managers/BMC/DedicatedNetworkPorts",
    "@Redfish.Copyright": "Copyright 2014-2023 DMTF. For the full DMTF copyright policy, see http://www.dmtf.org/about/policies/copyright."
}

done

1 similar comment
@abhilashraju
Copy link
Contributor Author

Need to Implement Port Collection as per dmtf mockups

{
    "@odata.type": "#PortCollection.PortCollection",
    "Name": "Port Collection",
    "Description": "Collection of dedicated network ports for this manager",
    "Members@odata.count": 1,
    "Members": [
        {
            "@odata.id": "/redfish/v1/Managers/BMC/DedicatedNetworkPorts/1"
        }
    ],
    "@odata.id": "/redfish/v1/Managers/BMC/DedicatedNetworkPorts",
    "@Redfish.Copyright": "Copyright 2014-2023 DMTF. For the full DMTF copyright policy, see http://www.dmtf.org/about/policies/copyright."
}

done

@@ -2265,7 +2400,7 @@ inline void requestEthernetInterfacesRoutes(App& app)
}

asyncResp->res.jsonValue["@odata.type"] =
"#EthernetInterface.v1_9_0.EthernetInterface";
"#EthernetInterface.v1_9_1.EthernetInterface";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason for this version change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

redfish-core/lib/ethernet.hpp Show resolved Hide resolved
@baemyung
Copy link
Contributor

Please add upstream gerrit commit info.

return;
}
std::string path = "/xyz/openbmc_project/network/";
path += portId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part is better to be like

 std::string path = std::format("/xyz/openbmc_project/network/{}", portId);

redfish-core/lib/ethernet.hpp Show resolved Hide resolved
@baemyung baemyung requested review from gtmills and jeaaustx January 13, 2025 15:43
Copy link
Contributor

@jeaaustx jeaaustx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few comments related to the schemas.

I believe the Redfish.md file also need to be updated.

I think it would be helpful to show the actual command output in the commit message.

Question on Testing: Were the Links verified as correct?

redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
@@ -2246,6 +2380,7 @@ inline void requestEthernetInterfacesRoutes(App& app)
managerId);
return;
}
populateConnectedPortLink(asyncResp, ifaceId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If getEthernetIfaceData() fails below the Links will have been populated but the required fields for EthernetInterface will not be populated. I don't think the links should be populated first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved

asyncResp->res.jsonValue["Members@odata.count"] = ifaceArray.size();
asyncResp->res.jsonValue["@odata.id"] = boost::urls::format(
"/redfish/v1/Managers/{}/EthernetInterfaces",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already filled in on line 2231.

@gtmills
Copy link
Contributor

gtmills commented Jan 13, 2025

Let's make sure to go upstream with this first

@abhilashraju abhilashraju force-pushed the adding_lldp_feature branch 2 times, most recently from 55cbd5c to 653d10f Compare January 14, 2025 07:27
@abhilashraju
Copy link
Contributor Author

Please add upstream gerrit commit info.

Done

@abhilashraju
Copy link
Contributor Author

Let's make sure to go upstream with this first

I have updated the upstream PR. But we cannot pull the commit here. The rebase status of this file very old.

redfish-core/lib/ethernet.hpp Outdated Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
redfish-core/lib/ethernet.hpp Show resolved Hide resolved
@abhilashraju abhilashraju force-pushed the adding_lldp_feature branch 2 times, most recently from 79c76b8 to b08feac Compare January 20, 2025 12:59
[asyncResp, portId](const boost::system::error_code& ec, bool enabled) {
if (ec.value() == EBADR)
{
BMCWEB_LOG_ERROR("Port {} Unreachable", portId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@abhilashraju
Copy link
Contributor Author

I had a few comments related to the schemas.

I believe the Redfish.md file also need to be updated.

I think it would be helpful to show the actual command output in the commit message.

Question on Testing: Were the Links verified as correct?

Yes I have updated the test result in commit log. Are you asking for the validator run? Yes validator run successfully

Link Layer Discovery Protocol is a Data Link Layer protocol used for
discovering devices on a local network. LLDP advertises information
about themselves to directly connected devices. This information
includes the device's identity, capabilities, and network management
information, which can be used for network monitoring, topology
discovery and network troubleshooting.

This commit will add Bmcweb changes required to enable/disable LLDP
property in ethernet interfaces.

Schema:
https://redfish.dmtf.org/schemas/v1/PortCollection.json
https://redfish.dmtf.org/schemas/v1/Port.v1_14_0.json

Tested by
Get/Patch on
curl https://xxx/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1

{
  "@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1",
  "@odata.type": "#Port.v1_14_0.Port",
  "Ethernet": {
    "LLDPEnabled": false
  },
  "Id": "eth1",
  "Links": {
    "EthernetInterfaces": [
      {
        "@odata.id": "/redfish/v1/Managers/bmc/EthernetInterfaces/eth1"
      }
    ]
  },
  "Name": "Manager Dedicated Network Port"
}

Get on
curl https://xxx/redfish/v1/Managers/bmc/EthernetInterfaces
{
...
"Links": {
    "Ports": [
      {
        "@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1"
      }
    ],
    "Ports@odata.count": 1
  }
...
}
Get on
curl https://xxx/redfish/v1/Managers/bmc/DedicatedNetworkPorts

{
  "@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts",
  "@odata.type": "#PortCollection.PortCollection",
  "Members": [
    {
      "@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth0"
    },
    {
      "@odata.id": "/redfish/v1/Managers/bmc/DedicatedNetworkPorts/eth1"
    }
  ],
  "Members@odata.count": 2,
  "Name": "Port Collection"
}

Signed-off-by: Abhilash Raju <abhilash.kollam@gmail.com>
Copy link
Contributor

@jeaaustx jeaaustx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to Redfish.md are still needed to document the new properties being returned.

redfish-core/lib/ethernet.hpp Show resolved Hide resolved
Copy link
Contributor

@baemyung baemyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this seems good to me for 1110.

@sunharis
Copy link
Contributor

@rfrandse This is a base bmcweb change for the complete LLDP feature. Please merge this to get the testing go smooth as per the plan. All major comments are already addressed and the pending comments are not closely related to this feature - can be done as separate PRs

@sunharis
Copy link
Contributor

https://jsw.ibm.com/browse/PFEBMC-3823 is created for the redfish.md document updates

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.

None yet

6 participants