From 6cd36a750f27d65a3f24a47fff0ebde9e21b57f6 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 19 Dec 2023 19:30:16 +0100 Subject: [PATCH] Make inspection URL optional if the collectors are provided With the new in-band inspection, we can derive the callback URL from the Ironic URL, there is no need to duplicate it. This change uses the presence of collectors as a sign to run inspection. The previous approach of setting an inspection URL, with or without explicitly setting collectors, still works for compatibility with ironic-inspector. Change-Id: Ie4279ee6d2995c9686f1dcdef1d6e5dc1dd20871 --- doc/source/admin/how_it_works.rst | 12 +++ ironic_python_agent/agent.py | 2 +- ironic_python_agent/config.py | 15 ++-- ironic_python_agent/inspector.py | 18 ++++- .../tests/unit/test_inspector.py | 76 ++++++++++++++++++- .../notes/inspect-url-15547d48432cd2b5.yaml | 13 ++++ 6 files changed, 122 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/inspect-url-15547d48432cd2b5.yaml diff --git a/doc/source/admin/how_it_works.rst b/doc/source/admin/how_it_works.rst index 80c34231d..7ba9f4925 100644 --- a/doc/source/admin/how_it_works.rst +++ b/doc/source/admin/how_it_works.rst @@ -49,6 +49,18 @@ for example:: Make sure your DHCP environment is set to boot IPA by default. +If you use the new built-in :ironic-doc:`Ironic in-band inspection +`, it is enough to only set a list of collectors +(see `inspection data`_), for example:: + + ipa-inspection-collectors=default,logs + +Then the correct callback URL will be determined from the Ironic URL in +``ipa-api-url``. + +Instance agent +~~~~~~~~~~~~~~ + For the cases where the infrastructure operator and cloud user are the same, an additional tool exists that can be installed alongside the agent inside a running instance. This is the ``ironic-collect-introspection-data`` diff --git a/ironic_python_agent/agent.py b/ironic_python_agent/agent.py index d41a03b61..a9ed441e5 100644 --- a/ironic_python_agent/agent.py +++ b/ironic_python_agent/agent.py @@ -526,7 +526,7 @@ class IronicPythonAgent(base.ExecuteCommandMixin): # interfaces to perform those actions over. self._wait_for_interface() - if cfg.CONF.inspection_callback_url: + if self.api_urls or cfg.CONF.inspection_callback_url: try: # Attempt inspection. This may fail, and previously # an error would be logged. diff --git a/ironic_python_agent/config.py b/ironic_python_agent/config.py index 35cde2729..6de4da826 100644 --- a/ironic_python_agent/config.py +++ b/ironic_python_agent/config.py @@ -23,7 +23,7 @@ CONF = cfg.CONF APARAMS = utils.get_agent_params() -INSPECTION_DEFAULT_COLLECTOR = 'default,logs' +INSPECTION_DEFAULT_COLLECTORS = 'default,logs' INSPECTION_DEFAULT_DHCP_WAIT_TIMEOUT = 60 cli_opts = [ @@ -162,16 +162,17 @@ cli_opts = [ 'A special value "mdns" can be specified to fetch the ' 'URL using multicast DNS service discovery. ' 'Can be supplied as "ipa-inspection-callback-url" ' - 'kernel parameter.'), + 'kernel parameter. If the URL is not provided but ' + 'inspection_collectors is not empty, the URL is detected ' + 'from api_url.'), cfg.StrOpt('inspection_collectors', - default=APARAMS.get('ipa-inspection-collectors', - INSPECTION_DEFAULT_COLLECTOR), + default=APARAMS.get('ipa-inspection-collectors'), help='Comma-separated list of plugins providing additional ' - 'hardware data for inspection, empty value gives ' - 'a minimum required set of plugins. ' + 'hardware data for inspection. The default are %s. ' 'Can be supplied as "ipa-inspection-collectors" ' - 'kernel parameter.'), + 'kernel parameter.' + % INSPECTION_DEFAULT_COLLECTORS), cfg.IntOpt('inspection_dhcp_wait_timeout', min=0, diff --git a/ironic_python_agent/inspector.py b/ironic_python_agent/inspector.py index 7d9dc0733..958db3ccc 100644 --- a/ironic_python_agent/inspector.py +++ b/ironic_python_agent/inspector.py @@ -52,8 +52,9 @@ def extension_manager(names): def _get_collector_names(): - return [x.strip() for x in CONF.inspection_collectors.split(',') - if x.strip()] + collectors = (CONF.inspection_collectors + or config.INSPECTION_DEFAULT_COLLECTORS) + return [x.strip() for x in collectors.split(',') if x.strip()] def inspect(): @@ -66,7 +67,8 @@ def inspect(): was not found in inspector cache. None is also returned if inspector support is not enabled. """ - if not CONF.inspection_callback_url: + if (not CONF.inspection_callback_url + and not (CONF.inspection_collectors and CONF.api_url)): LOG.info('Inspection is disabled, skipping') return @@ -121,6 +123,14 @@ _RETRY_WAIT_MAX = 30 _RETRY_ATTEMPTS = 5 +def _get_urls(): + urls = CONF.inspection_callback_url or CONF.api_url + urls = list(filter(None, urls.split(','))) + if not CONF.inspection_callback_url: + urls = [url.rstrip('/') + "/v1/continue_inspection" for url in urls] + return urls + + def call_inspector(data, failures): """Post data to inspector.""" data['error'] = failures.get_error() @@ -139,7 +149,7 @@ def call_inspector(data, failures): if CONF.global_request_id: headers["X-OpenStack-Request-ID"] = CONF.global_request_id - urls = list(filter(None, CONF.inspection_callback_url.split(','))) + urls = _get_urls() @tenacity.retry( retry=tenacity.retry_if_exception_type( diff --git a/ironic_python_agent/tests/unit/test_inspector.py b/ironic_python_agent/tests/unit/test_inspector.py index 5f05fed79..d5c6f35af 100644 --- a/ironic_python_agent/tests/unit/test_inspector.py +++ b/ironic_python_agent/tests/unit/test_inspector.py @@ -49,7 +49,7 @@ class AcceptingFailure(mock.Mock): class TestMisc(base.IronicAgentTest): def test_default_collector_loadable(self): - defaults = config.INSPECTION_DEFAULT_COLLECTOR.split(',') + defaults = config.INSPECTION_DEFAULT_COLLECTORS.split(',') # default should go first self.assertEqual('default', defaults[0]) # logs much go last @@ -71,7 +71,6 @@ class TestInspect(base.IronicAgentTest): def setUp(self): super(TestInspect, self).setUp() CONF.set_override('inspection_callback_url', 'http://foo/bar') - CONF.set_override('inspection_collectors', '') self.mock_collect = AcceptingFailure() self.mock_ext = mock.Mock(spec=['plugin', 'name'], plugin=self.mock_collect) @@ -85,6 +84,35 @@ class TestInspect(base.IronicAgentTest): self.mock_collect.assert_called_with_failure() mock_call.assert_called_with_failure() self.assertEqual('uuid1', result) + mock_ext_mgr.assert_called_once_with( + inspector._COLLECTOR_NS, ['default', 'logs'], + name_order=True, on_missing_entrypoints_callback=mock.ANY) + + def test_ok_with_ironic_url(self, mock_ext_mgr, mock_call): + CONF.set_override('api_url', 'http://url') + CONF.set_override('inspection_callback_url', '') + CONF.set_override('inspection_collectors', 'default') + mock_ext_mgr.return_value = [self.mock_ext] + mock_call.return_value = {'uuid': 'uuid1'} + + result = inspector.inspect() + + self.mock_collect.assert_called_with_failure() + mock_call.assert_called_with_failure() + self.assertEqual('uuid1', result) + mock_ext_mgr.assert_called_once_with( + inspector._COLLECTOR_NS, ['default'], + name_order=True, on_missing_entrypoints_callback=mock.ANY) + + def test_disabled(self, mock_ext_mgr, mock_call): + CONF.set_override('inspection_callback_url', '') + mock_ext_mgr.return_value = [self.mock_ext] + + result = inspector.inspect() + + self.mock_collect.assert_not_called() + mock_call.assert_not_called() + self.assertIsNone(result) @mock.patch('ironic_lib.mdns.get_endpoint', autospec=True) def test_mdns(self, mock_mdns, mock_ext_mgr, mock_call): @@ -169,6 +197,25 @@ class TestCallInspector(base.IronicAgentTest): timeout=30) self.assertEqual(mock_post.return_value.json.return_value, res) + def test_use_api_url(self, mock_post): + CONF.set_override('inspection_callback_url', '') + CONF.set_override('api_url', 'http://url1/,http://url2/baremetal') + + failures = utils.AccumulatedFailures() + data = collections.OrderedDict(data=42) + mock_post.return_value.status_code = 200 + + res = inspector.call_inspector(data, failures) + + mock_post.assert_called_once_with( + 'http://url1/v1/continue_inspection', + data='{"data": 42, "error": null}', + cert=None, verify=True, + headers={'Content-Type': 'application/json', + 'Accept': 'application/json'}, + timeout=30) + self.assertEqual(mock_post.return_value.json.return_value, res) + def test_send_failure(self, mock_post): failures = mock.Mock(spec=utils.AccumulatedFailures) failures.get_error.return_value = "boom" @@ -231,6 +278,31 @@ class TestCallInspector(base.IronicAgentTest): data='{"data": 42, "error": null}', timeout=30), ]) + def test_use_several_api_urls(self, mock_post): + CONF.set_override('inspection_callback_url', '') + CONF.set_override('api_url', 'http://url1/,http://url2/baremetal') + + good_resp = mock.Mock(status_code=200) + mock_post.side_effect = [ + requests.exceptions.ConnectionError, good_resp + ] + + failures = utils.AccumulatedFailures() + data = collections.OrderedDict(data=42) + mock_post.return_value.status_code = 200 + + res = inspector.call_inspector(data, failures) + + mock_post.assert_has_calls([ + mock.call('http://url1/v1/continue_inspection', + cert=None, verify=True, headers=mock.ANY, + data='{"data": 42, "error": null}', timeout=30), + mock.call('http://url2/baremetal/v1/continue_inspection', + cert=None, verify=True, headers=mock.ANY, + data='{"data": 42, "error": null}', timeout=30), + ]) + self.assertEqual(good_resp.json.return_value, res) + @mock.patch.object(inspector, '_RETRY_WAIT', 0.01) @mock.patch.object(inspector, '_RETRY_WAIT_MAX', 1) @mock.patch.object(inspector, '_RETRY_ATTEMPTS', 3) diff --git a/releasenotes/notes/inspect-url-15547d48432cd2b5.yaml b/releasenotes/notes/inspect-url-15547d48432cd2b5.yaml new file mode 100644 index 000000000..40fe6b1bc --- /dev/null +++ b/releasenotes/notes/inspect-url-15547d48432cd2b5.yaml @@ -0,0 +1,13 @@ +--- +features: + - | + When the new Ironic built-in inspection is used, + ``ipa-inspection-callback-url`` can now be automatically derived from + ``ipa-api-url``. In this case, inspection will be enabled if the + ``ipa-inspection-collectors`` option is set. +upgrade: + - | + If you currently set the ``ipa-inspection-collectors`` option without + setting ``ipa-inspection-callback-url``, it will now cause inspection + to run. Update your boot configuration to only supply the collectors + when inspection is desired.