From ff3c11b2fb27cded0e35299c5a20887678b02180 Mon Sep 17 00:00:00 2001 From: setu Date: Wed, 1 Apr 2026 20:29:23 +0000 Subject: [PATCH 1/2] Fix race between featured daemon-reload and Docker container start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Following procedure during boot causes suprious issue: - the featured daemon loops through every feature and for each one writes a systemd config file, calls systemctl daemon-reload (which temporarily tears down cgroup directories), - Next, docker container start — but the container started in the previous iteration may still be initializing - Here when daemon-reload fires for the next feature, it destroys the group path that the previous container's runc process is trying to write its PID to, resulting in the "no such device" error. The fix splits this boot-time loop into two phases: - first write all systemd config files and do a single daemon-reload, - Secondly start all the containers Therefore, no daemon-reload can race against a container is starting up. Signed-off-by: setu --- scripts/featured | 28 ++++++++++++++++++++++++++-- tests/featured/featured_test.py | 4 ---- 2 files changed, 26 insertions(+), 6 deletions(-) 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..b410e021 100644 --- a/tests/featured/featured_test.py +++ b/tests/featured/featured_test.py @@ -524,7 +524,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 +564,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 +589,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 +616,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)] From 3d36df49916e644ed72cc99c0597ba737ac9f770 Mon Sep 17 00:00:00 2001 From: setu Date: Wed, 1 Apr 2026 20:29:23 +0000 Subject: [PATCH 2/2] Fix race between featured daemon-reload and Docker container start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Following procedure during boot causes suprious issue: - the featured daemon loops through every feature and for each one writes a systemd config file, calls systemctl daemon-reload (which temporarily tears down cgroup directories), - Next, docker container start — but the container started in the previous iteration may still be initializing - Here when daemon-reload fires for the next feature, it destroys the group path that the previous container's runc process is trying to write its PID to, resulting in the "no such device" error. The fix splits this boot-time loop into two phases: - first write all systemd config files and do a single daemon-reload, - Secondly start all the containers Therefore, no daemon-reload can race against a container is starting up. Signed-off-by: setu --- tests/featured/featured_test.py | 83 ++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/tests/featured/featured_test.py b/tests/featured/featured_test.py index b410e021..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