Add API endpoints to ConfigDBConnector to support pre-loading data without blackout #587

alexrallen · 2022-03-07T17:55:50Z

Currently if someone wishes to use ConfigDBConnector to operate on updates coming from CONFIG_DB AND they wish to pull all the initial data from the table to initialize their only option is to pull the data and then run the listen() method on ConfigDBConnector.

This creates a "blackout" period where between the time the tables are downloaded and the listen() method is called, any new updates to CONFIG_DB will not be handled.

To resolve this I have changed two things in ConfigDBConnector. All of which are 100% backwards compatible...

I split listen() into listen() and process() where listen() may now be called with start=False as an argument which does not start immediately processing updates from CONFIG_DB and calling handlers. This allows you to call listen() first and then download table data and then call process() to start processing updates. This way no updates will be missed as they will be queued by redis while the table data is being downloaded by the consumer.
I added a cache argument to process in which you pass any initial table data your system is operating on. When a table update is processed the system checks if that keys value is different than what is stored in the cache (if it is present) and only fires the callback handler in the case that the data has changes (added / deleted / updated). This prevents the same data from being processed multiple times.

…for initialization without blackout

common/configdb.h

qiluo-msft · 2022-03-29T23:13:57Z

tests/test_redis_ut.py

+    client.flushdb()
+
+    # Init table data
+    config_db.set_entry(table_name_1, test_key, test_data_1)


test_data_1

Please check unit test failure:
NameError: global name 'test_data_1' is not defined
#Closed

alexrallen · 2022-03-30T18:47:29Z

@qiluo-msft Checkers are passing please take a look. Thanks.

dgsudharsan · 2022-03-30T20:34:34Z

@qiluo-msft Can you please help to merge this PR?

… updates (#10168) #### Why I did it As of sonic-net/sonic-swss-common#587 the blackout issue in ConfigDBConnector has been resolved. In the past hostcfgd was refactored to use SubscriberStateTable instead of ConfigDBConnector for subscribing to CONFIG_DB updates due to a "blackout" period between hostcfgd pulling the table data down and running the initialization and actually calling `listen()` on ConfigDBConnector which starts the update handler. However SusbscriberStateTable creates many file descriptors against the redis DB which is inefficient compared to ConfigDBConnector which only opens a single file descriptor. With the new fix to ConfigDBConnector I refactored hostcfgd to take advantage of these updates. #### How I did it Replaced SubscriberStateTable with ConfigDBConnector #### How to verify it The functionality of hostcfgd can be verified by booting the switch and verifying that NTP is properly configured. To check the blackout period you can add a delay in the hostcfgd `load()` function and also add a print statement before and after the load so you know when it occurs. Then restart hostcfgd and wait for the load to start, then during the load push a partial change to the FEATURE table and verify that the change is picked up and the feature is enabled after the load period finishes. #### Description for the changelog [hostcfgd] Move hostcfgd back to ConfigDBConnector for subscribing to updates

…thout blackout (sonic-net#587) Currently if someone wishes to use ConfigDBConnector to operate on updates coming from CONFIG_DB AND they wish to pull all the initial data from the table to initialize their only option is to pull the data and then run the `listen()` method on ConfigDBConnector. This creates a "blackout" period where between the time the tables are downloaded and the `listen()` method is called, any new updates to CONFIG_DB will not be handled. To resolve this I have changed two things in ConfigDBConnector. All of which are 100% backwards compatible... 1. I split `listen()` into `listen()` and `process()` where `listen()` may now be called with `start=False` as an argument which does not start immediately processing updates from CONFIG_DB and calling handlers. This allows you to call `listen()` *first* and then download table data and then call `process()` to start processing updates. This way no updates will be missed as they will be queued by redis while the table data is being downloaded by the consumer. 2. I added a `cache` argument to `process` in which you pass any initial table data your system is operating on. When a table update is processed the system checks if that keys value is different than what is stored in the cache (if it is present) and only fires the callback handler in the case that the data has changes (added / deleted / updated). This prevents the same data from being processed multiple times.

**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 #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.

…#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 sonic-net#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.

…#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 sonic-net#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.

alexrallen added 2 commits February 23, 2022 20:27

Add infra to allow consumers of ConfigDBConnector to load table data …

4ab5d1d

…for initialization without blackout

Add explanatory comment

a75c233

alexrallen mentioned this pull request Mar 7, 2022

[hostcfgd] Move hostcfgd back to ConfigDBConnector for subscribing to updates sonic-net/sonic-buildimage#10168

Merged

6 tasks

dgsudharsan requested review from qiluo-msft and renukamanavalan March 7, 2022 20:27

dgsudharsan previously approved these changes Mar 7, 2022

View reviewed changes

renukamanavalan previously approved these changes Mar 8, 2022

View reviewed changes

qiluo-msft reviewed Mar 8, 2022

View reviewed changes