Browse Source

Rework scheduling filters

Remove ValidationFilter, make it a part of reservation to avoid calling
it too many times. Fix AttributeError on failing the custom predicate.
Remove double validation in the reserver, we do another validation later
on anyway.

BREAKING: changes the exception classes.

Change-Id: Ibc3815989917ab777298a05810fd8f3e64debc8f
Story: #2003584
Task: #26178
Dmitry Tantsur 7 months ago
parent
commit
51a006e307

+ 10
- 15
metalsmith/_provisioner.py View File

@@ -95,29 +95,24 @@ class Provisioner(object):
95 95
 
96 96
         if candidates:
97 97
             nodes = [self._api.get_node(node) for node in candidates]
98
-            if resource_class:
99
-                nodes = [node for node in nodes
100
-                         if node.resource_class == resource_class]
101
-            if conductor_group is not None:
102
-                nodes = [node for node in nodes
103
-                         if node.conductor_group == conductor_group]
98
+            filters = [
99
+                _scheduler.NodeTypeFilter(resource_class, conductor_group),
100
+            ]
104 101
         else:
105 102
             nodes = self._api.list_nodes(resource_class=resource_class,
106 103
                                          conductor_group=conductor_group)
104
+            if not nodes:
105
+                raise exceptions.NodesNotFound(resource_class, conductor_group)
107 106
             # Ensure parallel executions don't try nodes in the same sequence
108 107
             random.shuffle(nodes)
108
+            # No need to filter by resource_class and conductor_group any more
109
+            filters = []
109 110
 
110
-        if not nodes:
111
-            raise exceptions.NodesNotFound(resource_class, conductor_group)
112
-
113
-        LOG.debug('Ironic nodes: %s', nodes)
111
+        LOG.debug('Candidate nodes: %s', nodes)
114 112
 
115
-        filters = [_scheduler.CapabilitiesFilter(capabilities),
116
-                   _scheduler.ValidationFilter(self._api)]
113
+        filters.append(_scheduler.CapabilitiesFilter(capabilities))
117 114
         if predicate is not None:
118
-            # NOTE(dtantsur): run the provided predicate before the validation,
119
-            # since validation requires network interactions.
120
-            filters.insert(-1, predicate)
115
+            filters.append(_scheduler.CustomPredicateFilter(predicate))
121 116
 
122 117
         reserver = _scheduler.IronicReserver(self._api)
