Metadata: handle process exceptions

mtomaska · mtomaska · commit 4698dd899fa1 · 2023-12-08T15:31:49.000-05:00
Both metadata agents (OVN and non-OVN) should handle process exceptions when spawning haproxy processes such that the agent can continue its operation for other haproxy processes. Note: neutron/tests/unit/agent/dhcp/test_agent.py#L836 Added `mock.call.get_pid_file_name()` to the expected list of process calls. This call was removed in stable/2031.1 and later. https://review.opendev.org/c/openstack/neutron/+/903149 Closes-Bug: #2033305 Change-Id: I6da1b135c83ecfc41ec91e907ebf8500325a7a80 (cherry picked from commit ed05157)
diff --git a/neutron/agent/metadata/driver.py b/neutron/agent/metadata/driver.py
@@ -294,7 +294,13 @@ def spawn_monitored_metadata_proxy(cls, monitor, ns_name, port, conf,
         pm = cls._get_metadata_proxy_process_manager(uuid, conf,
                                                      ns_name=ns_name,
                                                      callback=callback)
-        pm.enable()
+        try:
+            pm.enable()
+        except exceptions.ProcessExecutionError as exec_err:
+            LOG.error("Encountered process execution error %(err)s while "
+                      "starting process in namespace %(ns)s",
+                      {"err": exec_err, "ns": ns_name})
+            return
         monitor.register(uuid, METADATA_SERVICE_NAME, pm)
         cls.monitors[router_id] = pm
 
diff --git a/neutron/agent/ovn/metadata/driver.py b/neutron/agent/ovn/metadata/driver.py
@@ -205,7 +205,13 @@ def spawn_monitored_metadata_proxy(cls, monitor, ns_name, port, conf,
         pm = cls._get_metadata_proxy_process_manager(uuid, conf,
                                                      ns_name=ns_name,
                                                      callback=callback)
-        pm.enable()
+        try:
+            pm.enable()
+        except exceptions.ProcessExecutionError as exec_err:
+            LOG.error("Encountered process execution error %(err)s while "
+                      "starting process in namespace %(ns)s",
+                      {"err": exec_err, "ns": ns_name})
+            return
         monitor.register(uuid, METADATA_SERVICE_NAME, pm)
         cls.monitors[router_id] = pm
 
diff --git a/neutron/tests/unit/agent/dhcp/test_agent.py b/neutron/tests/unit/agent/dhcp/test_agent.py
@@ -807,23 +807,37 @@ def _process_manager_constructor_call(self, ns=FAKE_NETWORK_DHCP_NS):
     def _enable_dhcp_helper(self, network, enable_isolated_metadata=False,
                             is_isolated_network=False, is_ovn_network=False):
         self.dhcp._process_monitor = mock.Mock()
+        # The disable() call
+        gmppm_expected_calls = [mock.call(FAKE_NETWORK_UUID, cfg.CONF,
+                                ns_name=FAKE_NETWORK_DHCP_NS)]
         if enable_isolated_metadata:
             cfg.CONF.set_override('enable_isolated_metadata', True)
+            if is_isolated_network:
+                # The enable() call
+                gmppm_expected_calls.append(
+                    mock.call(FAKE_NETWORK_UUID, cfg.CONF,
+                              ns_name=FAKE_NETWORK_DHCP_NS,
+                              callback=mock.ANY))
         self.plugin.get_network_info.return_value = network
-        self.dhcp.enable_dhcp_helper(network.id)
+        process_instance = mock.Mock(active=False)
+        with mock.patch.object(metadata_driver.MetadataDriver,
+                               '_get_metadata_proxy_process_manager',
+                               return_value=process_instance) as gmppm:
+            self.dhcp.enable_dhcp_helper(network.id)
+            gmppm.assert_has_calls(gmppm_expected_calls)
         self.plugin.assert_has_calls([
             mock.call.get_network_info(network.id)])
         self.call_driver.assert_called_once_with('enable', network)
         self.cache.assert_has_calls([mock.call.put(network)])
         if (is_isolated_network and enable_isolated_metadata and not
                 is_ovn_network):
-            self.external_process.assert_has_calls([
-                self._process_manager_constructor_call(),
-                mock.call().enable()], any_order=True)
+            process_instance.assert_has_calls([
+                mock.call.disable(sig=str(int(signal.SIGTERM))),
+                mock.call.get_pid_file_name(),
+                mock.call.enable()])
         else:
-            self.external_process.assert_has_calls([
-                self._process_manager_constructor_call(),
-                mock.call().disable(sig=str(int(signal.SIGTERM)))])
+            process_instance.assert_has_calls([
+                mock.call.disable(sig=str(int(signal.SIGTERM)))])
 
     def test_enable_dhcp_helper_enable_metadata_isolated_network(self):
         self._enable_dhcp_helper(isolated_network,
@@ -997,11 +1011,16 @@ def test_disable_dhcp_helper_driver_failure(self):
 
     def test_enable_isolated_metadata_proxy(self):
         self.dhcp._process_monitor = mock.Mock()
-        self.dhcp.enable_isolated_metadata_proxy(fake_network)
-        self.external_process.assert_has_calls([
-            self._process_manager_constructor_call(),
-            mock.call().enable()
-        ], any_order=True)
+        process_instance = mock.Mock(active=False)
+        with mock.patch.object(metadata_driver.MetadataDriver,
+                               '_get_metadata_proxy_process_manager',
+                               return_value=process_instance) as gmppm:
+            self.dhcp.enable_isolated_metadata_proxy(fake_network)
+            gmppm.assert_called_with(FAKE_NETWORK_UUID,
+                                     cfg.CONF,
+                                     ns_name=FAKE_NETWORK_DHCP_NS,
+                                     callback=mock.ANY)
+        process_instance.enable.assert_called_once()
 
     def test_disable_isolated_metadata_proxy(self):
         method_path = ('neutron.agent.metadata.driver.MetadataDriver'
diff --git a/neutron/tests/unit/agent/metadata/test_driver.py b/neutron/tests/unit/agent/metadata/test_driver.py
@@ -18,6 +18,7 @@
 from unittest import mock
 
 from neutron_lib import constants
+from neutron_lib import exceptions as lib_exceptions
 from neutron_lib import fixture as lib_fixtures
 from oslo_config import cfg
 from oslo_utils import uuidutils
@@ -241,6 +242,26 @@ def test_spawn_metadata_proxy(self):
     def test_spawn_metadata_proxy_dad_failed(self):
         self._test_spawn_metadata_proxy(dad_failed=True)
 
+    @mock.patch.object(metadata_driver.LOG, 'error')
+    def test_spawn_metadata_proxy_handles_process_exception(self, error_log):
+        process_instance = mock.Mock(active=False)
+        process_instance.enable.side_effect = (
+            lib_exceptions.ProcessExecutionError('Something happened', -1))
+        with mock.patch.object(metadata_driver.MetadataDriver,
+                               '_get_metadata_proxy_process_manager',
+                               return_value=process_instance):
+            process_monitor = mock.Mock()
+            network_id = 123456
+            metadata_driver.MetadataDriver.spawn_monitored_metadata_proxy(
+                process_monitor,
+                'dummy_namespace',
+                self.METADATA_PORT,
+                cfg.CONF,
+                network_id=network_id)
+        error_log.assert_called_once()
+        process_monitor.register.assert_not_called()
+        self.assertNotIn(network_id, metadata_driver.MetadataDriver.monitors)
+
     def test_create_config_file_wrong_user(self):
         with mock.patch('pwd.getpwnam', side_effect=KeyError):
             config = metadata_driver.HaproxyConfigurator(_uuid(),
diff --git a/neutron/tests/unit/agent/ovn/metadata/test_driver.py b/neutron/tests/unit/agent/ovn/metadata/test_driver.py
@@ -16,6 +16,7 @@
 import os
 from unittest import mock
 
+from neutron_lib import exceptions as lib_exceptions
 from neutron_lib import fixture as lib_fixtures
 from oslo_config import cfg
 from oslo_utils import uuidutils
@@ -112,6 +113,29 @@ def test_spawn_metadata_proxy(self):
                                           run_as_root=True)
             ])
 
+    @mock.patch.object(metadata_driver.LOG, 'error')
+    def test_spawn_metadata_proxy_handles_process_exception(self, error_log):
+        process_instance = mock.Mock(active=False)
+        process_instance.enable.side_effect = (
+            lib_exceptions.ProcessExecutionError('Something happened', -1))
+
+        with mock.patch.object(metadata_driver.MetadataDriver,
+                               '_get_metadata_proxy_process_manager',
+                               return_value=process_instance):
+            process_monitor = mock.Mock()
+            network_id = 123456
+
+            metadata_driver.MetadataDriver.spawn_monitored_metadata_proxy(
+                process_monitor,
+                'dummy_namespace',
+                self.METADATA_PORT,
+                cfg.CONF,
+                network_id=network_id)
+
+        error_log.assert_called_once()
+        process_monitor.register.assert_not_called()
+        self.assertNotIn(network_id, metadata_driver.MetadataDriver.monitors)
+
     def test_create_config_file_wrong_user(self):
         with mock.patch('pwd.getpwnam', side_effect=KeyError):
             config = metadata_driver.HaproxyConfigurator(mock.ANY, mock.ANY,
