Use SDK for deprecated baremetal proxy API

We had indicated that we would never switch this over to SDK, however,
this is the sole remaining user of ironicclient which means users would
continue needing to install that just for this little API. Better to
switch this holdout over and finally delete all the things.

Change-Id: I880523935d73ca94c83e618f10c2e587362c53be
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

nova/api/openstack/compute/baremetal_nodes.py

@@ -14,110 +14,100 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-"""The bare-metal admin extension."""
+"""The baremetal admin extension."""
 
-from oslo_utils import importutils
+from openstack import exceptions as sdk_exc
 import webob
 
 from nova.api.openstack.api_version_request \
     import MAX_PROXY_API_SUPPORT_VERSION
-from nova.api.openstack import common
 from nova.api.openstack import wsgi
 import nova.conf
 from nova.i18n import _
 from nova.policies import baremetal_nodes as bn_policies
-
+from nova import utils
-ironic_client = importutils.try_import('ironicclient.client')
-ironic_exc = importutils.try_import('ironicclient.exc')
 
 CONF = nova.conf.CONF
 
 
-def _check_ironic_client_enabled():
-    """Check whether Ironic is installed or not."""
-    if ironic_client is None:
-        common.raise_feature_not_supported()
-
-
-def _get_ironic_client():
-    """return an Ironic client."""
-    # TODO(NobodyCam): Fix insecure setting
-    # NOTE(efried): This should all be replaced by ksa adapter options; but the
-    # nova-to-baremetal API is deprecated, so not changing it.
-    # https://docs.openstack.org/api-ref/compute/#bare-metal-nodes-os-baremetal-nodes-deprecated  # noqa
-    kwargs = {'os_username': CONF.ironic.admin_username,
-              'os_password': CONF.ironic.admin_password,
-              'os_auth_url': CONF.ironic.admin_url,
-              'os_tenant_name': CONF.ironic.admin_tenant_name,
-              'os_service_type': 'baremetal',
-              'os_endpoint_type': 'public',
-              'insecure': 'true',
-              'endpoint': CONF.ironic.endpoint_override}
-    # NOTE(mriedem): The 1 api_version arg here is the only valid value for
-    # the client, but it's not even used so it doesn't really matter. The
-    # ironic client wrapper in the virt driver actually uses a hard-coded
-    # microversion via the os_ironic_api_version kwarg.
-    icli = ironic_client.get_client(1, **kwargs)
-    return icli
-
-
 def _no_ironic_proxy(cmd):
-    raise webob.exc.HTTPBadRequest(
+    msg = _(
-                    explanation=_("Command Not supported. Please use Ironic "
+        "Command Not supported. Please use Ironic "
-                                  "command %(cmd)s to perform this "
+        "command %(cmd)s to perform this action."
-                                  "action.") % {'cmd': cmd})
+    )
+    raise webob.exc.HTTPBadRequest(explanation=msg % {'cmd': cmd})
 
 
 class BareMetalNodeController(wsgi.Controller):
     """The Bare-Metal Node API controller for the OpenStack API."""
 
+    def __init__(self):
+        super().__init__()
+
+        self._ironic_connection = None
+
+    @property
+    def ironic_connection(self):
+        if self._ironic_connection is None:
+            # Ask get_sdk_adapter to raise ServiceUnavailable if the baremetal
+            # service isn't ready yet. Consumers of ironic_connection are set
+            # up to handle this and raise VirtDriverNotReady as appropriate.
+            self._ironic_connection = utils.get_sdk_adapter(
+                'baremetal',
+                check_service=True,
+            )
+        return self._ironic_connection
+
     @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION)
     @wsgi.expected_errors((404, 501))
     def index(self, req):
         context = req.environ['nova.context']
-        context.can(bn_policies.BASE_POLICY_NAME % 'list',
+        context.can(bn_policies.BASE_POLICY_NAME % 'list', target={})
-                    target={})
+
         nodes = []
         # proxy command to Ironic
