Merge "Make inspection URL optional if the collectors are provided"

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
+<admin/inspection/index.html>`, 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``

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.

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',
+               default=APARAMS.get('ipa-inspection-collectors'),
-                                   INSPECTION_DEFAULT_COLLECTOR),
                help='Comma-separated list of plugins providing additional '
-                    'hardware data for inspection, empty value gives '
+                    'hardware data for inspection. The default are %s. '
-                    'a minimum required set of plugins. '
                     'Can be supplied as "ipa-inspection-collectors" '
-                    'kernel parameter.'),
+                    'kernel parameter.'
+                    % INSPECTION_DEFAULT_COLLECTORS),
 
     cfg.IntOpt('inspection_dhcp_wait_timeout',
                min=0,

ironic_python_agent/inspector.py

@@ -53,8 +53,9 @@ def extension_manager(names):
 
 
 def _get_collector_names():
-    return [x.strip() for x in CONF.inspection_collectors.split(',')
+    collectors = (CONF.inspection_collectors
-            if x.strip()]
+                  or config.INSPECTION_DEFAULT_COLLECTORS)
+    return [x.strip() for x in collectors.split(',') if x.strip()]
 
 
 def inspect():
@@ -67,7 +68,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
 
@@ -122,6 +124,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()
@@ -140,7 +150,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(

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)

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.