Add manage_boot parameter to introspection API

Adds a new node field "manage_boot" to store this value. When it is set
to False, neither boot device nor power state are touched for this node.
Instead, we expect a 3rd party to handle them.

We still manage the PXE filter because the node may need DHCP.

Change-Id: Id3585bd32138a069dfcfc0ab04ee4f5f10f0a5ea
Story: #1528920
Task: #11338

doc/source/user/http-api.rst

@@ -15,6 +15,11 @@ done prior to calling the endpoint.
 
 Requires X-Auth-Token header with Keystone token for authentication.
 
+Optional parameter:
+
+* ``manage_boot`` boolean value, whether to manage boot (boot device, power
+  and firewall) for a node. Defaults to true.
+
 Response:
 
 * 202 - accepted introspection request
@@ -390,3 +395,4 @@ Version History
 * **1.11** adds invert&multiple fields into rules response data
 * **1.12** this version indicates that support for setting IPMI credentials
   was completely removed from API (all versions).
+* **1.13** adds ``manage_boot`` parameter for the introspection API.

ironic_inspector/common/rpc.py

@@ -36,7 +36,7 @@ def get_transport():
 
 def get_client():
     target = messaging.Target(topic=TOPIC, server=SERVER_NAME,
-                              version='1.0')
+                              version='1.1')
     transport = get_transport()
     return messaging.RPCClient(transport, target)
 
@@ -48,7 +48,7 @@ def get_server():
     if _SERVER is None:
         transport = get_transport()
         target = messaging.Target(topic=TOPIC, server=SERVER_NAME,
-                                  version='1.0')
+                                  version='1.1')
         mgr = manager.ConductorManager()
         _SERVER = messaging.get_rpc_server(
             transport, target, [mgr], executor='eventlet',

ironic_inspector/conductor/manager.py

@@ -20,13 +20,14 @@ from ironic_inspector import utils
 
 class ConductorManager(object):
     """ironic inspector conductor manager"""
-    RPC_API_VERSION = '1.0'
+    RPC_API_VERSION = '1.1'
 
     target = messaging.Target(version=RPC_API_VERSION)
 
     @messaging.expected_exceptions(utils.Error)
-    def do_introspection(self, context, node_id, token=None):
+    def do_introspection(self, context, node_id, token=None,
-        introspect.introspect(node_id, token=token)
+                         manage_boot=True):
+        introspect.introspect(node_id, token=token, manage_boot=manage_boot)
 
     @messaging.expected_exceptions(utils.Error)
     def do_abort(self, context, node_id, token=None):

ironic_inspector/conf/default.py

@@ -69,7 +69,13 @@ _OPTS = [
                help=_('Path to the rootwrap configuration file to use for '
                       'running commands as root')),
     cfg.IntOpt('api_max_limit', default=1000, min=1,
-               help=_('Limit the number of elements an API list-call returns'))
+               help=_('Limit the number of elements an API list-call '
+                      'returns')),
+    cfg.BoolOpt('can_manage_boot', default=True,
+                help=_('Whether the current installation of ironic-inspector '
+                       'can manage PXE booting of nodes. If set to False, '
+                       'the API will reject introspection requests with '
+                       'manage_boot missing or set to True.'))
 ]
 
 

ironic_inspector/db.py

@@ -57,6 +57,7 @@ class Node(Base):
     started_at = Column(DateTime, nullable=True)
     finished_at = Column(DateTime, nullable=True)
     error = Column(Text, nullable=True)
+    manage_boot = Column(Boolean, nullable=True, default=True)
 
     # version_id is being tracked in the NodeInfo object
     # for the sake of consistency. See also SQLAlchemy docs:

ironic_inspector/introspect.py

@@ -34,10 +34,11 @@ _LAST_INTROSPECTION_TIME = 0
 _LAST_INTROSPECTION_LOCK = semaphore.BoundedSemaphore()
 
 
-def introspect(node_id, token=None):
+def introspect(node_id, manage_boot=True, token=None):
     """Initiate hardware properties introspection for a given node.
 
     :param node_id: node UUID or name
+    :param manage_boot: whether to manage boot for this node
     :param token: authentication token
     :raises: Error
     """