-        _check_ironic_client_enabled()
+        inodes = self.ironic_connection.nodes(details=True)
-        icli = _get_ironic_client()
+        for inode in inodes:
-        ironic_nodes = icli.node.list(detail=True)
+            node = {
-        for inode in ironic_nodes:
+                'id': inode.id,
-            node = {'id': inode.uuid,
-                    'interfaces': [],
-                    'host': 'IRONIC MANAGED',
-                    'task_state': inode.provision_state,
-                    'cpus': inode.properties.get('cpus', 0),
-                    'memory_mb': inode.properties.get('memory_mb', 0),
-                    'disk_gb': inode.properties.get('local_gb', 0)}
-            nodes.append(node)
-        return {'nodes': nodes}
-
-    @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION)
-    @wsgi.expected_errors((404, 501))
-    def show(self, req, id):
-        context = req.environ['nova.context']
-        context.can(bn_policies.BASE_POLICY_NAME % 'show',
-                    target={})
-        # proxy command to Ironic
-        _check_ironic_client_enabled()
-        icli = _get_ironic_client()
-        try:
-            inode = icli.node.get(id)
-        except ironic_exc.NotFound:
-            msg = _("Node %s could not be found.") % id
-            raise webob.exc.HTTPNotFound(explanation=msg)
-        iports = icli.node.list_ports(id)
-        node = {'id': inode.uuid,
                 'interfaces': [],
                 'host': 'IRONIC MANAGED',
                 'task_state': inode.provision_state,
                 'cpus': inode.properties.get('cpus', 0),
                 'memory_mb': inode.properties.get('memory_mb', 0),
                 'disk_gb': inode.properties.get('local_gb', 0),
-                'instance_uuid': inode.instance_uuid}
+            }
+            nodes.append(node)
+
+        return {'nodes': nodes}
+
+    @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION)
+    @wsgi.expected_errors((404, 501))
+    def show(self, req, id):
+        context = req.environ['nova.context']
+        context.can(bn_policies.BASE_POLICY_NAME % 'show', target={})
+
+        # proxy command to Ironic
+        try:
+            inode = self.ironic_connection.get_node(id)
+        except sdk_exc.NotFoundException:
+            msg = _("Node %s could not be found.") % id
+            raise webob.exc.HTTPNotFound(explanation=msg)
+
+        iports = self.ironic_connection.ports(node=id)
+        node = {
+            'id': inode.id,
+            'interfaces': [],
+            'host': 'IRONIC MANAGED',
+            'task_state': inode.provision_state,
+            'cpus': inode.properties.get('cpus', 0),
+            'memory_mb': inode.properties.get('memory_mb', 0),
+            'disk_gb': inode.properties.get('local_gb', 0),
+            'instance_uuid': inode.instance_id,
+        }
         for port in iports:
             node['interfaces'].append({'address': port.address})
+
         return {'node': node}
 
     @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION)

nova/tests/functional/api_sample_tests/test_baremetal_nodes.py

@@ -14,51 +14,65 @@
 
 from unittest import mock
 
+from openstack.baremetal.v1 import _proxy as baremetal_proxy
+from openstack.baremetal.v1 import node
+
 from nova.tests.functional.api_sample_tests import api_sample_base
 
 
-class FakeNode(object):
+fake_nodes = [
-    def __init__(self, uuid='058d27fa-241b-445a-a386-08c04f96db43'):
+    node.Node(
-        self.uuid = uuid
+        id=id_,
-        self.provision_state = 'active'
+        provision_state='active',
-        self.properties = {'cpus': '2',
+        properties={
-                           'memory_mb': '1024',
+            'cpus': '2',
-                           'local_gb': '10'}
+            'memory_mb': '1024',
-        self.instance_uuid = '1ea4e53e-149a-4f02-9515-590c9fb2315a'
+            'local_gb': '10',
+        },
+        instance_id='1ea4e53e-149a-4f02-9515-590c9fb2315a',
+    ) for id_ in (
+        '058d27fa-241b-445a-a386-08c04f96db43',
+        'e2025409-f3ce-4d6a-9788-c565cf3b1b1c',
+    )
+]
 
 
-class NodeManager(object):
+def nodes(*args, **kwargs):
-    def list(self, detail=False):
+    for fake_node in fake_nodes:
-        return [FakeNode(), FakeNode('e2025409-f3ce-4d6a-9788-c565cf3b1b1c')]
+        yield fake_node
-
-    def get(self, id):
-        return FakeNode(id)
-
-    def list_ports(self, id):
-        return []
 
 
-class fake_client(object):
+def get_node(*args, **kwargs):
-    node = NodeManager()
+    return fake_nodes[0]
 
 
+def ports(*args, **kwargs):
+    # return an empty generator
+    return
+    yield
+
+
+fake_client = mock.create_autospec(baremetal_proxy.Proxy)
+fake_client.nodes.side_effect = nodes
+fake_client.get_node.side_effect = get_node
+fake_client.ports.side_effect = ports
+
+
+@mock.patch(
+    "nova.api.openstack.compute.baremetal_nodes"
+    ".BareMetalNodeController.ironic_connection",
+    new_callable=mock.PropertyMock,
+    return_value=fake_client,
+)
 class BareMetalNodesSampleJsonTest(api_sample_base.ApiSampleTestBaseV21):
     ADMIN_API = True
     sample_dir = "os-baremetal-nodes"
 
