From 4dab08ffd15f2979f3396b93338deb3ded5f23d8 Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Mon, 22 Sep 2025 09:26:31 -0700 Subject: [PATCH 1/8] fixing state_db not having delete_field attribute causing a crash when DPUs in bad state --- config/chassis_modules.py | 1 - 1 file changed, 1 deletion(-) diff --git a/config/chassis_modules.py b/config/chassis_modules.py index 56768a5eb7..0e5cc067d6 100755 --- a/config/chassis_modules.py +++ b/config/chassis_modules.py @@ -75,7 +75,6 @@ def set_state_transition_in_progress(db, chassis_module_name, value): entry['transition_start_time'] = datetime.utcnow().isoformat() else: entry.pop('transition_start_time', None) - state_db.delete_field('CHASSIS_MODULE_TABLE', chassis_module_name, 'transition_start_time') state_db.set_entry('CHASSIS_MODULE_TABLE', chassis_module_name, entry) From 8ee9b1cbc8c5e4e723ffd535833de4a446cdeaf9 Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Tue, 7 Oct 2025 13:49:59 -0700 Subject: [PATCH 2/8] Addressed copilot review comment --- config/chassis_modules.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/config/chassis_modules.py b/config/chassis_modules.py index 0e5cc067d6..8f6e18225b 100755 --- a/config/chassis_modules.py +++ b/config/chassis_modules.py @@ -27,6 +27,12 @@ def set_entry(self, table, key, entry): for field, value in entry.items(): self.db.set("STATE_DB", redis_key, field, value) + def delete_field(self, table, key, field): + """Delete a specific field from table|key.""" + redis_key = f"{table}|{key}" + client = self.db.get_redis_client("STATE_DB") + return client.hdel(redis_key, field) + # # 'chassis_modules' group ('config chassis_modules ...') # @@ -74,7 +80,9 @@ def set_state_transition_in_progress(db, chassis_module_name, value): if value == 'True': entry['transition_start_time'] = datetime.utcnow().isoformat() else: + # Remove transition_start_time from both local entry and database entry.pop('transition_start_time', None) + state_db.delete_field('CHASSIS_MODULE_TABLE', chassis_module_name, 'transition_start_time') state_db.set_entry('CHASSIS_MODULE_TABLE', chassis_module_name, entry) From 0732d9bcf0c78514a818df478c11d6dc55405938 Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Tue, 7 Oct 2025 14:12:18 -0700 Subject: [PATCH 3/8] Addressed copilot review comment --- config/chassis_modules.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/config/chassis_modules.py b/config/chassis_modules.py index 8f6e18225b..f0d8b0431a 100755 --- a/config/chassis_modules.py +++ b/config/chassis_modules.py @@ -96,9 +96,18 @@ def is_transition_timed_out(db, chassis_module_name): if not start_time_str: return False try: + # Try ISO format first start_time = datetime.fromisoformat(start_time_str) except ValueError: - return False + try: + start_time = datetime.strptime(start_time_str, '%Y-%m-%d %H:%M:%S.%f') + except ValueError: + return False + + # Ensure both datetimes are offset-naive for comparison + if start_time.tzinfo is not None: + start_time = start_time.replace(tzinfo=None) + return datetime.utcnow() - start_time > TRANSITION_TIMEOUT # From d0b102f490bd2765bd4e9883067e5954f6672acb Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Tue, 7 Oct 2025 15:12:23 -0700 Subject: [PATCH 4/8] Addressed a test case test_delete_field to cover the newly added code --- tests/chassis_modules_test.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/chassis_modules_test.py b/tests/chassis_modules_test.py index 305d26b380..1691bcdba8 100755 --- a/tests/chassis_modules_test.py +++ b/tests/chassis_modules_test.py @@ -602,6 +602,26 @@ def test_is_transition_timed_out_all_paths(self): db.statedb.get_entry.return_value = {"transition_start_time": now} assert is_transition_timed_out(db, "DPU0") is False + def test_delete_field(self): + """Single test to cover missing delete_field and timezone handling lines""" + from config.chassis_modules import StateDBHelper + from datetime import timezone + + # Test delete_field method (covers lines 32-34) + mock_sonic_db = mock.MagicMock() + mock_redis_client = mock.MagicMock() + mock_sonic_db.get_redis_client.return_value = mock_redis_client + helper = StateDBHelper(mock_sonic_db) + helper.delete_field('TEST_TABLE', 'test_key', 'test_field') + mock_redis_client.hdel.assert_called_once_with("TEST_TABLE|test_key", "test_field") + + # Test timezone-aware datetime handling (covers line 109) + db = mock.MagicMock() + db.statedb = mock.MagicMock() + tz_time = (datetime.utcnow() - TRANSITION_TIMEOUT - timedelta(seconds=1)).replace(tzinfo=timezone.utc).isoformat() + db.statedb.get_entry.return_value = {"transition_start_time": tz_time} + assert is_transition_timed_out(db, "DPU0") is True + @classmethod def teardown_class(cls): print("TEARDOWN") From 6ae9fb4ea6b6b1727bcdec1d85db5555f55ad280 Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Tue, 7 Oct 2025 15:22:48 -0700 Subject: [PATCH 5/8] Fixed SA errors --- tests/chassis_modules_test.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/chassis_modules_test.py b/tests/chassis_modules_test.py index 1691bcdba8..54dc7009c4 100755 --- a/tests/chassis_modules_test.py +++ b/tests/chassis_modules_test.py @@ -606,7 +606,7 @@ def test_delete_field(self): """Single test to cover missing delete_field and timezone handling lines""" from config.chassis_modules import StateDBHelper from datetime import timezone - + # Test delete_field method (covers lines 32-34) mock_sonic_db = mock.MagicMock() mock_redis_client = mock.MagicMock() @@ -614,11 +614,12 @@ def test_delete_field(self): helper = StateDBHelper(mock_sonic_db) helper.delete_field('TEST_TABLE', 'test_key', 'test_field') mock_redis_client.hdel.assert_called_once_with("TEST_TABLE|test_key", "test_field") - + # Test timezone-aware datetime handling (covers line 109) db = mock.MagicMock() db.statedb = mock.MagicMock() - tz_time = (datetime.utcnow() - TRANSITION_TIMEOUT - timedelta(seconds=1)).replace(tzinfo=timezone.utc).isoformat() + tz_time = (datetime.utcnow() - TRANSITION_TIMEOUT - timedelta(seconds=1)).replace( + tzinfo=timezone.utc).isoformat() db.statedb.get_entry.return_value = {"transition_start_time": tz_time} assert is_transition_timed_out(db, "DPU0") is True From d5cf77159e2bd1c4f2e3fe852564224766950fd5 Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Wed, 8 Oct 2025 07:11:30 -0700 Subject: [PATCH 6/8] Addressing review comments --- config/chassis_modules.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/config/chassis_modules.py b/config/chassis_modules.py index f0d8b0431a..4c5134ac7f 100755 --- a/config/chassis_modules.py +++ b/config/chassis_modules.py @@ -96,13 +96,11 @@ def is_transition_timed_out(db, chassis_module_name): if not start_time_str: return False try: - # Try ISO format first - start_time = datetime.fromisoformat(start_time_str) + # Try ISO format first (handles most cases including timezone info) + start_time = datetime.fromisoformat(start_time_str.replace('Z', '+00:00')) except ValueError: - try: - start_time = datetime.strptime(start_time_str, '%Y-%m-%d %H:%M:%S.%f') - except ValueError: - return False + # If ISO fails, return False to be safe (timestamp format not supported) + return False # Ensure both datetimes are offset-naive for comparison if start_time.tzinfo is not None: From fe12399edeb063a17a101bffd3a3c414f3adf5ae Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Wed, 8 Oct 2025 14:18:52 -0700 Subject: [PATCH 7/8] Addressing review comments --- config/chassis_modules.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/config/chassis_modules.py b/config/chassis_modules.py index 4c5134ac7f..3d58f00c75 100755 --- a/config/chassis_modules.py +++ b/config/chassis_modules.py @@ -6,7 +6,7 @@ import subprocess import utilities_common.cli as clicommon from utilities_common.chassis import is_smartswitch, get_all_dpus -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone TIMEOUT_SECS = 10 TRANSITION_TIMEOUT = timedelta(seconds=240) # 4 minutes @@ -96,15 +96,17 @@ def is_transition_timed_out(db, chassis_module_name): if not start_time_str: return False try: - # Try ISO format first (handles most cases including timezone info) - start_time = datetime.fromisoformat(start_time_str.replace('Z', '+00:00')) + # Handle Zulu time format more robustly + if start_time_str.endswith('Z'): + start_time_str = start_time_str[:-1] + '+00:00' + start_time = datetime.fromisoformat(start_time_str) except ValueError: # If ISO fails, return False to be safe (timestamp format not supported) return False - # Ensure both datetimes are offset-naive for comparison + # Convert timezone-aware datetime to UTC naive for comparison if start_time.tzinfo is not None: - start_time = start_time.replace(tzinfo=None) + start_time = start_time.astimezone(timezone.utc).replace(tzinfo=None) return datetime.utcnow() - start_time > TRANSITION_TIMEOUT From fc016432a4d79dbb9eebd31f7f008759dc737122 Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Wed, 8 Oct 2025 20:15:22 -0700 Subject: [PATCH 8/8] Addressing review comments --- config/chassis_modules.py | 16 +++++++--------- tests/chassis_modules_test.py | 13 ++++++------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/config/chassis_modules.py b/config/chassis_modules.py index 3d58f00c75..ee8d6abd6a 100755 --- a/config/chassis_modules.py +++ b/config/chassis_modules.py @@ -78,7 +78,7 @@ def set_state_transition_in_progress(db, chassis_module_name, value): entry = state_db.get_entry('CHASSIS_MODULE_TABLE', chassis_module_name) or {} entry['state_transition_in_progress'] = value if value == 'True': - entry['transition_start_time'] = datetime.utcnow().isoformat() + entry['transition_start_time'] = datetime.now(timezone.utc).isoformat() else: # Remove transition_start_time from both local entry and database entry.pop('transition_start_time', None) @@ -96,19 +96,17 @@ def is_transition_timed_out(db, chassis_module_name): if not start_time_str: return False try: - # Handle Zulu time format more robustly - if start_time_str.endswith('Z'): - start_time_str = start_time_str[:-1] + '+00:00' start_time = datetime.fromisoformat(start_time_str) except ValueError: - # If ISO fails, return False to be safe (timestamp format not supported) return False - # Convert timezone-aware datetime to UTC naive for comparison - if start_time.tzinfo is not None: - start_time = start_time.astimezone(timezone.utc).replace(tzinfo=None) + # Use UTC everywhere for consistent comparison + current_time = datetime.now(timezone.utc) + if start_time.tzinfo is None: + # If stored time is naive, assume it's UTC + start_time = start_time.replace(tzinfo=timezone.utc) - return datetime.utcnow() - start_time > TRANSITION_TIMEOUT + return current_time - start_time > TRANSITION_TIMEOUT # # Name: check_config_module_state_with_timeout diff --git a/tests/chassis_modules_test.py b/tests/chassis_modules_test.py index 54dc7009c4..1a5a67e201 100755 --- a/tests/chassis_modules_test.py +++ b/tests/chassis_modules_test.py @@ -1,7 +1,7 @@ import sys import os from click.testing import CliRunner -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from config.chassis_modules import ( set_state_transition_in_progress, is_transition_timed_out, @@ -490,7 +490,7 @@ def test_shutdown_triggers_transition_in_progress(self): fvs = { 'admin_status': 'up', 'state_transition_in_progress': 'True', - 'transition_start_time': datetime.utcnow().isoformat() + 'transition_start_time': datetime.now(timezone.utc).isoformat() } db.cfgdb.set_entry('CHASSIS_MODULE', "DPU0", fvs) @@ -517,7 +517,7 @@ def test_shutdown_triggers_transition_timeout(self): fvs = { 'admin_status': 'up', 'state_transition_in_progress': 'True', - 'transition_start_time': (datetime.utcnow() - timedelta(minutes=30)).isoformat() + 'transition_start_time': (datetime.now(timezone.utc) - timedelta(minutes=30)).isoformat() } db.cfgdb.set_entry('CHASSIS_MODULE', "DPU0", fvs) @@ -593,12 +593,12 @@ def test_is_transition_timed_out_all_paths(self): assert is_transition_timed_out(db, "DPU0") is False # Case 4: Timed out - old_time = (datetime.utcnow() - TRANSITION_TIMEOUT - timedelta(seconds=1)).isoformat() + old_time = (datetime.now(timezone.utc) - TRANSITION_TIMEOUT - timedelta(seconds=1)).isoformat() db.statedb.get_entry.return_value = {"transition_start_time": old_time} assert is_transition_timed_out(db, "DPU0") is True # Case 5: Not timed out yet - now = datetime.utcnow().isoformat() + now = datetime.now(timezone.utc).isoformat() db.statedb.get_entry.return_value = {"transition_start_time": now} assert is_transition_timed_out(db, "DPU0") is False @@ -618,8 +618,7 @@ def test_delete_field(self): # Test timezone-aware datetime handling (covers line 109) db = mock.MagicMock() db.statedb = mock.MagicMock() - tz_time = (datetime.utcnow() - TRANSITION_TIMEOUT - timedelta(seconds=1)).replace( - tzinfo=timezone.utc).isoformat() + tz_time = (datetime.now(timezone.utc) - TRANSITION_TIMEOUT - timedelta(seconds=1)).isoformat() db.statedb.get_entry.return_value = {"transition_start_time": tz_time} assert is_transition_timed_out(db, "DPU0") is True