openstack/openstacksdk
Code Issues Proposed changes

Change approach to detailed listings of baremetal resources

Browse Source 
The current approach with separate ResourceDetail classes suffers
from serious flows and is barely usable. For example for nodes:
* get_node() returns complete Node objects
* nodes() returns incomplete Node objects
* nodes(details=True) returns NodeDetail objects, which are exactly
  like complete Node objects, except that you cannot do anything
  with them: updating, listing VIFs, etc.
This is confusing and essentially requires the caller to issue
a redundant get_node call after listing detailed nodes.

This change consolidates everything into Node (Port, etc) objects
and deprecates NodeDetail (PortDetail, etc). I would highly recommend
the same approach taken for other ResourceDetail objects.

Change-Id: Idedee2b394e2ed412a69880d4bc9315737e1c3ed
This commit is contained in:
Dmitry Tantsur 2018-11-22 15:00:45 +01:00
parent cdef4806ce
commit 5c5478d774
16 changed files with 145 additions and 247 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
openstack/baremetal/v1/_common.py
View File

@ -55,3 +55,32 @@ STATE_VERSIONS = {
VIF_VERSION = '1.28'
"""API version in which the VIF operations were introduced."""
class ListMixin(object):
@classmethod
def list(cls, session, details=False, **params):
"""This method is a generator which yields resource objects.
This resource object list generator handles pagination and takes query
params for response filtering.
:param session: The session to use for making this request.
:type session: :class:`~keystoneauth1.adapter.Adapter`
:param bool details: Whether to return detailed node records
:param dict params: These keyword arguments are passed through the
:meth:`~openstack.resource.QueryParameter._transpose` method
to find if any of them match expected query parameters to be
sent in the *params* argument to
:meth:`~keystoneauth1.adapter.Adapter.get`.
:return: A generator of :class:`openstack.resource.Resource` objects.
:raises: :exc:`~openstack.exceptions.InvalidResourceQuery` if query
contains invalid params.
"""
base_path = cls.base_path
if details:
base_path += '/detail'
return super(ListMixin, cls).list(session, paginated=True,
base_path=base_path, **params)

14
openstack/baremetal/v1/_proxy.py
View File

@ -61,8 +61,7 @@ class Proxy(proxy.Proxy):
:returns: A generator of chassis instances.
"""
cls = _chassis.ChassisDetail if details else _chassis.Chassis
return self._list(cls, paginated=True, **query)
return _chassis.Chassis.list(self, details=details, **query)
def create_chassis(self, **attrs):
"""Create a new chassis from attributes.
@ -198,10 +197,9 @@ class Proxy(proxy.Proxy):
direction of the server attribute that is provided as the
``sort_key``.
:returns: A generator of node instances.
:returns: A generator of :class:`~openstack.baremetal.v1.node.Node`
"""
cls = _node.NodeDetail if details else _node.Node
return self._list(cls, paginated=True, **query)
return _node.Node.list(self, details=details, **query)
def create_node(self, **attrs):
"""Create a new node from attributes.
@ -442,8 +440,7 @@ class Proxy(proxy.Proxy):
:returns: A generator of port instances.
"""
cls = _port.PortDetail if details else _port.Port
return self._list(cls, paginated=True, **query)
return _port.Port.list(self, details=details, **query)
def create_port(self, **attrs):
"""Create a new port from attributes.
@ -555,8 +552,7 @@ class Proxy(proxy.Proxy):
:returns: A generator of port group instances.
"""
cls = _portgroup.PortGroupDetail if details else _portgroup.PortGroup
return self._list(cls, paginated=True, **query)
return _portgroup.PortGroup.list(self, details=details, **query)
def create_port_group(self, **attrs):
"""Create a new portgroup from attributes.

17
openstack/baremetal/v1/chassis.py
View File

@ -10,10 +10,11 @@
# License for the specific language governing permissions and limitations
# under the License.
from openstack.baremetal.v1 import _common
from openstack import resource
class Chassis(resource.Resource):
class Chassis(_common.ListMixin, resource.Resource):
resources_key = 'chassis'
base_path = '/chassis'
@ -47,16 +48,4 @@ class Chassis(resource.Resource):
updated_at = resource.Body('updated_at')
class ChassisDetail(Chassis):
base_path = '/chassis/detail'
# capabilities
allow_create = False
allow_fetch = False
allow_commit = False
allow_delete = False
allow_list = True
#: The UUID for the chassis
id = resource.Body('uuid', alternate_id=True)
ChassisDetail = Chassis

23
openstack/baremetal/v1/node.py
View File

@ -34,7 +34,7 @@ class ValidationResult(object):
self.reason = reason
class Node(resource.Resource):
class Node(_common.ListMixin, resource.Resource):
resources_key = 'nodes'
base_path = '/nodes'
@ -589,23 +589,4 @@ class Node(resource.Resource):
for key, value in result.items()}
class NodeDetail(Node):
base_path = '/nodes/detail'
# capabilities
allow_create = False
allow_fetch = False
allow_commit = False
allow_delete = False
allow_list = True
_query_mapping = resource.QueryParameters(
'associated', 'conductor_group', 'driver', 'fault',
'provision_state', 'resource_class',
instance_id='instance_uuid',
is_maintenance='maintenance',
)
#: The UUID of the node resource.
id = resource.Body("uuid", alternate_id=True)
NodeDetail = Node

