From 7711691a24005003badef9eeaae672b33b3c533f Mon Sep 17 00:00:00 2001 From: divyagayathri-hcl <159437886+divyagayathri-hcl@users.noreply.github.com> Date: Tue, 18 Jun 2024 18:06:44 -0700 Subject: [PATCH] Add test to verify invalid P4 constraint behavior & Make fake_sonic_db_table thread-safe. (#206) Co-authored-by: rhalstea Co-authored-by: kishanps --- p4rt_app/sonic/adapters/BUILD.bazel | 2 ++ .../sonic/adapters/fake_sonic_db_table.cc | 19 ++++++++--- p4rt_app/sonic/adapters/fake_sonic_db_table.h | 24 +++++++++----- p4rt_app/tests/BUILD.bazel | 4 +-- .../tests/forwarding_pipeline_config_test.cc | 32 ++++++++++++++++--- 5 files changed, 62 insertions(+), 19 deletions(-) diff --git a/p4rt_app/sonic/adapters/BUILD.bazel b/p4rt_app/sonic/adapters/BUILD.bazel index c51dd79a..22bc9763 100644 --- a/p4rt_app/sonic/adapters/BUILD.bazel +++ b/p4rt_app/sonic/adapters/BUILD.bazel @@ -169,11 +169,13 @@ cc_library( hdrs = ["fake_sonic_db_table.h"], deps = [ "@com_github_google_glog//:glog", + "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", + "@com_google_absl//absl/synchronization", ], ) diff --git a/p4rt_app/sonic/adapters/fake_sonic_db_table.cc b/p4rt_app/sonic/adapters/fake_sonic_db_table.cc index 68fc9e30..a440a8d4 100644 --- a/p4rt_app/sonic/adapters/fake_sonic_db_table.cc +++ b/p4rt_app/sonic/adapters/fake_sonic_db_table.cc @@ -25,6 +25,7 @@ #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" +#include "absl/synchronization/mutex.h" #include "glog/logging.h" namespace p4rt_app { @@ -34,6 +35,7 @@ void FakeSonicDbTable::InsertTableEntry(const std::string &key, const SonicDbEntryList &values) { VLOG(1) << absl::StreamFormat("'%s' insert table entry: %s", debug_table_name_, key); + absl::WriterMutexLock lock(&entries_mutex_); auto &entry = entries_[key]; for (const auto &[field, data] : values) { entry.insert_or_assign(field, data); @@ -43,6 +45,7 @@ void FakeSonicDbTable::InsertTableEntry(const std::string &key, void FakeSonicDbTable::DeleteTableEntry(const std::string &key) { VLOG(1) << absl::StreamFormat("'%s' delete table entry: %s", debug_table_name_, key); + absl::WriterMutexLock lock(&entries_mutex_); if (auto iter = entries_.find(key); iter != entries_.end()) { entries_.erase(iter); } @@ -67,6 +70,7 @@ bool FakeSonicDbTable::PushNotification(const std::string &key) { } // If the key exists Insert into the StateDb, otherwise delete. + absl::WriterMutexLock lock(&entries_mutex_); auto entry_iter = entries_.find(key); if (entry_iter != entries_.end()) { InsertStateDbTableEntry(key, entry_iter->second); @@ -125,8 +129,11 @@ absl::StatusOr FakeSonicDbTable::ReadTableEntry( const std::string &key) const { VLOG(1) << absl::StreamFormat("'%s' read table entry: %s", debug_table_name_, key); - if (auto entry = entries_.find(key); entry != entries_.end()) { - return entry->second; + { + absl::ReaderMutexLock lock(&entries_mutex_); + if (auto entry = entries_.find(key); entry != entries_.end()) { + return entry->second; + } } return absl::Status(absl::StatusCode::kNotFound, absl::StrCat("AppDb missing: ", key)); @@ -135,8 +142,11 @@ absl::StatusOr FakeSonicDbTable::ReadTableEntry( std::vector FakeSonicDbTable::GetAllKeys() const { std::vector result; VLOG(1) << absl::StreamFormat("'%s' get all keys.", debug_table_name_); - for (const auto &entry : entries_) { - result.push_back(entry.first); + { + absl::ReaderMutexLock lock(&entries_mutex_); + for (const auto &entry : entries_) { + result.push_back(entry.first); + } } VLOG(2) << absl::StreamFormat("'%s' found keys: %s", debug_table_name_, absl::StrJoin(result, ", ")); @@ -144,6 +154,7 @@ std::vector FakeSonicDbTable::GetAllKeys() const { } void FakeSonicDbTable::DebugState() const { + absl::ReaderMutexLock lock(&entries_mutex_); for (const auto &[key, values] : entries_) { LOG(INFO) << "AppDb entry: " << key; for (const auto &[field, data] : values) { diff --git a/p4rt_app/sonic/adapters/fake_sonic_db_table.h b/p4rt_app/sonic/adapters/fake_sonic_db_table.h index 6a6fbf86..9b008979 100644 --- a/p4rt_app/sonic/adapters/fake_sonic_db_table.h +++ b/p4rt_app/sonic/adapters/fake_sonic_db_table.h @@ -20,8 +20,10 @@ #include #include +#include "absl/base/thread_annotations.h" #include "absl/container/flat_hash_map.h" #include "absl/status/statusor.h" +#include "absl/synchronization/mutex.h" namespace p4rt_app { namespace sonic { @@ -41,8 +43,6 @@ using SonicDbEntryMap = std::unordered_map; // Fakes how the OrchAgent updates AppDb tables. When an entry is inserted the // Orchagent will respond with a notification of success or failure. -// -// This class is NOT thread-safe. class FakeSonicDbTable { public: FakeSonicDbTable(const std::string &table_name = "SonicDb:TABLE") @@ -54,8 +54,10 @@ class FakeSonicDbTable { FakeSonicDbTable(const std::string &table_name, FakeSonicDbTable *state_db) : debug_table_name_(table_name), state_db_(state_db) {} - void InsertTableEntry(const std::string &key, const SonicDbEntryList &values); - void DeleteTableEntry(const std::string &key); + void InsertTableEntry(const std::string &key, const SonicDbEntryList &values) + ABSL_LOCKS_EXCLUDED(entries_mutex_); + void DeleteTableEntry(const std::string &key) + ABSL_LOCKS_EXCLUDED(entries_mutex_); void SetResponseForKey(const std::string &key, const std::string &code, const std::string &message); @@ -68,9 +70,11 @@ class FakeSonicDbTable { void GetNextNotification(std::string &op, std::string &data, SonicDbEntryList &values); - absl::StatusOr ReadTableEntry(const std::string &key) const; + absl::StatusOr ReadTableEntry(const std::string &key) const + ABSL_LOCKS_EXCLUDED(entries_mutex_); - std::vector GetAllKeys() const; + std::vector GetAllKeys() const + ABSL_LOCKS_EXCLUDED(entries_mutex_); // Method should only be used for debug purposes. void DebugState() const; @@ -87,11 +91,15 @@ class FakeSonicDbTable { bool UpdateAppStateDb(const std::string &key); + // Mutex to protect synchronization of entries_. + mutable absl::Mutex entries_mutex_; + // Debug table name is used in log messages to help distinguish messages. - std::string debug_table_name_; + const std::string debug_table_name_; // Current list of DB entries stored in the table. - absl::flat_hash_map entries_; + absl::flat_hash_map entries_ + ABSL_GUARDED_BY(entries_mutex_); // List of notifications the OrchAgent would have generated. One notification // is created per insert, and one is removed per notification check. diff --git a/p4rt_app/tests/BUILD.bazel b/p4rt_app/tests/BUILD.bazel index e985fb96..c33ba65d 100644 --- a/p4rt_app/tests/BUILD.bazel +++ b/p4rt_app/tests/BUILD.bazel @@ -155,20 +155,18 @@ cc_test( "//p4rt_app/p4runtime:p4runtime_impl", "//p4rt_app/tests/lib:app_db_entry_builder", "//p4rt_app/tests/lib:p4runtime_grpc_service", - "//p4rt_app/tests/lib:p4runtime_request_helpers", + "//sai_p4/instantiations/google:clos_stage", "//sai_p4/instantiations/google:instantiations", "//sai_p4/instantiations/google:sai_p4info_cc", "//sai_p4/instantiations/google:sai_p4info_fetcher_cc", "@com_github_google_glog//:glog", "@com_github_p4lang_p4runtime//:p4info_cc_proto", - "@com_github_p4lang_p4runtime//:p4runtime_cc_grpc", "@com_github_p4lang_p4runtime//:p4runtime_cc_proto", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", "@com_google_googletest//:gtest_main", - "@com_google_protobuf//:protobuf", "@sonic_swss_common//:common", ], ) diff --git a/p4rt_app/tests/forwarding_pipeline_config_test.cc b/p4rt_app/tests/forwarding_pipeline_config_test.cc index 37db246d..04b6f9a5 100644 --- a/p4rt_app/tests/forwarding_pipeline_config_test.cc +++ b/p4rt_app/tests/forwarding_pipeline_config_test.cc @@ -11,22 +11,25 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include #include +#include #include #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/ascii.h" +#include "absl/strings/match.h" #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "absl/time/clock.h" #include "absl/time/time.h" #include "glog/logging.h" #include "gmock/gmock.h" -#include "google/protobuf/repeated_field.h" -#include "grpcpp/client_context.h" #include "grpcpp/security/credentials.h" #include "grpcpp/support/status.h" #include "gtest/gtest.h" @@ -35,13 +38,12 @@ #include "gutil/status.h" #include "gutil/status_matchers.h" #include "p4/config/v1/p4info.pb.h" -#include "p4/v1/p4runtime.grpc.pb.h" #include "p4/v1/p4runtime.pb.h" #include "p4_pdpi/p4_runtime_session.h" #include "p4rt_app/p4runtime/p4runtime_impl.h" #include "p4rt_app/tests/lib/app_db_entry_builder.h" #include "p4rt_app/tests/lib/p4runtime_grpc_service.h" -#include "p4rt_app/tests/lib/p4runtime_request_helpers.h" +#include "sai_p4/instantiations/google/clos_stage.h" #include "sai_p4/instantiations/google/instantiations.h" #include "sai_p4/instantiations/google/sai_p4info.h" #include "sai_p4/instantiations/google/sai_p4info_fetcher.h" @@ -653,6 +655,28 @@ TEST_F(ForwardingPipelineConfigTest, StatusIs(absl::StatusCode::kInternal)); } +TEST_F(ForwardingPipelineConfigTest, InvalidP4ConstraintDoesNotGoCritical) { + constexpr absl::string_view kEntryRestrictionStr = "@entry_restriction"; + + // Go through all the tables an replace any P4 constraint annotation with an + // invalid one. + auto p4_info = sai::GetP4Info(sai::Instantiation::kMiddleblock); + for (p4::config::v1::Table& table : *p4_info.mutable_tables()) { + for (std::string& annon : + *table.mutable_preamble()->mutable_annotations()) { + if (absl::StartsWith(annon, kEntryRestrictionStr)) { + annon = absl::StrCat(kEntryRestrictionStr, "(invalid constraint)"); + } + } + } + + ASSERT_THAT( + pdpi::SetMetadataAndSetForwardingPipelineConfig( + p4rt_session_.get(), + SetForwardingPipelineConfigRequest::RECONCILE_AND_COMMIT, p4_info), + StatusIs(absl::StatusCode::kInvalidArgument)); +} + class PerConfigTest : public ForwardingPipelineConfigTest, public testing::WithParamInterface< std::tuple > {};