@@ -45,6 +46,7 @@ def introspect(node_id, token=None):
     node = ir_utils.get_node(node_id, ironic=ironic)
 
     ir_utils.check_provision_state(node)
+    if manage_boot:
         validation = ironic.node.validate(node.uuid)
         if not validation.power['result']:
             msg = _('Failed validation of power interface, reason: %s')
@@ -54,6 +56,7 @@ def introspect(node_id, token=None):
     bmc_address = ir_utils.get_ipmi_address(node)
     node_info = node_cache.start_introspection(node.uuid,
                                                bmc_address=bmc_address,
+                                               manage_boot=manage_boot,
                                                ironic=ironic)
 
     utils.executor().submit(_background_introspect, node_info, ironic)
@@ -98,6 +101,7 @@ def _background_introspect_locked(node_info, ironic):
     LOG.info('The following attributes will be used for look up: %s',
              attrs, node_info=node_info)
 
+    if node_info.manage_boot:
         try:
             ironic.node.set_boot_device(node_info.uuid, 'pxe',
                                         persistent=False)
@@ -113,6 +117,10 @@ def _background_introspect_locked(node_info, ironic):
                               exc, node_info=node_info)
         LOG.info('Introspection started successfully',
                  node_info=node_info)
+    else:
+        LOG.info('Introspection environment is ready, external power on '
+                 'is required within %d seconds', CONF.timeout,
+                 node_info=node_info)
 
 
 def abort(node_id, token=None):
@@ -142,6 +150,7 @@ def _abort(node_info, ironic):
     # runs in background
 
     LOG.debug('Forcing power-off', node_info=node_info)
+    if node_info.manage_boot:
         try:
             ironic.node.set_power_state(node_info.uuid, 'off')
         except Exception as exc:

ironic_inspector/main.py

@@ -15,6 +15,7 @@ import os
 import re
 
 import flask
+from oslo_utils import strutils
 from oslo_utils import uuidutils
 import six
 import werkzeug
@@ -39,7 +40,7 @@ app = flask.Flask(__name__)
 LOG = utils.getProcessingLogger(__name__)
 
 MINIMUM_API_VERSION = (1, 0)
-CURRENT_API_VERSION = (1, 12)
+CURRENT_API_VERSION = (1, 13)
 DEFAULT_API_VERSION = CURRENT_API_VERSION
 _LOGGING_EXCLUDED_KEYS = ('logs',)
 
@@ -239,8 +240,23 @@ def api_continue():
      methods=['GET', 'POST'])
 def api_introspection(node_id):
     if flask.request.method == 'POST':
+        args = flask.request.args
+
+        manage_boot = args.get('manage_boot', 'True')
+        try:
+            manage_boot = strutils.bool_from_string(manage_boot, strict=True)
+        except ValueError:
+            raise utils.Error(_('Invalid boolean value for manage_boot: %s') %
+                              manage_boot, code=400)
+
+        if manage_boot and not CONF.can_manage_boot:
+            raise utils.Error(_('Managed boot is requested, but this '
+                                'installation cannot manage boot ('
+                                '(can_manage_boot set to False)'),
+                              code=400)
         client = rpc.get_client()
         client.call({}, 'do_introspection', node_id=node_id,
+                    manage_boot=manage_boot,
                     token=flask.request.headers.get('X-Auth-Token'))
         return '', 202
     else:

ironic_inspector/migrations/versions/2970d2d44edc_add_manage_boot_to_nodes.py

@@ -0,0 +1,33 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+"""Add manage_boot to nodes
+
+Revision ID: 2970d2d44edc
+Revises: e169a4a81d88
+Create Date: 2016-05-16 14:03:02.861672
+
+"""
+
+from alembic import op
+import sqlalchemy as sa
+
+# revision identifiers, used by Alembic.
+revision = '2970d2d44edc'
+down_revision = '18440d0834af'
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    op.add_column('nodes', sa.Column('manage_boot', sa.Boolean(),
+                                     nullable=True, default=True))

ironic_inspector/node_cache.py

