From 5e792c81a71057b57893654a384713ee8a31df21 Mon Sep 17 00:00:00 2001 From: VSuryaprasad-hcl <159443973+VSuryaprasad-HCL@users.noreply.github.com> Date: Tue, 19 Nov 2024 17:36:58 +0000 Subject: [PATCH] [Comb] Add support for tunnel encap.Align SAI P4 naming more closely to SAI, step 2/6.Add route_metadata field and initializing logic to reflect the changes in SAI-P4 code.Remove L3Admit workaround (#733) --------- Co-authored-by: kishanps Co-authored-by: rhalstea Co-authored-by: smolkaj Co-authored-by: kheradmandG --- .github/workflows/ci.yml | 8 ++++- p4_symbolic/sai/deparser.cc | 2 ++ p4_symbolic/sai/deparser_test.cc | 2 +- p4_symbolic/sai/fields.cc | 29 +++++++++++++++++++ p4_symbolic/sai/fields.h | 7 ++++- p4_symbolic/sai/fields_test.cc | 2 +- p4_symbolic/sai/parser.cc | 7 ++++- p4_symbolic/sai/sai_test.cc | 2 +- p4_symbolic/symbolic/expected/table_hit_1.txt | 8 ++--- p4_symbolic/symbolic/test.bzl | 6 +++- p4_symbolic/tests/BUILD.bazel | 1 + p4_symbolic/tests/sai_p4_component_test.cc | 10 +++++++ .../p4info_verification_schema_test.cc | 2 +- .../p4runtime/p4info_verification_test.cc | 2 +- .../tests/forwarding_pipeline_config_test.cc | 2 +- p4rt_app/tests/p4_programs_test.cc | 2 +- tests/gnoi/BUILD.bazel | 1 - 17 files changed, 77 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 82a22a0d..430ae81a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,13 @@ on: push: branches: [ main ] pull_request: - branches: [ main ] + branches: [ master ] + schedule: + # Run hourly, at minute 0. This is when the cache key changes, and + # so this will ensure the new cache entry is generated on the master branch, + # making CI fast on all branches. + # https://crontab.guru/#0_*_*_*_* + - cron: "0 * * * *" jobs: build: diff --git a/p4_symbolic/sai/deparser.cc b/p4_symbolic/sai/deparser.cc index 3e584ac9..31dd1461 100644 --- a/p4_symbolic/sai/deparser.cc +++ b/p4_symbolic/sai/deparser.cc @@ -226,6 +226,8 @@ absl::StatusOr SaiDeparser(const SaiFields& packet, RETURN_IF_ERROR(Deparse(packet.headers.erspan_ipv4, model, result)); RETURN_IF_ERROR(Deparse(packet.headers.erspan_gre, model, result)); RETURN_IF_ERROR(Deparse(packet.headers.ethernet, model, result)); + RETURN_IF_ERROR(Deparse(packet.headers.tunnel_encap_ipv6, model, result)); + RETURN_IF_ERROR(Deparse(packet.headers.tunnel_encap_gre, model, result)); RETURN_IF_ERROR(Deparse(packet.headers.ipv4, model, result)); RETURN_IF_ERROR(Deparse(packet.headers.ipv6, model, result)); RETURN_IF_ERROR(Deparse(packet.headers.udp, model, result)); diff --git a/p4_symbolic/sai/deparser_test.cc b/p4_symbolic/sai/deparser_test.cc index 09276a13..2c82f249 100644 --- a/p4_symbolic/sai/deparser_test.cc +++ b/p4_symbolic/sai/deparser_test.cc @@ -192,7 +192,7 @@ TEST_P(SaiDeparserTest, IcmpPacketParserIntegrationTest) { } INSTANTIATE_TEST_SUITE_P(Instantiation, SaiDeparserTest, - testing::ValuesIn(sai::AllInstantiations())); + testing::ValuesIn(sai::AllSaiInstantiations())); } // namespace } // namespace p4_symbolic diff --git a/p4_symbolic/sai/fields.cc b/p4_symbolic/sai/fields.cc index da10e17f..a35821ce 100644 --- a/p4_symbolic/sai/fields.cc +++ b/p4_symbolic/sai/fields.cc @@ -154,6 +154,32 @@ absl::StatusOr GetSaiFields(const SymbolicPerPacketState& state) { .src_addr = get_field("ethernet.src_addr"), .ether_type = get_field("ethernet.ether_type"), }; + auto tunnel_encap_ipv6 = SaiIpv6{ + .valid = get_field("tunnel_encap_ipv6.$valid$"), + .version = get_field("tunnel_encap_ipv6.version"), + .dscp = get_field("tunnel_encap_ipv6.dscp"), + .ecn = get_field("tunnel_encap_ipv6.ecn"), + .flow_label = get_field("tunnel_encap_ipv6.flow_label"), + .payload_length = get_field("tunnel_encap_ipv6.payload_length"), + .next_header = get_field("tunnel_encap_ipv6.next_header"), + .hop_limit = get_field("tunnel_encap_ipv6.hop_limit"), + .src_addr = get_field("tunnel_encap_ipv6.src_addr"), + .dst_addr = get_field("tunnel_encap_ipv6.dst_addr"), + }; + auto tunnel_encap_gre = SaiGre{ + .valid = get_field("tunnel_encap_gre.$valid$"), + .checksum_present = get_field("tunnel_encap_gre.checksum_present"), + .routing_present = get_field("tunnel_encap_gre.routing_present"), + .key_present = get_field("tunnel_encap_gre.key_present"), + .sequence_present = get_field("tunnel_encap_gre.sequence_present"), + .strict_source_route = get_field("tunnel_encap_gre.strict_source_route"), + .recursion_control = get_field("tunnel_encap_gre.recursion_control"), + .acknowledgement_present = + get_field("tunnel_encap_gre.acknowledgement_present"), + .flags = get_field("tunnel_encap_gre.flags"), + .version = get_field("tunnel_encap_gre.version"), + .protocol = get_field("tunnel_encap_gre.protocol"), + }; auto ipv4 = SaiIpv4{ .valid = get_field("ipv4.$valid$"), .version = get_field("ipv4.version"), @@ -229,6 +255,7 @@ absl::StatusOr GetSaiFields(const SymbolicPerPacketState& state) { .l4_dst_port = get_metadata_field("l4_dst_port"), .mirror_session_id_valid = get_metadata_field("mirror_session_id_valid"), .ingress_port = get_metadata_field("ingress_port"), + .route_metadata = get_metadata_field("route_metadata"), }; auto standard_metadata = V1ModelStandardMetadata{ .ingress_port = get_field("standard_metadata.ingress_port"), @@ -249,6 +276,8 @@ absl::StatusOr GetSaiFields(const SymbolicPerPacketState& state) { .erspan_ipv4 = erspan_ipv4, .erspan_gre = erspan_gre, .ethernet = ethernet, + .tunnel_encap_ipv6 = tunnel_encap_ipv6, + .tunnel_encap_gre = tunnel_encap_gre, .ipv4 = ipv4, .ipv6 = ipv6, .udp = udp, diff --git a/p4_symbolic/sai/fields.h b/p4_symbolic/sai/fields.h index afde3f82..b70568f2 100644 --- a/p4_symbolic/sai/fields.h +++ b/p4_symbolic/sai/fields.h @@ -131,8 +131,12 @@ struct SaiHeaders { SaiEthernet erspan_ethernet; SaiIpv4 erspan_ipv4; SaiGre erspan_gre; - SaiEthernet ethernet; + + // Not extracted during parsing. + SaiIpv6 tunnel_encap_ipv6; + SaiGre tunnel_encap_gre; + SaiIpv4 ipv4; SaiIpv6 ipv6; SaiUdp udp; @@ -150,6 +154,7 @@ struct SaiLocalMetadata { z3::expr l4_dst_port; z3::expr mirror_session_id_valid; z3::expr ingress_port; + z3::expr route_metadata; }; // Symbolic version of `struct standard_metadata_t` in v1model.p4 diff --git a/p4_symbolic/sai/fields_test.cc b/p4_symbolic/sai/fields_test.cc index d67e7f51..c8c3b8a3 100644 --- a/p4_symbolic/sai/fields_test.cc +++ b/p4_symbolic/sai/fields_test.cc @@ -32,7 +32,7 @@ namespace p4_symbolic { namespace { TEST(GetSaiFields, CanGetIngressAndEgressFieldsForAllInstantiations) { - for (auto instantiation : sai::AllInstantiations()) { + for (auto instantiation : sai::AllSaiInstantiations()) { const auto config = sai::GetNonstandardForwardingPipelineConfig( instantiation, sai::NonstandardPlatform::kP4Symbolic); ASSERT_OK_AND_ASSIGN( diff --git a/p4_symbolic/sai/parser.cc b/p4_symbolic/sai/parser.cc index ee61c073..ee8e6a57 100644 --- a/p4_symbolic/sai/parser.cc +++ b/p4_symbolic/sai/parser.cc @@ -34,6 +34,8 @@ absl::StatusOr> EvaluateSaiParser( SaiIpv4& erspan_ipv4 = fields.headers.erspan_ipv4; SaiGre& erspan_gre = fields.headers.erspan_gre; SaiEthernet& ethernet = fields.headers.ethernet; + SaiIpv6& tunnel_encap_ipv6 = fields.headers.tunnel_encap_ipv6; + SaiGre& tunnel_encap_gre = fields.headers.tunnel_encap_gre; SaiIpv4& ipv4 = fields.headers.ipv4; SaiIpv6& ipv6 = fields.headers.ipv6; SaiUdp& udp = fields.headers.udp; @@ -49,11 +51,14 @@ absl::StatusOr> EvaluateSaiParser( constraints.push_back(!erspan_ethernet.valid); constraints.push_back(!erspan_ipv4.valid); constraints.push_back(!erspan_gre.valid); - constraints.push_back(local_metadata.admit_to_l3 == bv_true); + constraints.push_back(!tunnel_encap_ipv6.valid); + constraints.push_back(!tunnel_encap_gre.valid); + constraints.push_back(local_metadata.admit_to_l3 == bv_false); constraints.push_back(local_metadata.vrf_id == 0); constraints.push_back(local_metadata.mirror_session_id_valid == bv_false); constraints.push_back(local_metadata.ingress_port == standard_metadata.ingress_port); + constraints.push_back(local_metadata.route_metadata == 0); // `parse_ethernet` state. constraints.push_back(ethernet.valid == Z3Context().bool_val(true)); diff --git a/p4_symbolic/sai/sai_test.cc b/p4_symbolic/sai/sai_test.cc index b1be0bfe..a37d37a5 100644 --- a/p4_symbolic/sai/sai_test.cc +++ b/p4_symbolic/sai/sai_test.cc @@ -36,7 +36,7 @@ namespace { TEST(EvaluateSaiPipeline, SatisfiableForAllInstantiationsWithEmptyEntriesAndPorts) { - for (auto instantiation : sai::AllInstantiations()) { + for (auto instantiation : sai::AllSaiInstantiations()) { const auto config = sai::GetNonstandardForwardingPipelineConfig( instantiation, sai::NonstandardPlatform::kP4Symbolic); ASSERT_OK_AND_ASSIGN( diff --git a/p4_symbolic/symbolic/expected/table_hit_1.txt b/p4_symbolic/symbolic/expected/table_hit_1.txt index 8b0d0026..5c1521ac 100644 --- a/p4_symbolic/symbolic/expected/table_hit_1.txt +++ b/p4_symbolic/symbolic/expected/table_hit_1.txt @@ -3,16 +3,16 @@ Cannot find solution! Finding packet for table MyIngress.t_1 and row 0 Dropped = 0 - standard_metadata.ingress_port = #b000000000 - standard_metadata.egress_spec = #b000000011 + standard_metadata.ingress_port = #b000000001 + standard_metadata.egress_spec = #b000000010 Finding packet for table MyIngress.t_2 and row -1 Dropped = 0 - standard_metadata.ingress_port = #b000000000 + standard_metadata.ingress_port = #b000000001 standard_metadata.egress_spec = #b000000010 Finding packet for table MyIngress.t_2 and row 0 Dropped = 0 - standard_metadata.ingress_port = #b000000000 + standard_metadata.ingress_port = #b000000001 standard_metadata.egress_spec = #b000000011 diff --git a/p4_symbolic/symbolic/test.bzl b/p4_symbolic/symbolic/test.bzl index 8ae621e1..b0b58d53 100644 --- a/p4_symbolic/symbolic/test.bzl +++ b/p4_symbolic/symbolic/test.bzl @@ -105,7 +105,11 @@ def end_to_end_test( ("--entries=$(location %s)" % table_entries if table_entries else ""), ("--debug=$(location %s)" % smt_output_file), ("--port_count=%d" % port_count), - ("&> $(location %s)" % test_output_file), + # Redirect stderr to stdout, so we can capture both. + "2>&1", + # Use `tee` to simultaneously capture output in file yet still print it to stdout, to + # ease debugging. + ("| tee $(location %s)" % test_output_file), ]), ) diff --git a/p4_symbolic/tests/BUILD.bazel b/p4_symbolic/tests/BUILD.bazel index 9bf1f6c3..70a90fd6 100644 --- a/p4_symbolic/tests/BUILD.bazel +++ b/p4_symbolic/tests/BUILD.bazel @@ -5,6 +5,7 @@ package( cc_test( name = "sai_p4_component_test", + timeout = "long", srcs = ["sai_p4_component_test.cc"], deps = [ "//gutil:status_matchers", diff --git a/p4_symbolic/tests/sai_p4_component_test.cc b/p4_symbolic/tests/sai_p4_component_test.cc index 705468c8..dc665564 100644 --- a/p4_symbolic/tests/sai_p4_component_test.cc +++ b/p4_symbolic/tests/sai_p4_component_test.cc @@ -59,6 +59,16 @@ constexpr absl::string_view kTableEntries = R"pb( action { set_nexthop_id { nexthop_id: "nexthop-1" } } } } + entries { + l3_admit_table_entry { + match { + dst_mac { value: "66:55:44:33:22:10" mask: "ff:ff:ff:ff:ff:ff" } + in_port { value: "0x005" } + } + action { admit_to_l3 {} } + priority: 1 + } + } entries { nexthop_table_entry { match { nexthop_id: "nexthop-1" } diff --git a/p4rt_app/p4runtime/p4info_verification_schema_test.cc b/p4rt_app/p4runtime/p4info_verification_schema_test.cc index 45c85f8d..8a82e2be 100644 --- a/p4rt_app/p4runtime/p4info_verification_schema_test.cc +++ b/p4rt_app/p4runtime/p4info_verification_schema_test.cc @@ -895,7 +895,7 @@ TEST_P(GoogleInstantiationTest, SchemaSupportsInstantiation) { INSTANTIATE_TEST_SUITE_P( IsSupportedBySchemaTest, GoogleInstantiationTest, - ValuesIn(sai::AllInstantiations()), + ValuesIn(sai::AllSaiInstantiations()), [](const testing::TestParamInfo& info) { return sai::InstantiationToString(info.param); }); diff --git a/p4rt_app/p4runtime/p4info_verification_test.cc b/p4rt_app/p4runtime/p4info_verification_test.cc index a047196f..a83184f2 100644 --- a/p4rt_app/p4runtime/p4info_verification_test.cc +++ b/p4rt_app/p4runtime/p4info_verification_test.cc @@ -103,7 +103,7 @@ TEST_P(InstantiationTest, SaiP4InfoIsOk) { } INSTANTIATE_TEST_SUITE_P(P4InfoVerificationTest, InstantiationTest, - testing::ValuesIn(sai::AllInstantiations()), + testing::ValuesIn(sai::AllSaiInstantiations()), [](testing::TestParamInfo info) { return sai::InstantiationToString(info.param); }); diff --git a/p4rt_app/tests/forwarding_pipeline_config_test.cc b/p4rt_app/tests/forwarding_pipeline_config_test.cc index fab09353..88d29b89 100644 --- a/p4rt_app/tests/forwarding_pipeline_config_test.cc +++ b/p4rt_app/tests/forwarding_pipeline_config_test.cc @@ -766,7 +766,7 @@ std::string PerConfigTestCaseName( INSTANTIATE_TEST_SUITE_P( ForwardingPipelineConfig, PerConfigTest, - testing::Combine(testing::ValuesIn(sai::AllInstantiations()), + testing::Combine(testing::ValuesIn(sai::AllSaiInstantiations()), testing::ValuesIn(sai::AllStages())), PerConfigTestCaseName); diff --git a/p4rt_app/tests/p4_programs_test.cc b/p4rt_app/tests/p4_programs_test.cc index 66a37612..726b9b90 100644 --- a/p4rt_app/tests/p4_programs_test.cc +++ b/p4rt_app/tests/p4_programs_test.cc @@ -141,7 +141,7 @@ TEST_P(P4ProgramsTest, CompositeUdfFieldsShouldAlwaysUseHexStrings) { INSTANTIATE_TEST_SUITE_P( P4ProgramsTestInstance, P4ProgramsTest, - testing::ValuesIn(sai::AllInstantiations()), + testing::ValuesIn(sai::AllSaiInstantiations()), [](const testing::TestParamInfo& param) { return sai::InstantiationToString(param.param); }); diff --git a/tests/gnoi/BUILD.bazel b/tests/gnoi/BUILD.bazel index d86c11c4..fa89d2b2 100644 --- a/tests/gnoi/BUILD.bazel +++ b/tests/gnoi/BUILD.bazel @@ -39,7 +39,6 @@ cc_library( "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/flags:flag", "@com_google_absl//absl/random", - "@com_github_grpc_grpc//:grpc++", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", "@com_google_googletest//:gtest",