PINS Extension tables support #17

svshah-intel · 2022-11-09T01:16:00Z

High Level Design: sonic-net/SONiC#1088

This PR is for changes in sonic-pins repo

This changes include,

ConfigureAppDbTables() to create and publish Table definitions for all extension tables. Validate response for the success, if failed return failure back to P4RT client
A function to insert table definition in json format
A function to publish tables definition set into AppDb
New table types kTblsDefinitionSet and kExt, modified existing type kDefinition to kAclDefinition
Add/update table utility functions around new and modified table types
Add list of fixed table definitions, rest any other table definition is considered of EXT type

Not covered in this commit,

meter
CRM
batching

Signed-off-by: svshah-intel shitanshu.shah@intel.com

rhalstea · 2022-11-14T16:03:02Z

@kishanps is more familiar with this effort, adding him for review

kishanps

Thanks Shitanshu, happy to see this coming along.

kishanps · 2022-11-22T01:33:24Z

p4rt_app/p4runtime/p4info_verification_schema.cc

-      return gutil::NotFoundErrorBuilder()
-             << "Table '" << table_name << "' is not a known table.";
+      // Bypass further checks for extension tables
+      continue;


This will have a side effect of bypassing non-ext tables as well, can we strengthen the check here ?

This change was done at last minute due to latest change that was brought in just before this PR. Based on my understanding this should not bypass checks for any tables supported by schema. However, if there are other ways to handle, I am open for suggestion.

Suppose a new P4 table (non-ext) is added but does not exist in the supported schema, then this error condition gets ignored now.
I think you need to identify if its an EXT table before looking up into supported_schema_map.tables.

the ones which are not part of supported schema are considered extension tables. We don't want to enforce on naming convention or a specific set of tables as extension tables. It is expected that p4 programmer knows set of sai.p4 tables and thus knows that anything outside of that is considered as an extension table. Thus it is intended that a new table if does not exist in the supported schema, to be considered as extension table.

Thank you Kishan for your inputs. We did not want to restrict the P4 user to make changes to table/object names by adding ext. Current design takes any name the user wants and is more generic.
Any other suggestions you may have that are not blocking for now, we would like to discuss them with you in detail and definitely take it up in the next release itself. Hope this is ok with you.
Thanks,
Reshma

As a short term workaround, can we define a static list of ext tables that can be used to see if this is an allowed table or not ? When ext table verification is supported in the next release, we can remove this static list.

kishanps · 2022-11-22T01:34:47Z

p4rt_app/p4runtime/p4info_verification_schema_test.cc

@@ -632,28 +632,6 @@ TEST(IsSupportedBySchemaTest, ReturnsOkWithActionSubset) {
  EXPECT_OK(IsSupportedBySchema(schema_p4info, kSupportedSchema));
 }

-TEST(IsSupportedBySchemaTest, ReturnsErrorForUnknownTable) {


I believe if the IsSupported is strengthened, then this test can be enabled.

sure, that is possible.

p4rt_app/sonic/app_db_manager.cc

p4rt_app/p4runtime/p4runtime_impl.cc

kishanps · 2022-11-22T02:11:11Z

p4rt_app/sonic/app_db_manager.cc

+  json_key["context"] = oss.str();
+
+  std::string key = absl::Substitute("$0:$1",
+                          table::TypeName(table::Type::kTblsDefinitionSet),


Why is this not kExt ?

This is a definition itself. kExt is used for table that holds actual table-entries.

p4rt_app/sonic/app_db_manager.cc

p4rt_app/utils/table_utility.cc

kishanps · 2022-11-22T02:20:34Z

p4rt_app/utils/table_utility.cc

  }
  // TODO: return status failure.
 }

 absl::StatusOr<Type> TypeParse(absl::string_view type_name) {
  if (type_name == "ACL") return Type::kAcl;
  if (type_name == "FIXED") return Type::kFixed;
-  if (type_name == APP_P4RT_ACL_TABLE_DEFINITION_NAME) return Type::kDefinition;
+  if (type_name == "EXT") return Type::kExt;


Related to my other comments in this file, what is the difference between kExt & kTblsDefinitionSet

I hope earlier responses have clarified

kishanps · 2022-11-22T02:21:36Z

p4rt_app/utils/table_utility.cc

+                    table_name) != p4rt_app::table::FixedTables.end()) {
+        return table::Type::kFixed;
+      }
+      return table::Type::kExt;


