Improve race condition handling in featured daemon-reload and docker container startup

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)

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)]