[Nexthop][fboss2-dev] Add a utility class to help map ports to interfaces. #755

benoit-nexthop · 2025-12-18T00:34:03Z

Pre-submission checklist

I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
pre-commit run

Summary

This is a small helper class used to lookup ports and interfaces in a consistent fashion in CLI commands.

Note: this change is part of a series, the previous one is #754, the next one is #756.

Test Plan

Unit tests.

joseph5wu

Overall it looks pretty good to me. Please address the two main comments:

Removing vlog
Use fboss strong type for the ids

joseph5wu · 2025-12-23T00:40:47Z

fboss/cli/fboss2/utils/PortMap.h

+   * @param portName The name of the port (e.g., "eth1/20/1")
+   * @return The interface ID if found, std::nullopt otherwise
+   */
+  std::optional<int32_t> getInterfaceIdForPort(


Can we use FBOSS_STRONG_TYPE from https://github.com/facebook/fboss/blob/main/fboss/agent/types.h#L74
like we do in
https://github.com/facebook/fboss/blob/main/fboss/agent/ApplyThriftConfig.cpp#L674

Please update all the int32_t to the corresponding PortID or InterfaceID from this types.h

joseph5wu · 2025-12-23T00:49:39Z

fboss/cli/fboss2/utils/PortMap.cpp

+  }
+
+  VLOG(2) << "Built port maps with " << portNameToLogicalId_.size() << " ports";
+}


Will vlog dump this logging to the final output of your CLI?
We usually don't use vlog in fboss but xlog from folly.
If you don't need this logging, I'd suggest to remove it cause this is cli tool.

By default it's not visible in the output of the CLI, no, unless you turn on verbose logging.

Sample output: ``` Config Applied Information: =========================== Last Applied Time: 2025-10-11 09:29:36.589 Last Coldboot Applied Time: 2025-10-11 06:44:36.741 ```

Summary: **Pre-submission checklist** - [x] I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running `pip install -r requirements-dev.txt && pre-commit install` - [x] `pre-commit run` Sample output: ``` Config Applied Information: =========================== Last Applied Time: 2025-10-11 09:29:36.589 Last Coldboot Applied Time: 2025-10-11 06:44:36.741 ``` Note: this change is part of a series, the previous one is #753, the next one is #755. Pull Request resolved: #754 Test Plan: Unit tests. Reviewed By: KevinYakar Differential Revision: D89894234 Pulled By: joseph5wu fbshipit-source-id: 97c71bc011882b0cb19b5e9d947c50d8bc3edef2

meta-codesync · 2025-12-29T22:28:40Z

@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D89903145.

joseph5wu

Please adjust the two main comments I added.
Besides, I noticed there're some missing dependency issues on buck:

❯❯❯ sl diff
diff --git a/fbcode/fboss/cli/fboss2/BUCK b/fbcode/fboss/cli/fboss2/BUCK
--- a/fbcode/fboss/cli/fboss2/BUCK
+++ b/fbcode/fboss/cli/fboss2/BUCK
@@ -776,8 +776,14 @@
     exported_deps = [
         ":cmd-handler",
         ":fboss2-lib",
+        "//fboss/agent:agent_config-cpp2-types",
+        "//fboss/agent:fboss-types",
+        "//fboss/agent/if:ctrl-cpp2-types",
         "//folly:conv",
     ],
+    exported_external_deps = [
+        "glog",
+    ],
 )

 cpp_binary(
diff --git a/fbcode/fboss/cli/fboss2/test/BUCK b/fbcode/fboss/cli/fboss2/test/BUCK
--- a/fbcode/fboss/cli/fboss2/test/BUCK
+++ b/fbcode/fboss/cli/fboss2/test/BUCK
@@ -89,6 +89,9 @@
         "CmdStopPcapTest.cpp",
         "PortMapTest.cpp",
     ],
