Skip to content

[action] [PR:368] [featured]: Skip redundant CONFIG_DB writes in sync_feature_scope #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mssonicbld merged 1 commit into from
May 7, 2026
Merged

[action] [PR:368] [featured]: Skip redundant CONFIG_DB writes in sync_feature_scope #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 31 additions & 6 deletions scripts/featured
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,28 @@ class FeatureHandler(object):

return True

@staticmethod
def _conditional_update_scope(db, feature_name, new_per_asic, new_global):
"""Write scope fields to a CONFIG_DB only when the values have actually changed.

Args:
db: ConfigDBConnector instance (host or namespace).
feature_name (str): Name of the feature entry to update.
new_per_asic (str): Desired value for has_per_asic_scope ('True'/'False').
new_global (str): Desired value for has_global_scope ('True'/'False').

Returns:
None.
"""
current_entry = db.get_entry(FEATURE_TBL, feature_name) or {}
update_fields = {}
if current_entry.get('has_per_asic_scope') != new_per_asic:
update_fields['has_per_asic_scope'] = new_per_asic
if current_entry.get('has_global_scope') != new_global:
update_fields['has_global_scope'] = new_global
if update_fields:
db.mod_entry(FEATURE_TBL, feature_name, update_fields)

def sync_feature_scope(self, feature_config):
"""Updates the has_global_scope or has_per_asic_scope field in the FEATURE|* tables as the field
might have to be rendered based on DEVICE_METADATA table or Device Running configuration.
Expand Down Expand Up @@ -321,13 +343,16 @@ class FeatureHandler(object):
.format(feature_name, feature_suffixes[-1]))
self.set_feature_state(feature_config, self.FEATURE_STATE_FAILED)
return
self._config_db.mod_entry(FEATURE_TBL, feature_config.name, {'has_per_asic_scope': str(feature_config.has_per_asic_scope)})
self._config_db.mod_entry(FEATURE_TBL, feature_config.name, {'has_global_scope': str(feature_config.has_global_scope)})

# sync has_per_asic_scope to CONFIG_DB in namespaces in multi-asic platform
# Only write scope fields to CONFIG_DB if the values have actually changed.
# This avoids redundant writes that can race with config reload consumers
# (e.g. sonic-mgmt YANG validation checks reading CONFIG_DB snapshots).
new_per_asic = str(feature_config.has_per_asic_scope)
new_global = str(feature_config.has_global_scope)
self._conditional_update_scope(self._config_db, feature_config.name, new_per_asic, new_global)

# Sync both has_per_asic_scope and has_global_scope to CONFIG_DB in namespaces on multi-asic platforms
for ns, db in self.ns_cfg_db.items():
db.mod_entry(FEATURE_TBL, feature_config.name, {'has_per_asic_scope': str(feature_config.has_per_asic_scope)})
db.mod_entry(FEATURE_TBL, feature_config.name, {'has_global_scope': str(feature_config.has_global_scope)})
self._conditional_update_scope(db, feature_config.name, new_per_asic, new_global)

def update_systemd_config(self, feature_config):
"""Updates `Restart=` field in feature's systemd configuration file
Expand Down
109 changes: 109 additions & 0 deletions tests/featured/featured_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,115 @@ def test_enable_and_disable_feature_ExclusionList_skips_actions(self, mock_syslo
mocked_set_state.assert_not_called()
assert any("ExclusionList: skip disabling 'frr_bmp'" in str(c.args[1]) for c in mock_syslog.call_args_list)

@mock.patch('featured.FeatureHandler.update_systemd_config', mock.MagicMock())
@mock.patch('featured.FeatureHandler.update_feature_state', mock.MagicMock())
@mock.patch('featured.FeatureHandler.sync_feature_delay_state', mock.MagicMock())
@mock.patch('featured.FeatureHandler.get_systemd_unit_state', mock.MagicMock(return_value=""))
def test_sync_feature_scope_conditional_write(self):
"""Verify sync_feature_scope only writes when scope values differ or entry is missing."""
mock_db = mock.MagicMock()
mock_db.get_entry = mock.MagicMock()
mock_db.mod_entry = mock.MagicMock()
mock_feature_state_table = mock.MagicMock()

feature_handler = featured.FeatureHandler(mock_db, mock_feature_state_table, {}, False)

feature = featured.Feature('sflow', {
'state': 'enabled',
'has_global_scope': 'True',
'has_per_asic_scope': 'False',
})

# Values already match -> no write
mock_db.get_entry.return_value = {
'has_per_asic_scope': 'False',
'has_global_scope': 'True',
}
feature_handler.sync_feature_scope(feature)
mock_db.mod_entry.assert_not_called()
mock_db.mod_entry.reset_mock()

# Only has_per_asic_scope differs -> write only changed field in single call
mock_db.get_entry.return_value = {
'has_per_asic_scope': 'True',
'has_global_scope': 'True',
}
feature_handler.sync_feature_scope(feature)
mock_db.mod_entry.assert_called_once_with(featured.FEATURE_TBL, 'sflow', {'has_per_asic_scope': 'False'})
mock_db.mod_entry.reset_mock()

# Entry missing (None) -> write both fields in single call
mock_db.get_entry.return_value = None
feature_handler.sync_feature_scope(feature)
mock_db.mod_entry.assert_called_once_with(featured.FEATURE_TBL, 'sflow', {
'has_per_asic_scope': 'False',
'has_global_scope': 'True',
})

@mock.patch('featured.FeatureHandler.update_systemd_config', mock.MagicMock())
@mock.patch('featured.FeatureHandler.update_feature_state', mock.MagicMock())
@mock.patch('featured.FeatureHandler.sync_feature_delay_state', mock.MagicMock())
@mock.patch('featured.FeatureHandler.get_systemd_unit_state', mock.MagicMock(return_value=""))
def test_sync_feature_scope_namespace_dbs(self):
"""Verify sync_feature_scope propagates writes to per-namespace DBs on multi-ASIC."""
mock_db = mock.MagicMock()
mock_db.get_entry = mock.MagicMock()
mock_db.mod_entry = mock.MagicMock()
mock_feature_state_table = mock.MagicMock()

feature_handler = featured.FeatureHandler(mock_db, mock_feature_state_table, {}, False)
mock_ns_db_0 = mock.MagicMock()
mock_ns_db_1 = mock.MagicMock()
feature_handler.ns_cfg_db = {'asic0': mock_ns_db_0, 'asic1': mock_ns_db_1}

feature = featured.Feature('sflow', {
'state': 'enabled',
'has_global_scope': 'True',
'has_per_asic_scope': 'True',
})

# Host differs -> both host and namespaces should be written
mock_db.get_entry.return_value = {
'has_per_asic_scope': 'False',
'has_global_scope': 'False',
}
for mock_ns_db in [mock_ns_db_0, mock_ns_db_1]:
mock_ns_db.get_entry.return_value = {
'has_per_asic_scope': 'False',
'has_global_scope': 'False',
}
feature_handler.sync_feature_scope(feature)

for mock_ns_db in [mock_ns_db_0, mock_ns_db_1]:
mock_ns_db.mod_entry.assert_called_once_with(featured.FEATURE_TBL, 'sflow', {
'has_per_asic_scope': 'True',
'has_global_scope': 'True',
})

# Reset for next scenario
mock_db.mod_entry.reset_mock()
for mock_ns_db in [mock_ns_db_0, mock_ns_db_1]:
mock_ns_db.mod_entry.reset_mock()

# Host matches but namespaces are stale -> namespaces should still be written
mock_db.get_entry.return_value = {
'has_per_asic_scope': 'True',
'has_global_scope': 'True',
}
for mock_ns_db in [mock_ns_db_0, mock_ns_db_1]:
mock_ns_db.get_entry.return_value = {
'has_per_asic_scope': 'False',
'has_global_scope': 'False',
}
feature_handler.sync_feature_scope(feature)

mock_db.mod_entry.assert_not_called()
for mock_ns_db in [mock_ns_db_0, mock_ns_db_1]:
mock_ns_db.mod_entry.assert_called_once_with(featured.FEATURE_TBL, 'sflow', {
'has_per_asic_scope': 'True',
'has_global_scope': 'True',
})


@mock.patch("syslog.syslog", side_effect=syslog_side_effect)
@mock.patch('sonic_py_common.device_info.get_device_runtime_metadata')
Expand Down
Loading