From e81295491aed73735feba4c178db93024af3c339 Mon Sep 17 00:00:00 2001 From: vvbrcm <56567015+vvbrcm@users.noreply.github.com> Date: Tue, 5 Nov 2024 14:53:39 -0800 Subject: [PATCH 01/15] Adding VRRP_TABLE name in APPL_DB (#927) --- common/schema.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/schema.h b/common/schema.h index 009c3cf34..2bc8ec399 100644 --- a/common/schema.h +++ b/common/schema.h @@ -103,6 +103,8 @@ namespace swss { #define APP_NAPT_POOL_IP_TABLE_NAME "NAPT_POOL_IP_TABLE" #define APP_NAT_DNAT_POOL_TABLE_NAME "NAT_DNAT_POOL_TABLE" +#define APP_VRRP_TABLE_NAME "VRRP_TABLE" + #define APP_STP_VLAN_TABLE_NAME "STP_VLAN_TABLE" #define APP_STP_VLAN_PORT_TABLE_NAME "STP_VLAN_PORT_TABLE" #define APP_STP_VLAN_INSTANCE_TABLE_NAME "STP_VLAN_INSTANCE_TABLE" From acc4805a8c2766a4d200c17fdfaba04b94792274 Mon Sep 17 00:00:00 2001 From: Hua Liu <58683130+liuh-80@users.noreply.github.com> Date: Fri, 8 Nov 2024 09:36:21 +0800 Subject: [PATCH 02/15] Fix the ZMQ producer/consumer table does not update changes to the database issue. (#942) #### Why I did it Fix a race condition bug: when deleting ZmqProducerStateTable immediately after setting/deleting data, the data is sent to ZMQ but not updated in the database. #### How I did it Verify and wait for the data update before the db update thread exits. ##### Work item tracking #### How to verify it Pass all test cases. #### Which release branch to backport (provide reason below if selected) - [ ] 201811 - [ ] 201911 - [ ] 202006 - [ ] 202012 - [ ] 202106 - [ ] 202111 #### Description for the changelog Fix the ZMQ producer/consumer table does not update changes to the database issue. #### Link to config_db schema for YANG module changes #### A picture of a cute animal (not mandatory but encouraged) --- common/asyncdbupdater.cpp | 17 ++++++++++++++++- tests/zmq_state_ut.cpp | 27 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/common/asyncdbupdater.cpp b/common/asyncdbupdater.cpp index 4cf150d9f..cf3d74c5f 100644 --- a/common/asyncdbupdater.cpp +++ b/common/asyncdbupdater.cpp @@ -30,6 +30,7 @@ AsyncDBUpdater::~AsyncDBUpdater() // notify db update thread exit m_dbUpdateDataNotifyCv.notify_all(); m_dbUpdateThread->join(); + SWSS_LOG_DEBUG("AsyncDBUpdater dtor tableName: %s", m_tableName.c_str()); } void AsyncDBUpdater::update(std::shared_ptr pkco) @@ -61,16 +62,30 @@ void AsyncDBUpdater::dbUpdateThread() std::mutex cvMutex; std::unique_lock cvLock(cvMutex); - while (m_runThread) + while (true) { size_t count; count = queueSize(); if (count == 0) { + // Check if there still data in queue before exit + if (!m_runThread) + { + SWSS_LOG_NOTICE("dbUpdateThread for table: %s is exiting", m_tableName.c_str()); + break; + } + // when queue is empty, wait notification, when data come, continue to check queue size again m_dbUpdateDataNotifyCv.wait(cvLock); continue; } + else + { + if (!m_runThread) + { + SWSS_LOG_DEBUG("dbUpdateThread for table: %s still has %d records that need to be sent before exiting", m_tableName.c_str(), (int)count); + } + } for (size_t ie = 0; ie < count; ie++) { diff --git a/tests/zmq_state_ut.cpp b/tests/zmq_state_ut.cpp index 4818b7fd8..56a8299f9 100644 --- a/tests/zmq_state_ut.cpp +++ b/tests/zmq_state_ut.cpp @@ -438,3 +438,30 @@ TEST(ZmqConsumerStateTableBatchBufferOverflow, test) } EXPECT_ANY_THROW(p.send(kcos)); } + +TEST(ZmqProducerStateTableDeleteAfterSend, test) +{ + std::string testTableName = "ZMQ_PROD_DELETE_UT"; + std::string pushEndpoint = "tcp://localhost:1234"; + std::string pullEndpoint = "tcp://*:1234"; + std::string testKey = "testKey"; + + ZmqServer server(pullEndpoint); + + DBConnector db(TEST_DB, 0, true); + ZmqClient client(pushEndpoint); + + auto *p = new ZmqProducerStateTable(&db, testTableName, client, true); + std::vector values; + FieldValueTuple t("test", "test"); + values.push_back(t); + p->set(testKey,values); + delete p; + + sleep(1); + + Table table(&db, testTableName); + std::vector keys; + table.getKeys(keys); + EXPECT_EQ(keys.front(), testKey); +} From dc75f2335b6f18b4f82986b19f8a5b65e3456790 Mon Sep 17 00:00:00 2001 From: erer1243 <1377477+erer1243@users.noreply.github.com> Date: Sat, 9 Nov 2024 03:26:28 -0500 Subject: [PATCH 03/15] C-api: refine interface on Selectable classes (#945) Previously, methods like hasData/hasCachedData were exposed through the FFI, but they are only useful for swss::Select which is not exposed, so those were deleted. Methods *_getFd were added so that ffi code may select on those fds separately from swss::Select (we need this in Rust for tokio::AsyncFd). Comments were added to better explain the intended use - Either call readData with a timeout to block in place, or use getFd to select on the fd and then call readData with a zero timeout to reset the fd. Co-authored-by: erer1243 --- common/c-api/consumerstatetable.cpp | 9 +++++++ common/c-api/consumerstatetable.h | 11 ++++++++ common/c-api/subscriberstatetable.cpp | 17 ++++-------- common/c-api/subscriberstatetable.h | 16 +++++------- common/c-api/util.h | 10 +++++-- common/c-api/zmqconsumerstatetable.cpp | 23 +++++----------- common/c-api/zmqconsumerstatetable.h | 18 ++++++------- tests/c_api_ut.cpp | 36 +++++++++----------------- 8 files changed, 67 insertions(+), 73 deletions(-) diff --git a/common/c-api/consumerstatetable.cpp b/common/c-api/consumerstatetable.cpp index c01ed8229..df0aba112 100644 --- a/common/c-api/consumerstatetable.cpp +++ b/common/c-api/consumerstatetable.cpp @@ -32,3 +32,12 @@ SWSSKeyOpFieldValuesArray SWSSConsumerStateTable_pops(SWSSConsumerStateTable tbl return makeKeyOpFieldValuesArray(vkco); }); } + +uint32_t SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl) { + SWSSTry(return numeric_cast(((ConsumerStateTable *)tbl)->getFd())); +} + +SWSSSelectResult SWSSConsumerStateTable_readData(SWSSConsumerStateTable tbl, uint32_t timeout_ms, + uint8_t interrupt_on_signal) { + SWSSTry(return selectOne((ConsumerStateTable *)tbl, timeout_ms, interrupt_on_signal)); +} diff --git a/common/c-api/consumerstatetable.h b/common/c-api/consumerstatetable.h index bd2fdaaf0..468fb644b 100644 --- a/common/c-api/consumerstatetable.h +++ b/common/c-api/consumerstatetable.h @@ -21,6 +21,17 @@ void SWSSConsumerStateTable_free(SWSSConsumerStateTable tbl); // Result array and all of its members must be freed using free() SWSSKeyOpFieldValuesArray SWSSConsumerStateTable_pops(SWSSConsumerStateTable tbl); +// Return the underlying fd for polling/selecting on. +// Callers must NOT read/write on fd, it may only be used for epoll or similar. +// After the fd becomes readable, SWSSConsumerStateTable_readData must be used to +// reset the fd and read data into internal data structures. +uint32_t SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl); + +// Block until data is available to read or until a timeout elapses. +// A timeout of 0 means the call will return immediately. +SWSSSelectResult SWSSConsumerStateTable_readData(SWSSConsumerStateTable tbl, uint32_t timeout_ms, + uint8_t interrupt_on_signal); + #ifdef __cplusplus } #endif diff --git a/common/c-api/subscriberstatetable.cpp b/common/c-api/subscriberstatetable.cpp index b64829117..8bafa3903 100644 --- a/common/c-api/subscriberstatetable.cpp +++ b/common/c-api/subscriberstatetable.cpp @@ -34,19 +34,12 @@ SWSSKeyOpFieldValuesArray SWSSSubscriberStateTable_pops(SWSSSubscriberStateTable }); } -uint8_t SWSSSubscriberStateTable_hasData(SWSSSubscriberStateTable tbl) { - SWSSTry(return ((SubscriberStateTable *)tbl)->hasData() ? 1 : 0); -} - -uint8_t SWSSSubscriberStateTable_hasCachedData(SWSSSubscriberStateTable tbl) { - SWSSTry(return ((SubscriberStateTable *)tbl)->hasCachedData() ? 1 : 0); -} - -uint8_t SWSSSubscriberStateTable_initializedWithData(SWSSSubscriberStateTable tbl) { - SWSSTry(return ((SubscriberStateTable *)tbl)->initializedWithData() ? 1 : 0); +uint32_t SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl) { + SWSSTry(return numeric_cast(((SubscriberStateTable *)tbl)->getFd())); } SWSSSelectResult SWSSSubscriberStateTable_readData(SWSSSubscriberStateTable tbl, - uint32_t timeout_ms) { - SWSSTry(return selectOne((SubscriberStateTable *)tbl, timeout_ms)); + uint32_t timeout_ms, + uint8_t interrupt_on_signal) { + SWSSTry(return selectOne((SubscriberStateTable *)tbl, timeout_ms, interrupt_on_signal)); } diff --git a/common/c-api/subscriberstatetable.h b/common/c-api/subscriberstatetable.h index 4501a3af4..ed0924c81 100644 --- a/common/c-api/subscriberstatetable.h +++ b/common/c-api/subscriberstatetable.h @@ -22,19 +22,17 @@ void SWSSSubscriberStateTable_free(SWSSSubscriberStateTable tbl); // Result array and all of its members must be freed using free() SWSSKeyOpFieldValuesArray SWSSSubscriberStateTable_pops(SWSSSubscriberStateTable tbl); -// Returns 0 for false, 1 for true -uint8_t SWSSSubscriberStateTable_hasData(SWSSSubscriberStateTable tbl); - -// Returns 0 for false, 1 for true -uint8_t SWSSSubscriberStateTable_hasCachedData(SWSSSubscriberStateTable tbl); - -// Returns 0 for false, 1 for true -uint8_t SWSSSubscriberStateTable_initializedWithData(SWSSSubscriberStateTable tbl); +// Return the underlying fd for polling/selecting on. +// Callers must NOT read/write on fd, it may only be used for epoll or similar. +// After the fd becomes readable, SWSSSubscriberStateTable_readData must be used to +// reset the fd and read data into internal data structures. +uint32_t SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl); // Block until data is available to read or until a timeout elapses. // A timeout of 0 means the call will return immediately. SWSSSelectResult SWSSSubscriberStateTable_readData(SWSSSubscriberStateTable tbl, - uint32_t timeout_ms); + uint32_t timeout_ms, + uint8_t interrupt_on_sugnal); #ifdef __cplusplus } diff --git a/common/c-api/util.h b/common/c-api/util.h index 79eb93cfd..357818cbe 100644 --- a/common/c-api/util.h +++ b/common/c-api/util.h @@ -29,9 +29,14 @@ typedef struct { const SWSSKeyOpFieldValues *data; } SWSSKeyOpFieldValuesArray; +// FFI version of swss::Select::{OBJECT, TIMEOUT, SIGNALINT}. +// swss::Select::ERROR is left out because errors are handled separately typedef enum { + // Data is available in the object SWSSSelectResult_DATA = 0, + // Timed out waiting for data SWSSSelectResult_TIMEOUT = 1, + // Waiting was interrupted by a signal SWSSSelectResult_SIGNAL = 2, } SWSSSelectResult; @@ -74,11 +79,12 @@ extern bool cApiTestingDisableAbort; } \ } -static inline SWSSSelectResult selectOne(swss::Selectable *s, uint32_t timeout_ms) { +static inline SWSSSelectResult selectOne(swss::Selectable *s, uint32_t timeout_ms, + uint8_t interrupt_on_signal) { Select select; Selectable *sOut; select.addSelectable(s); - int ret = select.select(&sOut, numeric_cast(timeout_ms)); + int ret = select.select(&sOut, numeric_cast(timeout_ms), interrupt_on_signal); switch (ret) { case Select::OBJECT: return SWSSSelectResult_DATA; diff --git a/common/c-api/zmqconsumerstatetable.cpp b/common/c-api/zmqconsumerstatetable.cpp index 38cd87f93..62b0fe221 100644 --- a/common/c-api/zmqconsumerstatetable.cpp +++ b/common/c-api/zmqconsumerstatetable.cpp @@ -3,6 +3,7 @@ #include "util.h" #include "zmqconsumerstatetable.h" #include "zmqserver.h" +#include using namespace swss; using namespace std; @@ -32,24 +33,14 @@ SWSSKeyOpFieldValuesArray SWSSZmqConsumerStateTable_pops(SWSSZmqConsumerStateTab }); } -SWSSSelectResult SWSSZmqConsumerStateTable_readData(SWSSZmqConsumerStateTable tbl, - uint32_t timeout_ms) { - SWSSTry(return selectOne((ZmqConsumerStateTable *)tbl, timeout_ms)); -} - -// Returns 0 for false, 1 for true -uint8_t SWSSZmqConsumerStateTable_hasData(SWSSZmqConsumerStateTable tbl) { - SWSSTry(return ((ZmqConsumerStateTable *)tbl)->hasData() ? 1 : 0); +uint32_t SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl) { + SWSSTry(return numeric_cast(((ZmqConsumerStateTable *)tbl)->getFd())); } -// Returns 0 for false, 1 for true -uint8_t SWSSZmqConsumerStateTable_hasCachedData(SWSSZmqConsumerStateTable tbl) { - SWSSTry(return ((ZmqConsumerStateTable *)tbl)->hasCachedData() ? 1 : 0); -} - -// Returns 0 for false, 1 for true -uint8_t SWSSZmqConsumerStateTable_initializedWithData(SWSSZmqConsumerStateTable tbl) { - SWSSTry(return ((ZmqConsumerStateTable *)tbl)->initializedWithData() ? 1 : 0); +SWSSSelectResult SWSSZmqConsumerStateTable_readData(SWSSZmqConsumerStateTable tbl, + uint32_t timeout_ms, + uint8_t interrupt_on_signal) { + SWSSTry(return selectOne((ZmqConsumerStateTable *)tbl, timeout_ms, interrupt_on_signal)); } const struct SWSSDBConnectorOpaque * diff --git a/common/c-api/zmqconsumerstatetable.h b/common/c-api/zmqconsumerstatetable.h index 4810c3ef5..f5b934258 100644 --- a/common/c-api/zmqconsumerstatetable.h +++ b/common/c-api/zmqconsumerstatetable.h @@ -24,19 +24,17 @@ void SWSSZmqConsumerStateTable_free(SWSSZmqConsumerStateTable tbl); // Result array and all of its members must be freed using free() SWSSKeyOpFieldValuesArray SWSSZmqConsumerStateTable_pops(SWSSZmqConsumerStateTable tbl); +// Return the underlying fd for polling/selecting on. +// Callers must NOT read/write on fd, it may only be used for epoll or similar. +// After the fd becomes readable, SWSSZmqConsumerStateTable_readData must be used to +// reset the fd and read data into internal data structures. +uint32_t SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl); + // Block until data is available to read or until a timeout elapses. // A timeout of 0 means the call will return immediately. SWSSSelectResult SWSSZmqConsumerStateTable_readData(SWSSZmqConsumerStateTable tbl, - uint32_t timeout_ms); - -// Returns 0 for false, 1 for true -uint8_t SWSSZmqConsumerStateTable_hasData(SWSSZmqConsumerStateTable tbl); - -// Returns 0 for false, 1 for true -uint8_t SWSSZmqConsumerStateTable_hasCachedData(SWSSZmqConsumerStateTable tbl); - -// Returns 0 for false, 1 for true -uint8_t SWSSZmqConsumerStateTable_initializedWithData(SWSSZmqConsumerStateTable tbl); + uint32_t timeout_ms, + uint8_t interrupt_on_signal); const struct SWSSDBConnectorOpaque * SWSSZmqConsumerStateTable_getDbConnector(SWSSZmqConsumerStateTable tbl); diff --git a/tests/c_api_ut.cpp b/tests/c_api_ut.cpp index d16dac7c1..90af97162 100644 --- a/tests/c_api_ut.cpp +++ b/tests/c_api_ut.cpp @@ -94,6 +94,8 @@ TEST(c_api, ConsumerProducerStateTables) { SWSSProducerStateTable pst = SWSSProducerStateTable_new(db, "mytable"); SWSSConsumerStateTable cst = SWSSConsumerStateTable_new(db, "mytable", nullptr, nullptr); + SWSSConsumerStateTable_getFd(cst); + SWSSKeyOpFieldValuesArray arr = SWSSConsumerStateTable_pops(cst); ASSERT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); @@ -110,6 +112,7 @@ TEST(c_api, ConsumerProducerStateTables) { values.len = 1; SWSSProducerStateTable_set(pst, "mykey2", values); + ASSERT_EQ(SWSSConsumerStateTable_readData(cst, 300, true), SWSSSelectResult_DATA); arr = SWSSConsumerStateTable_pops(cst); vector kfvs = takeKeyOpFieldValuesArray(arr); sortKfvs(kfvs); @@ -131,7 +134,7 @@ TEST(c_api, ConsumerProducerStateTables) { ASSERT_EQ(fieldValues1.size(), 1); EXPECT_EQ(fieldValues1[0].first, "myfield3"); EXPECT_EQ(fieldValues1[0].second, "myvalue3"); - + arr = SWSSConsumerStateTable_pops(cst); EXPECT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); @@ -168,23 +171,20 @@ TEST(c_api, SubscriberStateTable) { SWSSDBConnector db = SWSSDBConnector_new_named("TEST_DB", 1000, true); SWSSSubscriberStateTable sst = SWSSSubscriberStateTable_new(db, "mytable", nullptr, nullptr); - EXPECT_EQ(SWSSSubscriberStateTable_readData(sst, 300), SWSSSelectResult_TIMEOUT); - EXPECT_FALSE(SWSSSubscriberStateTable_hasData(sst)); + SWSSSubscriberStateTable_getFd(sst); + + EXPECT_EQ(SWSSSubscriberStateTable_readData(sst, 300, true), SWSSSelectResult_TIMEOUT); SWSSKeyOpFieldValuesArray arr = SWSSSubscriberStateTable_pops(sst); EXPECT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); SWSSDBConnector_hset(db, "mytable:mykey", "myfield", "myvalue"); - EXPECT_EQ(SWSSSubscriberStateTable_readData(sst, 300), SWSSSelectResult_DATA); - EXPECT_EQ(SWSSSubscriberStateTable_readData(sst, 300), SWSSSelectResult_TIMEOUT); - EXPECT_TRUE(SWSSSubscriberStateTable_hasData(sst)); - + ASSERT_EQ(SWSSSubscriberStateTable_readData(sst, 300, true), SWSSSelectResult_DATA); arr = SWSSSubscriberStateTable_pops(sst); vector kfvs = takeKeyOpFieldValuesArray(arr); sortKfvs(kfvs); freeKeyOpFieldValuesArray(arr); - EXPECT_FALSE(SWSSSubscriberStateTable_hasData(sst)); ASSERT_EQ(kfvs.size(), 1); EXPECT_EQ(kfvKey(kfvs[0]), "mykey"); EXPECT_EQ(kfvOp(kfvs[0]), "SET"); @@ -211,11 +211,10 @@ TEST(c_api, ZmqConsumerProducerStateTable) { SWSSZmqConsumerStateTable cst = SWSSZmqConsumerStateTable_new(db, "mytable", srv, nullptr, nullptr); + SWSSZmqConsumerStateTable_getFd(cst); + ASSERT_EQ(SWSSZmqConsumerStateTable_getDbConnector(cst), db); - EXPECT_FALSE(SWSSZmqConsumerStateTable_hasData(cst)); - EXPECT_FALSE(SWSSZmqConsumerStateTable_hasCachedData(cst)); - EXPECT_FALSE(SWSSZmqConsumerStateTable_initializedWithData(cst)); SWSSKeyOpFieldValuesArray arr = SWSSZmqConsumerStateTable_pops(cst); ASSERT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); @@ -247,13 +246,8 @@ TEST(c_api, ZmqConsumerProducerStateTable) { else SWSSZmqClient_sendMsg(cli, "TEST_DB", "mytable", &arr); - ASSERT_EQ(SWSSZmqConsumerStateTable_readData(cst, 500), SWSSSelectResult_DATA); - EXPECT_TRUE(SWSSZmqConsumerStateTable_hasData(cst)); - EXPECT_TRUE(SWSSZmqConsumerStateTable_hasCachedData(cst)); + ASSERT_EQ(SWSSZmqConsumerStateTable_readData(cst, 1500, true), SWSSSelectResult_DATA); arr = SWSSZmqConsumerStateTable_pops(cst); - EXPECT_FALSE(SWSSZmqConsumerStateTable_hasData(cst)); - EXPECT_FALSE(SWSSZmqConsumerStateTable_hasCachedData(cst)); - EXPECT_EQ(SWSSZmqConsumerStateTable_readData(cst, 500), SWSSSelectResult_TIMEOUT); vector kfvs = takeKeyOpFieldValuesArray(arr); sortKfvs(kfvs); @@ -276,7 +270,6 @@ TEST(c_api, ZmqConsumerProducerStateTable) { EXPECT_EQ(fieldValues1[0].first, "myfield3"); EXPECT_EQ(fieldValues1[0].second, "myvalue3"); - EXPECT_FALSE(SWSSZmqConsumerStateTable_hasData(cst)); arr = SWSSZmqConsumerStateTable_pops(cst); ASSERT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); @@ -291,13 +284,8 @@ TEST(c_api, ZmqConsumerProducerStateTable) { else SWSSZmqClient_sendMsg(cli, "TEST_DB", "mytable", &arr); - ASSERT_EQ(SWSSZmqConsumerStateTable_readData(cst, 500), SWSSSelectResult_DATA); - EXPECT_TRUE(SWSSZmqConsumerStateTable_hasData(cst)); - EXPECT_TRUE(SWSSZmqConsumerStateTable_hasCachedData(cst)); + ASSERT_EQ(SWSSZmqConsumerStateTable_readData(cst, 500, true), SWSSSelectResult_DATA); arr = SWSSZmqConsumerStateTable_pops(cst); - EXPECT_FALSE(SWSSZmqConsumerStateTable_hasData(cst)); - EXPECT_FALSE(SWSSZmqConsumerStateTable_hasCachedData(cst)); - EXPECT_EQ(SWSSZmqConsumerStateTable_readData(cst, 500), SWSSSelectResult_TIMEOUT); kfvs = takeKeyOpFieldValuesArray(arr); sortKfvs(kfvs); From b686bb0942a20b3b2832baec91019dcd81a397a8 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Sat, 9 Nov 2024 10:31:19 +0200 Subject: [PATCH 04/15] Add helper function to validate interface name length (#931) Signed-off-by: Stepan Blyschak Co-authored-by: afeigin --- common/Makefile.am | 1 + common/interface.h | 19 +++++++++++++++++++ pyext/swsscommon.i | 4 +++- tests/test_interface.py | 8 ++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 common/interface.h create mode 100644 tests/test_interface.py diff --git a/common/Makefile.am b/common/Makefile.am index df41c3be1..724805e60 100644 --- a/common/Makefile.am +++ b/common/Makefile.am @@ -69,6 +69,7 @@ common_libswsscommon_la_SOURCES = \ common/zmqserver.cpp \ common/asyncdbupdater.cpp \ common/redis_table_waiter.cpp \ + common/interface.h \ common/c-api/util.cpp \ common/c-api/dbconnector.cpp \ common/c-api/consumerstatetable.cpp \ diff --git a/common/interface.h b/common/interface.h new file mode 100644 index 000000000..320ac883a --- /dev/null +++ b/common/interface.h @@ -0,0 +1,19 @@ +#ifndef __INTERFACE__ +#define __INTERFACE__ + +#include +#include + +namespace swss +{ + +const size_t IFACE_NAME_MAX_LEN = IFNAMSIZ - 1; + +bool isInterfaceNameValid(const std::string &ifaceName) +{ + return !ifaceName.empty() && (ifaceName.length() < IFNAMSIZ); +} + +} + +#endif diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index 2bf953b11..b3d015e03 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -58,6 +58,7 @@ #include "zmqproducerstatetable.h" #include #include +#include "interface.h" %} %include @@ -282,6 +283,7 @@ T castSelectableObj(swss::Selectable *temp) %include "zmqserver.h" %include "zmqclient.h" %include "zmqconsumerstatetable.h" +%include "interface.h" %extend swss::DBConnector { %template(hgetall) hgetall>; @@ -296,7 +298,7 @@ T castSelectableObj(swss::Selectable *temp) %include "table.h" #ifdef ENABLE_YANG_MODULES %include "decoratortable.h" -#endif +#endif %clear std::vector &keys; %clear std::vector &ops; %clear std::vector>> &fvss; diff --git a/tests/test_interface.py b/tests/test_interface.py new file mode 100644 index 000000000..25c809ce3 --- /dev/null +++ b/tests/test_interface.py @@ -0,0 +1,8 @@ +from swsscommon import swsscommon + +def test_is_interface_name_valid(): + invalid_interface_name = "TooLongInterfaceName" + assert not swsscommon.isInterfaceNameValid(invalid_interface_name) + + validInterfaceName = "OkInterfaceName" + assert swsscommon.isInterfaceNameValid(validInterfaceName) From 1408db8bb0c5cbf00c787167793cbeff4c47e6c6 Mon Sep 17 00:00:00 2001 From: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Date: Sat, 9 Nov 2024 17:30:22 +0800 Subject: [PATCH 05/15] Add more information when connect redis fail (#882) Co-authored-by: Liat Grozovik <44433539+liat-grozovik@users.noreply.github.com> Co-authored-by: Guohan Lu --- common/dbconnector.cpp | 4 ++-- tests/redis_ut.cpp | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 96334780f..47fe80d3b 100755 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -562,7 +562,7 @@ void RedisContext::initContext(const char *host, int port, const timeval *tv) if (m_conn->err) throw system_error(make_error_code(errc::address_not_available), - "Unable to connect to redis"); + "Unable to connect to redis - " + std::string(m_conn->errstr) + "(" + std::to_string(m_conn->err) + ")"); } void RedisContext::initContext(const char *path, const timeval *tv) @@ -578,7 +578,7 @@ void RedisContext::initContext(const char *path, const timeval *tv) if (m_conn->err) throw system_error(make_error_code(errc::address_not_available), - "Unable to connect to redis (unix-socket)"); + "Unable to connect to redis (unix-socket) - " + std::string(m_conn->errstr) + "(" + std::to_string(m_conn->err) + ")"); } redisContext *RedisContext::getContext() const diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index 4f691e88a..f53f891d4 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include "gtest/gtest.h" #include "common/dbconnector.h" #include "common/producertable.h" @@ -20,6 +22,7 @@ using namespace std; using namespace swss; +using namespace testing; #define NUMBER_OF_THREADS (64) // Spawning more than 256 threads causes libc++ to except #define NUMBER_OF_OPS (1000) @@ -1139,3 +1142,32 @@ TEST(Connector, hmset) // test empty multi hash db.hmset({}); } + +TEST(Connector, connectFail) +{ + // connect to an ip which is not a redis server + EXPECT_THROW({ + try + { + DBConnector db(0, "1.1.1.1", 6379, 1); + } + catch(const std::system_error& e) + { + EXPECT_THAT(e.what(), HasSubstr("Unable to connect to redis - ")); + throw; + } + }, std::system_error); + + // connect to an invalid unix socket address + EXPECT_THROW({ + try + { + DBConnector db(0, "/tmp/invalid", 1); + } + catch(const std::system_error& e) + { + EXPECT_THAT(e.what(), HasSubstr("Unable to connect to redis (unix-socket) - ")); + throw; + } + }, std::system_error); +} From f6c1614227f25dfa81ab5ccd0cb8cca265aaad7d Mon Sep 17 00:00:00 2001 From: divyagayathri-hcl <159437886+divyagayathri-hcl@users.noreply.github.com> Date: Sat, 9 Nov 2024 15:03:35 +0530 Subject: [PATCH 06/15] Add function ActionSchemaByNameAndObjectType to allow different ActionSchema formats. (#909) Summary: Add function ActionSchemaByNameAndObjectType to allow different ActionSchema formats. The SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT action redirects to different object types, which have different formats. Multicast groups are hex strings, while next hops and ports are regular strings. --- common/saiaclschema.cpp | 27 +++++++++++++++++++++++++++ common/saiaclschema.h | 4 ++++ tests/saiaclschema_ut.cpp | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+) diff --git a/common/saiaclschema.cpp b/common/saiaclschema.cpp index 6fd32214d..88c6f5175 100644 --- a/common/saiaclschema.cpp +++ b/common/saiaclschema.cpp @@ -328,5 +328,32 @@ const ActionSchema &ActionSchemaByName(const std::string &action_name) return lookup->second; } +const ActionSchema& ActionSchemaByNameAndObjectType( + const std::string& action_name, const std::string& object_type) { + static const auto* const kRedirectObjectTypes = + new std::unordered_map({ + {"SAI_OBJECT_TYPE_IPMC_GROUP", + {.format = Format::kHexString, .bitwidth = 16}}, + {"SAI_OBJECT_TYPE_L2MC_GROUP", + {.format = Format::kHexString, .bitwidth = 16}}, + // SAI_OBJECT_TYPE_BRIDGE_PORT + // SAI_OBJECT_TYPE_LAG + // SAI_OBJECT_TYPE_NEXT_HOP + // SAI_OBJECT_TYPE_NEXT_HOP_GROUP + // SAI_OBJECT_TYPE_PORT + // SAI_OBJECT_TYPE_SYSTEM_PORT + }); + + if (action_name == "SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT") { + auto lookup = kRedirectObjectTypes->find(object_type); + if (lookup != kRedirectObjectTypes->end()) { + return lookup->second; + } + } + // If we haven't defined the object type, fall through to the default + // SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT format. + return ActionSchemaByName(action_name); +} + } // namespace acl } // namespace swss diff --git a/common/saiaclschema.h b/common/saiaclschema.h index 156148b14..88e664232 100644 --- a/common/saiaclschema.h +++ b/common/saiaclschema.h @@ -83,6 +83,10 @@ const MatchFieldSchema &MatchFieldSchemaByName(const std::string &match_field_na // Throws std::invalid_argument for unknown actions and actions without schemas. const ActionSchema &ActionSchemaByName(const std::string &action_name); +// Allow further format differentiation based on a SAI object type. +const ActionSchema& ActionSchemaByNameAndObjectType( + const std::string& action_name, const std::string& object_type); + } // namespace acl } // namespace swss diff --git a/tests/saiaclschema_ut.cpp b/tests/saiaclschema_ut.cpp index fff9158d5..1f828f77b 100644 --- a/tests/saiaclschema_ut.cpp +++ b/tests/saiaclschema_ut.cpp @@ -60,6 +60,37 @@ TEST(SaiAclSchemaTest, ActionSchemaByNameSucceeds) AllOf(Field(&ActionSchema::format, Format::kHexString), Field(&ActionSchema::bitwidth, 12))); } +TEST(SaiAclSchemaTest, ActionSchemaByNameAndObjectTypeSucceeds) { + EXPECT_THAT( + ActionSchemaByNameAndObjectType("SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT", + "SAI_OBJECT_TYPE_IPMC_GROUP"), + AllOf(Field(&ActionSchema::format, Format::kHexString), + Field(&ActionSchema::bitwidth, 16))); + EXPECT_THAT( + ActionSchemaByNameAndObjectType("SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT", + "SAI_OBJECT_TYPE_L2MC_GROUP"), + AllOf(Field(&ActionSchema::format, Format::kHexString), + Field(&ActionSchema::bitwidth, 16))); + EXPECT_THAT( + ActionSchemaByNameAndObjectType("SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT", + "SAI_OBJECT_TYPE_NEXT_HOP"), + AllOf(Field(&ActionSchema::format, Format::kString), + Field(&ActionSchema::bitwidth, 0))); + EXPECT_THAT(ActionSchemaByNameAndObjectType( + "SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT", "SAI_OBJECT_TYPE_PORT"), + AllOf(Field(&ActionSchema::format, Format::kString), + Field(&ActionSchema::bitwidth, 0))); +} + +TEST(SaiAclSchemaTest, + ActionSchemaByNameAndObjectTypeWithNonRedirectActionSucceeds) { + EXPECT_THAT( + ActionSchemaByNameAndObjectType("SAI_ACL_ENTRY_ATTR_ACTION_DECREMENT_TTL", + "SAI_OBJECT_TYPE_UNKNOWN"), + AllOf(Field(&ActionSchema::format, Format::kHexString), + Field(&ActionSchema::bitwidth, 1))); +} + // Invalid Lookup Tests TEST(SaiAclSchemaTest, InvalidFormatNameThrowsException) @@ -82,6 +113,11 @@ TEST(SaiAclSchemaTest, InvalidActionNameThrowsException) EXPECT_THROW(ActionSchemaByName("Foo"), std::invalid_argument); } +TEST(SaiAclSchemaTest, InvalidActionNameAndObjectTypeThrowsException) { + EXPECT_THROW(ActionSchemaByNameAndObjectType("Foo", "unknown"), + std::invalid_argument); +} + } // namespace } // namespace acl } // namespace swss From 11e4055ae3fcb8872253b5e9df7d75e74cbe6cd1 Mon Sep 17 00:00:00 2001 From: Yevhen Fastiuk Date: Mon, 11 Nov 2024 18:35:29 +0200 Subject: [PATCH 07/15] Add LOGGING table (#783) Signed-off-by: Yevhen Fastiuk Co-authored-by: Saikrishna Arcot --- common/schema.h | 1 + 1 file changed, 1 insertion(+) diff --git a/common/schema.h b/common/schema.h index 2bc8ec399..e616f1282 100644 --- a/common/schema.h +++ b/common/schema.h @@ -460,6 +460,7 @@ namespace swss { #define CFG_TWAMP_SESSION_TABLE_NAME "TWAMP_SESSION" #define CFG_BANNER_MESSAGE_TABLE_NAME "BANNER_MESSAGE" +#define CFG_LOGGING_TABLE_NAME "LOGGING" #define CFG_DHCP_TABLE "DHCP_RELAY" From 378e82818165d75aa2fdc66961e72c4852eae69b Mon Sep 17 00:00:00 2001 From: erer1243 <1377477+erer1243@users.noreply.github.com> Date: Thu, 14 Nov 2024 00:45:32 -0500 Subject: [PATCH 08/15] C API: improve performance by not memcpy-ing string DB values across ffi boundary (#935) This introduces a couple of new FFI types, representing owned C++ strings and arrays. We use those owned strings for database values, so they do not need to be memcpy'd across ffi boundaries. This is in response to Riff's concerns that database values which hamgrd will be reading may be large and expensive to memcpy. Partner pr to sonic-net/sonic-dash-ha#11 Co-authored-by: erer1243 --- common/c-api/consumerstatetable.cpp | 2 + common/c-api/dbconnector.cpp | 29 +++-- common/c-api/dbconnector.h | 57 ++------ common/c-api/producerstatetable.cpp | 2 +- common/c-api/producerstatetable.h | 5 - common/c-api/subscriberstatetable.cpp | 2 + common/c-api/util.cpp | 30 +++++ common/c-api/util.h | 172 ++++++++++++++++++------- common/c-api/zmqclient.cpp | 4 +- common/c-api/zmqclient.h | 2 +- common/c-api/zmqconsumerstatetable.cpp | 2 + common/c-api/zmqproducerstatetable.cpp | 3 + tests/c_api_ut.cpp | 84 +++++++----- 13 files changed, 251 insertions(+), 143 deletions(-) diff --git a/common/c-api/consumerstatetable.cpp b/common/c-api/consumerstatetable.cpp index df0aba112..9765ceec3 100644 --- a/common/c-api/consumerstatetable.cpp +++ b/common/c-api/consumerstatetable.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -10,6 +11,7 @@ using namespace swss; using namespace std; +using boost::numeric_cast; SWSSConsumerStateTable SWSSConsumerStateTable_new(SWSSDBConnector db, const char *tableName, const int32_t *p_popBatchSize, diff --git a/common/c-api/dbconnector.cpp b/common/c-api/dbconnector.cpp index bb32f42aa..83f237cc0 100644 --- a/common/c-api/dbconnector.cpp +++ b/common/c-api/dbconnector.cpp @@ -1,5 +1,6 @@ #include #include +#include #include "../dbconnector.h" #include "dbconnector.h" @@ -37,14 +38,14 @@ int8_t SWSSDBConnector_del(SWSSDBConnector db, const char *key) { SWSSTry(return ((DBConnector *)db)->del(string(key)) ? 1 : 0); } -void SWSSDBConnector_set(SWSSDBConnector db, const char *key, const char *value) { - SWSSTry(((DBConnector *)db)->set(string(key), string(value))); +void SWSSDBConnector_set(SWSSDBConnector db, const char *key, SWSSStrRef value) { + SWSSTry(((DBConnector *)db)->set(string(key), takeStrRef(value))); } -char *SWSSDBConnector_get(SWSSDBConnector db, const char *key) { +SWSSString SWSSDBConnector_get(SWSSDBConnector db, const char *key) { SWSSTry({ shared_ptr s = ((DBConnector *)db)->get(string(key)); - return s ? strdup(s->c_str()) : nullptr; + return s ? makeString(move(*s)) : nullptr; }); } @@ -57,21 +58,29 @@ int8_t SWSSDBConnector_hdel(SWSSDBConnector db, const char *key, const char *fie } void SWSSDBConnector_hset(SWSSDBConnector db, const char *key, const char *field, - const char *value) { - SWSSTry(((DBConnector *)db)->hset(string(key), string(field), string(value))); + SWSSStrRef value) { + SWSSTry(((DBConnector *)db)->hset(string(key), string(field), takeStrRef(value))); } -char *SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field) { +SWSSString SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field) { SWSSTry({ shared_ptr s = ((DBConnector *)db)->hget(string(key), string(field)); - return s ? strdup(s->c_str()) : nullptr; + return s ? makeString(move(*s)) : nullptr; }); } SWSSFieldValueArray SWSSDBConnector_hgetall(SWSSDBConnector db, const char *key) { SWSSTry({ - auto map = ((DBConnector *)db)->hgetall(key); - return makeFieldValueArray(map); + auto map = ((DBConnector *)db)->hgetall(string(key)); + + // We can't move keys out of the map, we have to copy them, until C++17 map::extract so we + // copy them here into a vector to avoid needing an overload on makeFieldValueArray + vector> pairs; + pairs.reserve(map.size()); + for (auto &pair : map) + pairs.push_back(make_pair(pair.first, move(pair.second))); + + return makeFieldValueArray(std::move(pairs)); }); } diff --git a/common/c-api/dbconnector.h b/common/c-api/dbconnector.h index 8e6c51e0b..fe4acdf4d 100644 --- a/common/c-api/dbconnector.h +++ b/common/c-api/dbconnector.h @@ -29,11 +29,11 @@ void SWSSDBConnector_free(SWSSDBConnector db); // Returns 0 when key doesn't exist, 1 when key was deleted int8_t SWSSDBConnector_del(SWSSDBConnector db, const char *key); -void SWSSDBConnector_set(SWSSDBConnector db, const char *key, const char *value); +void SWSSDBConnector_set(SWSSDBConnector db, const char *key, SWSSStrRef value); -// Returns NULL if key doesn't exist. -// Result must be freed using free() -char *SWSSDBConnector_get(SWSSDBConnector db, const char *key); +// Returns NULL if key doesn't exist +// Result must be freed using SWSSString_free() +SWSSString SWSSDBConnector_get(SWSSDBConnector db, const char *key); // Returns 0 for false, 1 for true int8_t SWSSDBConnector_exists(SWSSDBConnector db, const char *key); @@ -41,59 +41,22 @@ int8_t SWSSDBConnector_exists(SWSSDBConnector db, const char *key); // Returns 0 when key or field doesn't exist, 1 when field was deleted int8_t SWSSDBConnector_hdel(SWSSDBConnector db, const char *key, const char *field); -void SWSSDBConnector_hset(SWSSDBConnector db, const char *key, const char *field, - const char *value); +void SWSSDBConnector_hset(SWSSDBConnector db, const char *key, const char *field, SWSSStrRef value); -// Returns NULL if key or field doesn't exist. -// Result must be freed using free() -char *SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field); +// Returns NULL if key or field doesn't exist +// Result must be freed using SWSSString_free() +SWSSString SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field); -// Returns an empty map when the key doesn't exist. -// Result array and all of its elements must be freed using free() +// Returns an empty map when the key doesn't exist +// Result array and all of its elements must be freed using appropriate free functions SWSSFieldValueArray SWSSDBConnector_hgetall(SWSSDBConnector db, const char *key); // Returns 0 when key or field doesn't exist, 1 when field exists int8_t SWSSDBConnector_hexists(SWSSDBConnector db, const char *key, const char *field); -// std::vector keys(const std::string &key); - -// std::pair> scan(int cursor = 0, const char -// *match = "", uint32_t count = 10); - -// template -// void hmset(const std::string &key, InputIterator start, InputIterator stop); - -// void hmset(const std::unordered_map>>& multiHash); - -// std::shared_ptr get(const std::string &key); - -// std::shared_ptr hget(const std::string &key, const std::string -// &field); - -// int64_t incr(const std::string &key); - -// int64_t decr(const std::string &key); - -// int64_t rpush(const std::string &list, const std::string &item); - -// std::shared_ptr blpop(const std::string &list, int timeout); - -// void subscribe(const std::string &pattern); - -// void psubscribe(const std::string &pattern); - -// void punsubscribe(const std::string &pattern); - -// int64_t publish(const std::string &channel, const std::string &message); - -// void config_set(const std::string &key, const std::string &value); - // Returns 1 on success, 0 on failure int8_t SWSSDBConnector_flushdb(SWSSDBConnector db); -// std::map>> getall(); #ifdef __cplusplus } #endif diff --git a/common/c-api/producerstatetable.cpp b/common/c-api/producerstatetable.cpp index 083536d7e..276d7c680 100644 --- a/common/c-api/producerstatetable.cpp +++ b/common/c-api/producerstatetable.cpp @@ -25,7 +25,7 @@ void SWSSProducerStateTable_setBuffered(SWSSProducerStateTable tbl, uint8_t buff void SWSSProducerStateTable_set(SWSSProducerStateTable tbl, const char *key, SWSSFieldValueArray values) { - SWSSTry(((ProducerStateTable *)tbl)->set(string(key), takeFieldValueArray(values))); + SWSSTry(((ProducerStateTable *)tbl)->set(string(key), takeFieldValueArray(std::move(values)))); } void SWSSProducerStateTable_del(SWSSProducerStateTable tbl, const char *key) { diff --git a/common/c-api/producerstatetable.h b/common/c-api/producerstatetable.h index e8db2c65d..1acb9af37 100644 --- a/common/c-api/producerstatetable.h +++ b/common/c-api/producerstatetable.h @@ -22,11 +22,6 @@ void SWSSProducerStateTable_set(SWSSProducerStateTable tbl, const char *key, SWS void SWSSProducerStateTable_del(SWSSProducerStateTable tbl, const char *key); -// Batched version of set() and del(). -// virtual void set(const std::vector& values); - -// virtual void del(const std::vector& keys); - void SWSSProducerStateTable_flush(SWSSProducerStateTable tbl); int64_t SWSSProducerStateTable_count(SWSSProducerStateTable tbl); diff --git a/common/c-api/subscriberstatetable.cpp b/common/c-api/subscriberstatetable.cpp index 8bafa3903..4d3a04953 100644 --- a/common/c-api/subscriberstatetable.cpp +++ b/common/c-api/subscriberstatetable.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -11,6 +12,7 @@ using namespace swss; using namespace std; +using boost::numeric_cast; SWSSSubscriberStateTable SWSSSubscriberStateTable_new(SWSSDBConnector db, const char *tableName, const int32_t *p_popBatchSize, diff --git a/common/c-api/util.cpp b/common/c-api/util.cpp index fb983d5cf..1dc6cd451 100644 --- a/common/c-api/util.cpp +++ b/common/c-api/util.cpp @@ -1,3 +1,33 @@ #include "util.h" +using namespace swss; + bool swss::cApiTestingDisableAbort = false; + +SWSSString SWSSString_new(const char *data, uint64_t length) { + SWSSTry(return makeString(std::string(data, numeric_cast(length)))); +} + +SWSSString SWSSString_new_c_str(const char *c_str) { + SWSSTry(return makeString(std::string(c_str))); +} + +const char *SWSSStrRef_c_str(SWSSStrRef s) { + SWSSTry(return ((std::string *)s)->c_str()); +} + +uint64_t SWSSStrRef_length(SWSSStrRef s) { + SWSSTry(return ((std::string *)s)->length()); +} + +void SWSSString_free(SWSSString s) { + SWSSTry(delete (std::string *)s); +} + +void SWSSFieldValueArray_free(SWSSFieldValueArray arr) { + SWSSTry(delete[] arr.data); +} + +void SWSSKeyOpFieldValuesArray_free(SWSSKeyOpFieldValuesArray kfvs) { + SWSSTry(delete[] kfvs.data); +} diff --git a/common/c-api/util.h b/common/c-api/util.h index 357818cbe..06aeac15e 100644 --- a/common/c-api/util.h +++ b/common/c-api/util.h @@ -8,25 +8,44 @@ extern "C" { #include +// FFI version of std::string&& +// This can be converted to an SWSSStrRef with a standard cast +typedef struct SWSSStringOpaque *SWSSString; + +// FFI version of std::string& +// This can be converted to an SWSSString with a standard cast +// Functions that take SWSSString will move data out of the underlying string, +// but functions that take SWSSStrRef will only view it. +typedef struct SWSSStrRefOpaque *SWSSStrRef; + +// FFI version of swss::FieldValueTuple typedef struct { const char *field; - const char *value; -} SWSSFieldValuePair; + SWSSString value; +} SWSSFieldValueTuple; +// FFI version of std::vector typedef struct { uint64_t len; - const SWSSFieldValuePair *data; + SWSSFieldValueTuple *data; } SWSSFieldValueArray; +typedef enum { + SWSSKeyOperation_SET, + SWSSKeyOperation_DEL, +} SWSSKeyOperation; + +// FFI version of swss::KeyOpFieldValuesTuple typedef struct { const char *key; - const char *operation; + SWSSKeyOperation operation; SWSSFieldValueArray fieldValues; } SWSSKeyOpFieldValues; +// FFI version of std::vector typedef struct { uint64_t len; - const SWSSKeyOpFieldValues *data; + SWSSKeyOpFieldValues *data; } SWSSKeyOpFieldValuesArray; // FFI version of swss::Select::{OBJECT, TIMEOUT, SIGNALINT}. @@ -40,21 +59,52 @@ typedef enum { SWSSSelectResult_SIGNAL = 2, } SWSSSelectResult; +// data should not include a null terminator +SWSSString SWSSString_new(const char *data, uint64_t length); + +// c_str should include a null terminator +SWSSString SWSSString_new_c_str(const char *c_str); + +// It is safe to pass null to this function (not to any other SWSSString functions). This is +// useful to take SWSSStrings from other SWSS structs - you can replace the strs in the +// structs with null and still safely free the structs. Then, you can call this function with the +// populated SWSSString later. +void SWSSString_free(SWSSString s); + +const char *SWSSStrRef_c_str(SWSSStrRef s); + +// Returns the length of the string, not including the null terminator that is implicitly added by +// SWSSStrRef_c_str. +uint64_t SWSSStrRef_length(SWSSStrRef s); + +// arr.data may be null. This is not recursive - elements must be freed separately (for finer +// grained control of ownership). +void SWSSFieldValueArray_free(SWSSFieldValueArray arr); + +// kfvs.data may be null. This is not recursive - elements must be freed separately (for finer +// grained control of ownership). +void SWSSKeyOpFieldValuesArray_free(SWSSKeyOpFieldValuesArray kfvs); + #ifdef __cplusplus } #endif // Internal utilities (used to help define c-facing functions) #ifdef __cplusplus -#include -#include + +#include +#include #include #include +#include +#include #include #include +#include #include "../logger.h" -#include "../rediscommand.h" +#include "../redisapi.h" +#include "../schema.h" #include "../select.h" using boost::numeric_cast; @@ -67,7 +117,7 @@ extern bool cApiTestingDisableAbort; // undefined behavior. It was also decided that no exceptions in swss-common are recoverable, so // there is no reason to convert exceptions into a returnable type. #define SWSSTry(...) \ - if (cApiTestingDisableAbort) { \ + if (swss::cApiTestingDisableAbort) { \ { __VA_ARGS__; } \ } else { \ try { \ @@ -99,22 +149,21 @@ static inline SWSSSelectResult selectOne(swss::Selectable *s, uint32_t timeout_m } } -// malloc() with safe numeric casting of the size parameter -template static inline void *mallocN(N size) { - return malloc(numeric_cast(size)); +static inline SWSSString makeString(std::string &&s) { + std::string *data_s = new std::string(std::move(s)); + return (struct SWSSStringOpaque *)data_s; } // T is anything that has a .size() method and which can be iterated over for pair -// eg unordered_map or vector> -template static inline SWSSFieldValueArray makeFieldValueArray(const T &in) { - SWSSFieldValuePair *data = - (SWSSFieldValuePair *)mallocN(in.size() * sizeof(SWSSFieldValuePair)); +// eg vector> +template static inline SWSSFieldValueArray makeFieldValueArray(T &&in) { + SWSSFieldValueTuple *data = new SWSSFieldValueTuple[in.size()]; size_t i = 0; - for (const auto &pair : in) { - SWSSFieldValuePair entry; + for (auto &pair : in) { + SWSSFieldValueTuple entry; entry.field = strdup(pair.first.c_str()); - entry.value = strdup(pair.second.c_str()); + entry.value = makeString(std::move(pair.second)); data[i++] = entry; } @@ -124,48 +173,40 @@ template static inline SWSSFieldValueArray makeFieldValueArray(const T return out; } -static inline std::vector -takeFieldValueArray(const SWSSFieldValueArray &in) { - std::vector out; - for (uint64_t i = 0; i < in.len; i++) { - auto field = std::string(in.data[i].field); - auto value = std::string(in.data[i].value); - out.push_back(std::make_pair(field, value)); +static inline SWSSKeyOperation makeKeyOperation(std::string &op) { + if (strcmp(op.c_str(), SET_COMMAND) == 0) { + return SWSSKeyOperation_SET; + } else if (strcmp(op.c_str(), DEL_COMMAND) == 0) { + return SWSSKeyOperation_DEL; + } else { + SWSS_LOG_THROW("Invalid key operation %s", op.c_str()); } - return out; } -static inline SWSSKeyOpFieldValues makeKeyOpFieldValues(const swss::KeyOpFieldsValuesTuple &in) { +static inline SWSSKeyOpFieldValues makeKeyOpFieldValues(swss::KeyOpFieldsValuesTuple &&in) { SWSSKeyOpFieldValues out; out.key = strdup(kfvKey(in).c_str()); - out.operation = strdup(kfvOp(in).c_str()); + out.operation = makeKeyOperation(kfvOp(in)); out.fieldValues = makeFieldValueArray(kfvFieldsValues(in)); return out; } -static inline swss::KeyOpFieldsValuesTuple takeKeyOpFieldValues(const SWSSKeyOpFieldValues &in) { - std::string key(in.key), op(in.operation); - auto fieldValues = takeFieldValueArray(in.fieldValues); - return std::make_tuple(key, op, fieldValues); -} - -template static inline const T &getReference(const T &t) { +template static inline T &getReference(T &t) { return t; } -template static inline const T &getReference(const std::shared_ptr &t) { +template static inline T &getReference(std::shared_ptr &t) { return *t; } // T is anything that has a .size() method and which can be iterated over for -// swss::KeyOpFieldValuesTuple -template static inline SWSSKeyOpFieldValuesArray makeKeyOpFieldValuesArray(const T &in) { - SWSSKeyOpFieldValues *data = - (SWSSKeyOpFieldValues *)mallocN(in.size() * sizeof(SWSSKeyOpFieldValues)); +// swss::KeyOpFieldValuesTuple, eg vector or deque +template static inline SWSSKeyOpFieldValuesArray makeKeyOpFieldValuesArray(T &&in) { + SWSSKeyOpFieldValues *data = new SWSSKeyOpFieldValues[in.size()]; size_t i = 0; - for (const auto &kfv : in) - data[i++] = makeKeyOpFieldValues(getReference(kfv)); + for (auto &kfv : in) + data[i++] = makeKeyOpFieldValues(std::move(getReference(kfv))); SWSSKeyOpFieldValuesArray out; out.len = (uint64_t)in.size(); @@ -173,11 +214,50 @@ template static inline SWSSKeyOpFieldValuesArray makeKeyOpFieldValuesA return out; } +static inline std::string takeString(SWSSString s) { + return std::string(std::move(*((std::string *)s))); +} + +static inline std::string &takeStrRef(SWSSStrRef s) { + return *((std::string *)s); +} + +static inline std::vector takeFieldValueArray(SWSSFieldValueArray in) { + std::vector out; + for (uint64_t i = 0; i < in.len; i++) { + const char *field = in.data[i].field; + SWSSString value = in.data[i].value; + auto pair = std::make_pair(std::string(field), takeString(std::move(value))); + out.push_back(pair); + } + return out; +} + +static inline std::string takeKeyOperation(SWSSKeyOperation op) { + switch (op) { + case SWSSKeyOperation_SET: + return SET_COMMAND; + case SWSSKeyOperation_DEL: + return DEL_COMMAND; + default: + SWSS_LOG_THROW("Impossible SWSSKeyOperation"); + } +} + +static inline swss::KeyOpFieldsValuesTuple takeKeyOpFieldValues(SWSSKeyOpFieldValues in) { + std::string key = in.key; + std::string op = takeKeyOperation(in.operation); + auto fieldValues = takeFieldValueArray(in.fieldValues); + return std::make_tuple(key, op, fieldValues); +} + static inline std::vector -takeKeyOpFieldValuesArray(const SWSSKeyOpFieldValuesArray &in) { +takeKeyOpFieldValuesArray(SWSSKeyOpFieldValuesArray in) { std::vector out; - for (uint64_t i = 0; i < in.len; i++) - out.push_back(takeKeyOpFieldValues(in.data[i])); + for (uint64_t i = 0; i < in.len; i++) { + SWSSKeyOpFieldValues kfv = in.data[i]; + out.push_back(takeKeyOpFieldValues(std::move(kfv))); + } return out; } diff --git a/common/c-api/zmqclient.cpp b/common/c-api/zmqclient.cpp index 7e4a58f87..49a9e05f7 100644 --- a/common/c-api/zmqclient.cpp +++ b/common/c-api/zmqclient.cpp @@ -24,9 +24,9 @@ void SWSSZmqClient_connect(SWSSZmqClient zmqc) { } void SWSSZmqClient_sendMsg(SWSSZmqClient zmqc, const char *dbName, const char *tableName, - const SWSSKeyOpFieldValuesArray *arr) { + SWSSKeyOpFieldValuesArray arr) { SWSSTry({ - vector kcos = takeKeyOpFieldValuesArray(*arr); + vector kcos = takeKeyOpFieldValuesArray(arr); size_t bufSize = BinarySerializer::serializedSize(dbName, tableName, kcos); vector v(bufSize); ((ZmqClient *)zmqc) diff --git a/common/c-api/zmqclient.h b/common/c-api/zmqclient.h index 47cd1efba..da832ab30 100644 --- a/common/c-api/zmqclient.h +++ b/common/c-api/zmqclient.h @@ -21,7 +21,7 @@ int8_t SWSSZmqClient_isConnected(SWSSZmqClient zmqc); void SWSSZmqClient_connect(SWSSZmqClient zmqc); void SWSSZmqClient_sendMsg(SWSSZmqClient zmqc, const char *dbName, const char *tableName, - const SWSSKeyOpFieldValuesArray *kcos); + SWSSKeyOpFieldValuesArray kcos); #ifdef __cplusplus } diff --git a/common/c-api/zmqconsumerstatetable.cpp b/common/c-api/zmqconsumerstatetable.cpp index 62b0fe221..ed416488e 100644 --- a/common/c-api/zmqconsumerstatetable.cpp +++ b/common/c-api/zmqconsumerstatetable.cpp @@ -1,3 +1,4 @@ +#include #include "../zmqconsumerstatetable.h" #include "../table.h" #include "util.h" @@ -7,6 +8,7 @@ using namespace swss; using namespace std; +using boost::numeric_cast; // Pass NULL for popBatchSize and/or pri to use the default values SWSSZmqConsumerStateTable SWSSZmqConsumerStateTable_new(SWSSDBConnector db, const char *tableName, diff --git a/common/c-api/zmqproducerstatetable.cpp b/common/c-api/zmqproducerstatetable.cpp index 3e50916e1..e1c186806 100644 --- a/common/c-api/zmqproducerstatetable.cpp +++ b/common/c-api/zmqproducerstatetable.cpp @@ -1,8 +1,11 @@ +#include + #include "zmqproducerstatetable.h" #include "../zmqproducerstatetable.h" using namespace std; using namespace swss; +using boost::numeric_cast; SWSSZmqProducerStateTable SWSSZmqProducerStateTable_new(SWSSDBConnector db, const char *tableName, SWSSZmqClient zmqc, uint8_t dbPersistence) { diff --git a/tests/c_api_ut.cpp b/tests/c_api_ut.cpp index 90af97162..ed814607e 100644 --- a/tests/c_api_ut.cpp +++ b/tests/c_api_ut.cpp @@ -1,4 +1,3 @@ -#include #include #include @@ -39,44 +38,63 @@ static void sortKfvs(vector &kfvs) { } } -template static void free(const T *ptr) { - std::free(const_cast(reinterpret_cast(ptr))); -} +#define free(x) std::free(const_cast(reinterpret_cast(x))); static void freeKeyOpFieldValuesArray(SWSSKeyOpFieldValuesArray arr) { for (uint64_t i = 0; i < arr.len; i++) { free(arr.data[i].key); - free(arr.data[i].operation); for (uint64_t j = 0; j < arr.data[i].fieldValues.len; j++) { free(arr.data[i].fieldValues.data[j].field); - free(arr.data[i].fieldValues.data[j].value); + SWSSString_free(arr.data[i].fieldValues.data[j].value); } - free(arr.data[i].fieldValues.data); + SWSSFieldValueArray_free(arr.data[i].fieldValues); } - free(arr.data); + SWSSKeyOpFieldValuesArray_free(arr); } +struct SWSSStringManager { + vector m_strings; + + SWSSString makeString(const char *c_str) { + SWSSString s = SWSSString_new_c_str(c_str); + m_strings.push_back(s); + return s; + } + + SWSSStrRef makeStrRef(const char *c_str) { + return (SWSSStrRef)makeString(c_str); + } + + ~SWSSStringManager() { + for (SWSSString s : m_strings) + SWSSString_free(s); + } +}; + TEST(c_api, DBConnector) { clearDB(); + SWSSStringManager sm; EXPECT_THROW(SWSSDBConnector_new_named("does not exist", 0, true), out_of_range); SWSSDBConnector db = SWSSDBConnector_new_named("TEST_DB", 1000, true); - EXPECT_FALSE(SWSSDBConnector_get(db, "mykey")); + EXPECT_EQ(SWSSDBConnector_get(db, "mykey"), nullptr); EXPECT_FALSE(SWSSDBConnector_exists(db, "mykey")); - SWSSDBConnector_set(db, "mykey", "myval"); - const char *val = SWSSDBConnector_get(db, "mykey"); - EXPECT_STREQ(val, "myval"); - free(val); + + SWSSDBConnector_set(db, "mykey", sm.makeStrRef("myval")); + SWSSString val = SWSSDBConnector_get(db, "mykey"); + EXPECT_STREQ(SWSSStrRef_c_str((SWSSStrRef)val), "myval"); + SWSSString_free(val); EXPECT_TRUE(SWSSDBConnector_exists(db, "mykey")); EXPECT_TRUE(SWSSDBConnector_del(db, "mykey")); EXPECT_FALSE(SWSSDBConnector_del(db, "mykey")); EXPECT_FALSE(SWSSDBConnector_hget(db, "mykey", "myfield")); EXPECT_FALSE(SWSSDBConnector_hexists(db, "mykey", "myfield")); - SWSSDBConnector_hset(db, "mykey", "myfield", "myval"); + SWSSDBConnector_hset(db, "mykey", "myfield", sm.makeStrRef("myval")); val = SWSSDBConnector_hget(db, "mykey", "myfield"); - EXPECT_STREQ(val, "myval"); - free(val); + EXPECT_STREQ(SWSSStrRef_c_str((SWSSStrRef)val), "myval"); + SWSSString_free(val); + EXPECT_TRUE(SWSSDBConnector_hexists(db, "mykey", "myfield")); EXPECT_FALSE(SWSSDBConnector_hget(db, "mykey", "notmyfield")); EXPECT_FALSE(SWSSDBConnector_hexists(db, "mykey", "notmyfield")); @@ -89,6 +107,7 @@ TEST(c_api, DBConnector) { TEST(c_api, ConsumerProducerStateTables) { clearDB(); + SWSSStringManager sm; SWSSDBConnector db = SWSSDBConnector_new_named("TEST_DB", 1000, true); SWSSProducerStateTable pst = SWSSProducerStateTable_new(db, "mytable"); @@ -100,15 +119,16 @@ TEST(c_api, ConsumerProducerStateTables) { ASSERT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); - SWSSFieldValuePair data[2] = {{.field = "myfield1", .value = "myvalue1"}, - {.field = "myfield2", .value = "myvalue2"}}; + SWSSFieldValueTuple data[2] = { + {.field = "myfield1", .value = sm.makeString("myvalue1")}, + {.field = "myfield2", .value = sm.makeString("myvalue2")}}; SWSSFieldValueArray values = { .len = 2, .data = data, }; SWSSProducerStateTable_set(pst, "mykey1", values); - data[0] = {.field = "myfield3", .value = "myvalue3"}; + data[0] = {.field = "myfield3", .value = sm.makeString("myvalue3")}; values.len = 1; SWSSProducerStateTable_set(pst, "mykey2", values); @@ -167,6 +187,7 @@ TEST(c_api, ConsumerProducerStateTables) { TEST(c_api, SubscriberStateTable) { clearDB(); + SWSSStringManager sm; SWSSDBConnector db = SWSSDBConnector_new_named("TEST_DB", 1000, true); SWSSSubscriberStateTable sst = SWSSSubscriberStateTable_new(db, "mytable", nullptr, nullptr); @@ -178,8 +199,8 @@ TEST(c_api, SubscriberStateTable) { EXPECT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); - SWSSDBConnector_hset(db, "mytable:mykey", "myfield", "myvalue"); - ASSERT_EQ(SWSSSubscriberStateTable_readData(sst, 300, true), SWSSSelectResult_DATA); + SWSSDBConnector_hset(db, "mytable:mykey", "myfield", sm.makeStrRef("myvalue")); + EXPECT_EQ(SWSSSubscriberStateTable_readData(sst, 300, true), SWSSSelectResult_DATA); arr = SWSSSubscriberStateTable_pops(sst); vector kfvs = takeKeyOpFieldValuesArray(arr); sortKfvs(kfvs); @@ -199,6 +220,7 @@ TEST(c_api, SubscriberStateTable) { TEST(c_api, ZmqConsumerProducerStateTable) { clearDB(); + SWSSStringManager sm; SWSSDBConnector db = SWSSDBConnector_new_named("TEST_DB", 1000, true); @@ -222,29 +244,29 @@ TEST(c_api, ZmqConsumerProducerStateTable) { // On flag = 0, we use the ZmqProducerStateTable // On flag = 1, we use the ZmqClient directly for (int flag = 0; flag < 2; flag++) { - SWSSFieldValuePair values_key1_data[2] = {{.field = "myfield1", .value = "myvalue1"}, - {.field = "myfield2", .value = "myvalue2"}}; + SWSSFieldValueTuple values_key1_data[2] = {{.field = "myfield1", .value = sm.makeString("myvalue1")}, + {.field = "myfield2", .value = sm.makeString("myvalue2")}}; SWSSFieldValueArray values_key1 = { .len = 2, .data = values_key1_data, }; - SWSSFieldValuePair values_key2_data[1] = {{.field = "myfield3", .value = "myvalue3"}}; + SWSSFieldValueTuple values_key2_data[1] = {{.field = "myfield3", .value = sm.makeString("myvalue3")}}; SWSSFieldValueArray values_key2 = { .len = 1, .data = values_key2_data, }; SWSSKeyOpFieldValues arr_data[2] = { - {.key = "mykey1", .operation = "SET", .fieldValues = values_key1}, - {.key = "mykey2", .operation = "SET", .fieldValues = values_key2}}; + {.key = "mykey1", .operation = SWSSKeyOperation_SET, .fieldValues = values_key1}, + {.key = "mykey2", .operation = SWSSKeyOperation_SET, .fieldValues = values_key2}}; arr = {.len = 2, .data = arr_data}; if (flag == 0) for (uint64_t i = 0; i < arr.len; i++) SWSSZmqProducerStateTable_set(pst, arr.data[i].key, arr.data[i].fieldValues); else - SWSSZmqClient_sendMsg(cli, "TEST_DB", "mytable", &arr); + SWSSZmqClient_sendMsg(cli, "TEST_DB", "mytable", arr); ASSERT_EQ(SWSSZmqConsumerStateTable_readData(cst, 1500, true), SWSSSelectResult_DATA); arr = SWSSZmqConsumerStateTable_pops(cst); @@ -274,15 +296,15 @@ TEST(c_api, ZmqConsumerProducerStateTable) { ASSERT_EQ(arr.len, 0); freeKeyOpFieldValuesArray(arr); - arr_data[0] = {.key = "mykey3", .operation = "DEL", .fieldValues = {}}; - arr_data[1] = {.key = "mykey4", .operation = "DEL", .fieldValues = {}}; - arr = { .len = 2, .data = arr_data }; + arr_data[0] = {.key = "mykey3", .operation = SWSSKeyOperation_DEL, .fieldValues = {}}; + arr_data[1] = {.key = "mykey4", .operation = SWSSKeyOperation_DEL, .fieldValues = {}}; + arr = {.len = 2, .data = arr_data}; if (flag == 0) for (uint64_t i = 0; i < arr.len; i++) SWSSZmqProducerStateTable_del(pst, arr.data[i].key); else - SWSSZmqClient_sendMsg(cli, "TEST_DB", "mytable", &arr); + SWSSZmqClient_sendMsg(cli, "TEST_DB", "mytable", arr); ASSERT_EQ(SWSSZmqConsumerStateTable_readData(cst, 500, true), SWSSSelectResult_DATA); arr = SWSSZmqConsumerStateTable_pops(cst); From 901f3b4482b7162848c3dd5536cf494f1039c9ac Mon Sep 17 00:00:00 2001 From: Yijiao Qin Date: Mon, 18 Nov 2024 09:45:53 -0800 Subject: [PATCH 09/15] [common] enable redispipeline to only publish after flush (#895) What I did optimize redispipeline flush performance by remove unnecessary publish commands add a new parameterbool flushPub in producerstatetable constructor function to enable/disable batch publish feature default value of m_flushPub is false, so no impact on existing codes optimization is effective only explicitly set this option remove individual publish command from the producerstatetable APIs' lua scripts add a publish command when the pipeline flushes [if m_flushPub is true] Why I did it save TCP traffic and increase fpmsyncd efficiency It's a feature included in BGP Loading Optimization HLD #1521 --- common/producerstatetable.cpp | 93 +++++++++++++++++++++------------- common/producerstatetable.h | 4 ++ common/redispipeline.h | 44 ++++++++++++++++ tests/redis_piped_state_ut.cpp | 56 ++++++++++++++++++++ 4 files changed, 162 insertions(+), 35 deletions(-) diff --git a/common/producerstatetable.cpp b/common/producerstatetable.cpp index d0db5e2a5..c7a35475e 100644 --- a/common/producerstatetable.cpp +++ b/common/producerstatetable.cpp @@ -14,39 +14,71 @@ using namespace std; namespace swss { ProducerStateTable::ProducerStateTable(DBConnector *db, const string &tableName) - : ProducerStateTable(new RedisPipeline(db, 1), tableName, false) + : ProducerStateTable(new RedisPipeline(db, 1), tableName, false, false) { m_pipeowned = true; } ProducerStateTable::ProducerStateTable(RedisPipeline *pipeline, const string &tableName, bool buffered) + : ProducerStateTable(pipeline, tableName, buffered, false) {} + +ProducerStateTable::ProducerStateTable(RedisPipeline *pipeline, const string &tableName, bool buffered, bool flushPub) : TableBase(tableName, SonicDBConfig::getSeparator(pipeline->getDBConnector())) , TableName_KeySet(tableName) + , m_flushPub(flushPub) , m_buffered(buffered) , m_pipeowned(false) , m_tempViewActive(false) , m_pipe(pipeline) { + reloadRedisScript(); + + string luaClear = + "redis.call('DEL', KEYS[1])\n" + "local keys = redis.call('KEYS', KEYS[2] .. '*')\n" + "for i,k in pairs(keys) do\n" + " redis.call('DEL', k)\n" + "end\n" + "redis.call('DEL', KEYS[3])\n"; + m_shaClear = m_pipe->loadRedisScript(luaClear); + + string luaApplyView = loadLuaScript("producer_state_table_apply_view.lua"); + m_shaApplyView = m_pipe->loadRedisScript(luaApplyView); +} + +ProducerStateTable::~ProducerStateTable() +{ + if (m_pipeowned) + { + delete m_pipe; + } +} + +void ProducerStateTable::reloadRedisScript() +{ + // Set m_flushPub to remove publish from a single lua string and let pipeline do publish once per flush + + // However, if m_buffered is false, follow the original one publish per lua design + // Hence we need to check both m_buffered and m_flushPub, and reload the redis script once setBuffered() changes m_buffered + + /* 1. Inform the pipeline of what channel to publish, when flushPub feature is enabled */ + if (m_buffered && m_flushPub) + m_pipe->addChannel(getChannelName(m_pipe->getDbId())); + + /* 2. Setup lua strings: determine whether to attach luaPub after each lua string */ + // num in luaSet and luaDel means number of elements that were added to the key set, // not including all the elements already present into the set. string luaSet = "local added = redis.call('SADD', KEYS[2], ARGV[2])\n" "for i = 0, #KEYS - 3 do\n" " redis.call('HSET', KEYS[3 + i], ARGV[3 + i * 2], ARGV[4 + i * 2])\n" - "end\n" - " if added > 0 then \n" - " redis.call('PUBLISH', KEYS[1], ARGV[1])\n" "end\n"; - m_shaSet = m_pipe->loadRedisScript(luaSet); string luaDel = "local added = redis.call('SADD', KEYS[2], ARGV[2])\n" "redis.call('SADD', KEYS[4], ARGV[2])\n" - "redis.call('DEL', KEYS[3])\n" - "if added > 0 then \n" - " redis.call('PUBLISH', KEYS[1], ARGV[1])\n" - "end\n"; - m_shaDel = m_pipe->loadRedisScript(luaDel); + "redis.call('DEL', KEYS[3])\n"; string luaBatchedSet = "local added = 0\n" @@ -59,11 +91,7 @@ ProducerStateTable::ProducerStateTable(RedisPipeline *pipeline, const string &ta " redis.call('HSET', KEYS[3] .. KEYS[4 + i], attr, val)\n" " end\n" " idx = idx + tonumber(ARGV[idx]) * 2 + 1\n" - "end\n" - "if added > 0 then \n" - " redis.call('PUBLISH', KEYS[1], ARGV[1])\n" "end\n"; - m_shaBatchedSet = m_pipe->loadRedisScript(luaBatchedSet); string luaBatchedDel = "local added = 0\n" @@ -71,36 +99,31 @@ ProducerStateTable::ProducerStateTable(RedisPipeline *pipeline, const string &ta " added = added + redis.call('SADD', KEYS[2], KEYS[5 + i])\n" " redis.call('SADD', KEYS[3], KEYS[5 + i])\n" " redis.call('DEL', KEYS[4] .. KEYS[5 + i])\n" - "end\n" - "if added > 0 then \n" - " redis.call('PUBLISH', KEYS[1], ARGV[1])\n" "end\n"; - m_shaBatchedDel = m_pipe->loadRedisScript(luaBatchedDel); - string luaClear = - "redis.call('DEL', KEYS[1])\n" - "local keys = redis.call('KEYS', KEYS[2] .. '*')\n" - "for i,k in pairs(keys) do\n" - " redis.call('DEL', k)\n" - "end\n" - "redis.call('DEL', KEYS[3])\n"; - m_shaClear = m_pipe->loadRedisScript(luaClear); - - string luaApplyView = loadLuaScript("producer_state_table_apply_view.lua"); - m_shaApplyView = m_pipe->loadRedisScript(luaApplyView); -} - -ProducerStateTable::~ProducerStateTable() -{ - if (m_pipeowned) + if (!m_flushPub || !m_buffered) { - delete m_pipe; + string luaPub = + "if added > 0 then \n" + " redis.call('PUBLISH', KEYS[1], ARGV[1])\n" + "end\n"; + luaSet += luaPub; + luaDel += luaPub; + luaBatchedSet += luaPub; + luaBatchedDel += luaPub; } + + /* 3. load redis script based on the lua string */ + m_shaSet = m_pipe->loadRedisScript(luaSet); + m_shaDel = m_pipe->loadRedisScript(luaDel); + m_shaBatchedSet = m_pipe->loadRedisScript(luaBatchedSet); + m_shaBatchedDel = m_pipe->loadRedisScript(luaBatchedDel); } void ProducerStateTable::setBuffered(bool buffered) { m_buffered = buffered; + reloadRedisScript(); } void ProducerStateTable::set(const string &key, const vector &values, diff --git a/common/producerstatetable.h b/common/producerstatetable.h index b6fa78684..b00453a5a 100644 --- a/common/producerstatetable.h +++ b/common/producerstatetable.h @@ -12,6 +12,7 @@ class ProducerStateTable : public TableBase, public TableName_KeySet public: ProducerStateTable(DBConnector *db, const std::string &tableName); ProducerStateTable(RedisPipeline *pipeline, const std::string &tableName, bool buffered = false); + ProducerStateTable(RedisPipeline *pipeline, const std::string &tableName, bool buffered, bool flushPub); virtual ~ProducerStateTable(); void setBuffered(bool buffered); @@ -51,6 +52,7 @@ class ProducerStateTable : public TableBase, public TableName_KeySet void apply_temp_view(); private: + bool m_flushPub; // publish per piepeline flush intead of per redis script bool m_buffered; bool m_pipeowned; bool m_tempViewActive; @@ -62,6 +64,8 @@ class ProducerStateTable : public TableBase, public TableName_KeySet std::string m_shaClear; std::string m_shaApplyView; TableDump m_tempViewState; + + void reloadRedisScript(); // redis script may change if m_buffered changes }; } diff --git a/common/redispipeline.h b/common/redispipeline.h index b8efa3840..be7561b6b 100644 --- a/common/redispipeline.h +++ b/common/redispipeline.h @@ -2,7 +2,10 @@ #include #include +#include #include +#include +#include #include "redisreply.h" #include "rediscommand.h" #include "dbconnector.h" @@ -22,9 +25,11 @@ class RedisPipeline { RedisPipeline(const DBConnector *db, size_t sz = 128) : COMMAND_MAX(sz) , m_remaining(0) + , m_shaPub("") { m_db = db->newConnector(NEWCONNECTOR_TIMEOUT); initializeOwnerTid(); + lastHeartBeat = std::chrono::steady_clock::now(); } ~RedisPipeline() { @@ -113,11 +118,19 @@ class RedisPipeline { void flush() { + lastHeartBeat = std::chrono::steady_clock::now(); + + if (m_remaining == 0) { + return; + } + while(m_remaining) { // Construct an object to use its dtor, so that resource is released RedisReply r(pop()); } + + publish(); } size_t size() @@ -145,12 +158,43 @@ class RedisPipeline { m_ownerTid = gettid(); } + void addChannel(std::string channel) + { + if (m_channels.find(channel) != m_channels.end()) + return; + + m_channels.insert(channel); + m_luaPub += "redis.call('PUBLISH', '" + channel + "', 'G');"; + m_shaPub = loadRedisScript(m_luaPub); + } + + int getIdleTime(std::chrono::time_point tcurrent=std::chrono::steady_clock::now()) + { + return static_cast(std::chrono::duration_cast(tcurrent - lastHeartBeat).count()); + } + + void publish() { + if (m_shaPub.empty()) { + return; + } + RedisCommand cmd; + cmd.format( + "EVALSHA %s 0", + m_shaPub.c_str()); + RedisReply r(m_db, cmd); + } + private: DBConnector *m_db; std::queue m_expectedTypes; size_t m_remaining; long int m_ownerTid; + std::string m_luaPub; + std::string m_shaPub; + std::chrono::time_point lastHeartBeat; // marks the timestamp of latest pipeline flush being invoked + std::unordered_set m_channels; + void mayflush() { if (m_remaining >= COMMAND_MAX) diff --git a/tests/redis_piped_state_ut.cpp b/tests/redis_piped_state_ut.cpp index ca3291907..f3173876e 100644 --- a/tests/redis_piped_state_ut.cpp +++ b/tests/redis_piped_state_ut.cpp @@ -730,3 +730,59 @@ TEST(ConsumerStateTable, async_multitable) cout << endl << "Done." << endl; } + +TEST(ConsumerStateTable, flushPub) +{ + clearDB(); + + /* Prepare producer */ + int index = 0; + string tableName = "UT_REDIS_THREAD_" + to_string(index); + DBConnector db(TEST_DB, 0, true); + RedisPipeline pipeline(&db); + ProducerStateTable p(&pipeline, tableName, false, true); + p.setBuffered(true); + + string key = "TheKey"; + int maxNumOfFields = 2; + + /* Set operation */ + { + vector fields; + for (int j = 0; j < maxNumOfFields; j++) + { + FieldValueTuple t(field(j), value(j)); + fields.push_back(t); + } + p.set(key, fields); + } + + /* Del operation */ + p.del(key); + p.flush(); + + /* Prepare consumer */ + ConsumerStateTable c(&db, tableName); + Select cs; + Selectable *selectcs; + cs.addSelectable(&c); + + /* First pop operation */ + { + int ret = cs.select(&selectcs); + EXPECT_EQ(ret, Select::OBJECT); + KeyOpFieldsValuesTuple kco; + c.pop(kco); + EXPECT_EQ(kfvKey(kco), key); + EXPECT_EQ(kfvOp(kco), "DEL"); + + auto fvs = kfvFieldsValues(kco); + EXPECT_EQ(fvs.size(), 0U); + } + + /* Second select operation */ + { + int ret = cs.select(&selectcs, 1000); + EXPECT_EQ(ret, Select::TIMEOUT); + } +} \ No newline at end of file From fe30ccdc567f7bce1a6291318573a70c351c3547 Mon Sep 17 00:00:00 2001 From: Sundara Gurunathan <105081231+sundar-pds@users.noreply.github.com> Date: Tue, 19 Nov 2024 09:43:43 -0800 Subject: [PATCH 10/15] [DASH] Add DASH Meter Policy , Rule , Counter table definitions (#949) * Adding DASH Meter Policy and Rule table definitions * Adding DASH Meter Counter ID list --- common/schema.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/schema.h b/common/schema.h index e616f1282..108fbc8dd 100644 --- a/common/schema.h +++ b/common/schema.h @@ -177,6 +177,8 @@ namespace swss { #define APP_DASH_ROUTE_GROUP_TABLE_NAME "DASH_ROUTE_GROUP_TABLE" #define APP_DASH_TUNNEL_TABLE_NAME "DASH_TUNNEL_TABLE" #define APP_DASH_PA_VALIDATION_TABLE_NAME "DASH_PA_VALIDATION_TABLE" +#define APP_DASH_METER_POLICY_TABLE_NAME "DASH_METER_POLICY_TABLE" +#define APP_DASH_METER_RULE_TABLE_NAME "DASH_METER_RULE_TABLE" #define APP_DASH_ROUTING_APPLIANCE_TABLE_NAME "DASH_ROUTING_APPLIANCE_TABLE" #define APP_PAC_PORT_TABLE_NAME "PAC_PORT_TABLE" @@ -261,6 +263,7 @@ namespace swss { #define QUEUE_ATTR_ID_LIST "QUEUE_ATTR_ID_LIST" #define BUFFER_POOL_COUNTER_ID_LIST "BUFFER_POOL_COUNTER_ID_LIST" #define ENI_COUNTER_ID_LIST "ENI_COUNTER_ID_LIST" +#define DASH_METER_COUNTER_ID_LIST "DASH_METER_COUNTER_ID_LIST" #define PFC_WD_STATE_TABLE "PFC_WD_STATE_TABLE" #define PFC_WD_PORT_COUNTER_ID_LIST "PORT_COUNTER_ID_LIST" #define PFC_WD_QUEUE_COUNTER_ID_LIST "QUEUE_COUNTER_ID_LIST" From ebd2afb0a2946420f6a42ba1f54f8b2c971781be Mon Sep 17 00:00:00 2001 From: Philo <135693886+philo-micas@users.noreply.github.com> Date: Thu, 21 Nov 2024 09:40:18 +0800 Subject: [PATCH 11/15] Supports FRR-VRRP configuration (#813) * Supports FRR-VRRP configuration Signed-off-by: philo * Update schema.h * Update schema.h * Update schema.h * triggle rebuild * triggle rebuild * triggle rebuild * triggle rebuild * triggle rebuild * Update schema.h * triggle rebuild * triggle rebuild --------- Signed-off-by: philo --- common/schema.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/common/schema.h b/common/schema.h index 108fbc8dd..e99f2ad81 100644 --- a/common/schema.h +++ b/common/schema.h @@ -429,7 +429,8 @@ namespace swss { #define CFG_MCLAG_UNIQUE_IP_TABLE_NAME "MCLAG_UNIQUE_IP" #define CFG_PORT_STORM_CONTROL_TABLE_NAME "PORT_STORM_CONTROL" - +#define CFG_VRRP_TABLE_NAME "VRRP" +#define CFG_VRRP6_TABLE_NAME "VRRP6" #define CFG_RATES_TABLE_NAME "RATES" #define CFG_FEATURE_TABLE_NAME "FEATURE" From 6bac82be1884f8e2c7e43aef2c8a9e6ee20c440f Mon Sep 17 00:00:00 2001 From: Hua Liu <58683130+liuh-80@users.noreply.github.com> Date: Mon, 25 Nov 2024 09:53:37 +0800 Subject: [PATCH 12/15] Improve memory usage by move ZMQ serialize buffer from ZmqProducerStateTable to ZmqClient (#955) #### Why I did it Every ZmqProducerStateTable will allocate 16MB buffer, this can be improve by share same buffer in ZmqClient. #### How I did it Improve memory usage by move ZMQ serialize buffer from ZmqProducerStateTable to ZmqClient. ##### Work item tracking #### How to verify it Pass all test cases. #### Which release branch to backport (provide reason below if selected) - [ ] 201811 - [ ] 201911 - [ ] 202006 - [ ] 202012 - [ ] 202106 - [ ] 202111 #### Description for the changelog Improve memory usage by move ZMQ serialize buffer from ZmqProducerStateTable to ZmqClient. #### Link to config_db schema for YANG module changes #### A picture of a cute animal (not mandatory but encouraged) --- common/c-api/zmqclient.cpp | 4 +--- common/zmqclient.cpp | 10 +++++----- common/zmqclient.h | 5 +++-- common/zmqproducerstatetable.cpp | 17 +++++------------ common/zmqproducerstatetable.h | 2 -- 5 files changed, 14 insertions(+), 24 deletions(-) diff --git a/common/c-api/zmqclient.cpp b/common/c-api/zmqclient.cpp index 49a9e05f7..fa1d59ca2 100644 --- a/common/c-api/zmqclient.cpp +++ b/common/c-api/zmqclient.cpp @@ -27,9 +27,7 @@ void SWSSZmqClient_sendMsg(SWSSZmqClient zmqc, const char *dbName, const char *t SWSSKeyOpFieldValuesArray arr) { SWSSTry({ vector kcos = takeKeyOpFieldValuesArray(arr); - size_t bufSize = BinarySerializer::serializedSize(dbName, tableName, kcos); - vector v(bufSize); ((ZmqClient *)zmqc) - ->sendMsg(string(dbName), string(tableName), kcos, v); + ->sendMsg(string(dbName), string(tableName), kcos); }); } diff --git a/common/zmqclient.cpp b/common/zmqclient.cpp index 0225d4374..5a84160e9 100644 --- a/common/zmqclient.cpp +++ b/common/zmqclient.cpp @@ -51,6 +51,7 @@ void ZmqClient::initialize(const std::string& endpoint, const std::string& vrf) m_context = nullptr; m_socket = nullptr; m_vrf = vrf; + m_sendbuffer.resize(MQ_RESPONSE_MAX_COUNT); connect(); } @@ -116,12 +117,11 @@ void ZmqClient::connect() void ZmqClient::sendMsg( const std::string& dbName, const std::string& tableName, - const std::vector& kcos, - std::vector& sendbuffer) + const std::vector& kcos) { int serializedlen = (int)BinarySerializer::serializeBuffer( - sendbuffer.data(), - sendbuffer.size(), + m_sendbuffer.data(), + m_sendbuffer.size(), dbName, tableName, kcos); @@ -144,7 +144,7 @@ void ZmqClient::sendMsg( std::lock_guard lock(m_socketMutex); // Use none block mode to use all bandwidth: http://api.zeromq.org/2-1%3Azmq-send - rc = zmq_send(m_socket, sendbuffer.data(), serializedlen, ZMQ_NOBLOCK); + rc = zmq_send(m_socket, m_sendbuffer.data(), serializedlen, ZMQ_NOBLOCK); } if (rc >= 0) diff --git a/common/zmqclient.h b/common/zmqclient.h index 313e65735..adc36b053 100644 --- a/common/zmqclient.h +++ b/common/zmqclient.h @@ -22,8 +22,7 @@ class ZmqClient void sendMsg(const std::string& dbName, const std::string& tableName, - const std::vector& kcos, - std::vector& sendbuffer); + const std::vector& kcos); private: void initialize(const std::string& endpoint, const std::string& vrf); @@ -38,6 +37,8 @@ class ZmqClient bool m_connected; std::mutex m_socketMutex; + + std::vector m_sendbuffer; }; } diff --git a/common/zmqproducerstatetable.cpp b/common/zmqproducerstatetable.cpp index ec9396b39..e2a31446b 100644 --- a/common/zmqproducerstatetable.cpp +++ b/common/zmqproducerstatetable.cpp @@ -38,8 +38,6 @@ ZmqProducerStateTable::ZmqProducerStateTable(RedisPipeline *pipeline, const stri void ZmqProducerStateTable::initialize(DBConnector *db, const std::string &tableName, bool dbPersistence) { - m_sendbuffer.resize(MQ_RESPONSE_MAX_COUNT); - if (dbPersistence) { SWSS_LOG_DEBUG("Database persistence enabled, tableName: %s", tableName.c_str()); @@ -64,8 +62,7 @@ void ZmqProducerStateTable::set( m_zmqClient.sendMsg( m_dbName, m_tableNameStr, - kcos, - m_sendbuffer); + kcos); if (m_asyncDBUpdater != nullptr) { @@ -93,8 +90,7 @@ void ZmqProducerStateTable::del( m_zmqClient.sendMsg( m_dbName, m_tableNameStr, - kcos, - m_sendbuffer); + kcos); if (m_asyncDBUpdater != nullptr) { @@ -112,8 +108,7 @@ void ZmqProducerStateTable::set(const std::vector &value m_zmqClient.sendMsg( m_dbName, m_tableNameStr, - values, - m_sendbuffer); + values); if (m_asyncDBUpdater != nullptr) { @@ -136,8 +131,7 @@ void ZmqProducerStateTable::del(const std::vector &keys) m_zmqClient.sendMsg( m_dbName, m_tableNameStr, - kcos, - m_sendbuffer); + kcos); if (m_asyncDBUpdater != nullptr) { @@ -157,8 +151,7 @@ void ZmqProducerStateTable::send(const std::vector &kcos m_zmqClient.sendMsg( m_dbName, m_tableNameStr, - kcos, - m_sendbuffer); + kcos); if (m_asyncDBUpdater != nullptr) { diff --git a/common/zmqproducerstatetable.h b/common/zmqproducerstatetable.h index 749107825..015419bd2 100644 --- a/common/zmqproducerstatetable.h +++ b/common/zmqproducerstatetable.h @@ -42,8 +42,6 @@ class ZmqProducerStateTable : public ProducerStateTable void initialize(DBConnector *db, const std::string &tableName, bool dbPersistence); ZmqClient& m_zmqClient; - - std::vector m_sendbuffer; const std::string m_dbName; const std::string m_tableNameStr; From aa1021fb14bd79c01e34ea99733b2213da70029a Mon Sep 17 00:00:00 2001 From: Yijiao Qin Date: Sun, 1 Dec 2024 18:35:15 -0800 Subject: [PATCH 13/15] Update redispipeline.h (#954) use "\n" instead of semicolon to separate redis commands in a c++ string --- common/redispipeline.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/redispipeline.h b/common/redispipeline.h index be7561b6b..96f97ab8b 100644 --- a/common/redispipeline.h +++ b/common/redispipeline.h @@ -164,7 +164,7 @@ class RedisPipeline { return; m_channels.insert(channel); - m_luaPub += "redis.call('PUBLISH', '" + channel + "', 'G');"; + m_luaPub += "redis.call('PUBLISH', '" + channel + "', 'G')\n"; m_shaPub = loadRedisScript(m_luaPub); } From fb6ce44e41f77dc8631f0e8bf43cdf58c3635d3a Mon Sep 17 00:00:00 2001 From: Arham-Nasir <100487254+Arham-Nasir@users.noreply.github.com> Date: Wed, 4 Dec 2024 00:15:35 +0500 Subject: [PATCH 14/15] Add definition for MEMORY_STATISTICS table in schema (#898) define table in common/schema.h file of sonic-swss-common --- common/schema.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/common/schema.h b/common/schema.h index e99f2ad81..461ea4d82 100644 --- a/common/schema.h +++ b/common/schema.h @@ -475,11 +475,12 @@ namespace swss { #define CFG_SUPPRESS_ASIC_SDK_HEALTH_EVENT_NAME "SUPPRESS_ASIC_SDK_HEALTH_EVENT" +#define CFG_MEMORY_STATISTICS_TABLE_NAME "MEMORY_STATISTICS" + #define CFG_PAC_PORT_CONFIG_TABLE "PAC_PORT_CONFIG_TABLE" #define CFG_PAC_GLOBAL_CONFIG_TABLE "PAC_GLOBAL_CONFIG_TABLE" #define CFG_PAC_HOSTAPD_GLOBAL_CONFIG_TABLE "HOSTAPD_GLOBAL_CONFIG_TABLE" - /***** STATE DATABASE *****/ #define STATE_SWITCH_CAPABILITY_TABLE_NAME "SWITCH_CAPABILITY" From 7425c4237b06c9c7b553a9a3bcb540c2f7720fcf Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Fri, 6 Dec 2024 17:03:31 -0800 Subject: [PATCH 15/15] Update the Azure Pipeline build to use Bookworm and Ubuntu 22.04 (#937) Update the Azure Pipeline build to use Bookworm and Ubuntu 22.04 Signed-off-by: Saikrishna Arcot --- .azure-pipelines/build-sairedis-template.yml | 6 +- .azure-pipelines/build-template.yml | 14 +- .azure-pipelines/docker-sonic-vs/Dockerfile | 24 ++- .azure-pipelines/docker-sonic-vs/start.sh | 187 ++++++++++++++++++ .../test-docker-sonic-vs-template.yml | 38 +--- azure-pipelines.yml | 91 +++++---- 6 files changed, 273 insertions(+), 87 deletions(-) create mode 100755 .azure-pipelines/docker-sonic-vs/start.sh diff --git a/.azure-pipelines/build-sairedis-template.yml b/.azure-pipelines/build-sairedis-template.yml index b37d2fa44..65e7c536d 100644 --- a/.azure-pipelines/build-sairedis-template.yml +++ b/.azure-pipelines/build-sairedis-template.yml @@ -66,7 +66,6 @@ jobs: set -ex sudo apt-get update sudo apt-get install -qq -y \ - qtbase5-dev \ libdbus-glib-1-dev \ libpcsclite-dev \ docbook-to-man \ @@ -90,7 +89,7 @@ jobs: sudo mkdir -m 755 /var/run/sswsyncd sudo apt-get install -y rsyslog - sudo service rsyslog start + sudo rsyslogd displayName: "Install dependencies" - task: DownloadPipelineArtifact@2 @@ -137,7 +136,8 @@ jobs: displayName: "Compile sonic sairedis" - script: | sudo cp azsyslog.conf /etc/rsyslog.conf - sudo service rsyslog restart + sudo killall rsyslogd + sudo rsyslogd displayName: "Update rsyslog.conf" - ${{ if eq(parameters.run_unit_test, true) }}: - script: | diff --git a/.azure-pipelines/build-template.yml b/.azure-pipelines/build-template.yml index d1813c23a..a4b607efe 100644 --- a/.azure-pipelines/build-template.yml +++ b/.azure-pipelines/build-template.yml @@ -98,16 +98,6 @@ jobs: mv ../*.deb . displayName: "Compile sonic swss common with coverage enabled" - ${{ if eq(parameters.run_unit_test, true) }}: - - script: | - set -ex - git clone https://github.com/gcovr/gcovr.git - cd gcovr/ - git checkout 5.2 - sudo pip3 install setuptools - sudo python3 setup.py install - cd .. - sudo rm -rf gcovr - displayName: "Install gcovr 5.2 (for --exclude-throw-branches support)" - script: | set -ex sudo pip install Pympler==0.8 pytest @@ -142,9 +132,9 @@ jobs: set -ex # Install .NET CORE curl -sSL https://packages.microsoft.com/keys/microsoft.asc | sudo apt-key add - - sudo apt-add-repository https://packages.microsoft.com/debian/11/prod + sudo apt-add-repository https://packages.microsoft.com/debian/12/prod sudo apt-get update - sudo apt-get install -y dotnet-sdk-6.0 + sudo apt-get install -y dotnet-sdk-8.0 displayName: "Install .NET CORE" - task: PublishCodeCoverageResults@1 inputs: diff --git a/.azure-pipelines/docker-sonic-vs/Dockerfile b/.azure-pipelines/docker-sonic-vs/Dockerfile index 3d2198050..f598ea6c8 100644 --- a/.azure-pipelines/docker-sonic-vs/Dockerfile +++ b/.azure-pipelines/docker-sonic-vs/Dockerfile @@ -1,11 +1,31 @@ FROM docker-sonic-vs ARG docker_container_name +ARG need_dbg COPY ["debs", "/debs"] # Remove existing packages first before installing the new/current packages. This is to overcome limitations with # Docker's diff detection mechanism, where only the file size and the modification timestamp (which will remain the # same, even though contents have changed) are checked between the previous and current layer. -RUN dpkg --purge libswsscommon python3-swsscommon sonic-db-cli libsaimetadata libsairedis libsaivs syncd-vs swss sonic-eventd libdashapi -RUN dpkg -i /debs/libdashapi_1.0.0_amd64.deb /debs/libswsscommon_1.0.0_amd64.deb /debs/python3-swsscommon_1.0.0_amd64.deb /debs/sonic-db-cli_1.0.0_amd64.deb /debs/libsaimetadata_1.0.0_amd64.deb /debs/libsairedis_1.0.0_amd64.deb /debs/libsaivs_1.0.0_amd64.deb /debs/syncd-vs_1.0.0_amd64.deb /debs/swss_1.0.0_amd64.deb +RUN dpkg --purge libswsscommon python3-swsscommon sonic-db-cli libsaimetadata libsairedis libsaivs syncd-vs swss sonic-eventd libdashapi + +RUN apt-get update + +RUN apt install -y /debs/libdashapi_1.0.0_amd64.deb \ + /debs/libswsscommon_1.0.0_amd64.deb \ + /debs/python3-swsscommon_1.0.0_amd64.deb \ + /debs/sonic-db-cli_1.0.0_amd64.deb \ + /debs/libsaimetadata_1.0.0_amd64.deb \ + /debs/libsairedis_1.0.0_amd64.deb \ + /debs/libsaivs_1.0.0_amd64.deb \ + /debs/syncd-vs_1.0.0_amd64.deb \ + /debs/swss_1.0.0_amd64.deb + +RUN if [ "$need_dbg" = "y" ] ; then dpkg -i /debs/libswsscommon-dbgsym_1.0.0_amd64.deb ; fi + +COPY ["start.sh", "/usr/bin/"] + +RUN pip3 install scapy==2.5.0 + +RUN apt-get -y install software-properties-common libdatetime-perl libcapture-tiny-perl build-essential libcpanel-json-xs-perl git python3-protobuf diff --git a/.azure-pipelines/docker-sonic-vs/start.sh b/.azure-pipelines/docker-sonic-vs/start.sh new file mode 100755 index 000000000..f7dbde8dc --- /dev/null +++ b/.azure-pipelines/docker-sonic-vs/start.sh @@ -0,0 +1,187 @@ +#!/bin/bash -e + +# Generate configuration + +# NOTE: 'PLATFORM' and 'HWSKU' environment variables are set +# in the Dockerfile so that they persist for the life of the container + +ln -sf /usr/share/sonic/device/$PLATFORM /usr/share/sonic/platform +ln -sf /usr/share/sonic/device/$PLATFORM/$HWSKU /usr/share/sonic/hwsku + +SWITCH_TYPE=switch +PLATFORM_CONF=platform.json +if [[ $HWSKU == "DPU-2P" ]]; then + SWITCH_TYPE=dpu + PLATFORM_CONF=platform-dpu-2p.json +fi + +pushd /usr/share/sonic/hwsku + +# filter available front panel ports in lanemap.ini +[ -f lanemap.ini.orig ] || cp lanemap.ini lanemap.ini.orig +for p in $(ip link show | grep -oE "eth[0-9]+" | grep -v eth0); do + grep ^$p: lanemap.ini.orig +done > lanemap.ini + +# filter available sonic front panel ports in port_config.ini +[ -f port_config.ini.orig ] || cp port_config.ini port_config.ini.orig +grep ^# port_config.ini.orig > port_config.ini +for lanes in $(awk -F ':' '{print $2}' lanemap.ini); do + grep -E "\s$lanes\s" port_config.ini.orig +done >> port_config.ini + +popd + +[ -d /etc/sonic ] || mkdir -p /etc/sonic + +# Note: libswsscommon requires a dabase_config file in /var/run/redis/sonic-db/ +# Prepare this file before any dependent application, such as sonic-cfggen +mkdir -p /var/run/redis/sonic-db +cp /etc/default/sonic-db/database_config.json /var/run/redis/sonic-db/ + +SYSTEM_MAC_ADDRESS=$(ip link show eth0 | grep ether | awk '{print $2}') +sonic-cfggen -t /usr/share/sonic/templates/init_cfg.json.j2 -a "{\"system_mac\": \"$SYSTEM_MAC_ADDRESS\", \"switch_type\": \"$SWITCH_TYPE\"}" > /etc/sonic/init_cfg.json + +if [[ -f /usr/share/sonic/virtual_chassis/default_config.json ]]; then + sonic-cfggen -j /etc/sonic/init_cfg.json -j /usr/share/sonic/virtual_chassis/default_config.json --print-data > /tmp/init_cfg.json + mv /tmp/init_cfg.json /etc/sonic/init_cfg.json +fi + +if [ -f /etc/sonic/config_db.json ]; then + sonic-cfggen -j /etc/sonic/init_cfg.json -j /etc/sonic/config_db.json --print-data > /tmp/config_db.json + mv /tmp/config_db.json /etc/sonic/config_db.json +else + # generate and merge buffers configuration into config file + if [ -f /usr/share/sonic/hwsku/buffers.json.j2 ]; then + sonic-cfggen -k $HWSKU -p /usr/share/sonic/device/$PLATFORM/$PLATFORM_CONF -t /usr/share/sonic/hwsku/buffers.json.j2 > /tmp/buffers.json + buffers_cmd="-j /tmp/buffers.json" + fi + if [ -f /usr/share/sonic/hwsku/qos.json.j2 ]; then + sonic-cfggen -j /etc/sonic/init_cfg.json -t /usr/share/sonic/hwsku/qos.json.j2 > /tmp/qos.json + qos_cmd="-j /tmp/qos.json" + fi + + sonic-cfggen -p /usr/share/sonic/device/$PLATFORM/$PLATFORM_CONF -k $HWSKU --print-data > /tmp/ports.json + # change admin_status from up to down; Test cases dependent + sed -i "s/up/down/g" /tmp/ports.json + sonic-cfggen -j /etc/sonic/init_cfg.json $buffers_cmd $qos_cmd -j /tmp/ports.json --print-data > /etc/sonic/config_db.json +fi + +sonic-cfggen -t /usr/share/sonic/templates/copp_cfg.j2 > /etc/sonic/copp_cfg.json + +if [ "$HWSKU" == "Mellanox-SN2700" ]; then + cp /usr/share/sonic/hwsku/sai_mlnx.profile /usr/share/sonic/hwsku/sai.profile +elif [ "$HWSKU" == "DPU-2P" ]; then + cp /usr/share/sonic/hwsku/sai_dpu_2p.profile /usr/share/sonic/hwsku/sai.profile +fi + +mkdir -p /etc/swss/config.d/ + +rm -f /var/run/rsyslogd.pid + +supervisorctl start rsyslogd + +supervisord_cfg="/etc/supervisor/conf.d/supervisord.conf" +chassisdb_cfg_file="/usr/share/sonic/virtual_chassis/default_config.json" +chassisdb_cfg_file_default="/etc/default/sonic-db/default_chassis_cfg.json" +host_template="/usr/share/sonic/templates/hostname.j2" +db_cfg_file="/var/run/redis/sonic-db/database_config.json" +db_cfg_file_tmp="/var/run/redis/sonic-db/database_config.json.tmp" + +if [ -r "$chassisdb_cfg_file" ]; then + echo $(sonic-cfggen -j $chassisdb_cfg_file -t $host_template) >> /etc/hosts +else + chassisdb_cfg_file="$chassisdb_cfg_file_default" + echo "10.8.1.200 redis_chassis.server" >> /etc/hosts +fi + +supervisorctl start redis-server + +start_chassis_db=`sonic-cfggen -v DEVICE_METADATA.localhost.start_chassis_db -y $chassisdb_cfg_file` +if [[ "$HOSTNAME" == *"supervisor"* ]] || [ "$start_chassis_db" == "1" ]; then + supervisorctl start redis-chassis +fi + +conn_chassis_db=`sonic-cfggen -v DEVICE_METADATA.localhost.connect_to_chassis_db -y $chassisdb_cfg_file` +if [ "$start_chassis_db" != "1" ] && [ "$conn_chassis_db" != "1" ]; then + cp $db_cfg_file $db_cfg_file_tmp + update_chassisdb_config -j $db_cfg_file_tmp -d + cp $db_cfg_file_tmp $db_cfg_file +fi + +if [ "$conn_chassis_db" == "1" ]; then + if [ -f /usr/share/sonic/virtual_chassis/coreportindexmap.ini ]; then + cp /usr/share/sonic/virtual_chassis/coreportindexmap.ini /usr/share/sonic/hwsku/ + + pushd /usr/share/sonic/hwsku + + # filter available front panel ports in coreportindexmap.ini + [ -f coreportindexmap.ini.orig ] || cp coreportindexmap.ini coreportindexmap.ini.orig + for p in $(ip link show | grep -oE "eth[0-9]+" | grep -v eth0); do + grep ^$p: coreportindexmap.ini.orig + done > coreportindexmap.ini + + popd + fi +fi + +/usr/bin/configdb-load.sh + +if [ "$HWSKU" = "brcm_gearbox_vs" ]; then + supervisorctl start gbsyncd + supervisorctl start gearsyncd +fi + +supervisorctl start syncd + +supervisorctl start portsyncd + +supervisorctl start orchagent + +supervisorctl start coppmgrd + +supervisorctl start neighsyncd + +supervisorctl start fdbsyncd + +supervisorctl start teamsyncd + +supervisorctl start fpmsyncd + +supervisorctl start teammgrd + +supervisorctl start vrfmgrd + +supervisorctl start portmgrd + +supervisorctl start intfmgrd + +supervisorctl start vlanmgrd + +supervisorctl start zebra + +supervisorctl start mgmtd + +supervisorctl start staticd + +supervisorctl start buffermgrd + +supervisorctl start nbrmgrd + +supervisorctl start vxlanmgrd + +supervisorctl start sflowmgrd + +supervisorctl start natmgrd + +supervisorctl start natsyncd + +supervisorctl start tunnelmgrd + +supervisorctl start fabricmgrd + +# Start arp_update when VLAN exists +VLAN=`sonic-cfggen -d -v 'VLAN.keys() | join(" ") if VLAN'` +if [ "$VLAN" != "" ]; then + supervisorctl start arp_update +fi diff --git a/.azure-pipelines/test-docker-sonic-vs-template.yml b/.azure-pipelines/test-docker-sonic-vs-template.yml index 81af9bd82..0a2a7018c 100644 --- a/.azure-pipelines/test-docker-sonic-vs-template.yml +++ b/.azure-pipelines/test-docker-sonic-vs-template.yml @@ -15,7 +15,7 @@ jobs: displayName: vstest timeoutInMinutes: ${{ parameters.timeout }} - pool: sonic-common + pool: sonictest steps: - checkout: self @@ -52,11 +52,10 @@ jobs: - script: | set -ex - ls -l sudo sonic-swss-common/.azure-pipelines/build_and_install_module.sh sudo apt-get install -y libhiredis0.14 libyang0.16 - sudo dpkg -i $(Build.ArtifactStagingDirectory)/download/libprotobuf*_amd64.deb $(Build.ArtifactStagingDirectory)/download/libprotobuf-lite*_amd64.deb $(Build.ArtifactStagingDirectory)/download/python3-protobuf*_amd64.deb + sudo dpkg -i $(Build.ArtifactStagingDirectory)/download/libprotobuf*_amd64.deb $(Build.ArtifactStagingDirectory)/download/libprotobuf-lite*_amd64.deb $(Build.ArtifactStagingDirectory)/download/python3-protobuf*_amd64.deb sudo dpkg -i $(Build.ArtifactStagingDirectory)/download/libdashapi*.deb sudo dpkg -i --force-confask,confnew $(Build.ArtifactStagingDirectory)/download/libswsscommon_1.0.0_amd64.deb || apt-get install -f sudo dpkg -i $(Build.ArtifactStagingDirectory)/download/python3-swsscommon_1.0.0_amd64.deb @@ -72,34 +71,17 @@ jobs: sudo docker load -i $(Build.ArtifactStagingDirectory)/download/docker-sonic-vs.gz docker ps ip netns list + sudo /sbin/ip link add Vrf1 type vrf table 1001 || { echo 'vrf command failed' ; exit 1; } + sudo /sbin/ip link del Vrf1 type vrf table 1001 pushd sonic-swss/tests - # run pytests in sets of 20 - all_tests=$(ls test_*.py) + all_tests=$(ls test_*.py | xargs) all_tests="${all_tests} p4rt dash" - test_set=() - for test in ${all_tests}; do - test_set+=("${test}") - if [ ${#test_set[@]} -ge 20 ]; then - test_name=$(echo "${test_set[0]}" | cut -d "." -f 1) - echo "${test_set[*]}" | xargs sudo py.test -v --force-flaky --junitxml="${test_name}_tr.xml" --keeptb --imgname=docker-sonic-vs:$(Build.DefinitionName).$(Build.BuildNumber) - container_count=$(docker ps -q -a | wc -l) - if [ ${container_count} -gt 0 ]; then - docker stop $(docker ps -q -a) - docker rm $(docker ps -q -a) - fi - test_set=() - fi - done - if [ ${#test_set[@]} -gt 0 ]; then - test_name=$(echo "${test_set[0]}" | cut -d "." -f 1) - echo "${test_set[*]}" | xargs sudo py.test -v --force-flaky --junitxml="${test_name}_tr.xml" --keeptb --imgname=docker-sonic-vs:$(Build.DefinitionName).$(Build.BuildNumber) - container_count=$(docker ps -q -a | wc -l) - if [ ${container_count} -gt 0 ]; then - docker stop $(docker ps -q -a) - docker rm $(docker ps -q -a) - fi - fi + + # Run the tests in parallel and retry + retry=3 + IMAGE_NAME=docker-sonic-vs:$(Build.DefinitionName).$(Build.BuildNumber) + echo $all_tests | xargs -n 1 | xargs -P 8 -I TEST_MODULE sudo ./run-tests.sh "$IMAGE_NAME" "--force-recreate-dvs" "TEST_MODULE" 3 rm -rf $(Build.ArtifactStagingDirectory)/download displayName: "Run vs tests" diff --git a/azure-pipelines.yml b/azure-pipelines.yml index f5f233262..eb9743886 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -40,7 +40,7 @@ resources: parameters: - name: debian_version type: string - default: bullseye + default: bookworm variables: - name: BUILD_BRANCH ${{ if eq(variables['Build.Reason'], 'PullRequest') }}: @@ -62,7 +62,7 @@ stages: sudo apt-get update sudo apt-get install -y make libtool m4 autoconf dh-exec debhelper cmake pkg-config nlohmann-json3-dev \ libhiredis-dev libnl-3-dev libnl-genl-3-dev libnl-route-3-dev libnl-nf-3-dev swig3.0 \ - libpython2.7-dev libboost-dev libboost-serialization-dev uuid-dev libzmq5 libzmq3-dev + libpython2.7-dev libboost-dev libboost-serialization-dev uuid-dev libzmq3-dev sudo apt-get install -y sudo sudo apt-get install -y redis-server redis-tools sudo apt-get install -y python3-pip @@ -88,11 +88,47 @@ stages: artifact: sonic-swss-common.amd64.ubuntu20_04 displayName: "Archive swss common debian packages" + - job: + displayName: "amd64/ubuntu-22.04" + pool: + vmImage: 'ubuntu-22.04' + + steps: + - script: | + sudo apt-get update + sudo apt-get install -y make libtool m4 autoconf dh-exec debhelper cmake pkg-config nlohmann-json3-dev \ + libhiredis-dev libnl-3-dev libnl-genl-3-dev libnl-route-3-dev libnl-nf-3-dev swig4.0 \ + libpython3-dev libboost-dev libboost-serialization-dev uuid-dev libzmq3-dev + sudo apt-get install -y sudo + sudo apt-get install -y redis-server redis-tools + sudo apt-get install -y python3-pip + sudo pip3 install pytest + sudo apt-get install -y python + sudo apt-get install cmake libgtest-dev libgmock-dev libyang-dev + cd /usr/src/gtest && sudo cmake . && sudo make + ARCH=$(dpkg --print-architecture) + set -x + sudo curl -fsSL -o /usr/local/bin/bazel \ + https://github.com/bazelbuild/bazelisk/releases/latest/download/bazelisk-linux-${ARCH} + sudo chmod 755 /usr/local/bin/bazel + displayName: "Install dependencies" + - script: | + ./autogen.sh + dpkg-buildpackage -rfakeroot -us -uc -Pnopython2 -b -j$(nproc) && cp ../*.deb . + displayName: "Compile sonic swss common" + - script: | + bazel build //... + bazel test //... + displayName: "Compile and test all Bazel targets" + - publish: $(System.DefaultWorkingDirectory)/ + artifact: sonic-swss-common.amd64.ubuntu22_04 + displayName: "Archive swss common debian packages" + - template: .azure-pipelines/build-template.yml parameters: arch: amd64 sonic_slave: sonic-slave-${{ parameters.debian_version }}:$(BUILD_BRANCH) - artifact_name: sonic-swss-common + artifact_name: sonic-swss-common-${{ parameters.debian_version }} run_unit_test: true archive_gcov: true debian_version: ${{ parameters.debian_version }} @@ -107,7 +143,7 @@ stages: timeout: 180 pool: sonicbld-armhf sonic_slave: sonic-slave-${{ parameters.debian_version }}-armhf:$(BUILD_BRANCH) - artifact_name: sonic-swss-common.armhf + artifact_name: sonic-swss-common-${{ parameters.debian_version }}.armhf debian_version: ${{ parameters.debian_version }} - template: .azure-pipelines/build-template.yml @@ -116,36 +152,7 @@ stages: timeout: 180 pool: sonicbld-arm64 sonic_slave: sonic-slave-${{ parameters.debian_version }}-arm64:$(BUILD_BRANCH) - artifact_name: sonic-swss-common.arm64 - debian_version: ${{ parameters.debian_version }} - -- stage: BuildBookworm - dependsOn: BuildArm - condition: succeeded('BuildArm') - jobs: - - template: .azure-pipelines/build-template.yml - parameters: - arch: amd64 - sonic_slave: sonic-slave-bookworm:$(BUILD_BRANCH) - artifact_name: sonic-swss-common-bookworm - debian_version: ${{ parameters.debian_version }} - - - template: .azure-pipelines/build-template.yml - parameters: - arch: armhf - timeout: 180 - pool: sonicbld-armhf - sonic_slave: sonic-slave-bookworm-armhf:$(BUILD_BRANCH) - artifact_name: sonic-swss-common-bookworm.armhf - debian_version: ${{ parameters.debian_version }} - - - template: .azure-pipelines/build-template.yml - parameters: - arch: arm64 - timeout: 180 - pool: sonicbld-arm64 - sonic_slave: sonic-slave-bookworm-arm64:$(BUILD_BRANCH) - artifact_name: sonic-swss-common-bookworm.arm64 + artifact_name: sonic-swss-common-${{ parameters.debian_version }}.arm64 debian_version: ${{ parameters.debian_version }} - stage: BuildSairedis @@ -156,8 +163,8 @@ stages: parameters: arch: amd64 sonic_slave: sonic-slave-${{ parameters.debian_version }}:$(BUILD_BRANCH) - swss_common_artifact_name: sonic-swss-common - artifact_name: sonic-sairedis + swss_common_artifact_name: sonic-swss-common-${{ parameters.debian_version }} + artifact_name: sonic-sairedis-${{ parameters.debian_version }} syslog_artifact_name: sonic-sairedis.syslog debian_version: ${{ parameters.debian_version }} @@ -169,9 +176,9 @@ stages: parameters: arch: amd64 sonic_slave: sonic-slave-${{ parameters.debian_version }}:$(BUILD_BRANCH) - swss_common_artifact_name: sonic-swss-common - sairedis_artifact_name: sonic-sairedis - artifact_name: sonic-swss + swss_common_artifact_name: sonic-swss-common-${{ parameters.debian_version }} + sairedis_artifact_name: sonic-sairedis-${{ parameters.debian_version }} + artifact_name: sonic-swss-${{ parameters.debian_version }} debian_version: ${{ parameters.debian_version }} - stage: BuildDocker @@ -180,9 +187,9 @@ stages: jobs: - template: .azure-pipelines/build-docker-sonic-vs-template.yml parameters: - swss_common_artifact_name: sonic-swss-common - sairedis_artifact_name: sonic-sairedis - swss_artifact_name: sonic-swss + swss_common_artifact_name: sonic-swss-common-${{ parameters.debian_version }} + sairedis_artifact_name: sonic-sairedis-${{ parameters.debian_version }} + swss_artifact_name: sonic-swss-${{ parameters.debian_version }} artifact_name: docker-sonic-vs - stage: Test