23
openstack/baremetal/v1/port.py
View File

@ -10,10 +10,11 @@
# License for the specific language governing permissions and limitations
# under the License.
from openstack.baremetal.v1 import _common
from openstack import resource
class Port(resource.Resource):
class Port(_common.ListMixin, resource.Resource):
resources_key = 'ports'
base_path = '/ports'
@ -29,6 +30,7 @@ class Port(resource.Resource):
_query_mapping = resource.QueryParameters(
'address', 'fields', 'node', 'portgroup',
node_id='node_uuid',
)
# The physical_network field introduced in 1.34
@ -67,21 +69,4 @@ class Port(resource.Resource):
updated_at = resource.Body('updated_at')
class PortDetail(Port):
base_path = '/ports/detail'
# capabilities
allow_create = False
allow_fetch = False
allow_commit = False
allow_delete = False
allow_list = True
_query_mapping = resource.QueryParameters(
'address', 'fields', 'node', 'portgroup',
node_id='node_uuid',
)
#: The UUID of the port
id = resource.Body('uuid', alternate_id=True)
PortDetail = Port

20
openstack/baremetal/v1/port_group.py
View File

@ -10,10 +10,11 @@
# License for the specific language governing permissions and limitations
# under the License.
from openstack.baremetal.v1 import _common
from openstack import resource
class PortGroup(resource.Resource):
class PortGroup(_common.ListMixin, resource.Resource):
resources_key = 'portgroups'
base_path = '/portgroups'
@ -66,19 +67,4 @@ class PortGroup(resource.Resource):
updated_at = resource.Body('updated_at')
class PortGroupDetail(PortGroup):
base_path = '/portgroups/detail'
allow_create = False
allow_fetch = False
allow_commit = False
allow_delete = False
allow_list = True
_query_mapping = resource.QueryParameters(
'node', 'address',
)
#: The UUID for the portgroup
id = resource.Body('uuid', alternate_id=True)
PortGroupDetail = PortGroup

11
openstack/resource.py
View File