@@ -71,7 +71,7 @@ class NodeInfo(object):
 
     def __init__(self, uuid, version_id=None, state=None, started_at=None,
                  finished_at=None, error=None, node=None, ports=None,
-                 ironic=None, lock=None):
+                 ironic=None, lock=None, manage_boot=True):
         self.uuid = uuid
         self.started_at = started_at
         self.finished_at = finished_at
@@ -85,6 +85,9 @@ class NodeInfo(object):
         self._ports = ports
         self._attributes = None
         self._ironic = ironic
+        # On upgrade existing records will have manage_boot=NULL, which is
+        # equivalent to True actually.
+        self._manage_boot = manage_boot if manage_boot is not None else True
         # This is a lock on a node UUID, not on a NodeInfo object
         self._lock = lock if lock is not None else _get_lock(uuid)
         # Whether lock was acquired using this NodeInfo object
@@ -262,6 +265,11 @@ class NodeInfo(object):
             self._ironic = ir_utils.get_client()
         return self._ironic
 
+    @property
+    def manage_boot(self):
+        """Whether to manage boot for this node."""
+        return self._manage_boot
+
     def set_option(self, name, value):
         """Set an option for a node."""
         encoded = json.dumps(value)
@@ -315,7 +323,7 @@ class NodeInfo(object):
         """Construct NodeInfo from a database row."""
         fields = {key: row[key]
                   for key in ('uuid', 'version_id', 'state', 'started_at',
-                              'finished_at', 'error')}
+                              'finished_at', 'error', 'manage_boot')}
         return cls(ironic=ironic, lock=lock, node=node, **fields)
 
     def invalidate_cache(self):
@@ -672,7 +680,7 @@ def start_introspection(uuid, **kwargs):
         return add_node(uuid, state, **kwargs)
 
 
-def add_node(uuid, state, **attributes):
+def add_node(uuid, state, manage_boot=True, **attributes):
     """Store information about a node under introspection.
 
     All existing information about this node is dropped.
@@ -680,6 +688,7 @@ def add_node(uuid, state, **attributes):
 
     :param uuid: Ironic node UUID
     :param state: The initial state of the node
+    :param manage_boot: whether to manage boot for this node
     :param attributes: attributes known about this node (like macs, BMC etc);
                        also ironic client instance may be passed under 'ironic'
     :returns: NodeInfo
@@ -689,10 +698,10 @@ def add_node(uuid, state, **attributes):
         _delete_node(uuid)
         version_id = uuidutils.generate_uuid()
         db.Node(uuid=uuid, state=state, version_id=version_id,
-                started_at=started_at).save(session)
+                started_at=started_at, manage_boot=manage_boot).save(session)
 
         node_info = NodeInfo(uuid=uuid, state=state, started_at=started_at,
-                             version_id=version_id,
+                             version_id=version_id, manage_boot=manage_boot,
                              ironic=attributes.pop('ironic', None))
         for (name, value) in attributes.items():
             if not value:
@@ -738,8 +747,9 @@ def introspection_active():
 
 def active_macs():
     """List all MAC's that are on introspection right now."""
-    return ({x.value for x in db.model_query(db.Attribute.value).
+    query = (db.model_query(db.Attribute.value).join(db.Node)
-            filter_by(name=MACS_ATTRIBUTE)})
+             .filter(db.Attribute.name == MACS_ATTRIBUTE))
+    return {x.value for x in query}
 
 
 def _list_node_uuids():

ironic_inspector/test/functional.py

@@ -167,9 +167,11 @@ class Base(base.NodeTest):
                 raise AssertionError(msg)
         return res
 
-    def call_introspect(self, uuid, **kwargs):
+    def call_introspect(self, uuid, manage_boot=True, **kwargs):
         endpoint = '/v1/introspection/%s' % uuid
-        return self.call('post', endpoint, **kwargs)
+        if manage_boot is not None:
+            endpoint = '%s?manage_boot=%s' % (endpoint, manage_boot)
+        return self.call('post', endpoint)
 
     def call_get_status(self, uuid, **kwargs):
         return self.call('get', '/v1/introspection/%s' % uuid, **kwargs).json()
@@ -257,6 +259,7 @@ class Test(Base):
         self.cli.port.create.assert_called_once_with(
             node_uuid=self.uuid, address='11:22:33:44:55:66', extra={},
             pxe_enabled=True)