Replace this logic with substring(table_name) == "EXT" ?

svshah-intel · 2022-11-22T06:06:25Z

Thanks Shitanshu, happy to see this coming along.

Thank you Kishan for taking time for the review and for review comments.

kishanps

Last set of minor comments, we are almost there.

kishanps · 2022-12-05T22:06:02Z

p4rt_app/utils/table_utility_test.cc

@@ -35,6 +35,8 @@ TEST(GetTableType, ReturnsAclForSaiAclAnnotation) {

 TEST(GetTableType, ReturnsFixedForNoAnnotation) {
  pdpi::IrTableDefinition ir_table;
+  google::protobuf::TextFormat::ParseFromString(
+        R"pb(preamble { alias: "router_interface_table"})pb", &ir_table);


The test definition/intent and the addition here is not aligning. The test was meant to ensure that no annotation returns fixed but we are now mandating fixed are part of a defined set.

Before there were only ACL and FIXED tables. This test had assumed that something that does not have "@sai_acl" annotation would mean Fixed only. With support of extension tables, absence of any annotation can't be assumed as Fixed, and hence using one of the fixed tables explicitly here.

kishanps · 2022-12-05T22:06:20Z

p4rt_app/utils/table_utility_test.cc

@@ -45,14 +47,23 @@ TEST(GetTableType, ReturnsFixedForNoAnnotation) {
 TEST(GetTableType, ReturnsFixedForNoSpecialAnnotation) {
  pdpi::IrTableDefinition ir_table;
  google::protobuf::TextFormat::ParseFromString(
-      R"pb(preamble { annotations: "@random()" })pb", &ir_table);
+      R"pb(preamble { alias: "router_interface_table" annotations: "@random()" })pb", &ir_table);


Same as above comment.

p4rt_app/utils/table_utility.cc

zhangyanzhao · 2022-12-06T18:27:59Z

@mint570 can you please help to review this PR? This is required by 202211 release and thanks.

kishanps

Thanks Shitanshu, looks good overall, last set of minor comments.

kishanps · 2022-12-08T01:42:36Z

p4rt_app/utils/table_utility.cc

+                    p4rt_app::table::FixedTables.end(),
+                    table_name) != p4rt_app::table::FixedTables.end()) {
+        return table::Type::kFixed;
+      }


Can we remove this logic given that we are using the table id to identify ext vs fixed table.

This will be required when table id related change is removed. I've kept it so that when extension table validation is added, there will be only one line change that is returning kExt instead of conditionalkExt.

kishanps · 2022-12-08T01:45:30Z

p4rt_app/utils/table_utility.cc

+      // Return kExt table-type conditionally, until extension tables
+      //     validation is in place. Once that is in place,
+      //     return kExt table-type unconditionally
+      return CONDITIONAL_kExt(ir_table);


As a general guideline in this dir, macros are not preferred. Can we inline that check here,
if (id > 0x02000000 & id < 0x03000000)
return kFixed
else if (id > 0x03000000)
return kExt

sure, will change it to inline

kishanps · 2022-12-08T01:46:50Z

p4rt_app/utils/table_utility.cc

 #include "p4_pdpi/utils/annotation_parser.h"
 #include "swss/schema.h"

 namespace p4rt_app {
 namespace table {

+// This is what orch agent understands as fixed tables
+std::vector<std::string> FixedTables {


Lets get rid of this global var, if at all we need it, lets make it a static list of all the fixed tables we have now.

I can make it to static. When-ever in future new fixed table is added, we will need to remember to add that entry here. Hope that is okay.

kishanps · 2022-12-08T01:48:01Z

p4rt_app/utils/table_utility.cc

+                table_name) == p4rt_app::table::FixedTables.end()) {
+    p4rt_app::table::FixedTables.push_back(table_name);
+  }
+}


As in the other comment, remove this logic ?

kishanps

One minor comment inline, thanks Shitanshu for working together on this.

kishanps · 2022-12-08T18:26:08Z

p4rt_app/utils/table_utility.cc

+    {
+        return table::Type::kExt;
+    }
+    return table::Type::kFixed;


Given we have already checked valid fixed tables just before this, this is not required here.

I would suggest to move this code inline at the caller place and return InvalidArgument if table_id is not greater than 0x030000.

PINS Extension tables support

244924a

svshah-intel mentioned this pull request Nov 11, 2022

PINS SONiC HLD for Generic SAI Extensions sonic-net/SONiC#1088

Merged

zhangyanzhao requested review from rhalstea and prsunny November 11, 2022 22:22

rhalstea requested a review from kishanps November 14, 2022 16:02

Allow extension tables to bypass supported schema verification

9327603

kishanps reviewed Nov 22, 2022

View reviewed changes

address review comments

7f5aece

zhangyanzhao requested a review from mint570 November 23, 2022 20:05

svshah-intel added 2 commits November 30, 2022 02:47

address review comments

71380de

address review comments

9b15a04

kishanps reviewed Dec 5, 2022

View reviewed changes

address review comments

939ee98

kishanps reviewed Dec 8, 2022

View reviewed changes

address review comments

6f14838

kishanps approved these changes Dec 8, 2022

View reviewed changes

svshah-intel added 3 commits December 13, 2022 22:08

update pins bazel pointing to swss-common repo including new definitions

13dddab

check zmq dependency for swss-common can be resolved

29846de

couple of additional lib dependencies for swss-common

5ea55c8

prsunny merged commit eded2a4 into sonic-net:main Jan 9, 2023

rhalstea mentioned this pull request May 3, 2023

[submodule][sonic-pins] Advance sonic-pins submodule sonic-net/sonic-buildimage#14930

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PINS Extension tables support #17

PINS Extension tables support #17

svshah-intel commented Nov 9, 2022

rhalstea commented Nov 14, 2022

kishanps left a comment

kishanps Nov 22, 2022

svshah-intel Nov 22, 2022

kishanps Nov 30, 2022

svshah-intel Nov 30, 2022

reshmaintel Nov 30, 2022 •

edited

Loading

kishanps Dec 7, 2022

kishanps Nov 22, 2022

svshah-intel Nov 22, 2022

kishanps Nov 22, 2022

svshah-intel Nov 22, 2022

kishanps Nov 22, 2022

svshah-intel Nov 22, 2022

kishanps Nov 22, 2022

svshah-intel commented Nov 22, 2022

kishanps left a comment

kishanps Dec 5, 2022

svshah-intel Dec 5, 2022

kishanps Dec 5, 2022

zhangyanzhao commented Dec 6, 2022

kishanps left a comment

kishanps Dec 8, 2022

svshah-intel Dec 8, 2022

kishanps Dec 8, 2022

svshah-intel Dec 8, 2022

kishanps Dec 8, 2022

svshah-intel Dec 8, 2022

kishanps Dec 8, 2022

kishanps left a comment

kishanps Dec 8, 2022

PINS Extension tables support #17

PINS Extension tables support #17

Conversation

svshah-intel commented Nov 9, 2022

rhalstea commented Nov 14, 2022

kishanps left a 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

reshmaintel Nov 30, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svshah-intel commented Nov 22, 2022

kishanps left a 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

zhangyanzhao commented Dec 6, 2022

kishanps left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishanps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reshmaintel Nov 30, 2022 •

edited

Loading