Changes as per previous discussion

Author: gpunathilell

sonic-chassisd/scripts/chassisd

@@ -104,6 +104,12 @@ MODULE_ADMIN_UP = 1
 MODULE_REBOOT_CAUSE_DIR = "/host/reboot-cause/module/"
 MAX_HISTORY_FILES = 10
 
+DP_STATE = 'dpu_data_plane_state'
+DP_UPDATE_TIME = 'dpu_data_plane_time'
+CP_STATE = 'dpu_control_plane_state'
+CP_UPDATE_TIME = 'dpu_control_plane_time'
+
+
 # This daemon should return non-zero exit code so that supervisord will
 # restart it automatically.
 exit_code = 0
@@ -819,16 +825,19 @@ class SmartSwitchModuleUpdater(ModuleUpdater):
             if not self.chassis_state_db:
                 self.chassis_state_db = daemon_base.db_connect("CHASSIS_STATE_DB")
 
-            # If state is down, delete the table first
-            if state == "down":
-                self.chassis_state_db.delete(key)
 
             # Prepare the fields to update
             updates = {
                 "dpu_midplane_link_state": state,
                 "dpu_midplane_link_reason": "",
                 "dpu_midplane_link_time": get_formatted_time(),
             }
+            # If midplane state is down, set control plane, data plane states to down as well
+            if state == "down":
+                updates[CP_STATE] = "down"
+                updates[CP_UPDATE_TIME] = get_formatted_time()
+                updates[DP_STATE] = "down"
+                updates[DP_UPDATE_TIME] = get_formatted_time()
 
             # Update each field directly
             for field, value in updates.items():
@@ -1175,11 +1184,6 @@ class SmartSwitchConfigManagerTask(ProcessTaskBase):
 
 class DpuStateUpdater(logger.Logger):
 
-    DP_STATE = 'dpu_data_plane_state'
-    DP_UPDATE_TIME = 'dpu_data_plane_time'
-    CP_STATE = 'dpu_control_plane_state'
-    CP_UPDATE_TIME = 'dpu_control_plane_time'
-
     def __init__(self, log_identifier, chassis):
         super(DpuStateUpdater, self).__init__(log_identifier)
 
@@ -1234,12 +1238,12 @@ class DpuStateUpdater(logger.Logger):
         return get_formatted_time()
 
     def _update_dp_dpu_state(self, state):
-        self.dpu_state_table.hset(self.name, self.DP_STATE, state)
-        self.dpu_state_table.hset(self.name, self.DP_UPDATE_TIME, self._time_now())
+        self.dpu_state_table.hset(self.name, DP_STATE, state)
+        self.dpu_state_table.hset(self.name, DP_UPDATE_TIME, self._time_now())
 
     def _update_cp_dpu_state(self, state):
-        self.dpu_state_table.hset(self.name, self.CP_STATE, state)
-        self.dpu_state_table.hset(self.name, self.CP_UPDATE_TIME, self._time_now())
+        self.dpu_state_table.hset(self.name, CP_STATE, state)
+        self.dpu_state_table.hset(self.name, CP_UPDATE_TIME, self._time_now())
 
     def get_dp_state(self):
         return 'up' if self._get_dp_state() else 'down'
@@ -1250,16 +1254,17 @@ class DpuStateUpdater(logger.Logger):
     def update_state(self):
 
         dp_current_state = self.get_dp_state()
-        _, dp_prev_state = self.dpu_state_table.hget(self.name, self.DP_STATE)
+        _, dp_prev_state = self.dpu_state_table.hget(self.name, DP_STATE)
 
         if dp_current_state != dp_prev_state:
             self._update_dp_dpu_state(dp_current_state)
 
         cp_current_state = self.get_cp_state()
-        _, cp_prev_state = self.dpu_state_table.hget(self.name, self.CP_STATE)
+        _, cp_prev_state = self.dpu_state_table.hget(self.name, CP_STATE)
 
         if cp_current_state != cp_prev_state:
             self._update_cp_dpu_state(cp_current_state)
+        return [dp_current_state, cp_current_state]
 
     def deinit(self):
         self._update_dp_dpu_state('down')
@@ -1409,6 +1414,8 @@ class DpuStateManagerTask(ProcessTaskBase):
         self.state_db = daemon_base.db_connect('STATE_DB')
         self.app_db = daemon_base.db_connect('APPL_DB')
         self.chassis_state_db = daemon_base.db_connect('CHASSIS_STATE_DB')
+        self.current_dp_state = None
+        self.current_cp_state = None
 
     def task_worker(self):
         sel = swsscommon.Select()
@@ -1437,7 +1444,6 @@ class DpuStateManagerTask(ProcessTaskBase):
                     result = s.pop()
                     update_required = True # If there is any selectable object, we need to update the state
                     if result is None:
-                        print("Here with None for {}".format(type(s)))
                         continue
                     key, op, fvp = result  # Changed from _ to fvp to match what we use below
                     # Check if this is the DPU_STATE table
