Reengineer Stratum changes for Split Architecture #2

ffoulkes · 2022-08-26T00:55:10Z

Extracted TDI classes, cleaned them up, and used them as the
basis of a new 'tdi' platform. Created tdi_hal and tdi_main
modules. Adapted existing unit tests for tdi.
Extracted classes that were heavily modified for IPDK and
created separate 'ipdk' variants. Created new ipdk_switch,
ipdk_hal, and ipdk_main modules. Restored delected code under
control of the P4OVS_CHANGES conditional. Reversed breaking
changes to shared class constructors and factory functions.
Adapted existing unit tests for ipdk.
Modified bf_chassis_manager_test to use NiceMock. This
suppresses the "uninteresting function call" warnings.
Corrected various spelling, punctuation, and grammar errors,
primarily in comments.
Modified common/BUILD to support gnmi_publisher_test and
yang_parse_tree_test.
Incorporated P4OVS additions to common.proto.
Suppressed unnecessary definitions and includes in phal_sim.h
to address some compilation issues.
Replaced P4SDE_DPDK_TARGET_STUB conditional with P4OVS_CHANGES
(or, in a few cases, DPDK_TARGET or TOFINO_TARGET).

Signed-off-by: Derek Foster derek.foster@intel.com

- Extracted TDI classes, cleaned them up, and used them as the basis of a new 'tdi' platform. Created tdi_hal and tdi_main modules. Adapted existing unit tests for tdi. - Extracted classes that were heavily modified for IPDK and created separate 'ipdk' variants. Created new ipdk_switch, ipdk_hal, and ipdk_main modules. Restored delected code under control of the P4OVS_CHANGES conditional. Reversed breaking changes to shared class constructors and factory functions. Adapted existing unit tests for ipdk. - Modified bf_chassis_manager_test to use NiceMock. This suppresses the "uninteresting function call" warnings. - Corrected various spelling, punctuation, and grammar errors, primarily in comments. - Modified common/BUILD to support gnmi_publisher_test and yang_parse_tree_test. - Incorporated P4OVS additions to common.proto. - Suppressed unnecessary definitions and includes in phal_sim.h to address some compilation issues. - Replaced P4SDE_DPDK_TARGET_STUB conditional with P4OVS_CHANGES (or, in a few cases, DPDK_TARGET or TOFINO_TARGET). Signed-off-by: Derek Foster <derek.foster@intel.com>

ffoulkes · 2022-08-26T01:21:24Z

This change set consists of the files needed for Stratum to build (using CMake) under the Networking Recipe and to run 78 unit tests (under Bazel). It is not complete (yang_parse_tree_paths is missing), but it includes all the other files on which I need feedback.

In particular, please take a look at the following files:

ipdk_chassis_manager.cc
ipdk_chassis_manager.h
ipdk_switch.cc
tdi_sde_wrapper.cc
tdi_sde_wrapper.h
config_monitoring_service.cc
p4_service.cc

These files contain undocumented edits made prior to my involvement with the project, which have been conditionalized using P4OVS_CHANGES, or in a few cases, DPDK_TARGET or TOFINO_TARGET.

I need to know:

The reason the change was made
The context in which the change is applicable (IPDK, DPDK, MEV, Tofino, etc.)
Whether the change was intended to be temporary or permanent
If temporary, what needs to be done before the change can be reverted

The objective is to eliminate as many of the conditionals as possible by determining which changes to keep, which changes to discard, and how best to incorporate the changes in a fashion that will support ongoing maintenance and development of the code. Bear in mind that we are striving for vendor neutrality, and that we must not make breaking changes to the existing Stratum code.

Bear in mind, we are expected to support IPUs based on MEV, DPDK, and Tofino. We cannot delete a piece of code that DPDK doesn't need but Tofino does. This is why I'm asking for you to provide context. The reason for a change may have been DPDK, but its context may call for #ifdef TOFINO_TARGET instead of #ifndef DPDK_TARGET. A careless review now could lead to unnecessary pain in the near future. Let's not make people have to go through this exercise a third time.

I've added some comments (most marked with P4OVS_CHANGES) where I had specific questions or observations to make.

Signed-off-by: Derek Foster <derek.foster@intel.com>

ffoulkes · 2022-08-30T17:32:14Z

stratum/hal/lib/tdi/tdi.proto

