diff --git a/lower-constraints.txt b/lower-constraints.txt index cf3274f..8bf064c 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -1,5 +1,6 @@ coverage==4.0 doc8==0.6.0 +enum34==1.0.4 fixtures==3.0.0 flake8-import-order==0.13 hacking==1.0.0 diff --git a/metalsmith/__init__.py b/metalsmith/__init__.py index 7b297fa..3e9da1c 100644 --- a/metalsmith/__init__.py +++ b/metalsmith/__init__.py @@ -15,6 +15,7 @@ from metalsmith._config import InstanceConfig from metalsmith._instance import Instance +from metalsmith._instance import InstanceState from metalsmith._provisioner import Provisioner -__all__ = ['Instance', 'InstanceConfig', 'Provisioner'] +__all__ = ['Instance', 'InstanceConfig', 'InstanceState', 'Provisioner'] diff --git a/metalsmith/_format.py b/metalsmith/_format.py index d107941..9371099 100644 --- a/metalsmith/_format.py +++ b/metalsmith/_format.py @@ -57,7 +57,8 @@ class DefaultFormat(object): def show(self, instances): for instance in instances: _print("Node %(node)s, current state is %(state)s", - node=_utils.log_res(instance.node), state=instance.state) + node=_utils.log_res(instance.node), + state=instance.state.name) if instance.is_deployed: ips = instance.ip_addresses() diff --git a/metalsmith/_instance.py b/metalsmith/_instance.py index 5a3cd52..eef0092 100644 --- a/metalsmith/_instance.py +++ b/metalsmith/_instance.py @@ -13,18 +13,74 @@ # See the License for the specific language governing permissions and # limitations under the License. +import enum +import warnings + from metalsmith import _utils _PROGRESS_STATES = frozenset(['deploying', 'wait call-back', 'deploy complete']) -# NOTE(dtantsur): include available since there is a period of time between -# claiming the instance and starting the actual provisioning via ironic. -_DEPLOYING_STATES = _PROGRESS_STATES | {'available'} _ACTIVE_STATES = frozenset(['active']) _ERROR_STATES = frozenset(['error', 'deploy failed']) +_RESERVED_STATES = frozenset(['available']) -_HEALTHY_STATES = _PROGRESS_STATES | _ACTIVE_STATES + +class InstanceState(enum.Enum): + """A state of an instance.""" + + DEPLOYING = 'deploying' + """Provisioning is in progress. + + This includes the case when a node is still in the ``available`` state, but + already has an instance associated with it. + """ + + ACTIVE = 'active' + """The instance is provisioned.""" + + MAINTENANCE = 'maintenance' + """The instance is provisioned but is in the maintenance mode.""" + + ERROR = 'error' + """The instance has a failure.""" + + UNKNOWN = 'unknown' + """The node is in an unexpected state. + + It can be unprovisioned or modified by a third party. + """ + + @property + def is_deployed(self): + """Whether the state designates a finished deployment.""" + return self in _DEPLOYED_STATES + + @property + def is_healthy(self): + """Whether the state is considered healthy.""" + return self in _HEALTHY_STATES + + # TODO(dtantsur): remove before 1.0 + + def __eq__(self, other): + if isinstance(other, str): + warnings.warn("Comparing states with strings is deprecated, " + "use InstanceState instead", DeprecationWarning) + return self.value == other + else: + return super(InstanceState, self).__eq__(other) + + def __ne__(self, other): + eq = self.__eq__(other) + return NotImplemented if eq is NotImplemented else not eq + + def __hash__(self): + return hash(self.value) + + +_HEALTHY_STATES = frozenset([InstanceState.ACTIVE, InstanceState.DEPLOYING]) +_DEPLOYED_STATES = frozenset([InstanceState.ACTIVE, InstanceState.MAINTENANCE]) class Instance(object): @@ -57,16 +113,12 @@ class Instance(object): @property def is_deployed(self): """Whether the node is deployed.""" - return self._node.provision_state in _ACTIVE_STATES - - @property - def _is_deployed_by_metalsmith(self): - return _utils.GetNodeMixin.HOSTNAME_FIELD in self._node.instance_info + return self.state.is_deployed @property def is_healthy(self): - """Whether the node is not at fault or maintenance.""" - return self.state in _HEALTHY_STATES and not self._node.is_maintenance + """Whether the instance is not at fault or maintenance.""" + return self.state.is_healthy and not self._node.is_maintenance def nics(self): """List NICs for this instance. @@ -90,32 +142,23 @@ class Instance(object): @property def state(self): - """Instance state. - - ``deploying`` - deployment is in progress - ``active`` - node is provisioned - ``maintenance`` - node is provisioned but is in maintenance mode - ``error`` - node has a failure - ``unknown`` - node in unexpected state (maybe unprovisioned or modified by - a third party) - """ + """Instance state, one of :py:class:`InstanceState`.""" prov_state = self._node.provision_state - if prov_state in _DEPLOYING_STATES: - return 'deploying' + if prov_state in _PROGRESS_STATES: + return InstanceState.DEPLOYING + # NOTE(dtantsur): include available since there is a period of time + # between claiming the instance and starting the actual provisioning. + elif prov_state in _RESERVED_STATES and self._node.instance_id: + return InstanceState.DEPLOYING elif prov_state in _ERROR_STATES: - return 'error' + return InstanceState.ERROR elif prov_state in _ACTIVE_STATES: if self._node.is_maintenance: - return 'maintenance' + return InstanceState.MAINTENANCE else: - return 'active' + return InstanceState.ACTIVE else: - return 'unknown' + return InstanceState.UNKNOWN def to_dict(self): """Convert instance to a dict.""" @@ -123,7 +166,7 @@ class Instance(object): 'hostname': self.hostname, 'ip_addresses': self.ip_addresses(), 'node': self._node.to_dict(), - 'state': self.state, + 'state': self.state.value, 'uuid': self._uuid, } diff --git a/metalsmith/_provisioner.py b/metalsmith/_provisioner.py index 1d5a1ac..d7ae5bb 100644 --- a/metalsmith/_provisioner.py +++ b/metalsmith/_provisioner.py @@ -419,6 +419,8 @@ class Provisioner(_utils.GetNodeMixin): :param instance_id: hostname, UUID or node name. :return: :py:class:`metalsmith.Instance` object. + :raises: :py:class:`metalsmith.exceptions.InvalidInstance` + if the instance is not a valid instance. """ return self.show_instances([instance_id])[0] @@ -431,14 +433,25 @@ class Provisioner(_utils.GetNodeMixin): :param instances: list of hostnames, UUIDs or node names. :return: list of :py:class:`metalsmith.Instance` objects in the same order as ``instances``. + :raises: :py:class:`metalsmith.exceptions.InvalidInstance` + if one of the instances are not valid instances. """ with self._cache_node_list_for_lookup(): - return [ + result = [ _instance.Instance( self.connection, self._get_node(inst, accept_hostname=True)) for inst in instances ] + # NOTE(dtantsur): do not accept node names as valid instances if they + # are not deployed or being deployed. + missing = [inst for (res, inst) in zip(result, instances) + if res.state == _instance.InstanceState.UNKNOWN] + if missing: + raise exceptions.InvalidInstance( + "Node(s)/instance(s) %s are not valid instances" + % ', '.join(map(str, missing))) + return result def list_instances(self): """List instances deployed by metalsmith. @@ -449,5 +462,5 @@ class Provisioner(_utils.GetNodeMixin): instances = [i for i in (_instance.Instance(self.connection, node) for node in nodes) - if i._is_deployed_by_metalsmith] + if i.state != _instance.InstanceState.UNKNOWN] return instances diff --git a/metalsmith/exceptions.py b/metalsmith/exceptions.py index 94a4bd9..1325d2c 100644 --- a/metalsmith/exceptions.py +++ b/metalsmith/exceptions.py @@ -120,3 +120,7 @@ class DeploymentFailure(Error): def __init__(self, message, nodes): self.nodes = nodes super(DeploymentFailure, self).__init__(message) + + +class InvalidInstance(Error): + """The node(s) does not have a metalsmith instance associated.""" diff --git a/metalsmith/test/test_cmd.py b/metalsmith/test/test_cmd.py index 69d18bc..1b27333 100644 --- a/metalsmith/test/test_cmd.py +++ b/metalsmith/test/test_cmd.py @@ -76,7 +76,7 @@ class TestDeploy(testtools.TestCase): instance.create_autospec(_instance.Instance) instance.node.name = None instance.node.id = '123' - instance.state = 'active' + instance.state = _instance.InstanceState.ACTIVE instance.is_deployed = True instance.ip_addresses.return_value = {'private': ['1.2.3.4']} @@ -99,7 +99,7 @@ class TestDeploy(testtools.TestCase): mock_log.getLogger.mock_calls) self.mock_print.assert_has_calls([ - mock.call(mock.ANY, node='123', state='active'), + mock.call(mock.ANY, node='123', state='ACTIVE'), mock.call(mock.ANY, ips='private=1.2.3.4') ]) @@ -132,14 +132,14 @@ class TestDeploy(testtools.TestCase): instance.ip_addresses.return_value = {} instance.node.name = None instance.node.id = '123' - instance.state = 'active' + instance.state = _instance.InstanceState.ACTIVE args = ['deploy', '--network', 'mynet', '--image', 'myimg', '--resource-class', 'compute'] self._check(mock_pr, args, {}, {}) self.mock_print.assert_called_once_with(mock.ANY, node='123', - state='active'), + state='ACTIVE'), def test_not_deployed_no_ips(self, mock_pr): instance = mock_pr.return_value.provision_node.return_value @@ -147,14 +147,14 @@ class TestDeploy(testtools.TestCase): instance.is_deployed = False instance.node.name = None instance.node.id = '123' - instance.state = 'deploying' + instance.state = _instance.InstanceState.DEPLOYING args = ['deploy', '--network', 'mynet', '--image', 'myimg', '--resource-class', 'compute'] self._check(mock_pr, args, {}, {}) self.mock_print.assert_called_once_with(mock.ANY, node='123', - state='deploying'), + state='DEPLOYING'), @mock.patch.object(_cmd.LOG, 'info', autospec=True) def test_no_logs_not_deployed(self, mock_log, mock_pr): @@ -581,11 +581,12 @@ class TestShowWait(testtools.TestCase): 'metalsmith._format._print', autospec=True)) self.mock_print = self.print_fixture.mock self.instances = [ - mock.Mock(spec=_instance.Instance, hostname=hostname, - uuid=hostname[-1], is_deployed=(hostname[-1] == '1'), - state=('active' if hostname[-1] == '1' else 'deploying'), - **{'ip_addresses.return_value': {'private': - ['1.2.3.4']}}) + mock.Mock( + spec=_instance.Instance, hostname=hostname, + uuid=hostname[-1], is_deployed=(hostname[-1] == '1'), + state=_instance.InstanceState.ACTIVE + if hostname[-1] == '1' else _instance.InstanceState.DEPLOYING, + **{'ip_addresses.return_value': {'private': ['1.2.3.4']}}) for hostname in ['hostname1', 'hostname2'] ] for inst in self.instances: @@ -599,9 +600,9 @@ class TestShowWait(testtools.TestCase): _cmd.main(args) self.mock_print.assert_has_calls([ - mock.call(mock.ANY, node='name-1 (UUID 1)', state='active'), + mock.call(mock.ANY, node='name-1 (UUID 1)', state='ACTIVE'), mock.call(mock.ANY, ips='private=1.2.3.4'), - mock.call(mock.ANY, node='name-2 (UUID 2)', state='deploying'), + mock.call(mock.ANY, node='name-2 (UUID 2)', state='DEPLOYING'), ]) mock_pr.return_value.show_instances.assert_called_once_with( ['uuid1', 'hostname2']) @@ -612,9 +613,9 @@ class TestShowWait(testtools.TestCase): _cmd.main(args) self.mock_print.assert_has_calls([ - mock.call(mock.ANY, node='name-1 (UUID 1)', state='active'), + mock.call(mock.ANY, node='name-1 (UUID 1)', state='ACTIVE'), mock.call(mock.ANY, ips='private=1.2.3.4'), - mock.call(mock.ANY, node='name-2 (UUID 2)', state='deploying'), + mock.call(mock.ANY, node='name-2 (UUID 2)', state='DEPLOYING'), ]) mock_pr.return_value.list_instances.assert_called_once_with() @@ -625,9 +626,9 @@ class TestShowWait(testtools.TestCase): _cmd.main(args) self.mock_print.assert_has_calls([ - mock.call(mock.ANY, node='name-1 (UUID 1)', state='active'), + mock.call(mock.ANY, node='name-1 (UUID 1)', state='ACTIVE'), mock.call(mock.ANY, ips='private=1.2.3.4'), - mock.call(mock.ANY, node='name-2 (UUID 2)', state='deploying'), + mock.call(mock.ANY, node='name-2 (UUID 2)', state='DEPLOYING'), ]) mock_pr.return_value.wait_for_provisioning.assert_called_once_with( ['uuid1', 'hostname2'], timeout=None) @@ -639,9 +640,9 @@ class TestShowWait(testtools.TestCase): _cmd.main(args) self.mock_print.assert_has_calls([ - mock.call(mock.ANY, node='name-1 (UUID 1)', state='active'), + mock.call(mock.ANY, node='name-1 (UUID 1)', state='ACTIVE'), mock.call(mock.ANY, ips='private=1.2.3.4'), - mock.call(mock.ANY, node='name-2 (UUID 2)', state='deploying'), + mock.call(mock.ANY, node='name-2 (UUID 2)', state='DEPLOYING'), ]) mock_pr.return_value.wait_for_provisioning.assert_called_once_with( ['uuid1', 'hostname2'], timeout=42) diff --git a/metalsmith/test/test_instance.py b/metalsmith/test/test_instance.py index a623aaa..8b91d4a 100644 --- a/metalsmith/test/test_instance.py +++ b/metalsmith/test/test_instance.py @@ -14,6 +14,7 @@ # limitations under the License. import mock +import six from metalsmith import _instance from metalsmith.test import test_provisioner @@ -57,47 +58,68 @@ class TestInstanceStates(test_provisioner.Base): def test_state_deploying(self): self.node.provision_state = 'wait call-back' - self.assertEqual('deploying', self.instance.state) + self.assertEqual(_instance.InstanceState.DEPLOYING, + self.instance.state) self.assertFalse(self.instance.is_deployed) self.assertTrue(self.instance.is_healthy) + self.assertTrue(self.instance.state.is_healthy) + self.assertFalse(self.instance.state.is_deployed) def test_state_deploying_when_available(self): self.node.provision_state = 'available' - self.assertEqual('deploying', self.instance.state) + self.node.instance_id = 'abcd' + self.assertEqual(_instance.InstanceState.DEPLOYING, + self.instance.state) self.assertFalse(self.instance.is_deployed) self.assertTrue(self.instance.is_healthy) + def test_state_unknown_when_available(self): + self.node.provision_state = 'available' + self.node.instance_id = None + self.assertEqual(_instance.InstanceState.UNKNOWN, self.instance.state) + self.assertFalse(self.instance.is_deployed) + self.assertFalse(self.instance.is_healthy) + self.assertFalse(self.instance.state.is_healthy) + def test_state_deploying_maintenance(self): self.node.is_maintenance = True self.node.provision_state = 'wait call-back' - self.assertEqual('deploying', self.instance.state) + self.assertEqual(_instance.InstanceState.DEPLOYING, + self.instance.state) self.assertFalse(self.instance.is_deployed) self.assertFalse(self.instance.is_healthy) + # The state itself is considered healthy + self.assertTrue(self.instance.state.is_healthy) def test_state_active(self): self.node.provision_state = 'active' - self.assertEqual('active', self.instance.state) + self.assertEqual(_instance.InstanceState.ACTIVE, self.instance.state) self.assertTrue(self.instance.is_deployed) self.assertTrue(self.instance.is_healthy) + self.assertTrue(self.instance.state.is_deployed) def test_state_maintenance(self): self.node.is_maintenance = True self.node.provision_state = 'active' - self.assertEqual('maintenance', self.instance.state) + self.assertEqual(_instance.InstanceState.MAINTENANCE, + self.instance.state) self.assertTrue(self.instance.is_deployed) self.assertFalse(self.instance.is_healthy) + self.assertFalse(self.instance.state.is_healthy) def test_state_error(self): self.node.provision_state = 'deploy failed' - self.assertEqual('error', self.instance.state) + self.assertEqual(_instance.InstanceState.ERROR, self.instance.state) self.assertFalse(self.instance.is_deployed) self.assertFalse(self.instance.is_healthy) + self.assertFalse(self.instance.state.is_healthy) def test_state_unknown(self): self.node.provision_state = 'enroll' - self.assertEqual('unknown', self.instance.state) + self.assertEqual(_instance.InstanceState.UNKNOWN, self.instance.state) self.assertFalse(self.instance.is_deployed) self.assertFalse(self.instance.is_healthy) + self.assertFalse(self.instance.state.is_healthy) @mock.patch.object(_instance.Instance, 'ip_addresses', autospec=True) def test_to_dict(self, mock_ips): @@ -106,9 +128,17 @@ class TestInstanceStates(test_provisioner.Base): self.node.instance_info = {'metalsmith_hostname': 'host'} mock_ips.return_value = {'private': ['1.2.3.4']} + to_dict = self.instance.to_dict() self.assertEqual({'hostname': 'host', 'ip_addresses': {'private': ['1.2.3.4']}, 'node': {'node': 'dict'}, 'state': 'deploying', 'uuid': self.node.id}, - self.instance.to_dict()) + to_dict) + # States are converted to strings + self.assertIsInstance(to_dict['state'], six.string_types) + + def test_deprecated_comparisons(self): + for item in _instance.InstanceState: + self.assertEqual(item, item.value) + self.assertFalse(item != item.value) # noqa diff --git a/metalsmith/test/test_provisioner.py b/metalsmith/test/test_provisioner.py index c948ec5..bc23e90 100644 --- a/metalsmith/test/test_provisioner.py +++ b/metalsmith/test/test_provisioner.py @@ -1305,6 +1305,10 @@ class TestUnprovisionNode(Base): class TestShowInstance(Base): + def setUp(self): + super(TestShowInstance, self).setUp() + self.node.provision_state = 'active' + def test_show_instance(self): self.mock_get_node.side_effect = lambda n, *a, **kw: self.node inst = self.pr.show_instance('id1') @@ -1315,7 +1319,7 @@ class TestShowInstance(Base): self.assertIs(inst.uuid, self.node.id) def test_show_instances(self): - self.mock_get_node.side_effect = [self.node, mock.Mock()] + self.mock_get_node.side_effect = [self.node, self.node] result = self.pr.show_instances(['1', '2']) self.mock_get_node.assert_has_calls([ mock.call(self.pr, '1', accept_hostname=True), @@ -1343,16 +1347,16 @@ class TestListInstances(Base): super(TestListInstances, self).setUp() self.nodes = [ mock.Mock(spec=NODE_FIELDS, provision_state=state, - instance_info={'metalsmith_hostname': '1234'}) + instance_id='1234') for state in ('active', 'active', 'deploying', 'wait call-back', - 'deploy failed', 'available') + 'deploy failed', 'available', 'available', 'enroll') ] - del self.nodes[-1].instance_info['metalsmith_hostname'] + self.nodes[6].instance_id = None self.api.baremetal.nodes.return_value = self.nodes def test_list(self): instances = self.pr.list_instances() self.assertTrue(isinstance(i, _instance.Instance) for i in instances) - self.assertEqual(self.nodes[:5], [i.node for i in instances]) + self.assertEqual(self.nodes[:6], [i.node for i in instances]) self.api.baremetal.nodes.assert_called_once_with(associated=True, details=True) diff --git a/releasenotes/notes/states-79b593683c0783d5.yaml b/releasenotes/notes/states-79b593683c0783d5.yaml new file mode 100644 index 0000000..2d04150 --- /dev/null +++ b/releasenotes/notes/states-79b593683c0783d5.yaml @@ -0,0 +1,17 @@ +--- +features: + - | + The ``Instance.state`` value is now a proper enumeration of type + ``metalsmith.InstanceState``. + - | + The ``list_instances`` call now returns any valid instances, not only ones + created by metalsmith. This is consistent with the ``show_instance(s)`` + behavior. +deprecations: + - | + Comparing an instance state with strings is deprecated, use enumeration + values instead. +fixes: + - | + Fixes the ``show_instance(s)`` calls to return an exception for nodes that + are not valid instances (state == ``UNKNOWN``). diff --git a/requirements.txt b/requirements.txt index eced6b3..41abe2d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,3 +5,4 @@ pbr!=2.1.0,>=2.0.0 # Apache-2.0 openstacksdk>=0.22.0 # Apache-2.0 requests>=2.18.4 # Apache-2.0 six>=1.10.0 # MIT +enum34>=1.0.4;python_version=='2.7' or python_version=='2.6' or python_version=='3.3' # BSD