+        self.assertTrue(self.cli.node.set_boot_device.called)
 
         status = self.call_get_status(self.uuid)
         self.check_status(status, finished=True, state=istate.States.finished)
@@ -358,6 +361,24 @@ class Test(Base):
                              status_.get('links')[0].get('href')).path).json()
                           for status_ in statuses])
 
+    def test_manage_boot(self):
+        self.call_introspect(self.uuid, manage_boot=False)
+        eventlet.greenthread.sleep(DEFAULT_SLEEP)
+        self.assertFalse(self.cli.node.set_power_state.called)
+
+        status = self.call_get_status(self.uuid)
+        self.check_status(status, finished=False, state=istate.States.waiting)
+
+        res = self.call_continue(self.data)
+        self.assertEqual({'uuid': self.uuid}, res)
+        eventlet.greenthread.sleep(DEFAULT_SLEEP)
+
+        self.cli.node.update.assert_called_with(self.uuid, mock.ANY)
+        self.assertFalse(self.cli.node.set_boot_device.called)
+
+        status = self.call_get_status(self.uuid)
+        self.check_status(status, finished=True, state=istate.States.finished)
+
     def test_rules_api(self):
         res = self.call_list_rules()
         self.assertEqual([], res)

ironic_inspector/test/unit/test_introspect.py

