diff --git a/scripts/featured b/scripts/featured index 636c24c6..ebd64daa 100644 --- a/scripts/featured +++ b/scripts/featured @@ -221,7 +221,14 @@ class FeatureHandler(object): Summary: Updates the state field in the FEATURE|* tables as the state field might have to be rendered based on DEVICE_METADATA table and generated Device Running Metadata + + Uses a two-phase approach to avoid a race condition between systemctl + daemon-reload and Docker container startup: + Phase 1: Write all systemd config files, followed by daemon-reload. + Phase 2: Start/stop all feature services (no daemon-reloads required). """ + # Phase 1: Write all systemd configs and cache features + syslog.syslog(syslog.LOG_INFO, "Writing systemd configs") for feature_name in feature_table.keys(): if not feature_name: syslog.syslog(syslog.LOG_WARNING, "Feature is None") @@ -233,7 +240,18 @@ class FeatureHandler(object): feature = Feature(feature_name, feature_table[feature_name], device_config) self._cached_config.setdefault(feature_name, feature) - self.update_systemd_config(feature) + self.update_systemd_config(feature, reload=False) + + # Perform daemon-reload after all configs are written + self.reload_systemd_config() + + # Phase 2: Start/stop services without daemon-reloads + syslog.syslog(syslog.LOG_INFO, "Restarting services") + for feature_name in feature_table.keys(): + if not feature_name: + syslog.syslog(syslog.LOG_WARNING, "Feature is None") + continue + feature = self._cached_config[feature_name] self.update_feature_state(feature) self.sync_feature_scope(feature) self.resync_feature_state(feature) @@ -354,13 +372,15 @@ class FeatureHandler(object): for ns, db in self.ns_cfg_db.items(): self._conditional_update_scope(db, feature_config.name, new_per_asic, new_global) - def update_systemd_config(self, feature_config): + def update_systemd_config(self, feature_config, reload=True): """Updates `Restart=` field in feature's systemd configuration file according to the value of `auto_restart` field in `FEATURE` table of `CONFIG_DB`. Args: feature: An object represents a feature's configuration in `FEATURE` table of `CONFIG_DB`. + reload: Whether to run systemctl daemon-reload after writing config. + Set to False when batching multiple config updates. Returns: None. @@ -398,6 +418,10 @@ class FeatureHandler(object): syslog.syslog(syslog.LOG_INFO, "Feature '{}' systemd config file related to auto-restart is updated!" .format(feature_name)) + if reload: + self.reload_systemd_config() + + def reload_systemd_config(self): try: syslog.syslog(syslog.LOG_INFO, "Reloading systemd configuration files ...") run_cmd(["sudo", "systemctl", "daemon-reload"], raise_exception=True) diff --git a/tests/featured/featured_test.py b/tests/featured/featured_test.py index ccf5c69a..98456d07 100644 --- a/tests/featured/featured_test.py +++ b/tests/featured/featured_test.py @@ -321,7 +321,88 @@ def test_feature_resync(self): } feature_handler.sync_state_field(feature_table) mock_db.mod_entry.assert_called_with('FEATURE', 'sflow', {'state': 'enabled'}) - + + def test_sync_state_field_two_phase_ordering(self): + """Verify all systemd configs are written (Phase 1) before any service + is started (Phase 2), with exactly one daemon-reload in between.""" + mock_db = mock.MagicMock() + mock_db.get_entry = mock.MagicMock(return_value=None) + mock_feature_state_table = mock.MagicMock() + + feature_handler = featured.FeatureHandler(mock_db, mock_feature_state_table, {}, False) + feature_handler.is_delayed_enabled = True + + call_order = [] + + def track_update_systemd(feature, reload=True): + call_order.append(('update_systemd_config', feature.name, reload)) + + def track_reload(): + call_order.append(('reload_systemd_config',)) + + def track_update_feature_state(feature): + call_order.append(('update_feature_state', feature.name)) + return True + + feature_table = { + 'sflow': {'state': 'enabled', 'auto_restart': 'enabled'}, + 'snmp': {'state': 'enabled', 'auto_restart': 'enabled'}, + 'lldp': {'state': 'enabled', 'auto_restart': 'disabled'}, + } + + with mock.patch.object(feature_handler, 'update_systemd_config', side_effect=track_update_systemd), \ + mock.patch.object(feature_handler, 'reload_systemd_config', side_effect=track_reload), \ + mock.patch.object(feature_handler, 'update_feature_state', side_effect=track_update_feature_state), \ + mock.patch.object(feature_handler, 'sync_feature_scope'), \ + mock.patch.object(feature_handler, 'resync_feature_state'), \ + mock.patch.object(feature_handler, 'sync_feature_delay_state'): + feature_handler.sync_state_field(feature_table) + + systemd_calls = [c for c in call_order if c[0] == 'update_systemd_config'] + assert len(systemd_calls) == 3 + for c in systemd_calls: + assert c[2] is False, "update_systemd_config for {} should use reload=False".format(c[1]) + + reload_calls = [c for c in call_order if c[0] == 'reload_systemd_config'] + assert len(reload_calls) == 1 + + last_config_idx = max(i for i, c in enumerate(call_order) if c[0] == 'update_systemd_config') + reload_idx = next(i for i, c in enumerate(call_order) if c[0] == 'reload_systemd_config') + state_indices = [i for i, c in enumerate(call_order) if c[0] == 'update_feature_state'] + assert last_config_idx < reload_idx, "All config writes must complete before daemon-reload" + assert all(idx > reload_idx for idx in state_indices), "All service starts must happen after daemon-reload" + + def test_update_systemd_config_reload_parameter(self): + """Verify update_systemd_config only triggers daemon-reload when reload=True (default).""" + mock_db = mock.MagicMock() + feature_handler = featured.FeatureHandler(mock_db, mock.MagicMock(), {}, False) + feature = featured.Feature('sflow', {'state': 'enabled', 'auto_restart': 'enabled'}) + + with mock.patch.object(feature_handler, 'reload_systemd_config') as mock_reload, \ + mock.patch.object(feature_handler, 'get_multiasic_feature_instances', + return_value=(['sflow'], ['service'])), \ + mock.patch('os.path.exists', return_value=True), \ + mock.patch('builtins.open', mock.mock_open()): + feature_handler.update_systemd_config(feature, reload=False) + mock_reload.assert_not_called() + + feature_handler.update_systemd_config(feature) + mock_reload.assert_called_once() + + def test_sync_state_field_empty_table(self): + """With no features, daemon-reload still fires and no services are started.""" + mock_db = mock.MagicMock() + feature_handler = featured.FeatureHandler(mock_db, mock.MagicMock(), {}, False) + + with mock.patch.object(feature_handler, 'update_systemd_config') as mock_config, \ + mock.patch.object(feature_handler, 'reload_systemd_config') as mock_reload, \ + mock.patch.object(feature_handler, 'update_feature_state') as mock_state: + feature_handler.sync_state_field({}) + + mock_config.assert_not_called() + mock_reload.assert_called_once() + mock_state.assert_not_called() + def test_port_init_done_twice(self): """There could be multiple "PortInitDone" event in case of swss restart(either due to crash or due to manual operation). swss @@ -524,7 +605,6 @@ def test_feature_events(self, mock_syslog, get_runtime): call(['sudo', 'systemctl', 'unmask', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'dhcp_relay.service'], capture_output=True, check=True, text=True), - call(['sudo', 'systemctl', 'daemon-reload'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'unmask', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'mux.service'], capture_output=True, check=True, text=True)] @@ -565,7 +645,6 @@ def test_delayed_service(self, mock_syslog, get_runtime): call(['sudo', 'systemctl', 'unmask', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'dhcp_relay.service'], capture_output=True, check=True, text=True), - call(['sudo', 'systemctl', 'daemon-reload'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'unmask', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'mux.service'], capture_output=True, check=True, text=True)] @@ -591,7 +670,6 @@ def test_advanced_reboot(self, mock_syslog, get_runtime): call(['sudo', 'systemctl', 'unmask', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'dhcp_relay.service'], capture_output=True, check=True, text=True), - call(['sudo', 'systemctl', 'daemon-reload'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'unmask', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'mux.service'], capture_output=True, check=True, text=True)] @@ -619,7 +697,6 @@ def test_portinit_timeout(self, mock_syslog, get_runtime): call(['sudo', 'systemctl', 'unmask', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'dhcp_relay.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'dhcp_relay.service'], capture_output=True, check=True, text=True), - call(['sudo', 'systemctl', 'daemon-reload'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'unmask', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'enable', 'mux.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'mux.service'], capture_output=True, check=True, text=True)]