+// This message describes the public Barefoot-specific definition of a single P4
+// program. It is passed to Stratum as the p4 device config of the P4Runtime
+// ForwardingPipelineConfig message.
+message BfPipelineConfig {


Should this be TdiPipelineConfig?

Yes in my opinion.

ffoulkes · 2022-08-30T17:35:49Z

stratum/hal/lib/tdi/tdi.proto

+
+import "p4/config/v1/p4info.proto";
+
+// This message describes the public Barefoot-specific definition of a single P4


If TDI is vendor-neutral, shouldn't we replace Barefoot, tofino.bin, bfruntime_info, and bfrt.json with generic names?

Please remove all references to bfrt, barefoot, tofino, etc.

So is this just a renaming exercise? The problem with renaming things is that if you ever want to sync from the upstream Stratum, it will cause a bunch of problems. Unless the plan is to never sync from upstream, I would not casually rename things in a fork.

stratum/hal/bin/tdi/BUILD

ravi861 · 2022-09-08T00:26:54Z

stratum/hal/bin/tdi/BUILD

+    ],
+    arches = HOST_ARCHES,
+    data = [
+        "tofino_skip_p4.conf",


Please add this file also in addition to the no_bsp version.

This is messy. TDI has a modified tofino_skip_p4_no_bsp.conf that appears to be DPDK-specific and no tofino_skip_p4.conf.

For now, I'm going to copy the files from the barefoot target. The build will require additional work to make it multi-target.

ravi861 · 2022-09-08T00:27:32Z

stratum/hal/bin/ipdk/ipdk_main.cc

@@ -0,0 +1,119 @@
+// Copyright 2018-2019 Barefoot Networks, Inc.


no bazel BUILD file in this directory

We're not ready to build IPDK with Bazel. Omitting the BUILD file causes Bazel to ignore the directory.

ravi861 · 2022-09-08T00:28:07Z

stratum/hal/bin/ipdk/ipdk_main.cc

+
+namespace stratum {
+namespace hal {
+namespace barefoot {


Please change namespace to ipdk everywhere

IPDK will probably be part of the Stratum tdi namespace once the latter has been defined.

ravi861 · 2022-09-08T00:28:49Z

stratum/hal/bin/tdi/BUILD

+        "systemd",
+        "telnet",
+    ],
+    description = "The Stratum package for Barefoot Tofino-based platform",


Please alter comments to indicate Tdi

It's going to be trickier than this. Unlike the other Stratum targets (bcm, barefoot, etc.), tdi and ipdk are multi-vendor and multi-p4-target.

This is not a TDI-based platform, it's a Tofino-based IPDK platform. (TDI is implementation detail, not a target type. We don't need to call it out unless we're building both TDI- and, say, BFRT-based IPDK distros.)

tdi_main.cc is Intel-specific. Broadcom, for example, will need their own version if they want to support TDI because their P4 object structure is different.

We will need customizations for Tofino, DPDK, and MEV. Some of these can be done via Bazel conditionals; others will be best done by having separate files.

ravi861 · 2022-09-08T00:37:10Z

stratum/hal/lib/common/p4_service.cc

                     node_id, "."));
  }

+#ifdef P4OVS_CHANGES


This is supported on Tofino. We should make the alteration in the tdi_node code or lower perhaps in the per target code. SetForwardingPipeline is a commonly used API in P4Runtime and expected to be supported on targets supporting P4Runtime.

ravi861 · 2022-09-08T00:38:57Z

stratum/hal/lib/tdi/tdi_id_mapper.cc

+#include "stratum/glue/gtl/map_util.h"
+#include "stratum/hal/lib/tdi/tdi_constants.h"
+#include "stratum/hal/lib/tdi/macros.h"
+#include "tdi_rt/tdi_rt_defs.h"


These are DPDK specific header file. Please add a P4OVS_CHANGES wrapper.
The tofino version is tdi_tofino/tdi_tofino_defs.h.

This is unfortunate. We shouldn't be exposing backend-specific TDI symbols to users of TDI. It undercuts TDI's claim to be vendor-neutral.

(But thanks for the catch!)

ravi861 · 2022-09-08T00:40:24Z

stratum/hal/lib/tdi/tdi_id_mapper.cc

