From e795f6c841096356fbd1c5c8a3769bfc4eb1d4d4 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 22 Jan 2019 14:33:15 +0100 Subject: [PATCH] Make sure to not try reserving a reserved node After the switch to openstacksdk we no longer have a sufficient check on node's availability or maintenance. This patch restores it. Change-Id: I2c85cf0adb02061b3dd85f19dd10c8a5af1118da --- metalsmith/_provisioner.py | 22 ++++++++----- metalsmith/_scheduler.py | 33 +++++++++++++++---- metalsmith/exceptions.py | 4 +-- metalsmith/test/test_provisioner.py | 19 ++++++++--- .../notes/associated-993c26ac5dc0cfc0.yaml | 5 +++ 5 files changed, 61 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/associated-993c26ac5dc0cfc0.yaml diff --git a/metalsmith/_provisioner.py b/metalsmith/_provisioner.py index d7ae5bb..9f2c3be 100644 --- a/metalsmith/_provisioner.py +++ b/metalsmith/_provisioner.py @@ -98,25 +98,29 @@ class Provisioner(_utils.GetNodeMixin): if candidates: nodes = [self._get_node(node) for node in candidates] - filters = [ - _scheduler.NodeTypeFilter(resource_class, conductor_group), - ] else: + kwargs = {} + if conductor_group: + kwargs['conductor_group'] = conductor_group nodes = list(self.connection.baremetal.nodes( + associated=False, + provision_state='available', + maintenance=False, resource_class=resource_class, - conductor_group=conductor_group, - details=True)) + details=True, + **kwargs)) if not nodes: raise exceptions.NodesNotFound(resource_class, conductor_group) # Ensure parallel executions don't try nodes in the same sequence random.shuffle(nodes) - # No need to filter by resource_class and conductor_group any more - filters = [] LOG.debug('Candidate nodes: %s', nodes) - filters.append(_scheduler.CapabilitiesFilter(capabilities)) - filters.append(_scheduler.TraitsFilter(traits)) + filters = [ + _scheduler.NodeTypeFilter(resource_class, conductor_group), + _scheduler.CapabilitiesFilter(capabilities), + _scheduler.TraitsFilter(traits), + ] if predicate is not None: filters.append(_scheduler.CustomPredicateFilter(predicate)) diff --git a/metalsmith/_scheduler.py b/metalsmith/_scheduler.py index 91aa45d..fd943b7 100644 --- a/metalsmith/_scheduler.py +++ b/metalsmith/_scheduler.py @@ -123,12 +123,33 @@ class NodeTypeFilter(Filter): self.conductor_group = conductor_group def __call__(self, node): - return ( - (self.resource_class is None or - node.resource_class == self.resource_class) and - (self.conductor_group is None or - node.conductor_group == self.conductor_group) - ) + if node.instance_id: + LOG.debug('Node %s is already reserved', _utils.log_res(node)) + return False + + if node.is_maintenance: + LOG.debug('Node %s is in maintenance', _utils.log_res(node)) + return False + + if (self.resource_class is not None + and node.resource_class != self.resource_class): + LOG.debug('Resource class %(real)s does not match the expected ' + 'value of %(exp)s for node %(node)s', + {'node': _utils.log_res(node), + 'exp': self.resource_class, + 'real': node.resource_class}) + return False + + if (self.conductor_group is not None + and node.conductor_group != self.conductor_group): + LOG.debug('Conductor group %(real)s does not match the expected ' + 'value of %(exp)s for node %(node)s', + {'node': _utils.log_res(node), + 'exp': self.conductor_group, + 'real': node.conductor_group}) + return False + + return True def fail(self): raise exceptions.NodesNotFound(self.resource_class, diff --git a/metalsmith/exceptions.py b/metalsmith/exceptions.py index 1325d2c..fb92547 100644 --- a/metalsmith/exceptions.py +++ b/metalsmith/exceptions.py @@ -35,9 +35,9 @@ class NodesNotFound(ReservationFailed): def __init__(self, resource_class, conductor_group): message = "No available nodes%(rc)s found%(cg)s" % { - 'rc': 'with resource class %s' % resource_class + 'rc': ' with resource class %s' % resource_class if resource_class else '', - 'cg': 'in conductor group %s' % (conductor_group or '') + 'cg': ' in conductor group %s' % (conductor_group or '') if conductor_group is not None else '' } self.requested_resource_class = resource_class diff --git a/metalsmith/test/test_provisioner.py b/metalsmith/test/test_provisioner.py index 9b97632..46bde89 100644 --- a/metalsmith/test/test_provisioner.py +++ b/metalsmith/test/test_provisioner.py @@ -29,7 +29,7 @@ from metalsmith import sources NODE_FIELDS = ['name', 'id', 'instance_info', 'instance_id', 'is_maintenance', 'maintenance_reason', 'properties', 'provision_state', 'extra', - 'last_error', 'traits'] + 'last_error', 'traits', 'resource_class', 'conductor_group'] class TestInit(testtools.TestCase): @@ -98,6 +98,8 @@ class TestReserveNode(Base): kwargs.setdefault('id', '000') kwargs.setdefault('properties', {'local_gb': 100}) kwargs.setdefault('instance_info', {}) + kwargs.setdefault('instance_id', None) + kwargs.setdefault('is_maintenance', False) return mock.Mock(spec=NODE_FIELDS, **kwargs) def test_no_nodes(self): @@ -108,7 +110,7 @@ class TestReserveNode(Base): self.assertFalse(self.api.baremetal.update_node.called) def test_simple_ok(self): - nodes = [self._node()] + nodes = [self._node(resource_class='control')] self.api.baremetal.nodes.return_value = nodes node = self.pr.reserve_node('control') @@ -129,7 +131,8 @@ class TestReserveNode(Base): def test_with_capabilities(self): nodes = [ - self._node(properties={'local_gb': 100, 'capabilities': caps}) + self._node(properties={'local_gb': 100, 'capabilities': caps}, + resource_class='control') for caps in ['answer:1', 'answer:42', None] ] expected = nodes[1] @@ -235,8 +238,14 @@ class TestReserveNode(Base): instance_info={'capabilities': {'cat': 'meow'}}) def test_provided_nodes_no_match(self): - nodes = [self._node(resource_class='compute', conductor_group='loc1'), - self._node(resource_class='control', conductor_group='loc2')] + nodes = [ + self._node(resource_class='compute', conductor_group='loc1'), + self._node(resource_class='control', conductor_group='loc2'), + self._node(resource_class='control', conductor_group='loc1', + is_maintenance=True), + self._node(resource_class='control', conductor_group='loc1', + instance_id='abcd') + ] self.assertRaises(exceptions.NodesNotFound, self.pr.reserve_node, candidates=nodes, diff --git a/releasenotes/notes/associated-993c26ac5dc0cfc0.yaml b/releasenotes/notes/associated-993c26ac5dc0cfc0.yaml new file mode 100644 index 0000000..7933f48 --- /dev/null +++ b/releasenotes/notes/associated-993c26ac5dc0cfc0.yaml @@ -0,0 +1,5 @@ +--- +critical: + - | + Fixes a regression that caused deployed nodes to be picked for deployment + again.