psud: Adding Redis Pipeline for performance improvement #711
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
GauravNagesh
force-pushed
the
psud-redis-pipeline
branch
from
0454141 to
f728bfb
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR introduces performance optimizations to the PSU daemon by implementing Redis batching through RedisPipeline. Instead of maintaining separate database connections for each table and sending individual requests, the daemon now uses a shared RedisPipeline instance that batches multiple operations together before flushing them to STATE_DB.
Key changes:
- Replaced individual Table connections with a shared RedisPipeline instance (batch_size=10)
- Added periodic pipeline flush at the end of each monitoring cycle with exception handling
- Updated test mocks to support the new RedisPipeline and buffered Table constructors
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| sonic-psud/scripts/psud | Implemented RedisPipeline for batching database operations across all tables and added flush logic with error handling in the main run loop |
| sonic-psud/tests/test_DaemonPsud.py | Added test coverage for Redis pipeline flush exception scenario |
| sonic-psud/tests/mocked_libs/swsscommon/swsscommon.py | Added RedisPipeline mock class and updated Table mock to support both legacy and buffered constructors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sonic-psud/scripts/psud
Outdated
| if self.first_run: | ||
| self.first_run = False | ||
|
|
||
| # flush any remaining requests on the pipeline to STAT_DB at end of every cycle |
There was a problem hiding this comment.
Typo in comment: "STAT_DB" should be "STATE_DB" to match the actual database name used throughout the codebase.
| # flush any remaining requests on the pipeline to STAT_DB at end of every cycle | |
| # flush any remaining requests on the pipeline to STATE_DB at end of every cycle |
| class Table: | ||
| def __init__(self, db, table_name): | ||
| def __init__(self, db_or_pipeline, table_name, buffered=False): | ||
| # Mock to support both both constructors (db, table_name) and (pipeline, table_name, buffered) |
There was a problem hiding this comment.
Duplicate word in comment: "both both" should be "both".
| # Mock to support both both constructors (db, table_name) and (pipeline, table_name, buffered) | |
| # Mock to support both constructors (db, table_name) and (pipeline, table_name, buffered) |
| daemon_psud.run() | ||
| assert daemon_psud.first_run is False |
There was a problem hiding this comment.
The test should verify that statedb_redisPipeline.flush() is called during the successful execution path. Currently, only the exception scenario is explicitly tested. Consider adding an assertion like daemon_psud.statedb_redisPipeline.flush.assert_called() after line 108 to verify the flush operation is invoked in the normal case.
|
|
||
| class Table: | ||
| def __init__(self, db, table_name): | ||
| def __init__(self, db_or_pipeline, table_name, buffered=False): |
There was a problem hiding this comment.
Extra whitespace before parameter: there are two spaces before db_or_pipeline. Remove one space for consistent formatting.
| def __init__(self, db_or_pipeline, table_name, buffered=False): | |
| def __init__(self, db_or_pipeline, table_name, buffered=False): |
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | ||
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | ||
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | ||
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) |
There was a problem hiding this comment.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) | |
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE) |
There was a problem hiding this comment.
This parameter is needed for the Redis Pipeline implementation and the Table constructor actually does allow these arguments as shown here Table Constructor
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | ||
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | ||
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | ||
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) |
There was a problem hiding this comment.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) | |
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE) |
There was a problem hiding this comment.
This parameter is needed for the Redis Pipeline implementation and the Table constructor actually does allow these arguments as shown here Table Constructor
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | ||
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | ||
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | ||
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) |
There was a problem hiding this comment.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) | |
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE) |
There was a problem hiding this comment.
This parameter is needed for the Redis Pipeline implementation and the Table constructor actually does allow these arguments as shown here Table Constructor
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | ||
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | ||
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | ||
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) |
There was a problem hiding this comment.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
Call to Table.init with too many arguments; should be no more than 2.
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE, True) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE, True) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE, True) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE, True) | |
| self.chassis_tbl = swsscommon.Table(self.statedb_redisPipeline, CHASSIS_INFO_TABLE) | |
| self.psu_tbl = swsscommon.Table(self.statedb_redisPipeline, PSU_INFO_TABLE) | |
| self.fan_tbl = swsscommon.Table(self.statedb_redisPipeline, FAN_INFO_TABLE) | |
| self.phy_entity_tbl = swsscommon.Table(self.statedb_redisPipeline, PHYSICAL_ENTITY_INFO_TABLE) |
There was a problem hiding this comment.
This parameter is needed for the Redis Pipeline implementation and the Table constructor actually does allow these arguments as shown here Table Constructor
| def flush(self): | ||
| # Mock flush operation - just clear the queue | ||
| self.queue.clear() | ||
| pass |
There was a problem hiding this comment.
Unnecessary 'pass' statement.
Signed-off-by: GauravNagesh <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 participants
Description
This PR introduces significant performance improvements to the PSU daemon by optimizing Redis database operations through batching. The key change is Batching.
The daemon now uses
RedisPipeline, which instead of sending 1 request separately each time, batches multiple Redis request together and sends them. Also flushes any unfilled batches at the end of each monitoring cycle if any requests still remain. This reduces the number of round trips to the Redis database.Additionally, existing tests were updated to include above change changes and to mock the new
RedisPipelineandFieldValuePairsconstructors.Motivation and Context
Why RedisPipeline and Batching :
Currently, the psud daemon creates and maintains a separate db connection internally for each table used and an additional initial connection to state_db as shown here Table Constructor and RedisPipeline Constructor
So in total we maintain 5 separate Redis database connections in psud (1 connection each for
CHASSIS_INFO_TABLE,PSU_INFO_TABLE,FAN_INFO_TABLE,PHYSICAL_ENTITY_INFO_TABLEand the initial connection toSTATE_DB) which is waste of Redis and system resources. Moreover, since psud purely a single process executing all requests in a sequential manner, we don't need multiple connections. Just one connection shared across all the tables is sufficient.Also in the existing flow, individual
set()calls are made for each device and their attribute. Rather we can collect all device data in batches using RedisPipeline and then do a single bulk update, which will be faster.We can also optimize database cleanup operations, where we do multiple individual delete operations in
__del__method for each table key across all tables. Rather we can also batch the delete operations using RedisPipeline for faster cleanup during daemon shutdown, which will be quick in signal handling and reboot scenarios.Overall benefit :
How Has This Been Tested?
The changes were tested on both virtual and physical platforms. Performance benchmarking was conducted to compare the baseline and the updated implementation, capturing relevant metrics across all database operations.
Additional Information (Optional)