@@ -1446,16 +1452,17 @@ class DpuStateManagerTask(ProcessTaskBase):
                         if key != self.dpu_state_updater.name:
                             update_required = False
                             continue
-                        # Don't update if this is our own dataplane/control plane state update to avoid recursion
                         if op == 'SET' and isinstance(fvp, tuple):
                             fvs = dict(fvp)
-                            if 'dpu_data_plane_state' in fvs and 'dpu_control_plane_state' in fvs:
+                            # No need to update if the state is the same as the current state
+                            if ('dpu_data_plane_state' in fvs and fvs['dpu_data_plane_state'] == self.current_dp_state) and \
+                                ('dpu_control_plane_state' in fvs and fvs['dpu_control_plane_state'] == self.current_cp_state):
                                 update_required = False
                                 continue
                         self.logger.log_info(f"DPU_STATE change detected: operation={op}, key={key}")
 
                 if update_required:
-                    self.dpu_state_updater.update_state()
+                    [self.current_dp_state, self.current_cp_state] = self.dpu_state_updater.update_state()
 
         except KeyboardInterrupt:
             pass

sonic-chassisd/tests/test_chassisd.py

@@ -1765,3 +1765,71 @@ def is_valid_date(date_str):
     if not date_value:
         AssertionError("Date is not set!")
     assert is_valid_date(date_value)
+
+def test_smartswitch_moduleupdater_midplane_state_change():
+    """Test that when midplane goes down, control plane and data plane states are set to down"""
+    chassis = MockSmartSwitchChassis()
+    index = 0
+    name = "DPU0"
+    desc = "DPU Module 0"
+    slot = 0
+    serial = "DPU0-0000"
+    module_type = ModuleBase.MODULE_TYPE_DPU
+    module = MockModule(index, name, desc, module_type, slot, serial)
+    module.set_midplane_ip()
+    chassis.module_list.append(module)
+
+    # Create the updater
+    module_updater = SmartSwitchModuleUpdater(SYSLOG_IDENTIFIER, chassis)
+    module_updater.midplane_initialized = True
+
+    # Mock chassis_state_db
+    chassis_state_db = {}
+    def mock_hset(key, field, value):
+        if key not in chassis_state_db:
+            chassis_state_db[key] = {}
+        chassis_state_db[key][field] = value
+
+    def mock_hget(key, field):
+        if key in chassis_state_db and field in chassis_state_db[key]:
+            return chassis_state_db[key][field]
+        return None
+
+    with patch.object(module_updater, 'chassis_state_db') as mock_db:
+        mock_db.hset = MagicMock(side_effect=mock_hset)
+        mock_db.hget = MagicMock(side_effect=mock_hget)
+
+        # Initially set midplane as up
+        module.set_midplane_reachable(True)
+        module_updater.check_midplane_reachability()
+
+        # Verify initial state
+        key = "DPU_STATE|" + name
+        assert chassis_state_db[key]["dpu_midplane_link_state"] == "up"
+
+        # Now set midplane as down
+        module.set_midplane_reachable(False)
+        module_updater.check_midplane_reachability()
+
+        # Verify all states are set to down
+        assert chassis_state_db[key]["dpu_midplane_link_state"] == "down"
+        assert chassis_state_db[key]["dpu_control_plane_state"] == "down"
+        assert chassis_state_db[key]["dpu_data_plane_state"] == "down"
+
+        # Verify timestamps are set
+        assert "dpu_midplane_link_time" in chassis_state_db[key]
+        assert "dpu_control_plane_time" in chassis_state_db[key]
+        assert "dpu_data_plane_time" in chassis_state_db[key]
+
+        # Verify time format
+        date_format = "%a %b %d %I:%M:%S %p UTC %Y"
+        def is_valid_date(date_str):
+            try:
+                datetime.strptime(date_str, date_format)
+                return True
+            except ValueError:
+                return False
+
+        assert is_valid_date(chassis_state_db[key]["dpu_midplane_link_time"])
+        assert is_valid_date(chassis_state_db[key]["dpu_control_plane_time"])
+        assert is_valid_date(chassis_state_db[key]["dpu_data_plane_time"])

sonic-chassisd/tests/test_dpu_chassisd.py

@@ -268,7 +268,7 @@ def hset(key, field, value):
                 }}
 
 
-def test_dpu_state_manager_specific_key_update():
+def test_dpu_state_manager_none_result():
     chassis = MockDpuChassis()
     chassis.get_dpu_id = MagicMock(return_value=0)
     chassis.get_dataplane_state = MagicMock(return_value=True)
@@ -281,8 +281,11 @@ def hset(key, field, value):
             chassis_state_db[key] = {}
         chassis_state_db[key][field] = value
 
+    def mock_pop():
+        return None
+
     with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset):
-        with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', return_value=('DPU0', 'SET', (('dpu_control_plane_state', 'up'), ('dpu_data_plane_state', 'up')))):
+        with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', side_effect=mock_pop):
             with mock.patch.object(swsscommon.Select, 'select',
                 side_effect=[(swsscommon.Select.OBJECT, None), KeyboardInterrupt]):
 
