From e0f394cd44370370e872711b04a15e497b63e0f8 Mon Sep 17 00:00:00 2001 From: Oleksandr Ivantsiv Date: Mon, 10 Jul 2023 09:15:58 +0200 Subject: [PATCH] Fix the issue of ignoring callback calls for removed keys. (#789) **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 | 2 +- tests/test_redis_ut.py | 45 +++++++++++++++++++++++++++++++----------- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/common/configdb.h b/common/configdb.h index 5acba65e..7f2a602c 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 raw_data is None: + if not raw_data or not raw_data.keys(): 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 17322b57..363f1e06 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -634,20 +634,29 @@ 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'} + + queue = multiprocessing.Queue() manager = multiprocessing.Manager() ret_data = manager.dict() - def test_handler(table, key, data, ret): - ret[table] = {key: data} + 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_init_handler(data, ret): + 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) @@ -655,8 +664,10 @@ def thread_listen(ret): 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)) + config_db.listen(init_data_handler=lambda data: test_init_handler(data, ret, queue)) config_db = ConfigDBConnector() config_db.connect(wait_for_init=False) @@ -666,14 +677,26 @@ def thread_listen(ret): # 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,)) + thread = multiprocessing.Process(target=thread_listen, args=(ret_data, queue)) thread.start() - 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} + 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 + + thread.terminate() def test_DBConnectFailure():