From 9028d55d41a75fc5cb796556aabe3a44487096a4 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Tue, 23 Sep 2025 14:53:30 +0000 Subject: [PATCH 1/3] Add multiple key handling --- sonic-chassisd/scripts/chassisd | 4 ++ sonic-chassisd/tests/test_dpu_chassisd.py | 82 +++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 23f9f927b..49777df21 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -1555,6 +1555,10 @@ class DpuStateManagerTask(ProcessTaskBase): if result is None: continue key, op, fvp = result # Changed from _ to fvp to match what we use below + if key != '' and s.getDbConnector().getDbName() != 'CHASSIS_STATE_DB': + # If there is any change in the non-DPU_STATE table, we need to update the state + update_required = True + break # Check if this is the DPU_STATE table if s.getDbConnector().getDbName() == 'CHASSIS_STATE_DB': # Don't update if this is a change for another DPU diff --git a/sonic-chassisd/tests/test_dpu_chassisd.py b/sonic-chassisd/tests/test_dpu_chassisd.py index 39e5665f5..293699814 100644 --- a/sonic-chassisd/tests/test_dpu_chassisd.py +++ b/sonic-chassisd/tests/test_dpu_chassisd.py @@ -4,6 +4,8 @@ import pytest import signal import threading +import time +from datetime import datetime from imp import load_source import re @@ -455,3 +457,83 @@ def hset(key, field, value): # Verify current states were updated assert dpu_state_mng.current_dp_state == 'up' assert dpu_state_mng.current_cp_state == 'down' + + +def test_dpu_state_manager_update_required_logic(): + """Test that DpuStateManagerTask correctly sets update_required based on various conditions""" + chassis = MockDpuChassis() + chassis.get_dpu_id = MagicMock(return_value=0) + chassis.get_dataplane_state = MagicMock(return_value=True) + chassis.get_controlplane_state = MagicMock(return_value=True) + + chassis_state_db = {} + update_count = 0 + + def hset(key, field, value): + nonlocal update_count + update_count += 1 + if key not in chassis_state_db: + chassis_state_db[key] = {} + chassis_state_db[key][field] = value + + # Test case 1: update_required should be True when there are changes in non-DPU_STATE tables + with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset): + with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', + return_value=('PORT_TABLE_KEY', 'SET', None)): + with mock.patch.object(swsscommon.Select, 'select', + side_effect=[(swsscommon.Select.OBJECT, None), KeyboardInterrupt]): + + dpu_updater = DpuStateUpdater(SYSLOG_IDENTIFIER, chassis) + dpu_updater._time_now = MagicMock(return_value='Sat Jan 01 12:00:00 AM UTC 2000') + + dpu_state_mng = DpuStateManagerTask(SYSLOG_IDENTIFIER, dpu_updater) + dpu_state_mng.current_dp_state = 'up' + dpu_state_mng.current_cp_state = 'up' + + # Mock the selectable to return a non-CHASSIS_STATE_DB table + mock_selectable = MagicMock() + mock_selectable.getDbConnector.return_value.getDbName.return_value = 'APPL_DB' + dpu_state_mng.task_worker() + + # Verify state was updated since update_required should be True + assert update_count > 0 + + # Reset for test case 2 + update_count = 0 + chassis_state_db = {} + + # Test case 2: update_required should be True when pop returns multiple values + # and one key returns STATE_DB and another CHASSIS_STATE_DB + with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset): + # Mock pop to return multiple values (simulating multiple changes) + def mock_pop_multiple(): + return ('SYSTEM_READY_KEY', 'SET', (('Status', 'UP'), ('Other', 'Value'))) + + # Mock selectables with different database names + mock_selectable_state_db = MagicMock() + mock_selectable_state_db.getDbConnector.return_value.getDbName.return_value = 'STATE_DB' + + mock_selectable_chassis_state_db = MagicMock() + mock_selectable_chassis_state_db.getDbConnector.return_value.getDbName.return_value = 'CHASSIS_STATE_DB' + + # Mock the selectables list to include both types + mock_selectables = [mock_selectable_state_db, mock_selectable_chassis_state_db] + + with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', side_effect=mock_pop_multiple): + with mock.patch.object(swsscommon.Select, 'select', + side_effect=[(swsscommon.Select.OBJECT, None), KeyboardInterrupt]): + + dpu_updater = DpuStateUpdater(SYSLOG_IDENTIFIER, chassis) + dpu_updater._time_now = MagicMock(return_value='Sat Jan 01 12:00:00 AM UTC 2000') + + dpu_state_mng = DpuStateManagerTask(SYSLOG_IDENTIFIER, dpu_updater) + dpu_state_mng.current_dp_state = 'up' + dpu_state_mng.current_cp_state = 'up' + + # Mock the selectables to return different database names + with mock.patch.object(dpu_state_mng, 'selectable', mock_selectables): + dpu_state_mng.task_worker() + + # Verify state was updated since update_required should be True for multiple values + # even with mixed STATE_DB and CHASSIS_STATE_DB + assert update_count > 0 From 08ab51e1003e27438956a9f8299b0ea5df23fc8f Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Tue, 23 Sep 2025 15:11:36 +0000 Subject: [PATCH 2/3] Test fix --- sonic-chassisd/tests/test_dpu_chassisd.py | 48 +++++++++++++++-------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/sonic-chassisd/tests/test_dpu_chassisd.py b/sonic-chassisd/tests/test_dpu_chassisd.py index 293699814..118c7df37 100644 --- a/sonic-chassisd/tests/test_dpu_chassisd.py +++ b/sonic-chassisd/tests/test_dpu_chassisd.py @@ -478,8 +478,25 @@ def hset(key, field, value): # Test case 1: update_required should be True when there are changes in non-DPU_STATE tables with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset): - with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', - return_value=('PORT_TABLE_KEY', 'SET', None)): + # Create mock selectables + mock_selectable_app_db = MagicMock() + mock_selectable_app_db.getDbConnector.return_value.getDbName.return_value = 'APPL_DB' + mock_selectable_app_db.pop.return_value = ('PORT_TABLE_KEY', 'SET', None) + + mock_selectable_state_db = MagicMock() + mock_selectable_state_db.getDbConnector.return_value.getDbName.return_value = 'STATE_DB' + mock_selectable_state_db.pop.return_value = None + + mock_selectable_chassis_state_db = MagicMock() + mock_selectable_chassis_state_db.getDbConnector.return_value.getDbName.return_value = 'CHASSIS_STATE_DB' + mock_selectable_chassis_state_db.pop.return_value = None + + # Mock the SubscriberStateTable constructor to return our mock selectables + with mock.patch.object(swsscommon, 'SubscriberStateTable', side_effect=[ + mock_selectable_app_db, # PORT_TABLE + mock_selectable_state_db, # SYSTEM_READY + mock_selectable_chassis_state_db # DPU_STATE + ]): with mock.patch.object(swsscommon.Select, 'select', side_effect=[(swsscommon.Select.OBJECT, None), KeyboardInterrupt]): @@ -490,9 +507,6 @@ def hset(key, field, value): dpu_state_mng.current_dp_state = 'up' dpu_state_mng.current_cp_state = 'up' - # Mock the selectable to return a non-CHASSIS_STATE_DB table - mock_selectable = MagicMock() - mock_selectable.getDbConnector.return_value.getDbName.return_value = 'APPL_DB' dpu_state_mng.task_worker() # Verify state was updated since update_required should be True @@ -505,21 +519,25 @@ def hset(key, field, value): # Test case 2: update_required should be True when pop returns multiple values # and one key returns STATE_DB and another CHASSIS_STATE_DB with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset): - # Mock pop to return multiple values (simulating multiple changes) - def mock_pop_multiple(): - return ('SYSTEM_READY_KEY', 'SET', (('Status', 'UP'), ('Other', 'Value'))) + # Create mock selectables with different database names + mock_selectable_app_db = MagicMock() + mock_selectable_app_db.getDbConnector.return_value.getDbName.return_value = 'APPL_DB' + mock_selectable_app_db.pop.return_value = None - # Mock selectables with different database names mock_selectable_state_db = MagicMock() mock_selectable_state_db.getDbConnector.return_value.getDbName.return_value = 'STATE_DB' + mock_selectable_state_db.pop.return_value = ('SYSTEM_READY_KEY', 'SET', (('Status', 'UP'), ('Other', 'Value'))) mock_selectable_chassis_state_db = MagicMock() mock_selectable_chassis_state_db.getDbConnector.return_value.getDbName.return_value = 'CHASSIS_STATE_DB' + mock_selectable_chassis_state_db.pop.return_value = None - # Mock the selectables list to include both types - mock_selectables = [mock_selectable_state_db, mock_selectable_chassis_state_db] - - with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', side_effect=mock_pop_multiple): + # Mock the SubscriberStateTable constructor to return our mock selectables + with mock.patch.object(swsscommon, 'SubscriberStateTable', side_effect=[ + mock_selectable_app_db, # PORT_TABLE + mock_selectable_state_db, # SYSTEM_READY + mock_selectable_chassis_state_db # DPU_STATE + ]): with mock.patch.object(swsscommon.Select, 'select', side_effect=[(swsscommon.Select.OBJECT, None), KeyboardInterrupt]): @@ -530,9 +548,7 @@ def mock_pop_multiple(): dpu_state_mng.current_dp_state = 'up' dpu_state_mng.current_cp_state = 'up' - # Mock the selectables to return different database names - with mock.patch.object(dpu_state_mng, 'selectable', mock_selectables): - dpu_state_mng.task_worker() + dpu_state_mng.task_worker() # Verify state was updated since update_required should be True for multiple values # even with mixed STATE_DB and CHASSIS_STATE_DB From ec846e353b2862c26111e306814516fd1814f6e2 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Tue, 23 Sep 2025 20:05:07 +0000 Subject: [PATCH 3/3] Review comment fix --- sonic-chassisd/scripts/chassisd | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sonic-chassisd/scripts/chassisd b/sonic-chassisd/scripts/chassisd index 49777df21..42f4dee6f 100755 --- a/sonic-chassisd/scripts/chassisd +++ b/sonic-chassisd/scripts/chassisd @@ -1555,10 +1555,6 @@ class DpuStateManagerTask(ProcessTaskBase): if result is None: continue key, op, fvp = result # Changed from _ to fvp to match what we use below - if key != '' and s.getDbConnector().getDbName() != 'CHASSIS_STATE_DB': - # If there is any change in the non-DPU_STATE table, we need to update the state - update_required = True - break # Check if this is the DPU_STATE table if s.getDbConnector().getDbName() == 'CHASSIS_STATE_DB': # Don't update if this is a change for another DPU @@ -1573,6 +1569,10 @@ class DpuStateManagerTask(ProcessTaskBase): update_required = False continue self.logger.log_info(f"DPU_STATE change detected: operation={op}, key={key}") + elif key: + # If there is any change in the non-DPU_STATE table, we need to update the state + update_required = True + break if update_required: [self.current_dp_state, self.current_cp_state] = self.dpu_state_updater.update_state()