123 118
         node = _scheduler.schedule_node(nodes, filters, reserver,

+ 38
- 31
metalsmith/_scheduler.py View File

@@ -114,6 +114,26 @@ def schedule_node(nodes, filters, reserver, dry_run=False):
114 114
     assert False, "BUG: %s.fail did not raise" % reserver.__class__.__name__
115 115
 
116 116
 
117
+class NodeTypeFilter(Filter):
118
+    """Filter that checks resource class and conductor group."""
119
+
120
+    def __init__(self, resource_class=None, conductor_group=None):
121
+        self.resource_class = resource_class
122
+        self.conductor_group = conductor_group
123
+
124
+    def __call__(self, node):
125
+        return (
126
+            (self.resource_class is None or
127
+             node.resource_class == self.resource_class) and
128
+            (self.conductor_group is None or
129
+             node.conductor_group == self.conductor_group)
130
+        )
131
+
132
+    def fail(self):
133
+        raise exceptions.NodesNotFound(self.resource_class,
134
+                                       self.conductor_group)
135
+
136
+
117 137
 class CapabilitiesFilter(Filter):
118 138
     """Filter that checks capabilities."""
119 139
 
@@ -161,31 +181,22 @@ class CapabilitiesFilter(Filter):
161 181
         raise exceptions.CapabilitiesNotFound(message, self._capabilities)
162 182
 
163 183
 
164
-class ValidationFilter(Filter):
165
-    """Filter that runs validation on nodes."""
184
+class CustomPredicateFilter(Filter):
166 185
 
167
-    def __init__(self, api):
168
-        self._api = api
169
-        self._messages = []
186
+    def __init__(self, predicate):
187
+        self.predicate = predicate
170 188
         self._failed_nodes = []
171 189
 
172 190
     def __call__(self, node):
173
-        try:
174
-            self._api.validate_node(node.uuid)
175
-        except RuntimeError as exc:
176
-            message = ('Node %(node)s failed validation: %(err)s' %
177
-                       {'node': _utils.log_node(node), 'err': exc})
178
-            LOG.warning(message)
179
-            self._messages.append(message)
191
+        if not self.predicate(node):
180 192
             self._failed_nodes.append(node)
181 193
             return False
182 194
 
183 195
         return True
184 196
 
185 197
     def fail(self):
186
-        errors = ", ".join(self._messages)
187
-        message = "All available nodes have failed validation: %s" % errors
188
-        raise exceptions.ValidationFailed(message, self._failed_nodes)
198
+        message = 'No nodes satisfied the custom predicate %s' % self.predicate
199
+        raise exceptions.CustomPredicateFailed(message, self._failed_nodes)
189 200
 
190 201
 
191 202
 class IronicReserver(Reserver):
@@ -194,26 +205,22 @@ class IronicReserver(Reserver):
194 205
         self._api = api
195 206
         self._failed_nodes = []
196 207
 
197
-    def __call__(self, node):
208
+    def validate(self, node):
198 209
         try:
199
-            result = self._api.reserve_node(node, instance_uuid=node.uuid)
200
-
201
-            # Try validation again to be sure nothing has changed
202
-            validator = ValidationFilter(self._api)
203
-            if not validator(result):
204
-                LOG.warning('Validation of node %s failed after reservation',
205
-                            _utils.log_node(node))
206
-                try:
207
-                    self._api.release_node(node)
208
-                except Exception:
209
-                    LOG.exception('Failed to release the reserved node %s',
210
-                                  _utils.log_node(node))
211
-                validator.fail()
210
+            self._api.validate_node(node)
211
+        except RuntimeError as exc:
212
+            message = ('Node %(node)s failed validation: %(err)s' %
213
+                       {'node': _utils.log_node(node), 'err': exc})
214
+            LOG.warning(message)
215
+            raise exceptions.ValidationFailed(message)
212 216
 
213
-            return result
217
+    def __call__(self, node):
218
+        try:
219
+            self.validate(node)
220
+            return self._api.reserve_node(node, instance_uuid=node.uuid)
214 221
         except Exception:
215 222
             self._failed_nodes.append(node)
216 223
             raise
217 224
 
218 225
     def fail(self):
219
-        raise exceptions.AllNodesReserved(self._failed_nodes)
226
+        raise exceptions.NoNodesReserved(self._failed_nodes)

+ 17
- 12
metalsmith/exceptions.py View File

@@ -45,6 +45,17 @@ class NodesNotFound(ReservationFailed):
45 45
         super(NodesNotFound, self).__init__(message)
46 46
 
47 47
 
48
+class CustomPredicateFailed(ReservationFailed):
49
+    """Custom predicate yielded no nodes.
50
+
51
+    :ivar nodes: List of nodes that were checked.
52
+    """
53
+
54
+    def __init__(self, message, nodes):
55
+        self.nodes = nodes
56
+        super(CustomPredicateFailed, self).__init__(message)
57
+
58
+
48 59
 class CapabilitiesNotFound(ReservationFailed):
49 60
     """Requested capabilities do not match any nodes.
50 61
 
@@ -57,26 +68,20 @@ class CapabilitiesNotFound(ReservationFailed):
57 68
 
58 69
 
59 70
 class ValidationFailed(ReservationFailed):
60
-    """Validation failed for all requested nodes.
61
-
62
-    :ivar nodes: List of nodes that were checked.
63
-    """
64
-
65
-    def __init__(self, message, nodes):
66
-        self.nodes = nodes
67
-        super(ValidationFailed, self).__init__(message)
71
+    """Validation failed for all requested nodes."""
68 72
 
69 73
 
70
-class AllNodesReserved(ReservationFailed):
71
-    """All nodes are already reserved.
74
+class NoNodesReserved(ReservationFailed):
75
+    """All nodes are already reserved or failed validation.
72 76
 
73 77
     :ivar nodes: List of nodes that were checked.
74 78
     """
75 79
 
76 80
     def __init__(self, nodes):
77 81
         self.nodes = nodes
78
-        message = 'All the candidate nodes are already reserved'
79
-        super(AllNodesReserved, self).__init__(message)
82
+        message = ('All the candidate nodes are already reserved '
83
+                   'or failed validation')
84
+        super(NoNodesReserved, self).__init__(message)
80 85
 
81 86
 
82 87
 class InvalidImage(Error):

+ 39
- 0
metalsmith/test/test_provisioner.py View File

@@ -156,6 +156,25 @@ class TestReserveNode(Base):
156 156
         self.assertEqual(node, nodes[1])
157 157
         self.assertFalse(self.api.update_node.called)
158 158
 
159
+    def test_custom_predicate_false(self):
160
+        nodes = [
161
+            mock.Mock(spec=['uuid', 'name', 'properties'],
162
+                      properties={'local_gb': 100}),
163
+            mock.Mock(spec=['uuid', 'name', 'properties'],
164
+                      properties={'local_gb': 150}),
165
+            mock.Mock(spec=['uuid', 'name', 'properties'],
166
+                      properties={'local_gb': 200}),
167
+        ]
168
+        self.api.list_nodes.return_value = nodes[:]
169
+
170
+        self.assertRaisesRegex(exceptions.CustomPredicateFailed,
171
+                               'custom predicate',
172
+                               self.pr.reserve_node,
173
+                               predicate=lambda node: False)
174
+
175
+        self.assertFalse(self.api.update_node.called)
176
+        self.assertFalse(self.api.reserve_node.called)
177
+
159 178
     def test_provided_node(self):
160 179
         nodes = [
161 180
             mock.Mock(spec=['uuid', 'name', 'properties'],
@@ -226,6 +245,26 @@ class TestReserveNode(Base):
226 245
         self.api.update_node.assert_called_once_with(
227 246
             node, {'/instance_info/capabilities': {'cat': 'meow'}})
228 247
 
248
+    def test_provided_nodes_no_match(self):
249
+        nodes = [
250
+            mock.Mock(spec=['uuid', 'name', 'properties', 'resource_class',
251
+                            'conductor_group'],
252
+                      properties={'local_gb': 100}, resource_class='compute',
253
+                      conductor_group='loc1'),
254
+            mock.Mock(spec=['uuid', 'name', 'properties', 'resource_class',
255
+                            'conductor_group'],
256
+                      properties={'local_gb': 100}, resource_class='control',
257
+                      conductor_group='loc2'),
258
+        ]
259
+
260
+        self.assertRaises(exceptions.NodesNotFound,
261
+                          self.pr.reserve_node, candidates=nodes,
262
+                          resource_class='control', conductor_group='loc1')
263
+
264
+        self.assertFalse(self.api.list_nodes.called)
265
+        self.assertFalse(self.api.reserve_node.called)
266
+        self.assertFalse(self.api.update_node.called)
267
+
229 268
 
230 269
 CLEAN_UP = {
231 270
     '/extra/metalsmith_created_ports': _os_api.REMOVE,

+ 14
- 52
metalsmith/test/test_scheduler.py View File

@@ -164,77 +164,39 @@ class TestCapabilitiesFilter(testtools.TestCase):
164 164
                                fltr.fail)
165 165
 
166 166
 
167
-class TestValidationFilter(testtools.TestCase):
168
-
169
-    def setUp(self):
170
-        super(TestValidationFilter, self).setUp()
171
-        self.api = mock.Mock(spec=['validate_node'])
172
-        self.fltr = _scheduler.ValidationFilter(self.api)
173
-
174
-    def test_pass(self):
175
-        node = mock.Mock(spec=['uuid', 'name'])
176
-        self.assertTrue(self.fltr(node))
177
-
178
-    def test_fail_validation(self):
179
-        node = mock.Mock(spec=['uuid', 'name'])
180
-        self.api.validate_node.side_effect = RuntimeError('boom')
181
-        self.assertFalse(self.fltr(node))
182
-
183
-        self.assertRaisesRegex(exceptions.ValidationFailed,
184
-                               'All available nodes have failed validation: '
185
-                               'Node .* failed validation: boom',
186
-                               self.fltr.fail)
187
-
188
-
189
-@mock.patch.object(_scheduler, 'ValidationFilter', autospec=True)
190 167
 class TestIronicReserver(testtools.TestCase):
191 168
 
192 169
     def setUp(self):
193 170
         super(TestIronicReserver, self).setUp()
194 171
         self.node = mock.Mock(spec=['uuid', 'name'])
195
-        self.api = mock.Mock(spec=['reserve_node', 'release_node'])
172
+        self.api = mock.Mock(spec=['reserve_node', 'release_node',
173
+                                   'validate_node'])
196 174
         self.api.reserve_node.side_effect = lambda node, instance_uuid: node
197 175
         self.reserver = _scheduler.IronicReserver(self.api)
198 176
 
199
-    def test_fail(self, mock_validation):
200
-        self.assertRaisesRegex(exceptions.AllNodesReserved,
177
+    def test_fail(self):
178
+        self.assertRaisesRegex(exceptions.NoNodesReserved,
201 179
                                'All the candidate nodes are already reserved',
202 180
                                self.reserver.fail)
203 181
 
204
-    def test_ok(self, mock_validation):
182
+    def test_ok(self):
205 183
         self.assertEqual(self.node, self.reserver(self.node))
184
+        self.api.validate_node.assert_called_with(self.node)
206 185
         self.api.reserve_node.assert_called_once_with(
207 186
             self.node, instance_uuid=self.node.uuid)
208
-        mock_validation.return_value.assert_called_once_with(self.node)
209 187
 
210
-    def test_reservation_failed(self, mock_validation):
188
+    def test_reservation_failed(self):
211 189
         self.api.reserve_node.side_effect = RuntimeError('conflict')
212 190
         self.assertRaisesRegex(RuntimeError, 'conflict',
213 191
                                self.reserver, self.node)
192
+        self.api.validate_node.assert_called_with(self.node)
214 193
         self.api.reserve_node.assert_called_once_with(
215 194
             self.node, instance_uuid=self.node.uuid)
216
-        self.assertFalse(mock_validation.return_value.called)
217 195
 
218
-    def test_validation_failed(self, mock_validation):
219
-        mock_validation.return_value.return_value = False
220
-        mock_validation.return_value.fail.side_effect = RuntimeError('fail')
221
-        self.assertRaisesRegex(RuntimeError, 'fail',
222
-                               self.reserver, self.node)
223
-        self.api.reserve_node.assert_called_once_with(
224
-            self.node, instance_uuid=self.node.uuid)
225
-        mock_validation.return_value.assert_called_once_with(self.node)
226
-        self.api.release_node.assert_called_once_with(self.node)
227
-
228
-    @mock.patch.object(_scheduler.LOG, 'exception', autospec=True)
229
-    def test_validation_and_release_failed(self, mock_log_exc,
230
-                                           mock_validation):
231
-        mock_validation.return_value.return_value = False
232
-        mock_validation.return_value.fail.side_effect = RuntimeError('fail')
233
-        self.api.release_node.side_effect = Exception()
234
-        self.assertRaisesRegex(RuntimeError, 'fail',
196
+    def test_validation_failed(self):
197
+        self.api.validate_node.side_effect = RuntimeError('fail')
198
+        self.assertRaisesRegex(exceptions.ValidationFailed, 'fail',
235 199
                                self.reserver, self.node)
236
-        self.api.reserve_node.assert_called_once_with(
237
-            self.node, instance_uuid=self.node.uuid)
238
-        mock_validation.return_value.assert_called_once_with(self.node)
239
-        self.api.release_node.assert_called_once_with(self.node)
240
-        self.assertTrue(mock_log_exc.called)
200
+        self.api.validate_node.assert_called_once_with(self.node)
201
+        self.assertFalse(self.api.reserve_node.called)
202
+        self.assertFalse(self.api.release_node.called)

Loading…
Cancel
Save