diff --git a/openstack/baremetal/v1/_common.py b/openstack/baremetal/v1/_common.py index 7981c95f5..54f1862e7 100644 --- a/openstack/baremetal/v1/_common.py +++ b/openstack/baremetal/v1/_common.py @@ -59,6 +59,7 @@ EXPECTED_POWER_STATES = { """Mapping of target power states to expected power states.""" STATE_VERSIONS = { + 'available': '1.1', 'enroll': '1.11', 'manageable': '1.4', } diff --git a/openstack/baremetal/v1/_proxy.py b/openstack/baremetal/v1/_proxy.py index 6b61d2e17..5148b2bf4 100644 --- a/openstack/baremetal/v1/_proxy.py +++ b/openstack/baremetal/v1/_proxy.py @@ -276,6 +276,9 @@ class Proxy(proxy.Proxy): def create_node(self, **attrs): """Create a new node from attributes. + See :meth:`~openstack.baremetal.v1.node.Node.create` for an explanation + of the initial provision state. + :param dict attrs: Keyword arguments that will be used to create a :class:`~openstack.baremetal.v1.node.Node`. diff --git a/openstack/baremetal/v1/node.py b/openstack/baremetal/v1/node.py index 5db579a58..0a3a1385d 100644 --- a/openstack/baremetal/v1/node.py +++ b/openstack/baremetal/v1/node.py @@ -257,12 +257,21 @@ class Node(_common.ListMixin, resource.Resource): The overridden version is capable of handling the populated ``provision_state`` field of one of three values: ``enroll``, - ``manageable`` or ``available``. The default is currently - ``available``, since it's the only state supported by all API versions. + ``manageable`` or ``available``. If not provided, the server default + is used (``enroll`` in newer versions of Ironic). + + This call does not cause a node to go through automated cleaning. + If you need it, use ``provision_state=manageable`` followed by + a call to :meth:`set_provision_state`. Note that Bare Metal API 1.4 is required for ``manageable`` and 1.11 is required for ``enroll``. + .. warning:: + Using ``provision_state=available`` is only possible with API + versions 1.1 to 1.10 and thus is incompatible with setting any + fields that appeared after 1.11. + :param session: The session to use for making this request. :type session: :class:`~keystoneauth1.adapter.Adapter` @@ -274,44 +283,41 @@ class Node(_common.ListMixin, resource.Resource): supported by the server. """ expected_provision_state = self.provision_state - if expected_provision_state is None: - expected_provision_state = 'available' - - if expected_provision_state not in ('enroll', - 'manageable', - 'available'): - raise ValueError( - "Node's provision_state must be one of 'enroll', " - "'manageable' or 'available' for creation, got %s" % - expected_provision_state) session = self._get_session(session) - # Verify that the requested provision state is reachable with the API - # version we are going to use. - try: - expected_version = _common.STATE_VERSIONS[expected_provision_state] - except KeyError: - pass + if expected_provision_state is not None: + # Verify that the requested provision state is reachable with + # the API version we are going to use. + try: + microversion = _common.STATE_VERSIONS[ + expected_provision_state] + except KeyError: + raise ValueError( + "Node's provision_state must be one of %s for creation, " + "got %s" % (', '.join(_common.STATE_VERSIONS), + expected_provision_state)) + else: + error_message = ("Cannot create a node with initial provision " + "state %s" % expected_provision_state) + # Nodes cannot be created as available using new API versions + maximum = ('1.10' if expected_provision_state == 'available' + else None) + microversion = self._assert_microversion_for( + session, 'create', microversion, maximum=maximum, + error_message=error_message, + ) else: - self._assert_microversion_for( - session, 'create', expected_version, - error_message="Cannot create a node with initial provision " - "state %s" % expected_provision_state) + microversion = None # use the base negotiation # Ironic cannot set provision_state itself, so marking it as unchanged self._clean_body_attrs({'provision_state'}) - super(Node, self).create(session, *args, **kwargs) - if (self.provision_state == 'enroll' - and expected_provision_state != 'enroll'): - self.set_provision_state(session, 'manage', wait=True) + super(Node, self).create(session, *args, microversion=microversion, + **kwargs) - if (self.provision_state == 'manageable' - and expected_provision_state == 'available'): - self.set_provision_state(session, 'provide', wait=True) - - if (self.provision_state == 'available' - and expected_provision_state == 'manageable'): + if (expected_provision_state == 'manageable' + and self.provision_state != 'manageable'): + # Manageable is not reachable directly self.set_provision_state(session, 'manage', wait=True) return self diff --git a/openstack/resource.py b/openstack/resource.py index 7adebd1b7..323383a91 100644 --- a/openstack/resource.py +++ b/openstack/resource.py @@ -1342,6 +1342,7 @@ class Resource(dict): action, expected, error_message=None, + maximum=None, ): """Enforce that the microversion for action satisfies the requirement. @@ -1350,6 +1351,7 @@ class Resource(dict): :param expected: Expected microversion. :param error_message: Optional error message with details. Will be prepended to the message generated here. + :param maximum: Maximum microversion. :return: resulting microversion as string. :raises: :exc:`~openstack.exceptions.NotSupported` if the version used for the action is lower than the expected one. @@ -1364,23 +1366,29 @@ class Resource(dict): actual = self._get_microversion(session, action=action) - if expected is None: - return actual - elif actual is None: + if actual is None: message = ( "API version %s is required, but the default " "version will be used." ) % expected _raise(message) - actual_n = discover.normalize_version_number(actual) - expected_n = discover.normalize_version_number(expected) - if actual_n < expected_n: - message = ( - "API version %(expected)s is required, but %(actual)s " - "will be used." - ) % {'expected': expected, 'actual': actual} - _raise(message) + + if expected is not None: + expected_n = discover.normalize_version_number(expected) + if actual_n < expected_n: + message = ( + "API version %(expected)s is required, but %(actual)s " + "will be used." + ) % {'expected': expected, 'actual': actual} + _raise(message) + if maximum is not None: + maximum_n = discover.normalize_version_number(maximum) + # Assume that if a service supports higher versions, it also + # supports lower ones. Breaks for services that remove old API + # versions (which is not something they should do). + if actual_n > maximum_n: + return maximum return actual diff --git a/openstack/tests/functional/baremetal/test_baremetal_node.py b/openstack/tests/functional/baremetal/test_baremetal_node.py index aa44c2265..bdd6cd8b6 100644 --- a/openstack/tests/functional/baremetal/test_baremetal_node.py +++ b/openstack/tests/functional/baremetal/test_baremetal_node.py @@ -22,7 +22,7 @@ class TestBareMetalNode(base.BaseBaremetalTest): node = self.create_node(name='node-name') self.assertEqual(node.name, 'node-name') self.assertEqual(node.driver, 'fake-hardware') - self.assertEqual(node.provision_state, 'available') + self.assertEqual(node.provision_state, 'enroll') self.assertFalse(node.is_maintenance) # NOTE(dtantsur): get_node and find_node only differ in handing missing @@ -50,6 +50,16 @@ class TestBareMetalNode(base.BaseBaremetalTest): self.assertRaises(exceptions.ResourceNotFound, self.conn.baremetal.get_node, self.node_id) + def test_node_create_in_available(self): + node = self.create_node(name='node-name', provision_state='available') + self.assertEqual(node.name, 'node-name') + self.assertEqual(node.driver, 'fake-hardware') + self.assertEqual(node.provision_state, 'available') + + self.conn.baremetal.delete_node(node, ignore_missing=False) + self.assertRaises(exceptions.ResourceNotFound, + self.conn.baremetal.get_node, self.node_id) + def test_node_update(self): node = self.create_node(name='node-name', extra={'foo': 'bar'}) node.name = 'new-name' @@ -128,7 +138,7 @@ class TestBareMetalNode(base.BaseBaremetalTest): self.create_node(name='node-name', extra={'foo': 'bar'}) node = next(n for n in self.conn.baremetal.nodes(details=True, - provision_state='available', + provision_state='enroll', is_maintenance=False, associated=False) if n.name == 'node-name') @@ -139,7 +149,7 @@ class TestBareMetalNode(base.BaseBaremetalTest): self.conn.baremetal.delete_node(node, ignore_missing=False) def test_node_create_in_enroll_provide(self): - node = self.create_node(provision_state='enroll') + node = self.create_node() self.node_id = node.id self.assertEqual(node.driver, 'fake-hardware') @@ -157,7 +167,7 @@ class TestBareMetalNode(base.BaseBaremetalTest): def test_node_create_in_enroll_provide_by_name(self): name = 'node-%d' % random.randint(0, 1000) - node = self.create_node(provision_state='enroll', name=name) + node = self.create_node(name=name) self.node_id = node.id self.assertEqual(node.driver, 'fake-hardware') @@ -297,16 +307,6 @@ class TestNodeRetired(base.BaseBaremetalTest): node = self.create_node() - # Set retired when node state available should fail! - self.assertRaises( - exceptions.ConflictException, - self.conn.baremetal.update_node, node, is_retired=True) - - # Set node state to manageable - self.conn.baremetal.set_node_provision_state(node, 'manage', - wait=True) - self.assertEqual(node.provision_state, 'manageable') - # Set retired without reason node = self.conn.baremetal.update_node(node, is_retired=True) self.assertTrue(node.is_retired) @@ -348,6 +348,14 @@ class TestNodeRetired(base.BaseBaremetalTest): self.assertTrue(node.is_retired) self.assertEqual(reason, node.retired_reason) + def test_retired_in_available(self): + node = self.create_node(provision_state='available') + + # Set retired when node state available should fail! + self.assertRaises( + exceptions.ConflictException, + self.conn.baremetal.update_node, node, is_retired=True) + class TestBareMetalNodeFields(base.BaseBaremetalTest): diff --git a/openstack/tests/unit/baremetal/v1/test_node.py b/openstack/tests/unit/baremetal/v1/test_node.py index f977182d2..3d001497b 100644 --- a/openstack/tests/unit/baremetal/v1/test_node.py +++ b/openstack/tests/unit/baremetal/v1/test_node.py @@ -347,6 +347,7 @@ class TestNodeCreate(base.TestCase): self.session.post.side_effect = _change_state def test_available_old_version(self, mock_prov): + self.node.provision_state = 'available' result = self.node.create(self.session) self.assertIs(result, self.node) self.session.post.assert_called_once_with( @@ -356,24 +357,16 @@ class TestNodeCreate(base.TestCase): self.assertFalse(mock_prov.called) def test_available_new_version(self, mock_prov): - def _change_state(*args, **kwargs): - self.node.provision_state = 'manageable' - self.session.default_microversion = '1.11' self.node.provision_state = 'available' - self.new_state = 'enroll' - mock_prov.side_effect = _change_state result = self.node.create(self.session) self.assertIs(result, self.node) self.session.post.assert_called_once_with( mock.ANY, json={'driver': FAKE['driver']}, - headers=mock.ANY, microversion=self.session.default_microversion, + headers=mock.ANY, microversion='1.10', params={}) - mock_prov.assert_has_calls([ - mock.call(self.node, self.session, 'manage', wait=True), - mock.call(self.node, self.session, 'provide', wait=True) - ]) + mock_prov.assert_not_called() def test_no_enroll_in_old_version(self, mock_prov): self.node.provision_state = 'enroll' diff --git a/releasenotes/notes/node-create-027ea99193f344ef.yaml b/releasenotes/notes/node-create-027ea99193f344ef.yaml new file mode 100644 index 000000000..3f74ff270 --- /dev/null +++ b/releasenotes/notes/node-create-027ea99193f344ef.yaml @@ -0,0 +1,9 @@ +--- +upgrade: + - | + Changes the baremetal ``create_node`` call to be closer to how Ironic + behaves. If no provision state is requested, the default state of the + current microversion is used (which usually means ``enroll``). + If the ``available`` state is requested, the node does not go through + cleaning (it won't work without creating ports), an old API version is + used to achieve this provision state.