@@ -292,11 +295,17 @@ def hset(key, field, value):
                 dpu_state_mng = DpuStateManagerTask(SYSLOG_IDENTIFIER, dpu_updater)
                 dpu_state_mng.task_worker()
 
-                # Verify no state update occurred since this was our own state update
-                assert chassis_state_db == {}
+                # Verify state was updated even with None result
+                assert chassis_state_db == {'DPU0': {
+                    'dpu_data_plane_state': 'up',
+                    'dpu_data_plane_time': 'Sat Jan 01 12:00:00 AM UTC 2000',
+                    'dpu_control_plane_state': 'up',
+                    'dpu_control_plane_time': 'Sat Jan 01 12:00:00 AM UTC 2000'
+                }}
 
 
-def test_dpu_state_manager_none_result():
+def test_dpu_state_manager_state_tracking():
+    """Test that DpuStateManagerTask correctly tracks current states and avoids unnecessary updates"""
     chassis = MockDpuChassis()
     chassis.get_dpu_id = MagicMock(return_value=0)
     chassis.get_dataplane_state = MagicMock(return_value=True)
@@ -309,11 +318,9 @@ def hset(key, field, value):
             chassis_state_db[key] = {}
         chassis_state_db[key][field] = value
 
-    def mock_pop():
-        return None
-
+    # First update - should set initial states
     with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset):
-        with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', side_effect=mock_pop):
+        with mock.patch.object(swsscommon.SubscriberStateTable, 'pop', return_value=('DPU0', 'SET', None)):
             with mock.patch.object(swsscommon.Select, 'select',
                 side_effect=[(swsscommon.Select.OBJECT, None), KeyboardInterrupt]):
 
@@ -323,10 +330,128 @@ def mock_pop():
                 dpu_state_mng = DpuStateManagerTask(SYSLOG_IDENTIFIER, dpu_updater)
                 dpu_state_mng.task_worker()
 
-                # Verify state was updated even with None result
+                # Verify initial state was set
                 assert chassis_state_db == {'DPU0': {
                     'dpu_data_plane_state': 'up',
                     'dpu_data_plane_time': 'Sat Jan 01 12:00:00 AM UTC 2000',
                     'dpu_control_plane_state': 'up',
                     'dpu_control_plane_time': 'Sat Jan 01 12:00:00 AM UTC 2000'
                 }}
+
+                # Verify current states are tracked
+                assert dpu_state_mng.current_dp_state == 'up'
+                assert dpu_state_mng.current_cp_state == 'up'
+
+
+def test_dpu_state_manager_avoid_duplicate_updates():
+    """Test that DpuStateManagerTask avoids duplicate state updates"""
+    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 with same state update
+    with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset):
+        with mock.patch.object(swsscommon.SubscriberStateTable, 'pop',
+            return_value=('DPU0', 'SET', (('dpu_data_plane_state', 'up'), ('dpu_control_plane_state', 'up')))):
+            with mock.patch.object(swsscommon.Select, 'select',
+                side_effect=[(swsscommon.Select.OBJECT, None), (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'
+                dpu_state_mng.task_worker()
+
+                # Verify no updates occurred since states were unchanged
+                assert update_count == 0
+
+
+def test_dpu_state_manager_different_dpu():
+    """Test that DpuStateManagerTask handles updates for different DPUs correctly"""
+    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 with update for different DPU
+    with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset):
+        with mock.patch.object(swsscommon.SubscriberStateTable, 'pop',
+            return_value=('DPU1', 'SET', (('dpu_data_plane_state', 'down'), ('dpu_control_plane_state', 'down')))):
+            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'
+                dpu_state_mng.task_worker()
+
+                # Verify no updates occurred since it was for a different DPU
+                assert update_count == 0
+
+
+def test_dpu_state_manager_state_change():
+    """Test that DpuStateManagerTask handles state changes correctly"""
+    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=False)  # CP state changed to False
+
+    chassis_state_db = {}
+
+    def hset(key, field, value):
+        if key not in chassis_state_db:
+            chassis_state_db[key] = {}
+        chassis_state_db[key][field] = value
+
+    # Test with state change
+    with mock.patch.object(swsscommon.Table, 'hset', side_effect=hset):
+        with mock.patch.object(swsscommon.SubscriberStateTable, 'pop',
+            return_value=('DPU0', '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'  # Previous state was up
+                dpu_state_mng.task_worker()
+
+                # Verify state was updated since control plane state changed
+                assert chassis_state_db == {'DPU0': {
+                    'dpu_data_plane_state': 'up',
+                    'dpu_data_plane_time': 'Sat Jan 01 12:00:00 AM UTC 2000',
+                    'dpu_control_plane_state': 'down',  # Changed to down
+                    'dpu_control_plane_time': 'Sat Jan 01 12:00:00 AM UTC 2000'
+                }}
+
+                # Verify current states were updated
+                assert dpu_state_mng.current_dp_state == 'up'
+                assert dpu_state_mng.current_cp_state == 'down'