-    @mock.patch("nova.api.openstack.compute.baremetal_nodes"
-                "._get_ironic_client")
     def test_baremetal_nodes_list(self, mock_get_irc):
-        mock_get_irc.return_value = fake_client()
-
         response = self._do_get('os-baremetal-nodes')
         self._verify_response('baremetal-node-list-resp', {}, response, 200)
 
-    @mock.patch("nova.api.openstack.compute.baremetal_nodes"
-                "._get_ironic_client")
     def test_baremetal_nodes_get(self, mock_get_irc):
-        mock_get_irc.return_value = fake_client()
-
         response = self._do_get('os-baremetal-nodes/'
                                 '058d27fa-241b-445a-a386-08c04f96db43')
         self._verify_response('baremetal-node-get-resp', {}, response, 200)

nova/tests/unit/api/openstack/compute/test_baremetal_nodes.py

@@ -13,12 +13,11 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from unittest import mock
+import fixtures
-
+from openstack import exceptions as sdk_exc
-from ironicclient import exc as ironic_exc
 from webob import exc
 
-from nova.api.openstack.compute import baremetal_nodes as b_nodes_v21
+from nova.api.openstack.compute import baremetal_nodes
 from nova import context
 from nova import exception
 from nova import test
@@ -26,105 +25,69 @@ from nova.tests.unit.api.openstack import fakes
 from nova.tests.unit.virt.ironic import utils as ironic_utils
 
 
-def fake_node(**updates):
+class BareMetalNodesTest(test.NoDBTestCase):
-    node = {
+    mod = baremetal_nodes
-        'id': 1,
-        'service_host': "host",
-        'cpus': 8,
-        'memory_mb': 8192,
-        'local_gb': 128,
-        'pm_address': "10.1.2.3",
-        'pm_user': "pm_user",
-        'pm_password': "pm_pass",
-        'terminal_port': 8000,
-        'interfaces': [],
-        'instance_uuid': 'fake-instance-uuid',
-    }
-    if updates:
-        node.update(updates)
-    return node
-
-
-def fake_node_ext_status(**updates):
-    node = fake_node(uuid='fake-uuid',
-                     task_state='fake-task-state',
-                     updated_at='fake-updated-at',
-                     pxe_config_path='fake-pxe-config-path')
-    if updates:
-        node.update(updates)
-    return node
-
-
-FAKE_IRONIC_CLIENT = ironic_utils.FakeClient()
-
-
-@mock.patch.object(b_nodes_v21, '_get_ironic_client',
-                   lambda *_: FAKE_IRONIC_CLIENT)
-class BareMetalNodesTestV21(test.NoDBTestCase):
-    mod = b_nodes_v21
 
     def setUp(self):
-        super(BareMetalNodesTestV21, self).setUp()
+        super().setUp()
 
         self._setup()
         self.context = context.get_admin_context()
         self.request = fakes.HTTPRequest.blank('', use_admin_context=True)
 
-    def _setup(self):
+        # stub out openstacksdk
-        self.controller = b_nodes_v21.BareMetalNodeController()
+        self.mock_conn = self.useFixture(
+            fixtures.MockPatchObject(self.controller, '_ironic_connection'),
+        ).mock
 
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'list')
+    def _setup(self):
-    def test_index_ironic(self, mock_list):
+        self.controller = baremetal_nodes.BareMetalNodeController()
+
+    def test_index_ironic(self):
         properties = {'cpus': 2, 'memory_mb': 1024, 'local_gb': 20}
         node = ironic_utils.get_test_node(properties=properties)
-        mock_list.return_value = [node]
+        self.mock_conn.nodes.return_value = iter([node])
 
         res_dict = self.controller.index(self.request)
+
         expected_output = {'nodes':
                             [{'memory_mb': properties['memory_mb'],
                              'host': 'IRONIC MANAGED',
                              'disk_gb': properties['local_gb'],
                              'interfaces': [],
                              'task_state': None,
-                             'id': node.uuid,
+                             'id': node.id,
                              'cpus': properties['cpus']}]}
         self.assertEqual(expected_output, res_dict)
-        mock_list.assert_called_once_with(detail=True)
+        self.mock_conn.nodes.assert_called_once_with(details=True)
 
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'list')
+    def test_index_ironic_missing_properties(self):
-    def test_index_ironic_missing_properties(self, mock_list):
         properties = {'cpus': 2}
         node = ironic_utils.get_test_node(properties=properties)
