From af248c340a16d4b352c3b1121973a9419925a504 Mon Sep 17 00:00:00 2001 From: Chris Sommers Date: Thu, 21 Jul 2022 15:24:18 -0700 Subject: [PATCH] Fix https://github.com/Azure/DASH/issues/158 - libsai delete operation failure and log msg. THe test for error code was backwards and log message used same write updateType string for all operations. I fixed the compare and used a dynamic enum print method. --- dash-pipeline/SAI/templates/saiapi.cpp.j2 | 2 +- dash-pipeline/SAI/templates/utils.cpp.j2 | 11 +++++++-- dash-pipeline/tests/init_switch/Makefile | 2 +- dash-pipeline/tests/vnet_out/Makefile | 2 +- dash-pipeline/tests/vnet_out/vnet_out.cpp | 29 ++++++++++++++++++++++- 5 files changed, 40 insertions(+), 6 deletions(-) diff --git a/dash-pipeline/SAI/templates/saiapi.cpp.j2 b/dash-pipeline/SAI/templates/saiapi.cpp.j2 index cdbff686b..4c08873c7 100644 --- a/dash-pipeline/SAI/templates/saiapi.cpp.j2 +++ b/dash-pipeline/SAI/templates/saiapi.cpp.j2 @@ -286,7 +286,7 @@ sai_status_t sai_remove_{{ table.name }}( {% endfor %} retCode = MutateTableEntry(matchActionEntry, p4::v1::Update_Type_DELETE); - if (grpc::StatusCode::OK != retCode) { + if (grpc::StatusCode::OK == retCode) { delete matchActionEntry; return 0; } diff --git a/dash-pipeline/SAI/templates/utils.cpp.j2 b/dash-pipeline/SAI/templates/utils.cpp.j2 index 70f5eb577..f28025bbc 100644 --- a/dash-pipeline/SAI/templates/utils.cpp.j2 +++ b/dash-pipeline/SAI/templates/utils.cpp.j2 @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include "p4/v1/p4runtime.grpc.pb.h" @@ -129,7 +130,13 @@ int GetDeviceId() { return deviceId; } +string updateTypeStr(p4::v1::Update_Type updateType) { + const google::protobuf::EnumDescriptor *descriptor = p4::v1::Update_Type_descriptor(); + return descriptor->FindValueByNumber(updateType)->name(); +} + grpc::StatusCode MutateTableEntry(p4::v1::TableEntry *entry, p4::v1::Update_Type updateType) { + p4::v1::WriteRequest request; request.set_device_id(GetDeviceId()); auto update = request.add_updates(); @@ -141,11 +148,11 @@ grpc::StatusCode MutateTableEntry(p4::v1::TableEntry *entry, p4::v1::Update_Type grpc::ClientContext context; grpc::Status status = stub->Write(&context, request, &rep); if (status.ok()) { - LOG("GRPC call Write::add_one_entry OK: "); + LOG("GRPC call Write::" << updateTypeStr(updateType) << " OK" << std::endl); } else { LOG("GRPC ERROR["<< status.error_code() <<"]: " << status.error_message() << ", " << status.error_details()); - LOG("GRPC call Write::add_one_entry ERROR: " << std::endl << entry->ShortDebugString()); + LOG("GRPC call Write::" << updateTypeStr(updateType) << " ERROR: " << std::endl << entry->ShortDebugString()); } //MILIND?? What is this? reference release? memory release? entity->release_table_entry(); diff --git a/dash-pipeline/tests/init_switch/Makefile b/dash-pipeline/tests/init_switch/Makefile index 5e7db3aa4..786fa3f4f 100644 --- a/dash-pipeline/tests/init_switch/Makefile +++ b/dash-pipeline/tests/init_switch/Makefile @@ -1,5 +1,5 @@ all:init_switch -init_switch: init_switch.cpp +init_switch: init_switch.cpp /SAI/lib/libsai.so echo "building $@ ..." g++ \ -I /SAI/lib \ diff --git a/dash-pipeline/tests/vnet_out/Makefile b/dash-pipeline/tests/vnet_out/Makefile index a7c03ce3f..d96e3c6b1 100644 --- a/dash-pipeline/tests/vnet_out/Makefile +++ b/dash-pipeline/tests/vnet_out/Makefile @@ -1,5 +1,5 @@ all:vnet_out -vnet_out: vnet_out.cpp +vnet_out: vnet_out.cpp /SAI/lib/libsai.so echo "building $@ ..." g++ \ -I /SAI/SAI/inc \ diff --git a/dash-pipeline/tests/vnet_out/vnet_out.cpp b/dash-pipeline/tests/vnet_out/vnet_out.cpp index 9f80ec668..7a0e1cc71 100644 --- a/dash-pipeline/tests/vnet_out/vnet_out.cpp +++ b/dash-pipeline/tests/vnet_out/vnet_out.cpp @@ -9,16 +9,22 @@ extern sai_status_t sai_create_direction_lookup_entry( _In_ const sai_direction_lookup_entry_t *direction_lookup_entry, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list); +extern sai_status_t sai_remove_direction_lookup_entry( + _In_ const sai_direction_lookup_entry_t *direction_lookup_entry); extern sai_status_t sai_create_eni_ether_address_map_entry( _In_ const sai_eni_ether_address_map_entry_t *outbound_eni_lookup_from_vm_entry, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list); +extern sai_status_t sai_remove_eni_ether_address_map_entry( + _In_ const sai_eni_ether_address_map_entry_t *outbound_eni_lookup_from_vm_entry); extern sai_status_t sai_create_outbound_eni_to_vni_entry( _In_ const sai_outbound_eni_to_vni_entry_t *outbound_eni_to_vni_entry, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list); +extern sai_status_t sai_remove_outbound_eni_to_vni_entry( + _In_ const sai_outbound_eni_to_vni_entry_t *outbound_eni_to_vni_entry); extern sai_dash_api_t sai_dash_api_impl; @@ -83,9 +89,30 @@ int main(int argc, char **argv) std::cout << "Failed to create ENI To VNI" << std::endl; return 1; } - attrs.clear(); + // Delete everything in reverse order + status = sai_remove_outbound_eni_to_vni_entry(&e2v); + if (status != SAI_STATUS_SUCCESS) + { + std::cout << "Failed to remove ENI To VNI" << std::endl; + return 1; + } + + status = sai_remove_eni_ether_address_map_entry(&eam); + if (status != SAI_STATUS_SUCCESS) + { + std::cout << "Failed to remove ENI Lookup From VM" << std::endl; + return 1; + } + + status = sai_remove_direction_lookup_entry(&dle); + if (status != SAI_STATUS_SUCCESS) + { + std::cout << "Failed to remove Direction Lookup Entry" << std::endl; + return 1; + } + std::cout << "Done." << std::endl;