From 82581cfc0b5fa99bd1944e47db17fbe452a1b1d7 Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Mon, 31 Jul 2023 21:31:50 -0700 Subject: [PATCH 1/9] Make sure new binaries replace existing binaries in docker-sonic-vs (#807) PR #716 removed a step where existing deb packages were removed before installing the new deb packages. The problem is that due to Docker's change detection code, where it only checks the file size and the modification timestamp, even if the file gets modified when installing a newer debian package, the file size may remain the same, and the modification timestamp will remain the same (since this is based on the debian/changelog timestamp). This caused changed binaries to not actually replace the existing binaries. Fix this by re-adding the line that removed existing packages first. --- .azure-pipelines/docker-sonic-vs/Dockerfile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.azure-pipelines/docker-sonic-vs/Dockerfile b/.azure-pipelines/docker-sonic-vs/Dockerfile index 0d378f141..8e4cbb8d9 100644 --- a/.azure-pipelines/docker-sonic-vs/Dockerfile +++ b/.azure-pipelines/docker-sonic-vs/Dockerfile @@ -4,4 +4,8 @@ ARG docker_container_name 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 RUN dpkg -i /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 From 7ff768a72e6fbfc02ac1052b00e49690fc9d4f1b Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 1 Aug 2023 11:52:38 -0700 Subject: [PATCH 2/9] Revert "Fix the issue of ignoring callback calls for removed keys. (#789)" (#804) This reverts commit e0f394cd44370370e872711b04a15e497b63e0f8. The reason is unexpected behavior change of `get_entry`. ref: https://github.com/sonic-net/sonic-swss-common/pull/789#issuecomment-1639525084 --- common/configdb.h | 2 +- tests/test_redis_ut.py | 45 +++++++++++------------------------------- 2 files changed, 12 insertions(+), 35 deletions(-) diff --git a/common/configdb.h b/common/configdb.h index 7f2a602c3..5acba65e5 100644 --- a/common/configdb.h +++ b/common/configdb.h @@ -120,7 +120,7 @@ class ConfigDBConnector_Native : public SonicV2Connector_Native ## Dynamic typed functions used in python @staticmethod def raw_to_typed(raw_data): - if not raw_data or not raw_data.keys(): + if raw_data is None: return None typed_data = {} for raw_key in raw_data: diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index 363f1e065..17322b57d 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -634,29 +634,20 @@ def thread_coming_entry(): def test_ConfigDBInit(): table_name_1 = 'TEST_TABLE_1' table_name_2 = 'TEST_TABLE_2' - table_name_3 = 'TEST_TABLE_3' test_key = 'key1' test_data = {'field1': 'value1'} - - queue = multiprocessing.Queue() + test_data_update = {'field1': 'value2'} manager = multiprocessing.Manager() ret_data = manager.dict() - def test_handler(table, key, data, ret, q=None): - if data is None: - ret[table] = {k: v for k, v in ret[table].items() if k != key} - else: - ret[table] = {key: data} - - if q: - q.put(ret[table]) + def test_handler(table, key, data, ret): + ret[table] = {key: data} - def test_init_handler(data, ret, queue): + def test_init_handler(data, ret): ret.update(data) - queue.put(ret) - def thread_listen(ret, queue): + def thread_listen(ret): config_db = ConfigDBConnector() config_db.connect(wait_for_init=False) @@ -664,10 +655,8 @@ def thread_listen(ret, queue): fire_init_data=False) config_db.subscribe(table_name_2, lambda table, key, data: test_handler(table, key, data, ret), fire_init_data=True) - config_db.subscribe(table_name_3, lambda table, key, data: test_handler(table, key, data, ret, queue), - fire_init_data=False) - config_db.listen(init_data_handler=lambda data: test_init_handler(data, ret, queue)) + config_db.listen(init_data_handler=lambda data: test_init_handler(data, ret)) config_db = ConfigDBConnector() config_db.connect(wait_for_init=False) @@ -677,27 +666,15 @@ def thread_listen(ret, queue): # Init table data config_db.set_entry(table_name_1, test_key, test_data) config_db.set_entry(table_name_2, test_key, test_data) - config_db.set_entry(table_name_3, test_key, {}) - thread = multiprocessing.Process(target=thread_listen, args=(ret_data, queue)) + thread = multiprocessing.Process(target=thread_listen, args=(ret_data,)) thread.start() - - init_data = queue.get(5) - - # Verify that all tables initialized correctly - assert init_data[table_name_1] == {test_key: test_data} - assert init_data[table_name_2] == {test_key: test_data} - assert init_data[table_name_3] == {test_key: {}} - - # Remove the entry (with no attributes) from the table. - # Verify that the update is received and a callback is called - config_db.set_entry(table_name_3, test_key, None) - - table_3_data = queue.get(5) - assert test_key not in table_3_data - + time.sleep(5) thread.terminate() + assert ret_data[table_name_1] == {test_key: test_data} + assert ret_data[table_name_2] == {test_key: test_data} + def test_DBConnectFailure(): """ Verify that a DB connection failure will not cause a process abort From 5966d8b6f35a822d0e72899b2c4621bb0df54aca Mon Sep 17 00:00:00 2001 From: Hua Liu <58683130+liuh-80@users.noreply.github.com> Date: Thu, 3 Aug 2023 18:10:20 +0800 Subject: [PATCH 3/9] Fix binary serializer can't deserialize protopuf buffer content issue (#810) Fix binary serializer can't deserialize protopuf buffer content issue. #### Work item tracking Microsoft ADO (number only): 17753804 #### Why I did it Fix binary serializer can't deserialize protopuf buffer content issue. #### How I did it Fix code but when deserialize binary string. #### How to verify it Add UT. Pass all UT. #### Which release branch to backport (provide reason below if selected) - [ ] 201811 - [ ] 201911 - [ ] 202006 - [ ] 202012 - [ ] 202106 - [ ] 202111 #### Description for the changelog Fix binary serializer can't deserialize protopuf buffer content issue. #### Link to config_db schema for YANG module changes #### A picture of a cute animal (not mandatory but encouraged) --- common/binaryserializer.h | 11 ++++---- tests/binary_serializer_ut.cpp | 47 +++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/common/binaryserializer.h b/common/binaryserializer.h index 2d97a70f0..18fc34723 100644 --- a/common/binaryserializer.h +++ b/common/binaryserializer.h @@ -3,6 +3,8 @@ #include "common/armhelper.h" +using namespace std; + namespace swss { class BinarySerializer { @@ -64,7 +66,7 @@ class BinarySerializer { size); } - auto pkey = tmp_buffer; + auto pkey = string(tmp_buffer, *pkeylen); tmp_buffer += *pkeylen; WARNINGS_NO_CAST_ALIGN; @@ -79,7 +81,7 @@ class BinarySerializer { size); } - auto pval = tmp_buffer; + auto pval = string(tmp_buffer, *pvallen); tmp_buffer += *pvallen; values.push_back(std::make_pair(pkey, pval)); @@ -107,9 +109,8 @@ class BinarySerializer { void setKeyAndValue(const char* key, size_t klen, const char* value, size_t vlen) { - // to improve deserialize performance, copy null-terminated string. - setData(key, klen + 1); - setData(value, vlen + 1); + setData(key, klen); + setData(value, vlen); m_kvp_count++; } diff --git a/tests/binary_serializer_ut.cpp b/tests/binary_serializer_ut.cpp index beae65d4b..19219cb82 100644 --- a/tests/binary_serializer_ut.cpp +++ b/tests/binary_serializer_ut.cpp @@ -29,7 +29,7 @@ TEST(BinarySerializer, serialize_deserialize) test_table); string serialized_str(buffer); - EXPECT_EQ(serialized_len, 107); + EXPECT_EQ(serialized_len, 101); auto ptr = std::make_shared(); KeyOpFieldsValuesTuple &kco = *ptr; @@ -84,3 +84,48 @@ TEST(BinarySerializer, deserialize_overflow) auto& deserialized_values = kfvFieldsValues(kco); EXPECT_THROW(BinarySerializer::deserializeBuffer(buffer, serialized_len - 10, deserialized_values), runtime_error); } + +TEST(BinarySerializer, protocol_buffer) +{ + string test_entry_key = "test_key"; + string test_command = "test_command"; + string test_db = "test_db"; + string test_table = "test_table"; + string test_key = "key"; + unsigned char binary_proto_buf[] = {0x0a, 0x05, 0x0d, 0x0a, 0x01, 0x00, 0x20, 0x10, 0xe1, 0x21}; + string proto_buf_val = string((const char *)binary_proto_buf, sizeof(binary_proto_buf)); + EXPECT_TRUE(proto_buf_val.length() == sizeof(binary_proto_buf)); + + char buffer[200]; + std::vector values; + values.push_back(std::make_pair(test_key, proto_buf_val)); + int serialized_len = (int)BinarySerializer::serializeBuffer( + buffer, + sizeof(buffer), + test_entry_key, + values, + test_command, + test_db, + test_table); + string serialized_str(buffer); + + EXPECT_EQ(serialized_len, 106); + + auto ptr = std::make_shared(); + KeyOpFieldsValuesTuple &kco = *ptr; + auto& deserialized_values = kfvFieldsValues(kco); + BinarySerializer::deserializeBuffer(buffer, serialized_len, deserialized_values); + + swss::FieldValueTuple fvt = deserialized_values.at(0); + EXPECT_TRUE(fvField(fvt) == test_db); + EXPECT_TRUE(fvValue(fvt) == test_table); + + fvt = deserialized_values.at(1); + EXPECT_TRUE(fvField(fvt) == test_entry_key); + EXPECT_TRUE(fvValue(fvt) == test_command); + + fvt = deserialized_values.at(2); + EXPECT_TRUE(fvField(fvt) == test_key); + EXPECT_TRUE(fvValue(fvt) == proto_buf_val); + EXPECT_TRUE(fvValue(fvt).length() == sizeof(binary_proto_buf)); +} From be425ede57b4412c51b240f285d80a10c40fa077 Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Fri, 4 Aug 2023 08:51:34 +0800 Subject: [PATCH 4/9] [redisCommand]: Not store the error return code of redisFormat (#809) In the previous implementation, the private member variable, len, will store the error code of redisFormat. This PR is for fixing that if the redisFormat raise an unexpected error code, the `len` will be assigned to zero. --- common/rediscommand.cpp | 16 +++++++++++----- tests/Makefile.am | 3 ++- tests/redis_command_ut.cpp | 12 ++++++++++++ 3 files changed, 25 insertions(+), 6 deletions(-) create mode 100644 tests/redis_command_ut.cpp diff --git a/common/rediscommand.cpp b/common/rediscommand.cpp index 3b8ed7041..5cc7422b9 100644 --- a/common/rediscommand.cpp +++ b/common/rediscommand.cpp @@ -25,16 +25,18 @@ void RedisCommand::format(const char *fmt, ...) redisFreeCommand(temp); temp = nullptr; } + len = 0; va_list ap; va_start(ap, fmt); - len = redisvFormatCommand(&temp, fmt, ap); + int ret = redisvFormatCommand(&temp, fmt, ap); va_end(ap); - if (len == -1) { + if (ret == -1) { throw std::bad_alloc(); - } else if (len == -2) { + } else if (ret == -2) { throw std::invalid_argument("fmt"); } + len = ret; } void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen) @@ -44,11 +46,13 @@ void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen redisFreeCommand(temp); temp = nullptr; } + len = 0; - len = redisFormatCommandArgv(&temp, argc, argv, argvlen); - if (len == -1) { + int ret = redisFormatCommandArgv(&temp, argc, argv, argvlen); + if (ret == -1) { throw std::bad_alloc(); } + len = ret; } void RedisCommand::format(const vector &commands) @@ -135,6 +139,8 @@ std::string RedisCommand::toPrintableString() const const char *RedisCommand::c_str() const { + if (len == 0) + return nullptr; return temp; } diff --git a/tests/Makefile.am b/tests/Makefile.am index 2be3114ea..ac69ddce6 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -5,6 +5,7 @@ LDADD_GTEST = -L/usr/src/gtest -lgtest -lgtest_main -lgmock -lgmock_main tests_tests_SOURCES = tests/redis_ut.cpp \ tests/redis_piped_ut.cpp \ + tests/redis_command_ut.cpp \ tests/redis_state_ut.cpp \ tests/redis_piped_state_ut.cpp \ tests/tokenize_ut.cpp \ @@ -44,5 +45,5 @@ tests_tests_SOURCES = tests/redis_ut.cpp \ tests/main.cpp tests_tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) -tests_tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) +tests_tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) -fno-access-control tests_tests_LDADD = $(LDADD_GTEST) -lpthread common/libswsscommon.la $(LIBNL_LIBS) $(CODE_COVERAGE_LIBS) sonic-db-cli/libsonicdbcli.la -lzmq -luuid -lboost_serialization diff --git a/tests/redis_command_ut.cpp b/tests/redis_command_ut.cpp new file mode 100644 index 000000000..4e7e40922 --- /dev/null +++ b/tests/redis_command_ut.cpp @@ -0,0 +1,12 @@ +#include "gtest/gtest.h" +#include "common/rediscommand.h" + +TEST(RedisCommand, invalid_redis_command) +{ + swss::RedisCommand cmd; + EXPECT_THROW(cmd.format("Invalid redis command %l^", 1), std::invalid_argument); + EXPECT_EQ(cmd.c_str(), nullptr); + EXPECT_EQ(cmd.length(), 0); + EXPECT_EQ(cmd.len, 0); + EXPECT_EQ(cmd.temp, nullptr); +} From dc5135d735ab9ff3c35c9b0135e9c83410d8210f Mon Sep 17 00:00:00 2001 From: DavidZagury <32644413+DavidZagury@users.noreply.github.com> Date: Mon, 14 Aug 2023 03:56:18 +0300 Subject: [PATCH 5/9] Fix minor issue found during static code scan (#740) Fix issue of using uninitialized value on unit tests. --- tests/stringutility_ut.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/stringutility_ut.cpp b/tests/stringutility_ut.cpp index 0536958b0..c72ecdc63 100644 --- a/tests/stringutility_ut.cpp +++ b/tests/stringutility_ut.cpp @@ -8,7 +8,7 @@ TEST(STRINGUTILITY, cast_int) { - int i; + int i = 0; EXPECT_NO_THROW(swss::lexical_convert("123", i)); EXPECT_EQ(i, 123); @@ -39,7 +39,7 @@ TEST(STRINGUTILITY, cast_alpha_bool) TEST(STRINGUTILITY, cast_mix) { - int i; + int i = 0; swss::AlphaBoolean b; std::string s; From 5b6377cd260e5421470263ebd7e27ac8aca42c52 Mon Sep 17 00:00:00 2001 From: Oleksandr Ivantsiv Date: Sat, 19 Aug 2023 17:31:39 -0700 Subject: [PATCH 6/9] [dash] Add DASH_PREFIX_TAG_TABLE table to the schema. (#805) --- common/schema.h | 1 + 1 file changed, 1 insertion(+) diff --git a/common/schema.h b/common/schema.h index c8aa89c8e..a9fc48928 100644 --- a/common/schema.h +++ b/common/schema.h @@ -147,6 +147,7 @@ namespace swss { #define APP_DASH_ACL_OUT_TABLE_NAME "DASH_ACL_OUT_TABLE" #define APP_DASH_ACL_GROUP_TABLE_NAME "DASH_ACL_GROUP_TABLE" #define APP_DASH_ACL_RULE_TABLE_NAME "DASH_ACL_RULE_TABLE" +#define APP_DASH_PREFIX_TAG_TABLE_NAME "DASH_PREFIX_TAG_TABLE" #define APP_DASH_ROUTING_TYPE_TABLE_NAME "DASH_ROUTING_TYPE_TABLE" #define APP_DASH_APPLIANCE_TABLE_NAME "DASH_APPLIANCE_TABLE" #define APP_DASH_ROUTE_TABLE_NAME "DASH_ROUTE_TABLE" From f8a23c3a487c7f99a19931e282a482ebc0e9f6c5 Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Wed, 23 Aug 2023 23:47:49 +0800 Subject: [PATCH 7/9] [azp] Fix swss missing libs (#815) Swss containter needs dash_api to properly build, based on https://github.com/sonic-net/sonic-swss/pull/2722, attempting to add missing libs Signed-off-by: Ze Gan --- .azure-pipelines/build-swss-template.yml | 4 ++++ .../test-docker-sonic-vs-template.yml | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/.azure-pipelines/build-swss-template.yml b/.azure-pipelines/build-swss-template.yml index a9c19bfcb..5bda87c25 100644 --- a/.azure-pipelines/build-swss-template.yml +++ b/.azure-pipelines/build-swss-template.yml @@ -107,6 +107,10 @@ jobs: target/debs/${{ parameters.debian_version }}/libnl-nf*.deb target/debs/${{ parameters.debian_version }}/libyang-*.deb target/debs/${{ parameters.debian_version }}/libyang_*.deb + target/debs/${{ parameters.debian_version }}/libprotobuf*.deb + target/debs/${{ parameters.debian_version }}/libprotoc*.deb + target/debs/${{ parameters.debian_version }}/protobuf-compiler*.deb + target/debs/${{ parameters.debian_version }}/libdashapi*.deb displayName: "Download common libs" - script: | diff --git a/.azure-pipelines/test-docker-sonic-vs-template.yml b/.azure-pipelines/test-docker-sonic-vs-template.yml index 8eb3ea533..7c0998fba 100644 --- a/.azure-pipelines/test-docker-sonic-vs-template.yml +++ b/.azure-pipelines/test-docker-sonic-vs-template.yml @@ -6,6 +6,10 @@ parameters: - name: log_artifact_name type: string +- name: sonic_buildimage_ubuntu20_04 + type: string + default: '$(BUILD_BRANCH)' + jobs: - job: displayName: vstest @@ -35,6 +39,16 @@ jobs: artifact: sonic-swss-common.amd64.ubuntu20_04 path: $(Build.ArtifactStagingDirectory)/download displayName: "Download pre-stage built sonic-swss-common.amd64.ubuntu20_04" + - task: DownloadPipelineArtifact@2 + inputs: + source: specific + project: build + pipeline: sonic-net.sonic-buildimage-ubuntu20.04 + artifact: sonic-buildimage.amd64.ubuntu20_04 + runVersion: 'latestFromBranch' + runBranch: 'refs/heads/${{ parameters.sonic_buildimage_ubuntu20_04 }}' + path: $(Build.ArtifactStagingDirectory)/download + displayName: "Download sonic buildimage ubuntu20.04 deb packages" - script: | set -ex @@ -42,6 +56,8 @@ jobs: 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/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 From 2365fe81f14197330c2034dd8231b9dfb49016b9 Mon Sep 17 00:00:00 2001 From: Zain Budhwani <99770260+zbud-msft@users.noreply.github.com> Date: Thu, 24 Aug 2023 17:03:01 -0700 Subject: [PATCH 8/9] Add check before deserializing messages (#812) --- common/events_common.h | 4 +++- tests/events_common_ut.cpp | 7 +++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/common/events_common.h b/common/events_common.h index 3bcc66754..92770d7c5 100644 --- a/common/events_common.h +++ b/common/events_common.h @@ -258,7 +258,9 @@ struct serialization int deserialize(const string& s, Map& data) { - + if (s.size() < 2) { // zmq identifying message of length 1 + return 0; + } try { istringstream ss(s); boost::archive::text_iarchive iarch(ss); diff --git a/tests/events_common_ut.cpp b/tests/events_common_ut.cpp index 7df48588b..485a9fcda 100644 --- a/tests/events_common_ut.cpp +++ b/tests/events_common_ut.cpp @@ -81,10 +81,10 @@ TEST(events_common, send_recv) void *sock_p1 = zmq_socket (zmq_ctx, ZMQ_PAIR); EXPECT_EQ(0, zmq_bind (sock_p1, path)); - string source("Hello"), source1; + string source("Hello"), source1, source2("#"); map m = {{"foo", "bar"}, {"hello", "world"}, {"good", "day"}}; - map m1; + map m1, m2; EXPECT_EQ(0, zmq_message_send(sock_p0, source, m)); @@ -92,6 +92,9 @@ TEST(events_common, send_recv) EXPECT_EQ(source, source1); EXPECT_EQ(m, m1); + + EXPECT_EQ(0, deserialize(source2, m2)); + zmq_close(sock_p0); zmq_close(sock_p1); zmq_ctx_term(zmq_ctx); From dc14d3627c6516495fd5d8c9579bdb939a4c9930 Mon Sep 17 00:00:00 2001 From: Oleksandr Ivantsiv Date: Tue, 29 Aug 2023 14:48:12 -0700 Subject: [PATCH 9/9] Fix the issue of ignoring callback calls for removed keys. (#811) **What I did** Fix the issue of ignoring callback calls for removed keys. **Why I did it** ConfigDBConnector.listen method has a caching mechanism (added in https://github.com/sonic-net/sonic-swss-common/pull/587 PR) that preloads the DB state before starting. When the notification about the changed key is received the listen method gets key data from the DB (in all cases when the key was added, updated, or removed) and compares the data with the cache. It fires the callback only if data differ from the cache. Otherwise, the callback is ignored. If the `client.hgetall(key)` is called for the removed key it returns an empty dictionary (`{}`). This can be confused with the data of the key with no attributes. For example: `{"TABLE": {"KEY": {}}}`. So if preloaded data contains a key with no attributes and the listener method receives a notification about the removal of such key the callback is not called. The listener will simply remove the key from the cache without calling the callback. This leads to the situation when the removal of the key is not handled. The solution is to get data for the added or updated keys, and for the removed keys use `None` instead. This will ensure that the comparison with the cache will work as expected. **How I verified it** Compile the package and run the unit test. Unit tests are extended to cover the expected behavior. --- common/configdb.h | 7 +++- tests/test_redis_ut.py | 90 +++++++++++++++++++++++++++++++++--------- 2 files changed, 76 insertions(+), 21 deletions(-) diff --git a/common/configdb.h b/common/configdb.h index 5acba65e5..dc5eea52e 100644 --- a/common/configdb.h +++ b/common/configdb.h @@ -105,8 +105,11 @@ class ConfigDBConnector_Native : public SonicV2Connector_Native try: (table, row) = key.split(self.TABLE_NAME_SEPARATOR, 1) if table in self.handlers: - client = self.get_redis_client(self.db_name) - data = self.raw_to_typed(client.hgetall(key)) + if item['data'] == 'del': + data = None + else: + client = self.get_redis_client(self.db_name) + data = self.raw_to_typed(client.hgetall(key)) if table in init_data and row in init_data[table]: cache_hit = init_data[table][row] == data del init_data[table][row] diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index 17322b57d..2a8de621e 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -634,46 +634,98 @@ def thread_coming_entry(): def test_ConfigDBInit(): table_name_1 = 'TEST_TABLE_1' table_name_2 = 'TEST_TABLE_2' + table_name_3 = 'TEST_TABLE_3' test_key = 'key1' - test_data = {'field1': 'value1'} - test_data_update = {'field1': 'value2'} + test_data = {'field1': 'value1', 'field2': 'value2'} + + queue = multiprocessing.Queue() manager = multiprocessing.Manager() ret_data = manager.dict() - def test_handler(table, key, data, ret): - ret[table] = {key: data} - - def test_init_handler(data, ret): + def test_handler(table, key, data, ret, q=None): + if table not in ret: + ret[table] = {} + if data is None: + ret[table] = {k: v for k, v in ret[table].items() if k != key} + if q: + q.put(ret[table]) + elif key not in ret[table] or ret[table][key] != data: + ret[table] = {key: data} + if q: + q.put(ret[table]) + + def test_init_handler(data, ret, queue): ret.update(data) + queue.put(ret) - def thread_listen(ret): + def thread_listen(ret, queue): config_db = ConfigDBConnector() config_db.connect(wait_for_init=False) - config_db.subscribe(table_name_1, lambda table, key, data: test_handler(table, key, data, ret), + config_db.subscribe(table_name_1, lambda table, key, data: test_handler(table, key, data, ret, queue), fire_init_data=False) - config_db.subscribe(table_name_2, lambda table, key, data: test_handler(table, key, data, ret), + config_db.subscribe(table_name_2, lambda table, key, data: test_handler(table, key, data, ret, queue), fire_init_data=True) + config_db.subscribe(table_name_3, lambda table, key, data: test_handler(table, key, data, ret, queue), + fire_init_data=False) - config_db.listen(init_data_handler=lambda data: test_init_handler(data, ret)) + config_db.listen(init_data_handler=lambda data: test_init_handler(data, ret, queue)) config_db = ConfigDBConnector() config_db.connect(wait_for_init=False) client = config_db.get_redis_client(config_db.CONFIG_DB) client.flushdb() - # Init table data - config_db.set_entry(table_name_1, test_key, test_data) - config_db.set_entry(table_name_2, test_key, test_data) + # Prepare unique data per each table to track if correct data are received in the update + table_1_data = {f'{table_name_1}_{k}': v for k, v in test_data.items()} + config_db.set_entry(table_name_1, test_key, table_1_data) + table_2_data = {f'{table_name_2}_{k}': v for k, v in test_data.items()} + config_db.set_entry(table_name_2, test_key, table_2_data) + config_db.set_entry(table_name_3, test_key, {}) - thread = multiprocessing.Process(target=thread_listen, args=(ret_data,)) - thread.start() - time.sleep(5) - thread.terminate() + # Run the listener in a separate process. It is not possible to stop a listener when it is running as a thread. + # When it runs in a separate process we can terminate it with a signal. + listener = multiprocessing.Process(target=thread_listen, args=(ret_data, queue)) + listener.start() - assert ret_data[table_name_1] == {test_key: test_data} - assert ret_data[table_name_2] == {test_key: test_data} + try: + # During the subscription to table 2 'fire_init_data=True' is passed. The callback should be called before the listener. + # Verify that callback is fired before listener initialization. + data = queue.get(timeout=5) + assert data == {test_key: table_2_data} + + # Wait for init data + init_data = queue.get(timeout=5) + + # Verify that all tables initialized correctly + assert init_data[table_name_1] == {test_key: table_1_data} + assert init_data[table_name_2] == {test_key: table_2_data} + assert init_data[table_name_3] == {test_key: {}} + + # Remove one key-value pair from the data. Verify that the entry is updated correctly + table_1_data.popitem() + config_db.set_entry(table_name_1, test_key, table_1_data) + data = queue.get(timeout=5) + assert data == {test_key: table_1_data} + + # Remove all key-value pairs. Verify that the table still contains key + config_db.set_entry(table_name_1, test_key, {}) + data = queue.get(timeout=5) + assert data == {test_key: {}} + + # Remove the key + config_db.set_entry(table_name_1, test_key, None) + data = queue.get(timeout=5) + assert test_key not in data + + # Remove the entry (with no attributes) from the table. + # Verify that the update is received and a callback is called + config_db.set_entry(table_name_3, test_key, None) + table_3_data = queue.get(timeout=5) + assert test_key not in table_3_data + finally: + listener.terminate() def test_DBConnectFailure():