diff --git a/scripts/featured b/scripts/featured index 66fa1f56..a642c5b6 100644 --- a/scripts/featured +++ b/scripts/featured @@ -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. @@ -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 diff --git a/tests/featured/featured_test.py b/tests/featured/featured_test.py index 25544eaf..ed6329fb 100644 --- a/tests/featured/featured_test.py +++ b/tests/featured/featured_test.py @@ -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')