+    # Required for parameterized tests (TEST_P) - static listing doesn't support them
+    # See: https://fburl.com/parameterized-test-not-supported
+    supports_static_listing = False,
     deps = [
         "fbsource//third-party/googletest:gmock",
         ":cmd_test_utils",
@@ -122,9 +125,11 @@
         "//fboss/cli/fboss2/commands/show/route:model-cpp2-types",
         "//fboss/cli/fboss2/commands/show/teflow:model-cpp2-types",
         "//fboss/cli/fboss2/commands/show/transceiver:model-cpp2-types",
+        "//folly:file_util",
         "//folly:network_address",
         "//folly/json:dynamic",
         "//neteng/fboss/bgp/if:bgp_thrift-cpp2-services",
+        "//thrift/lib/cpp2/protocol:protocol",
         "//thrift/lib/cpp2/reflection:testing",
     ],
     external_deps = [("boost", None, "boost_algorithm")],

Besides the missing dependency, supports_static_listing = False, is needed because you're using INSTANTIATE_TEST_SUITE_P in PortMapTest.cpp. Once you address the two major comments, please make sure to include the above buck fixes in the PR

joseph5wu · 2025-12-29T23:39:11Z

fboss/cli/fboss2/utils/PortMap.cpp

+  for (auto& port : *switchConfig.ports()) {
+    // logicalID and name are required fields, so we can dereference directly
+    PortID logicalId(*port.logicalID());
+    const std::string& portName = *port.name();


Because an older version of Port used to not have a PortName(https://github.com/facebook/fboss/blob/main/fboss/agent/switch_config.thrift#L1182),
port name is actually not a required field in our thrift struct.

Without specifically dealing with the potential optional field, our internal signal is not happy on this PR:

There may be an unchecked access to an optional Thrift field. Make sure you check if the field has a value before dereferencing it or calling `value()`. Otherwise these will throw an exception if the field doesn't have a value. You can suppress this warning by wrapping the expression in `apache::thrift::can_throw`, e.g. `can_throw(*obj.field())` or `can_throw(obj.field()->func())` `port.name` is unchecked.

You can consider to do something similar to https://github.com/facebook/fboss/blob/main/fboss/agent/ApplyThriftConfig.cpp#L191
to enforce port name has to exist, like throwing an exception if it doesn't

joseph5wu · 2025-12-30T00:04:29Z

fboss/cli/fboss2/test/PortMapTest.cpp

+// Helper function to get all config files from the link_test_configs directory
+std::vector<std::string> getAllConfigFiles() {
+  std::vector<std::string> configFiles;
+  std::string configDir = "/var/FBOSS/fboss/fboss/oss/link_test_configs/";


Unfortunately we can't guarantee the lint_test_configs in the exact directory when running the unit test.
Most of the time, we run our unit tests either on our dev servers or a remote machine just build the test binary and run it on a test virtual environment.
I'd suggest to not keep these tests as unit tests if we expect certain configs only exist in some fixed directory.
We can consider to have these tests in end-to-end tests, which we can setup test environment before running tests.

✗ Fail: fbcode//fboss/cli/fboss2/test:cmd_test - PortMapTest.MontblancVlanMapping (0.8s) Note: Google Test filter = PortMapTest.MontblancVlanMapping [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from PortMapTest [ RUN ] PortMapTest.MontblancVlanMapping Warning: Could not read config directory: filesystem error: directory iterator cannot open directory: No such file or directory [/var/FBOSS/fboss/fboss/oss/link_test_configs/] terminate called after throwing an instance of 'std::runtime_error' what(): Failed to read config file: /var/FBOSS/fboss/fboss/oss/link_test_configs/montblanc.materialized_JSON *** Aborted at 1767051882 (Unix time, try 'date -d @1767051882') *** *** Signal 6 (SIGABRT) (0x1cc25001be8db) received by PID 1829083 (pthread TID 0x7fc95ad8e240) (linux TID 1829083) (maybe from PID 1829083, UID 117797) (code: -6), stack trace: *** ==1829083==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!

joseph5wu

Add some linter issues

joseph5wu · 2025-12-30T00:18:37Z

fboss/cli/fboss2/test/PortMapTest.cpp

+
+    cfg::AgentConfig agentConfig;
+    apache::thrift::SimpleJSONSerializer::deserialize<cfg::AgentConfig>(
+        configJson.c_str(), agentConfig);


This is one of our internal linter:

This constructs a StringPiece inefficiently. This requires an extra call to strlen, which needs to traverse the entire string. Unless your string has embedded nulls, you should instead construct a StringPiece from the C++ string directly, as this would not require an extra call to strlen. (If your string has embedded nulls and you want this behavior, you can suppress this warning by using the StringPiece(const char *, size_t) constructor and passing strlen explicitly.) Lint code: CLANGTIDY Lint name: facebook-hte-StringPieceConstruction - configJson.c_str(), agentConfig); + configJson, agentConfig);

joseph5wu · 2025-12-30T00:19:52Z

fboss/cli/fboss2/test/PortMapTest.cpp

+
+    cfg::AgentConfig agentConfig;
+    apache::thrift::SimpleJSONSerializer::deserialize<cfg::AgentConfig>(
+        configJson.c_str(), agentConfig);


Same clang inter as below

joseph5wu · 2025-12-30T00:22:31Z

fboss/cli/fboss2/utils/PortMap.cpp

+}
+
+void PortMap::buildVlanMaps(const cfg::SwitchConfig& switchConfig) {
+  if (!switchConfig.vlanPorts().has_value()) {


linter:

'has_value' is deprecated: Avoid using has_value() API for non-optional field since it's often used incorrectly. If this is a legit use-case, please migrate to apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref()). fbcode/thrift/lib/cpp2/FieldRef.h:136:5: note: 'has_value' has been explicitly marked deprecated here [[deprecated( ^ Lint code: CLANGTIDY Lint name: clang-diagnostic-deprecated-declarations

joseph5wu · 2025-12-30T00:23:12Z

fboss/cli/fboss2/utils/PortMap.cpp

+}
+
+void PortMap::buildInterfaceMaps(const cfg::SwitchConfig& switchConfig) {
+  if (!switchConfig.interfaces().has_value()) {


linter:

'has_value' is deprecated: Avoid using has_value() API for non-optional field since it's often used incorrectly. If this is a legit use-case, please migrate to apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref()). fbcode/thrift/lib/cpp2/FieldRef.h:136:5: note: 'has_value' has been explicitly marked deprecated here [[deprecated( ^ Lint code: CLANGTIDY Lint name: clang-diagnostic-deprecated-declarations

joseph5wu · 2025-12-30T00:24:23Z

fboss/cli/fboss2/utils/PortMap.cpp

+    interfaceIdToInterface_[intfId] = const_cast<cfg::Interface*>(&interface);
+
+    // vlanID is optional
+    if (interface.vlanID().has_value()) {


linter:

'has_value' is deprecated: Avoid using has_value() API for non-optional field since it's often used incorrectly. If this is a legit use-case, please migrate to apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref()). fbcode/thrift/lib/cpp2/FieldRef.h:136:5: note: 'has_value' has been explicitly marked deprecated here [[deprecated( ^ Lint code: CLANGTIDY Lint name: clang-diagnostic-deprecated-declarations

joseph5wu · 2025-12-30T00:24:45Z

fboss/cli/fboss2/utils/PortMap.cpp

+    portLogicalId = PortID(*interface.portID());
+    VLOG(3) << "Interface " << intfId
+            << " mapped to port via portID: " << *portLogicalId;
+  } else if (interface.vlanID().has_value()) {


linter:

'has_value' is deprecated: Avoid using has_value() API for non-optional field since it's often used incorrectly. If this is a legit use-case, please migrate to apache::thrift::is_non_optional_field_set_manually_or_by_serializer(obj.field_ref()). fbcode/thrift/lib/cpp2/FieldRef.h:136:5: note: 'has_value' has been explicitly marked deprecated here [[deprecated( ^ Lint code: CLANGTIDY Lint name: clang-diagnostic-deprecated-declarations

joseph5wu · 2025-12-30T00:25:38Z

fboss/cli/fboss2/utils/PortMap.cpp

+}
+
+std::optional<std::string> PortMap::getPortNameForInterface(
+    InterfaceID interfaceId) const {


linter:

the parameter 'interfaceId' is copied for each invocation but only used as a const reference; consider making it a const reference Lint code: CLANGTIDY Lint name: performance-unnecessary-value-param - InterfaceID interfaceId) const { + const InterfaceID& interfaceId) const {

joseph5wu · 2025-12-30T00:26:10Z

fboss/cli/fboss2/utils/PortMap.cpp

+  return portNameToLogicalId_.find(portName) != portNameToLogicalId_.end();
+}
+
+bool PortMap::hasInterface(InterfaceID interfaceId) const {


linter:

the parameter 'interfaceId' is copied for each invocation but only used as a const reference; consider making it a const reference Lint code: CLANGTIDY Lint name: performance-unnecessary-value-param -bool PortMap::hasInterface(InterfaceID interfaceId) const { +bool PortMap::hasInterface(const InterfaceID& interfaceId) const {

joseph5wu · 2025-12-30T00:26:50Z

fboss/cli/fboss2/utils/PortMap.cpp

+  return nullptr;
+}
+
+cfg::Interface* PortMap::getInterface(InterfaceID interfaceId) const {


linter:

the parameter 'interfaceId' is copied for each invocation but only used as a const reference; consider making it a const reference Lint code: CLANGTIDY Lint name: performance-unnecessary-value-param -cfg::Interface* PortMap::getInterface(InterfaceID interfaceId) const { +cfg::Interface* PortMap::getInterface(const InterfaceID& interfaceId) const {

joseph5wu · 2025-12-30T00:28:53Z

fboss/cli/fboss2/utils/PortMap.cpp

+  if (it != interfaceIdToInterface_.end()) {
+    return it->second;
+  }
+  return nullptr;


linter:

This function may return a nullable value but is not marked nullable. Consider annotating your return type with FOLLY_NULLABLE defined in <folly/CppAttributes.h>. See https://fburl.com/nullability for more information. Lint code: CLANGTIDY Lint name: facebook-hte-NullableReturn -cfg::Interface* PortMap::getInterface(InterfaceID interfaceId) const { +cfg::Interface* FOLLY_NULLABLE PortMap::getInterface(const InterfaceID& interfaceId) const

meta-cla bot added the CLA Signed label Dec 18, 2025

benoit-nexthop marked this pull request as ready for review December 18, 2025 00:36

srikrishnagopu requested a review from joseph5wu December 18, 2025 21:27

srikrishnagopu assigned joseph5wu Dec 18, 2025

benoit-nexthop force-pushed the fboss2-cli-prototype_part03 branch from 04bd218 to ab2a9fb Compare December 19, 2025 23:32

joseph5wu reviewed Dec 23, 2025

View reviewed changes

benoit-nexthop force-pushed the fboss2-cli-prototype_part03 branch from ab2a9fb to 1e8d134 Compare December 23, 2025 21:38

This was referenced Dec 23, 2025

[Nexthop][fboss2-dev] Add fboss2 config applied-info. #754

Closed

[Nexthop][fboss2-dev] Add a very basic prototype of a config session. #756

Open

[Nexthop] fboss2 CLI config prototype #668

Closed

benoit-nexthop force-pushed the fboss2-cli-prototype_part03 branch 3 times, most recently from 3038f6a to 7a586bc Compare December 23, 2025 23:37

benoit-nexthop added 2 commits December 25, 2025 22:57

Add fboss2 config applied-info.

92b1800

Sample output: ``` Config Applied Information: =========================== Last Applied Time: 2025-10-11 09:29:36.589 Last Coldboot Applied Time: 2025-10-11 06:44:36.741 ```

Add a utility class to help map ports to interfaces.

4102813

benoit-nexthop force-pushed the fboss2-cli-prototype_part03 branch from 7a586bc to 4102813 Compare December 25, 2025 22:00

Merge branch 'main' into fboss2-cli-prototype_part03

2da98c7

joseph5wu requested changes Dec 30, 2025

View reviewed changes

[Nexthop][fboss2-dev] Add a utility class to help map ports to interfaces. #755

Are you sure you want to change the base?

[Nexthop][fboss2-dev] Add a utility class to help map ports to interfaces. #755

Conversation

benoit-nexthop commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

joseph5wu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meta-codesync bot commented Dec 29, 2025

Uh oh!

joseph5wu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joseph5wu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

benoit-nexthop commented Dec 18, 2025 •

edited

Loading