Clean up the edge cases around states

Currently show_instance returns an Instance even if the requested node
is not actually an instance (e.g. just an available node). This change
corrects it. Make list_instances consistent with it.

Also make the states a proper enum to avoid consumers from using invalid
values (I did it several times when working on this patch).

Change-Id: If9aad0d7f4d10a7119d1f0bccc1cc32a918a72e3
This commit is contained in:
Dmitry Tantsur 2019-01-18 17:35:51 +01:00
parent a57d584935
commit 02932096df
11 changed files with 184 additions and 68 deletions

View File

@ -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

View File

@ -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']

View File

@ -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()

View File

@ -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,
}

View File

@ -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

View File

@ -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."""

View File

@ -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)

View File

@ -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

View File

@ -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)

View File

@ -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``).

View File

@ -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