-        mock_list.return_value = [node]
+        self.mock_conn.nodes.return_value = iter([node])
 
         res_dict = self.controller.index(self.request)
+
         expected_output = {'nodes':
                             [{'memory_mb': 0,
                              'host': 'IRONIC MANAGED',
                              'disk_gb': 0,
                              'interfaces': [],
                              'task_state': None,
-                             'id': node.uuid,
+                             'id': node.id,
                              'cpus': properties['cpus']}]}
         self.assertEqual(expected_output, res_dict)
-        mock_list.assert_called_once_with(detail=True)
+        self.mock_conn.nodes.assert_called_once_with(details=True)
 
-    def test_index_ironic_not_implemented(self):
+    def test_show_ironic(self):
-        with mock.patch.object(self.mod, 'ironic_client', None):
-            self.assertRaises(exc.HTTPNotImplemented,
-                              self.controller.index,
-                              self.request)
-
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'list_ports')
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'get')
-    def test_show_ironic(self, mock_get, mock_list_ports):
         properties = {'cpus': 1, 'memory_mb': 512, 'local_gb': 10}
         node = ironic_utils.get_test_node(properties=properties)
         port = ironic_utils.get_test_port()
-        mock_get.return_value = node
+        self.mock_conn.get_node.return_value = node
-        mock_list_ports.return_value = [port]
+        self.mock_conn.ports.return_value = iter([port])
+
+        res_dict = self.controller.show(self.request, node.id)
 
-        res_dict = self.controller.show(self.request, node.uuid)
         expected_output = {'node':
                             {'memory_mb': properties['memory_mb'],
                              'instance_uuid': None,
@@ -132,22 +95,21 @@ class BareMetalNodesTestV21(test.NoDBTestCase):
                              'disk_gb': properties['local_gb'],
                              'interfaces': [{'address': port.address}],
                              'task_state': None,
-                             'id': node.uuid,
+                             'id': node.id,
                              'cpus': properties['cpus']}}
         self.assertEqual(expected_output, res_dict)
-        mock_get.assert_called_once_with(node.uuid)
+        self.mock_conn.get_node.assert_called_once_with(node.id)
-        mock_list_ports.assert_called_once_with(node.uuid)
+        self.mock_conn.ports.assert_called_once_with(node=node.id)
 
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'list_ports')
+    def test_show_ironic_no_properties(self):
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'get')
-    def test_show_ironic_no_properties(self, mock_get, mock_list_ports):
         properties = {}
         node = ironic_utils.get_test_node(properties=properties)
         port = ironic_utils.get_test_port()
-        mock_get.return_value = node
+        self.mock_conn.get_node.return_value = node
-        mock_list_ports.return_value = [port]
+        self.mock_conn.ports.return_value = iter([port])
+
+        res_dict = self.controller.show(self.request, node.id)
 
-        res_dict = self.controller.show(self.request, node.uuid)
         expected_output = {'node':
                             {'memory_mb': 0,
                              'instance_uuid': None,
@@ -155,39 +117,35 @@ class BareMetalNodesTestV21(test.NoDBTestCase):
                              'disk_gb': 0,
                              'interfaces': [{'address': port.address}],
                              'task_state': None,
-                             'id': node.uuid,
+                             'id': node.id,
                              'cpus': 0}}
         self.assertEqual(expected_output, res_dict)
-        mock_get.assert_called_once_with(node.uuid)
+        self.mock_conn.get_node.assert_called_once_with(node.id)
-        mock_list_ports.assert_called_once_with(node.uuid)
+        self.mock_conn.ports.assert_called_once_with(node=node.id)
 
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'list_ports')
+    def test_show_ironic_no_interfaces(self):
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'get')
-    def test_show_ironic_no_interfaces(self, mock_get, mock_list_ports):
         properties = {'cpus': 1, 'memory_mb': 512, 'local_gb': 10}
         node = ironic_utils.get_test_node(properties=properties)
-        mock_get.return_value = node
+        self.mock_conn.get_node.return_value = node
-        mock_list_ports.return_value = []
+        self.mock_conn.ports.return_value = iter([])
+
+        res_dict = self.controller.show(self.request, node.id)
 
-        res_dict = self.controller.show(self.request, node.uuid)
         self.assertEqual([], res_dict['node']['interfaces'])