+  for (const auto* table : tdi_tables) {
+    std::string table_name;
+    tdi_id_t table_id;
+    auto table_type = static_cast<tdi_rt_table_type_e>(


tdi_rt_table_type_e and TDI_RT_TABLE_TYPE_ACTION_PROFILE are DPDK specific.
We need to get the relevant folks to figure this out.
This also requires a P4OVS_CHANGES.

Same comment for lines 203 and 207.

This is wrong. The set of possible table types should be common; otherwise, TDI is not abstracting the underlying driver.

ravi861 · 2022-09-08T00:45:52Z

stratum/hal/lib/tdi/tdi_sde_wrapper.cc

+#include "absl/synchronization/notification.h"
+#include "absl/time/time.h"
+
+#ifdef TOFINO_TARGET


This file in its current form does not compile for Tofino. We will need some interim fixes so that we can start testing this code on Tofino since that is the most urgent target.
I have a patch with DPDK_TARGET and TOFINO_TARGET all over to get it to compile for both targets although it is not the most desirable solution at this time.

ravi861 · 2022-09-08T00:47:06Z

stratum/hal/lib/tdi/tdi_sde_wrapper.cc

@@ -0,0 +1,3817 @@
+// Copyright 2019-present Barefoot Networks, Inc.


The rest of the comments for this file are tofino vs dpdk. So I will not add any more and just leave the understanding that we will need to figure this out.

- Also corrected some grammar and spelling errors and removed a couple of unnecessary files. Signed-off-by: Derek Foster <derek.foster@intel.com>

Signed-off-by: Ravi Vantipalli <ravi.vantipalli@intel.com> Signed-off-by: Derek Foster <derek.foster@intel.com>

Signed-off-by: Derek Foster <derek.foster@intel.com>

* TDI-specific bazel changes * more code compilation fixes Signed-off-by: Ravi Vantipalli <ravi.vantipalli@intel.com> Signed-off-by: Derek Foster <derek.foster@intel.com>

- Fix compile errors when building for DPDK target. - Change #ifndef P4OVS_CHANGES to #ifdef TOFINO_TARGET in tdi_sde_wrapper. - Make minor changes to a couple of copyright notices. Signed-off-by: Derek Foster <derek.foster@intel.com>

- Removed hal/lib/ipdk and hal/bin/ipdk directories. - Removed P4OVS_CHANGES defines from hal/lib/tdi/BUILD. Signed-off-by: Derek Foster <derek.foster@intel.com>

* Debian packaging * fix stratum flags Signed-off-by: Ravi Vantipalli <ravi.vantipalli@intel.com>

* Implement TDI target selection parameter - Added support for --define target=tofino. Bazel will abort the build if you omit the target parameter when building TDI. Signed-off-by: Derek Foster <derek.foster@intel.com>

Sync with ipdk-io startum

Changes in stratum to rename from ES2000 to ES2K

ffoulkes requested review from Namrata-intel, n-sandeep and nupuruttarwar August 26, 2022 00:55

Rename tdi/bf.proto to tdi/tdi.proto

eb082e6

Signed-off-by: Derek Foster <derek.foster@intel.com>

ffoulkes commented Aug 30, 2022

View reviewed changes

ffoulkes requested a review from ravi861 September 7, 2022 22:50

ravi861 reviewed Sep 8, 2022

View reviewed changes

ffoulkes requested a review from jamescchoi September 8, 2022 19:52

ffoulkes and others added 8 commits September 14, 2022 17:06

Refine TDI code

e7d0e2c

- Also corrected some grammar and spelling errors and removed a couple of unnecessary files. Signed-off-by: Derek Foster <derek.foster@intel.com>

Tofino target changes (#1)

ba4da7d

Signed-off-by: Ravi Vantipalli <ravi.vantipalli@intel.com> Signed-off-by: Derek Foster <derek.foster@intel.com>

Fix compile errors

370ad71

Signed-off-by: Derek Foster <derek.foster@intel.com>

TDI specific bazel changes (#2)

4325d54

* TDI-specific bazel changes * more code compilation fixes Signed-off-by: Ravi Vantipalli <ravi.vantipalli@intel.com> Signed-off-by: Derek Foster <derek.foster@intel.com>

Fix DPDK target build errors (#3)

192a35c

- Fix compile errors when building for DPDK target. - Change #ifndef P4OVS_CHANGES to #ifdef TOFINO_TARGET in tdi_sde_wrapper. - Make minor changes to a couple of copyright notices. Signed-off-by: Derek Foster <derek.foster@intel.com>

Remove IPDK files from initial commit (#4)

1b13abf

- Removed hal/lib/ipdk and hal/bin/ipdk directories. - Removed P4OVS_CHANGES defines from hal/lib/tdi/BUILD. Signed-off-by: Derek Foster <derek.foster@intel.com>

Debian packaging (#5)

4c449a1

* Debian packaging * fix stratum flags Signed-off-by: Ravi Vantipalli <ravi.vantipalli@intel.com>

Implement TDI target selection parameter (#6)

a30963c

* Implement TDI target selection parameter - Added support for --define target=tofino. Bazel will abort the build if you omit the target parameter when building TDI. Signed-off-by: Derek Foster <derek.foster@intel.com>

ffoulkes marked this pull request as ready for review September 21, 2022 17:44

ffoulkes merged commit 698214e into ipdk-io:split-arch Sep 21, 2022

nupuruttarwar added a commit that referenced this pull request Oct 17, 2022

Merge pull request #2 from ipdk-io/main

9ca92d4

Sync with ipdk-io startum

vsureshkumarp pushed a commit that referenced this pull request Feb 27, 2023

Merge pull request #2 from intel-innersource/rename_es2000_to_es2k

8a97066

Changes in stratum to rename from ES2000 to ES2K


		import "p4/config/v1/p4info.proto";

		// This message describes the public Barefoot-specific definition of a single P4

		@@ -0,0 +1,119 @@
		// Copyright 2018-2019 Barefoot Networks, Inc.

		@@ -0,0 +1,3817 @@
		// Copyright 2019-present Barefoot Networks, Inc.

Reengineer Stratum changes for Split Architecture #2

Reengineer Stratum changes for Split Architecture #2

Uh oh!

Conversation

ffoulkes commented Aug 26, 2022

Uh oh!

ffoulkes commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

3 participants

ffoulkes commented Aug 26, 2022 •

edited

Loading