Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion config/chassis_modules.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@gpunathilell gpunathilell Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gpunathilell Thank you for the suggestion about using the database connector abstraction consistently. While I agree this would improve consistency, this logic is currently undergoing broader refactoring in PRs #4031 and related PRs across sonic-platform-common, sonic-host-services, and sonic-platform-daemons. To avoid conflicts and duplicated effort, I believe the database access pattern consistency should be addressed as part of that broader refactoring work rather than making incremental changes here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since refactoring will be done, please do not resolve the review comments, I will approve as this is blocking bug fix. Thank you


#
# 'chassis_modules' group ('config chassis_modules ...')
#
Expand Down Expand Up @@ -74,6 +80,7 @@ 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)
Expand All @@ -89,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

#
Expand Down
21 changes: 21 additions & 0 deletions tests/chassis_modules_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,27 @@ 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")
Expand Down
Loading