-        mock_get.assert_called_once_with(node.uuid)
+        self.mock_conn.get_node.assert_called_once_with(node.id)
-        mock_list_ports.assert_called_once_with(node.uuid)
+        self.mock_conn.ports.assert_called_once_with(node=node.id)
+
+    def test_show_ironic_node_not_found(self):
+        self.mock_conn.get_node.side_effect = sdk_exc.NotFoundException()
+
+        error = self.assertRaises(
+            exc.HTTPNotFound,
+            self.controller.show,
+            self.request, 'fake-uuid',
+        )
 
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'get',
-                       side_effect=ironic_exc.NotFound())
-    def test_show_ironic_node_not_found(self, mock_get):
-        error = self.assertRaises(exc.HTTPNotFound, self.controller.show,
-                                  self.request, 'fake-uuid')
         self.assertIn('fake-uuid', str(error))
 
-    def test_show_ironic_not_implemented(self):
-        with mock.patch.object(self.mod, 'ironic_client', None):
-            properties = {'cpus': 1, 'memory_mb': 512, 'local_gb': 10}
-            node = ironic_utils.get_test_node(properties=properties)
-            self.assertRaises(exc.HTTPNotImplemented, self.controller.show,
-                              self.request, node.uuid)
-
     def test_create_ironic_not_supported(self):
         self.assertRaises(exc.HTTPBadRequest,
                           self.controller.create,
@@ -209,11 +167,11 @@ class BareMetalNodesTestV21(test.NoDBTestCase):
                           self.request, 'fake-id', 'fake-body')
 
 
-class BareMetalNodesTestDeprecation(test.NoDBTestCase):
+class BareMetalNodesTestV236(test.NoDBTestCase):
 
     def setUp(self):
-        super(BareMetalNodesTestDeprecation, self).setUp()
+        super().setUp()
-        self.controller = b_nodes_v21.BareMetalNodeController()
+        self.controller = baremetal_nodes.BareMetalNodeController()
         self.req = fakes.HTTPRequest.blank('', version='2.36')
 
     def test_all_apis_return_not_found(self):

nova/tests/unit/policies/test_baremetal_nodes.py

@@ -10,8 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from unittest import mock
+import fixtures
-
 from oslo_utils.fixture import uuidsentinel as uuids
 
 from nova.api.openstack.compute import baremetal_nodes
@@ -22,9 +21,6 @@ from nova.tests.unit.policies import base
 from nova.tests.unit.virt.ironic import utils as ironic_utils
 
 
-FAKE_IRONIC_CLIENT = ironic_utils.FakeClient()
-
-
 class BaremetalNodesPolicyTest(base.BasePolicyTest):
     """Test Baremetal Nodes APIs policies with all possible context.
 
@@ -35,32 +31,38 @@ class BaremetalNodesPolicyTest(base.BasePolicyTest):
     """
 
     def setUp(self):
-        super(BaremetalNodesPolicyTest, self).setUp()
+        super().setUp()
+
         self.controller = baremetal_nodes.BareMetalNodeController()
         self.req = fakes.HTTPRequest.blank('')
-        self.stub_out('nova.api.openstack.compute.'
+
-                      'baremetal_nodes._get_ironic_client',
+        # stub out openstacksdk
-                      lambda *_: FAKE_IRONIC_CLIENT)
+        self.mock_conn = self.useFixture(
+            fixtures.MockPatchObject(self.controller, '_ironic_connection'),
+        ).mock
+
         # With legacy rule and scope check disabled by default, system admin,
         # legacy admin, and project admin will be able to get baremetal nodes.
         self.project_admin_authorized_contexts = [
-            self.legacy_admin_context, self.system_admin_context,
+            self.legacy_admin_context,
-            self.project_admin_context]
+            self.system_admin_context,
+            self.project_admin_context,
+        ]
 
     def test_index_nodes_policy(self):
         rule_name = "os_compute_api:os-baremetal-nodes:list"
+        self.mock_conn.nodes.return_value = iter([])
+
         self.common_policy_auth(self.project_admin_authorized_contexts,
                                 rule_name, self.controller.index,
                                 self.req)
 
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'list_ports')
+    def test_show_node_policy(self):
-    @mock.patch.object(FAKE_IRONIC_CLIENT.node, 'get')
-    def test_show_node_policy(self, mock_get, mock_port):
         rule_name = "os_compute_api:os-baremetal-nodes:show"
         properties = {'cpus': 1, 'memory_mb': 512, 'local_gb': 10}
         node = ironic_utils.get_test_node(properties=properties)
-        mock_get.return_value = node
+        self.mock_conn.get_node.return_value = node
-        mock_port.return_value = []
+        self.mock_conn.ports.return_value = iter([])
 
         self.common_policy_auth(self.project_admin_authorized_contexts,
                                 rule_name,