@ -1206,7 +1206,7 @@ class Resource(dict):
return self
@classmethod
def list(cls, session, paginated=False, **params):
def list(cls, session, paginated=False, base_path=None, **params):
"""This method is a generator which yields resource objects.
This resource object list generator handles pagination and takes query
@ -1220,6 +1220,9 @@ class Resource(dict):
**When paginated is False only one
page of data will be returned regardless
of the API's support of pagination.**
:param str base_path: Base part of the URI for listing resources, if
different from
:data:`~openstack.resource.Resource.base_path`.
:param dict params: These keyword arguments are passed through the
:meth:`~openstack.resource.QueryParamter._transpose` method
to find if any of them match expected query parameters to be
@ -1241,9 +1244,11 @@ class Resource(dict):
session = cls._get_session(session)
microversion = cls._get_microversion_for_list(session)
cls._query_mapping._validate(params, base_path=cls.base_path)
if base_path is None:
base_path = cls.base_path
cls._query_mapping._validate(params, base_path=base_path)
query_params = cls._query_mapping._transpose(params)
uri = cls.base_path % params
uri = base_path % params
limit = query_params.get('limit')

14
openstack/tests/functional/baremetal/test_baremetal_node.py
View File

@ -86,6 +86,20 @@ class TestBareMetalNode(base.BaseBaremetalTest):
node = self.conn.baremetal.get_node('node-name')
self.assertIsNone(node.instance_id)
def test_node_list_update_delete(self):
self.create_node(name='node-name', extra={'foo': 'bar'})
node = next(n for n in
self.conn.baremetal.nodes(details=True,
provision_state='available',
is_maintenance=False,
associated=False)
if n.name == 'node-name')
self.assertEqual(node.extra, {'foo': 'bar'})
# This test checks that resources returned from listing are usable
self.conn.baremetal.update_node(node, extra={'foo': 42})
self.conn.baremetal.delete_node(node, ignore_missing=False)
def test_node_create_in_enroll_provide(self):
node = self.create_node(provision_state='enroll')
self.node_id = node.id

11
openstack/tests/functional/baremetal/test_baremetal_port.py
View File

@ -53,6 +53,17 @@ class TestBareMetalPort(base.BaseBaremetalTest):
ports = self.conn.baremetal.ports(node='test-node')
self.assertEqual([p.id for p in ports], [port1.id])
def test_port_list_update_delete(self):
self.create_port(address='11:22:33:44:55:66', node_id=self.node.id,
extra={'foo': 'bar'})
port = next(self.conn.baremetal.ports(details=True,
address='11:22:33:44:55:66'))
self.assertEqual(port.extra, {'foo': 'bar'})
# This test checks that resources returned from listing are usable
self.conn.baremetal.update_port(port, extra={'foo': 42})
self.conn.baremetal.delete_port(port, ignore_missing=False)
def test_port_update(self):
port = self.create_port(address='11:22:33:44:55:66')
port.address = '66:55:44:33:22:11'

11
openstack/tests/functional/baremetal/test_baremetal_port_group.py
View File

@ -51,6 +51,17 @@ class TestBareMetalPortGroup(base.BaseBaremetalTest):
pgs = self.conn.baremetal.port_groups(node='test-node')
self.assertEqual([p.id for p in pgs], [pg1.id])
def test_port_list_update_delete(self):
self.create_port_group(address='11:22:33:44:55:66',
extra={'foo': 'bar'})
port_group = next(self.conn.baremetal.port_groups(
details=True, address='11:22:33:44:55:66'))
self.assertEqual(port_group.extra, {'foo': 'bar'})
# This test checks that resources returned from listing are usable
self.conn.baremetal.update_port_group(port_group, extra={'foo': 42})
self.conn.baremetal.delete_port_group(port_group, ignore_missing=False)
def test_port_group_update(self):
port_group = self.create_port_group()
port_group.extra = {'answer': 42}

24
openstack/tests/unit/baremetal/v1/test_chassis.py
View File

@ -66,27 +66,3 @@ class TestChassis(base.TestCase):
self.assertEqual(FAKE['links'], sot.links)
self.assertEqual(FAKE['nodes'], sot.nodes)
self.assertEqual(FAKE['updated_at'], sot.updated_at)
class TestChassisDetail(base.TestCase):
def test_basic(self):
sot = chassis.ChassisDetail()
self.assertIsNone(sot.resource_key)
self.assertEqual('chassis', sot.resources_key)
self.assertEqual('/chassis/detail', sot.base_path)
self.assertFalse(sot.allow_create)
self.assertFalse(sot.allow_fetch)
self.assertFalse(sot.allow_commit)
self.assertFalse(sot.allow_delete)
self.assertTrue(sot.allow_list)
def test_instantiate(self):
sot = chassis.ChassisDetail(**FAKE)
self.assertEqual(FAKE['uuid'], sot.id)
self.assertEqual(FAKE['created_at'], sot.created_at)
self.assertEqual(FAKE['description'], sot.description)
self.assertEqual(FAKE['extra'], sot.extra)
self.assertEqual(FAKE['links'], sot.links)
self.assertEqual(FAKE['nodes'], sot.nodes)
self.assertEqual(FAKE['updated_at'], sot.updated_at)

52
openstack/tests/unit/baremetal/v1/test_node.py
View File

@ -153,58 +153,6 @@ class TestNode(base.TestCase):
self.assertEqual('available', sot.provision_state)
class TestNodeDetail(base.TestCase):
def test_basic(self):
sot = node.NodeDetail()
self.assertIsNone(sot.resource_key)
self.assertEqual('nodes', sot.resources_key)
self.assertEqual('/nodes/detail', sot.base_path)
self.assertFalse(sot.allow_create)
self.assertFalse(sot.allow_fetch)
self.assertFalse(sot.allow_commit)
self.assertFalse(sot.allow_delete)
self.assertTrue(sot.allow_list)
def test_instantiate(self):
sot = node.NodeDetail(**FAKE)
self.assertEqual(FAKE['uuid'], sot.id)
self.assertEqual(FAKE['name'], sot.name)
self.assertEqual(FAKE['chassis_uuid'], sot.chassis_id)
self.assertEqual(FAKE['clean_step'], sot.clean_step)
self.assertEqual(FAKE['created_at'], sot.created_at)
self.assertEqual(FAKE['driver'], sot.driver)
self.assertEqual(FAKE['driver_info'], sot.driver_info)
self.assertEqual(FAKE['driver_internal_info'],
sot.driver_internal_info)
self.assertEqual(FAKE['extra'], sot.extra)
self.assertEqual(FAKE['instance_info'], sot.instance_info)
self.assertEqual(FAKE['instance_uuid'], sot.instance_id)
self.assertEqual(FAKE['console_enabled'], sot.is_console_enabled)
self.assertEqual(FAKE['maintenance'], sot.is_maintenance)
self.assertEqual(FAKE['last_error'], sot.last_error)
self.assertEqual(FAKE['links'], sot.links)
self.assertEqual(FAKE['maintenance_reason'], sot.maintenance_reason)
self.assertEqual(FAKE['name'], sot.name)
self.assertEqual(FAKE['network_interface'], sot.network_interface)
self.assertEqual(FAKE['ports'], sot.ports)
self.assertEqual(FAKE['portgroups'], sot.port_groups)
self.assertEqual(FAKE['power_state'], sot.power_state)
self.assertEqual(FAKE['properties'], sot.properties)
self.assertEqual(FAKE['provision_state'], sot.provision_state)
self.assertEqual(FAKE['raid_config'], sot.raid_config)
self.assertEqual(FAKE['reservation'], sot.reservation)
self.assertEqual(FAKE['resource_class'], sot.resource_class)
self.assertEqual(FAKE['states'], sot.states)
self.assertEqual(FAKE['target_provision_state'],
sot.target_provision_state)
self.assertEqual(FAKE['target_power_state'], sot.target_power_state)
self.assertEqual(FAKE['target_raid_config'], sot.target_raid_config)
self.assertEqual(FAKE['updated_at'], sot.updated_at)
@mock.patch('time.sleep', lambda _t: None)
@mock.patch.object(node.Node, 'fetch', autospec=True)
class TestNodeWaitForProvisionState(base.TestCase):

29
openstack/tests/unit/baremetal/v1/test_port.py
View File

@ -70,32 +70,3 @@ class TestPort(base.TestCase):
self.assertEqual(FAKE['portgroup_uuid'], sot.port_group_id)
self.assertEqual(FAKE['pxe_enabled'], sot.is_pxe_enabled)
self.assertEqual(FAKE['updated_at'], sot.updated_at)
class TestPortDetail(base.TestCase):
def test_basic(self):
sot = port.PortDetail()
self.assertIsNone(sot.resource_key)
self.assertEqual('ports', sot.resources_key)
self.assertEqual('/ports/detail', sot.base_path)
self.assertFalse(sot.allow_create)
self.assertFalse(sot.allow_fetch)
self.assertFalse(sot.allow_commit)
self.assertFalse(sot.allow_delete)
self.assertTrue(sot.allow_list)
def test_instantiate(self):
sot = port.PortDetail(**FAKE)
self.assertEqual(FAKE['uuid'], sot.id)
self.assertEqual(FAKE['address'], sot.address)
self.assertEqual(FAKE['created_at'], sot.created_at)
self.assertEqual(FAKE['extra'], sot.extra)
self.assertEqual(FAKE['internal_info'], sot.internal_info)
self.assertEqual(FAKE['links'], sot.links)
self.assertEqual(FAKE['local_link_connection'],
sot.local_link_connection)
self.assertEqual(FAKE['node_uuid'], sot.node_id)
self.assertEqual(FAKE['portgroup_uuid'], sot.port_group_id)
self.assertEqual(FAKE['pxe_enabled'], sot.is_pxe_enabled)
self.assertEqual(FAKE['updated_at'], sot.updated_at)

29
openstack/tests/unit/baremetal/v1/test_port_group.py
View File

@ -75,32 +75,3 @@ class TestPortGroup(base.TestCase):
self.assertEqual(FAKE['standalone_ports_supported'],
sot.is_standalone_ports_supported)
self.assertEqual(FAKE['updated_at'], sot.updated_at)
class TestPortGroupDetail(base.TestCase):
def test_basic(self):
sot = port_group.PortGroupDetail()
self.assertIsNone(sot.resource_key)
self.assertEqual('portgroups', sot.resources_key)
self.assertEqual('/portgroups/detail', sot.base_path)
self.assertFalse(sot.allow_create)
self.assertFalse(sot.allow_fetch)
self.assertFalse(sot.allow_commit)
self.assertFalse(sot.allow_delete)
self.assertTrue(sot.allow_list)
def test_instantiate(self):
sot = port_group.PortGroupDetail(**FAKE)
self.assertEqual(FAKE['uuid'], sot.id)
self.assertEqual(FAKE['address'], sot.address)
self.assertEqual(FAKE['created_at'], sot.created_at)
self.assertEqual(FAKE['extra'], sot.extra)
self.assertEqual(FAKE['internal_info'], sot.internal_info)
self.assertEqual(FAKE['links'], sot.links)
self.assertEqual(FAKE['name'], sot.name)
self.assertEqual(FAKE['node_uuid'], sot.node_id)
self.assertEqual(FAKE['ports'], sot.ports)
self.assertEqual(FAKE['standalone_ports_supported'],
sot.is_standalone_ports_supported)
self.assertEqual(FAKE['updated_at'], sot.updated_at)

73
openstack/tests/unit/baremetal/v1/test_proxy.py
View File

@ -17,6 +17,7 @@ from openstack.baremetal.v1 import chassis
from openstack.baremetal.v1 import driver
from openstack.baremetal.v1 import node
from openstack.baremetal.v1 import port
from openstack.baremetal.v1 import port_group
from openstack import exceptions
from openstack.tests.unit import base
from openstack.tests.unit import test_proxy_base
@ -34,17 +35,17 @@ class TestBaremetalProxy(test_proxy_base.TestProxyBase):
def test_get_driver(self):
self.verify_get(self.proxy.get_driver, driver.Driver)
def test_chassis_detailed(self):
self.verify_list(self.proxy.chassis, chassis.ChassisDetail,
paginated=True,
method_kwargs={"details": True, "query": 1},
expected_kwargs={"query": 1})
@mock.patch.object(chassis.Chassis, 'list')
def test_chassis_detailed(self, mock_list):
result = self.proxy.chassis(details=True, query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=True, query=1)
def test_chassis_not_detailed(self):
self.verify_list(self.proxy.chassis, chassis.Chassis,
paginated=True,
method_kwargs={"details": False, "query": 1},
expected_kwargs={"query": 1})
@mock.patch.object(chassis.Chassis, 'list')
def test_chassis_not_detailed(self, mock_list):
result = self.proxy.chassis(query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=False, query=1)
def test_create_chassis(self):
self.verify_create(self.proxy.create_chassis, chassis.Chassis)
@ -64,17 +65,17 @@ class TestBaremetalProxy(test_proxy_base.TestProxyBase):
def test_delete_chassis_ignore(self):
self.verify_delete(self.proxy.delete_chassis, chassis.Chassis, True)
def test_nodes_detailed(self):
self.verify_list(self.proxy.nodes, node.NodeDetail,
paginated=True,
method_kwargs={"details": True, "query": 1},
expected_kwargs={"query": 1})
@mock.patch.object(node.Node, 'list')
def test_nodes_detailed(self, mock_list):
result = self.proxy.nodes(details=True, query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=True, query=1)
def test_nodes_not_detailed(self):
self.verify_list(self.proxy.nodes, node.Node,
paginated=True,
method_kwargs={"details": False, "query": 1},
expected_kwargs={"query": 1})
@mock.patch.object(node.Node, 'list')
def test_nodes_not_detailed(self, mock_list):
result = self.proxy.nodes(query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=False, query=1)
def test_create_node(self):
self.verify_create(self.proxy.create_node, node.Node)
@ -106,17 +107,17 @@ class TestBaremetalProxy(test_proxy_base.TestProxyBase):
def test_delete_node_ignore(self):
self.verify_delete(self.proxy.delete_node, node.Node, True)
def test_ports_detailed(self):
self.verify_list(self.proxy.ports, port.PortDetail,
paginated=True,
method_kwargs={"details": True, "query": 1},
expected_kwargs={"query": 1})
@mock.patch.object(port.Port, 'list')
def test_ports_detailed(self, mock_list):
result = self.proxy.ports(details=True, query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=True, query=1)
def test_ports_not_detailed(self):
self.verify_list(self.proxy.ports, port.Port,
paginated=True,
method_kwargs={"details": False, "query": 1},
expected_kwargs={"query": 1})
@mock.patch.object(port.Port, 'list')
def test_ports_not_detailed(self, mock_list):
result = self.proxy.ports(query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=False, query=1)
def test_create_port(self):
self.verify_create(self.proxy.create_port, port.Port)
@ -136,6 +137,18 @@ class TestBaremetalProxy(test_proxy_base.TestProxyBase):
def test_delete_port_ignore(self):
self.verify_delete(self.proxy.delete_port, port.Port, True)
@mock.patch.object(port_group.PortGroup, 'list')
def test_port_groups_detailed(self, mock_list):
result = self.proxy.port_groups(details=True, query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=True, query=1)
@mock.patch.object(port_group.PortGroup, 'list')
def test_port_groups_not_detailed(self, mock_list):
result = self.proxy.port_groups(query=1)
self.assertIs(result, mock_list.return_value)
mock_list.assert_called_once_with(self.proxy, details=False, query=1)
@mock.patch('time.sleep', lambda _sec: None)
@mock.patch.object(_proxy.Proxy, 'get_node', autospec=True)

12
releasenotes/notes/baremetal-details-09b27fba82111cfb.yaml Normal file
View File

@ -0,0 +1,12 @@
---
features:
- |
The objects returned by baremetal detailed listing functions
(``connection.baremetal.{nodes,ports,chassis,port_groups}``) are now
fully functional, e.g. can be directly updated or deleted.
deprecations:
- |
The following baremetal resource classes are no longer used and will be
removed in a future release: ``NodeDetail``, ``PortDetail``,
``ChassisDetail`` and ``PortGroupDetail``. The regular ``Node``, ``Port``,
``Chassis`` and ``PortGroup`` are now used instead.