[ovsp4rt] Implement Client class #732

ffoulkes · 2025-01-08T17:11:48Z

This is a project to improve the testability of ovsp4rt. See issue #701 for details.

Updated cmake listfile to use newovsp4rt.cc instead of ovsp4rt.cc when the BUILD_CLIENT option is enabled.
Modified newovsp4rt.cc to use a Client object instead of an OvsP4rtSession object to communicate with the P4Runtime server.
Made each public C API function a wrapper around a C++ function that accepts a Client object as a parameter. This allows a test to replace the object with a test double (e.g. a mock).
Moved the C API functions to another file (ovsp4rt_standard_api.cc), separating the user interface from the implementation.
Created a second implementation of the C API (ovsp4rt_journal_api.cc) that enables journaling.
Added an ovsp4rt_build_and_test job to the pipeline, to verify that ovsp4rt builds when BUILD_CLIENT and BUILD_JOURNAL are enabled.

Note

This PR adds a new file. To see the deltas between ovsp4rt.cc and newovsp4rt.cc, see 5ca68cb.

This PR supersedes PR #727.

Signed-off-by: Derek Foster <justffoulkes@gmail.com>

- Updated listfile to use newovsp4rt.cc instead of ovsp4rt.cc when the BUILD_CLIENT option is enabled. - Modified newovsp4rt.cc to use a `Client` object instead of an `OvsP4rtSession` object to communicate with the P4Runtime server. - Made each public C API function a wrapper around a C++ function that accepts a Client object as a parameter. This allows a unit test to replace the object with a test double (e.g. a mock). - Moved the C API functions to another file, separating the user interface from the implementation. Signed-off-by: Derek Foster <justffoulkes@gmail.com>

Signed-off-by: Derek Foster <justffoulkes@gmail.com>

ffoulkes · 2025-01-08T19:09:44Z

ovs-p4rt/sidecar/CMakeLists.txt