@@ -67,6 +67,7 @@ class TestIntrospect(BaseTest):
 
         start_mock.assert_called_once_with(self.uuid,
                                            bmc_address=self.bmc_address,
+                                           manage_boot=True,
                                            ironic=cli)
         self.node_info.ports.assert_called_once_with()
         self.node_info.add_attribute.assert_called_once_with('mac',
@@ -92,6 +93,7 @@ class TestIntrospect(BaseTest):
 
         start_mock.assert_called_once_with(self.uuid,
                                            bmc_address=None,
+                                           manage_boot=True,
                                            ironic=cli)
         self.node_info.ports.assert_called_once_with()
         self.node_info.add_attribute.assert_called_once_with('mac',
@@ -115,6 +117,7 @@ class TestIntrospect(BaseTest):
 
         start_mock.assert_called_with(self.uuid,
                                       bmc_address=self.bmc_address,
+                                      manage_boot=True,
                                       ironic=cli)
 
     def test_power_failure(self, client_mock, start_mock):
@@ -129,6 +132,7 @@ class TestIntrospect(BaseTest):
 
         start_mock.assert_called_once_with(self.uuid,
                                            bmc_address=self.bmc_address,
+                                           manage_boot=True,
                                            ironic=cli)
         cli.node.set_boot_device.assert_called_once_with(self.uuid,
                                                          'pxe',
@@ -151,6 +155,7 @@ class TestIntrospect(BaseTest):
 
         start_mock.assert_called_once_with(self.uuid,
                                            bmc_address=self.bmc_address,
+                                           manage_boot=True,
                                            ironic=cli)
         self.assertFalse(cli.node.set_boot_device.called)
         start_mock.return_value.finished.assert_called_once_with(
@@ -169,6 +174,7 @@ class TestIntrospect(BaseTest):
 
         start_mock.assert_called_once_with(self.uuid,
                                            bmc_address=self.bmc_address,
+                                           manage_boot=True,
                                            ironic=cli)
         self.assertFalse(self.node_info.add_attribute.called)
         self.assertFalse(self.sync_filter_mock.called)
@@ -315,6 +321,27 @@ class TestIntrospect(BaseTest):
         # updated to the current time.time()
         self.assertEqual(100, introspect._LAST_INTROSPECTION_TIME)
 
+    def test_no_manage_boot(self, client_mock, add_mock):
+        cli = self._prepare(client_mock)
+        self.node_info.manage_boot = False
+        add_mock.return_value = self.node_info
+
+        introspect.introspect(self.node.uuid, manage_boot=False)
+
+        cli.node.get.assert_called_once_with(self.uuid)
+
+        add_mock.assert_called_once_with(self.uuid,
+                                         bmc_address=self.bmc_address,
+                                         manage_boot=False,
+                                         ironic=cli)
+        self.node_info.ports.assert_called_once_with()
+        self.node_info.add_attribute.assert_called_once_with('mac',
+                                                             self.macs)
+        self.sync_filter_mock.assert_called_with(cli)
+        self.assertFalse(cli.node.validate.called)
+        self.assertFalse(cli.node.set_boot_device.called)
+        self.assertFalse(cli.node.set_power_state.called)
+
 
 @mock.patch.object(node_cache, 'get_node', autospec=True)
 @mock.patch.object(ir_utils, 'get_client', autospec=True)
@@ -346,6 +373,25 @@ class TestAbort(BaseTest):
             introspect.istate.Events.abort_end, error='Canceled by operator')
         self.node_info.fsm_event.assert_has_calls(self.fsm_calls)
 
+    def test_no_manage_boot(self, client_mock, get_mock):
+        cli = self._prepare(client_mock)
+        get_mock.return_value = self.node_info
+        self.node_info.acquire_lock.return_value = True
+        self.node_info.started_at = time.time()
+        self.node_info.finished_at = None
+        self.node_info.manage_boot = False
+
+        introspect.abort(self.node.uuid)
+
+        get_mock.assert_called_once_with(self.uuid, ironic=cli,
+                                         locked=False)
+        self.node_info.acquire_lock.assert_called_once_with(blocking=False)
+        self.sync_filter_mock.assert_called_once_with(cli)
+        self.assertFalse(cli.node.set_power_state.called)
+        self.node_info.finished.assert_called_once_with(
+            introspect.istate.Events.abort_end, error='Canceled by operator')
+        self.node_info.fsm_event.assert_has_calls(self.fsm_calls)
+
     def test_node_not_found(self, client_mock, get_mock):
         cli = self._prepare(client_mock)
         exc = utils.Error('Not found.', code=404)

ironic_inspector/test/unit/test_main.py

@@ -66,11 +66,11 @@ class TestApiIntrospect(BaseAPITest):
         self.assertEqual(202, res.status_code)
         self.client_mock.call.assert_called_once_with({}, 'do_introspection',
                                                       node_id=self.uuid,
+                                                      manage_boot=True,
                                                       token=None)
 
     def test_intospect_failed(self):
         self.client_mock.call.side_effect = utils.Error("boom")
-
         res = self.app.post('/v1/introspection/%s' % self.uuid)
 
         self.assertEqual(400, res.status_code)
@@ -79,8 +79,40 @@ class TestApiIntrospect(BaseAPITest):
             json.loads(res.data.decode('utf-8'))['error']['message'])
         self.client_mock.call.assert_called_once_with({}, 'do_introspection',
                                                       node_id=self.uuid,
+                                                      manage_boot=True,
                                                       token=None)
 
+    def test_introspect_no_manage_boot(self):
+        res = self.app.post('/v1/introspection/%s?manage_boot=0' % self.uuid)
+        self.assertEqual(202, res.status_code)
+        self.client_mock.call.assert_called_once_with({}, 'do_introspection',
+                                                      node_id=self.uuid,
+                                                      manage_boot=False,
+                                                      token=None)
+
+    def test_introspect_can_manage_boot_false(self):
+        CONF.set_override('can_manage_boot', False)
+        res = self.app.post('/v1/introspection/%s?manage_boot=0' % self.uuid)
+        self.assertEqual(202, res.status_code)
+        self.client_mock.call.assert_called_once_with({}, 'do_introspection',
+                                                      node_id=self.uuid,
+                                                      manage_boot=False,
+                                                      token=None)
+
+    def test_introspect_can_manage_boot_false_failed(self):
+        CONF.set_override('can_manage_boot', False)
+        res = self.app.post('/v1/introspection/%s' % self.uuid)
+        self.assertEqual(400, res.status_code)
+        self.assertFalse(self.client_mock.call.called)
+
+    def test_introspect_wrong_manage_boot(self):
+        res = self.app.post('/v1/introspection/%s?manage_boot=foo' % self.uuid)
+        self.assertEqual(400, res.status_code)
+        self.assertFalse(self.client_mock.call.called)
+        self.assertEqual(
+            'Invalid boolean value for manage_boot: foo',
+            json.loads(res.data.decode('utf-8'))['error']['message'])
+
     @mock.patch.object(utils, 'check_auth', autospec=True)
     def test_introspect_failed_authentication(self, auth_mock):
         CONF.set_override('auth_strategy', 'keystone')

ironic_inspector/test/unit/test_manager.py

@@ -37,7 +37,16 @@ class TestManagerIntrospect(BaseManagerTest):
     def test_do_introspect(self, introspect_mock):
         self.manager.do_introspection(self.context, self.uuid, self.token)
 
-        introspect_mock.assert_called_once_with(self.uuid, self.token)
+        introspect_mock.assert_called_once_with(self.uuid, token=self.token,
+                                                manage_boot=True)
+
+    @mock.patch.object(introspect, 'introspect', autospec=True)
+    def test_do_introspect_with_manage_boot(self, introspect_mock):
+        self.manager.do_introspection(self.context, self.uuid, self.token,
+                                      False)
+
+        introspect_mock.assert_called_once_with(self.uuid, token=self.token,
+                                                manage_boot=False)
 
     @mock.patch.object(introspect, 'introspect', autospec=True)
     def test_introspect_failed(self, introspect_mock):
@@ -48,7 +57,8 @@ class TestManagerIntrospect(BaseManagerTest):
                                 self.context, self.uuid, self.token)
 
         self.assertEqual(utils.Error, exc.exc_info[0])
-        introspect_mock.assert_called_once_with(self.uuid, token=None)
+        introspect_mock.assert_called_once_with(self.uuid, token=None,
+                                                manage_boot=True)
 
 
 class TestManagerAbort(BaseManagerTest):

ironic_inspector/test/unit/test_migrations.py

@@ -433,6 +433,14 @@ class MigrationCheckersMixin(object):
         self.assertEqual('foo', row.name)
         self.assertEqual('bar', row.value)
 
+    def _check_2970d2d44edc(self, engine, data):
+        nodes = db_utils.get_table(engine, 'nodes')
+        data = {'uuid': 'abcd'}
+        nodes.insert().execute(data)
+
+        n = nodes.select(nodes.c.uuid == 'abcd').execute().first()
+        self.assertIsNone(n['manage_boot'])
+
     def test_upgrade_and_version(self):
         with patch_with_engine(self.engine):
             self.migration_ext.upgrade('head')

ironic_inspector/test/unit/test_node_cache.py

@@ -120,15 +120,22 @@ class TestNodeCache(test_base.NodeTest):
 
     def test_active_macs(self):
         session = db.get_writer_session()
+        uuid2 = uuidutils.generate_uuid()
         with session.begin():
             db.Node(uuid=self.node.uuid,
                     state=istate.States.starting).save(session)
+            db.Node(uuid=uuid2,
+                    state=istate.States.starting,
+                    manage_boot=False).save(session)
             values = [('mac', '11:22:11:22:11:22', self.uuid),
-                      ('mac', '22:11:22:11:22:11', self.uuid)]
+                      ('mac', '22:11:22:11:22:11', self.uuid),
+                      ('mac', 'aa:bb:cc:dd:ee:ff', uuid2)]
             for value in values:
                 db.Attribute(uuid=uuidutils.generate_uuid(), name=value[0],
                              value=value[1], node_uuid=value[2]).save(session)
-        self.assertEqual({'11:22:11:22:11:22', '22:11:22:11:22:11'},
+        self.assertEqual({'11:22:11:22:11:22', '22:11:22:11:22:11',
+                          # We still need to serve DHCP to unmanaged nodes
+                          'aa:bb:cc:dd:ee:ff'},
                          node_cache.active_macs())
 
     def test__list_node_uuids(self):

releasenotes/notes/manage-boot-2ae986f87098576b.yaml

@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    Adds new parameter ``manage_boot`` to the introspection API to allow
+    disabling boot management (setting the boot device and rebooting)
+    for a specific node. If it is set to ``False``, the boot is supposed
+    to be managed by a 3rd party.
+
+    If the new option ``can_manage_boot`` is set to ``False`` (the default is
+    ``True), then ``manage_boot`` must be explicitly set to ``False``.