@@ -26,7 +26,7 @@ endif()
 #-----------------------------------------------------------------------
 add_library(ovs_sidecar_o OBJECT
    ${OVSP4RT_INCLUDE_DIR}/ovsp4rt/ovs-p4rt.h
-    ovsp4rt.cc
+    $<IF:$<BOOL:${BUILD_CLIENT}>,newovsp4rt.cc,ovsp4rt.cc>


This is a cmake generator expression. It selects newovsp4rt.cc if BUILD_CLIENT evaluates to TRUE, and ovsp4rt.cc if it evaluates to FALSE.

ffoulkes · 2025-01-08T19:11:20Z

ovs-p4rt/sidecar/CMakeLists.txt

@@ -26,7 +26,7 @@ endif()
 #-----------------------------------------------------------------------
 add_library(ovs_sidecar_o OBJECT
    ${OVSP4RT_INCLUDE_DIR}/ovsp4rt/ovs-p4rt.h
-    ovsp4rt.cc
+    $<IF:$<BOOL:BUILD_CLIENT>,newovsp4rt.cc,ovsp4rt.cc>


This generator expression has a bug that's fixed in a subsequent commit.

ffoulkes · 2025-01-08T19:13:18Z

ovs-p4rt/sidecar/ovsp4rt_internal_api.h

@@ -13,7 +13,7 @@
 namespace ovsp4rt {

 extern void ConfigFdbEntry(ClientInterface& client,
-                           const struct mac_learning_info& learn_info,
+                           struct mac_learning_info learn_info,


learn_info is passed by value because the ES2K version makes local modifications to it. This fact is mentioned in an implementation comment.

ffoulkes · 2025-01-08T19:22:16Z

ovs-p4rt/sidecar/newovsp4rt.cc

-      status_or_read_response =
-          GetTxAccVsiTableEntry(session.get(), learn_info.src_port, p4info);
-      if (!status_or_read_response.ok()) {
+      //


The proposed refactoring is implemented in #729. It will need to be revised for newovsp4rt.cc.

ffoulkes · 2025-01-08T19:49:29Z

ovs-p4rt/sidecar/CMakeLists.txt

This commit does two things.

It replaces ovsp4rt::OvsP4rtSession* session with ClientInterface client&.

It converts each of the C APIs into an internal C++ function (in this file) and a C wrapper function (in another file).

Replacing the explicit OvsP4rtSession object with an abstract ClientInterface object decouples the core logic from the P4Runtime server. We can now replace the server with a test double, such as a mock, giving us the ability to unit-test the code.

It also hides the details of the server interface, replacing a lot of duplicated code with much simpler method calls.

Splitting the interface into a C wrapper and a C++ implementation creates a seam. "A seam is a place where you can alter behavior in your program without editing in that place." (Michael Feathers, Working Effectively with Legacy Code)

Taken together, these two things give us testability.

ffoulkes · 2025-01-08T20:00:18Z

ovs-p4rt/sidecar/newovsp4rt.cc

  if (!insert_entry) {
    auto status_or_read_response =
-        GetL2ToTunnelV4TableEntry(session.get(), learn_info, p4info);
+        GetL2ToTunnelV4TableEntry(client, learn_info, p4info);


PR #728* defines predicate functions that allow us to simplify this code to:

if (HaveL2ToTunnelV4TableEntry(client, learn_info, p4info)) { learn_info.is_tunnel = true; }

See commit 175a9f4 for details.

* This PR will need to be revised for newovsp4rt.cc.

ffoulkes · 2025-01-08T20:06:31Z

ovs-p4rt/sidecar/newovsp4rt.cc

-  // Unwrap the session from the StatusOr object.
-  std::unique_ptr<ovsp4rt::OvsP4rtSession> session =
-      std::move(status_or_session).value();
+  status = client.connect(grpc_addr);


I would suggest reviewing the old server interface code against:

ovsp4rt_client.cc

ovsp4rt_client.h

ovsp4rt_client_interface.h

ffoulkes · 2025-01-08T20:07:25Z

ovs-p4rt/sidecar/newovsp4rt.cc

-  ::p4::v1::ReadResponse read_response =
-      std::move(status_or_read_response).value();
+  //
+  // GetVsiSrcPort(ClientInterface& client, const P4Info& p4info,


See previous comment.

ffoulkes · 2025-01-08T20:09:19Z

ovs-p4rt/sidecar/ovsp4rt_journal_api.cc

@@ -1,4 +1,4 @@
-// Copyright 2022-2024 Intel Corporation
+// Copyright 2022-2025 Intel Corporation
 // SPDX-License-Identifier: Apache-2.0

 // Journaling implementation of the ovsp4rt C API.


This is an early draft of the API.

5abeel

LGTM

ffoulkes added 3 commits January 8, 2025 07:37

[ovsp4rt] Copy ovsp4rt.cc to newovsp4rt.cc

00292b0

Signed-off-by: Derek Foster <justffoulkes@gmail.com>

[ovsp4rt] Add ovsp4rt_build_and_test to pipeline

6ab629f

Signed-off-by: Derek Foster <justffoulkes@gmail.com>

ffoulkes requested review from 5abeel, n-sandeep and vsureshkumarp as code owners January 8, 2025 17:11

ffoulkes mentioned this pull request Jan 8, 2025

[ovsp4rt] Implement Client class (DRAFT) #727

Closed

ffoulkes added 2 commits January 8, 2025 10:12

[ovsp4rt] Fix bug in cmake generator expression

2e6b8a0

Signed-off-by: Derek Foster <justffoulkes@gmail.com>

Merge branch 'main' into newovsp4rt.cc

4c21534

Signed-off-by: Derek Foster <justffoulkes@gmail.com>

ffoulkes added enhancement New feature or request cmake Affects CMake build system github_actions Pull requests that update GitHub Actions code labels Jan 8, 2025

ffoulkes commented Jan 8, 2025

View reviewed changes

5abeel approved these changes Jan 8, 2025

View reviewed changes

ffoulkes merged commit 4a92a84 into main Jan 8, 2025
10 checks passed

ffoulkes deleted the newovsp4rt.cc branch January 8, 2025 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ovsp4rt] Implement Client class #732

[ovsp4rt] Implement Client class #732

ffoulkes commented Jan 8, 2025 •

edited

Loading

ffoulkes Jan 8, 2025

ffoulkes Jan 8, 2025

ffoulkes Jan 8, 2025

ffoulkes Jan 8, 2025

ffoulkes Jan 8, 2025

ffoulkes Jan 8, 2025

ffoulkes Jan 8, 2025 •

edited

Loading

ffoulkes Jan 8, 2025

ffoulkes Jan 8, 2025

5abeel left a comment

[ovsp4rt] Implement Client class #732

[ovsp4rt] Implement Client class #732

Conversation

ffoulkes commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffoulkes Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5abeel left a comment

Choose a reason for hiding this comment

ffoulkes commented Jan 8, 2025 •

edited

Loading

ffoulkes Jan 8, 2025 •

edited

Loading