Merge lp:~andreserl/maas/lp1604962_lp1604987 into lp:~maas-committers/maas/trunk
- lp1604962_lp1604987
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Andres Rodriguez |
Approved revision: | no longer in the source branch. |
Merged at revision: | 5204 |
Proposed branch: | lp:~andreserl/maas/lp1604962_lp1604987 |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
291 lines (+85/-40) 7 files modified
src/maasserver/models/node.py (+33/-20) src/maasserver/models/tests/test_node.py (+12/-3) src/maasserver/rpc/nodes.py (+1/-1) src/maasserver/status_monitor.py (+1/-1) src/metadataserver/api.py (+18/-12) src/metadataserver/tests/test_api_status.py (+5/-3) src/provisioningserver/events.py (+15/-0) |
To merge this branch: | bzr merge lp:~andreserl/maas/lp1604962_lp1604987 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Andres Rodriguez (community) | Approve | ||
Gavin Panella (community) | Approve | ||
Review via email: mp+300978@code.launchpad.net |
Commit message
Fixes to correctly log cloud-init/curtin FAIL events in the node event log.
Fixes to correctly log "mark_failed" events. We did this by combining the mark_failed with _mark_failed functions, provided that the latter wasn't really storing anything in the event log and it should. It now differentiates between a user driven "Mark Failed" vs a system driven.
Description of the change
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~andreserl/maas/lp1604962_lp1604987 into lp:maas failed. Below is the output from the failed tests.
Hit:1 http://
Hit:2 http://
Get:3 http://
Hit:4 http://
Fetched 95.7 kB in 0s (193 kB/s)
Reading package lists...
sudo DEBIAN_
--no-
Reading package lists...
Building dependency tree...
Reading state information...
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
build-essential is already the newest version (12.1ubuntu2).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubun
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is already the newest version (3:6.03+
python-formencode is already the newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1build1).
python-netaddr is already the newest version (0.7.18-1).
python-netifaces is already the newes...
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~andreserl/maas/lp1604962_lp1604987 into lp:maas failed. Below is the output from the failed tests.
Get:1 http://
Hit:2 http://
Get:3 http://
Hit:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Fetched 257 kB in 0s (529 kB/s)
Reading package lists...
sudo DEBIAN_
--no-
Reading package lists...
Building dependency tree...
Reading state information...
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
build-essential is already the newest version (12.1ubuntu2).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubun
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is...
MAAS Lander (maas-lander) wrote : | # |
There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.
Andres Rodriguez (andreserl) : | # |
Preview Diff
1 | === modified file 'src/maasserver/models/node.py' |
2 | --- src/maasserver/models/node.py 2016-07-21 23:33:35 +0000 |
3 | +++ src/maasserver/models/node.py 2016-07-25 22:18:24 +0000 |
4 | @@ -1132,20 +1132,29 @@ |
5 | |
6 | def _register_request_event( |
7 | self, user, type_name, action='', comment=None): |
8 | - """Register a node user request event.""" |
9 | - # don't register system generated non-user requests |
10 | + """Register a node request event. |
11 | + |
12 | + It registers events like start_commission (started by a user), |
13 | + or mark_failed (started by the system)""" |
14 | + |
15 | + # the description will be the comment, if any. |
16 | + description = comment if comment else '' |
17 | + # if the user exists, we need to construct the description with |
18 | + # the user. as it would be a user-driven request. |
19 | if user is not None: |
20 | - event_details = EVENT_DETAILS[type_name] |
21 | - description = "(%s)" % user.username |
22 | - if comment: |
23 | - description = "%s - %s" % (description, comment) |
24 | - # Avoid circular imports. |
25 | - from maasserver.models.event import Event |
26 | - Event.objects.register_event_and_event_type( |
27 | - self.system_id, type_name, type_level=event_details.level, |
28 | - type_description=event_details.description, |
29 | - event_action=action, |
30 | - event_description=description) |
31 | + if len(description) == 0: |
32 | + description = "(%s)" % user |
33 | + else: |
34 | + description = "(%s) - %s" % (user, description) |
35 | + event_details = EVENT_DETAILS[type_name] |
36 | + |
37 | + # Avoid circular imports. |
38 | + from maasserver.models.event import Event |
39 | + Event.objects.register_event_and_event_type( |
40 | + self.system_id, type_name, type_level=event_details.level, |
41 | + type_description=event_details.description, |
42 | + event_action=action, |
43 | + event_description=description) |
44 | |
45 | def storage_layout_issues(self): |
46 | """Return any errors with the storage layout. |
47 | @@ -2408,18 +2417,22 @@ |
48 | arch, subarch = self.architecture.split('/') |
49 | return (arch, subarch) |
50 | |
51 | - def mark_failed(self, user, comment=None): |
52 | - self._register_request_event( |
53 | - user, EVENT_TYPES.REQUEST_NODE_MARK_FAILED, action='mark_failed', |
54 | - comment=comment) |
55 | - self._mark_failed(user, comment) |
56 | - |
57 | - def _mark_failed(self, user, comment=None, commit=True): |
58 | + def mark_failed(self, user=None, comment=None, commit=True): |
59 | """Mark this node as failed. |
60 | |
61 | The actual 'failed' state depends on the current status of the |
62 | node. |
63 | """ |
64 | + if user is None: |
65 | + # This is a system-driven event. Log level is ERROR. |
66 | + event_type = EVENT_TYPES.REQUEST_NODE_MARK_FAILED_SYSTEM |
67 | + else: |
68 | + # This is a user-driven event. Log level is INFO. |
69 | + event_type = EVENT_TYPES.REQUEST_NODE_MARK_FAILED |
70 | + self._register_request_event( |
71 | + user, event_type, action='mark_failed', |
72 | + comment=comment) |
73 | + |
74 | new_status = get_failed_status(self.status) |
75 | if new_status is not None: |
76 | self.status = new_status |
77 | |
78 | === modified file 'src/maasserver/models/tests/test_node.py' |
79 | --- src/maasserver/models/tests/test_node.py 2016-07-18 20:06:21 +0000 |
80 | +++ src/maasserver/models/tests/test_node.py 2016-07-25 22:18:24 +0000 |
81 | @@ -3148,14 +3148,23 @@ |
82 | event_action=event_action, |
83 | event_description=event_description)) |
84 | |
85 | - def test__register_request_event_with_none_user_saves_no_event(self): |
86 | + def test__register_request_event_none_user_saves_comment_not_user(self): |
87 | node = factory.make_Node() |
88 | log_mock = self.patch_autospec( |
89 | Event.objects, 'register_event_and_event_type') |
90 | event_name = EVENT_TYPES.REQUEST_NODE_START |
91 | + event_action = factory.make_name('action') |
92 | + event_details = EVENT_DETAILS[event_name] |
93 | comment = factory.make_name("comment") |
94 | - node._register_request_event(None, event_name, comment) |
95 | - self.assertThat(log_mock, MockNotCalled()) |
96 | + event_description = "%s" % (comment) |
97 | + node._register_request_event(None, event_name, event_action, comment) |
98 | + self.assertThat(log_mock, MockCalledOnceWith( |
99 | + node.system_id, |
100 | + EVENT_TYPES.REQUEST_NODE_START, |
101 | + type_level=event_details.level, |
102 | + type_description=event_details.description, |
103 | + event_action=event_action, |
104 | + event_description=event_description)) |
105 | |
106 | def test__status_message_returns_most_recent_event(self): |
107 | # The first event won't be returned. |
108 | |
109 | === modified file 'src/maasserver/rpc/nodes.py' |
110 | --- src/maasserver/rpc/nodes.py 2016-05-11 00:25:14 +0000 |
111 | +++ src/maasserver/rpc/nodes.py 2016-07-25 22:18:24 +0000 |
112 | @@ -49,7 +49,7 @@ |
113 | except Node.DoesNotExist: |
114 | raise NoSuchNode.from_system_id(system_id) |
115 | try: |
116 | - node.mark_failed(None, error_description) |
117 | + node.mark_failed(comment=error_description) |
118 | except exceptions.NodeStateViolation as e: |
119 | raise NodeStateViolation(e) |
120 | |
121 | |
122 | === modified file 'src/maasserver/status_monitor.py' |
123 | --- src/maasserver/status_monitor.py 2016-07-22 14:41:58 +0000 |
124 | +++ src/maasserver/status_monitor.py 2016-07-25 22:18:24 +0000 |
125 | @@ -43,7 +43,7 @@ |
126 | NODE_STATUS_CHOICES_DICT[node.status], |
127 | NODE_FAILURE_MONITORED_STATUS_TIMEOUTS[node.status], |
128 | ) |
129 | - node._mark_failed(None, commit=False, comment=comment) |
130 | + node.mark_failed(commit=False, comment=comment) |
131 | node.status_expires = None |
132 | node.save() |
133 | |
134 | |
135 | === modified file 'src/metadataserver/api.py' |
136 | --- src/metadataserver/api.py 2016-07-22 00:41:50 +0000 |
137 | +++ src/metadataserver/api.py 2016-07-25 22:18:24 +0000 |
138 | @@ -157,12 +157,19 @@ |
139 | raise UnknownMetadataVersion("Unknown metadata version: %s" % version) |
140 | |
141 | |
142 | -def add_event_to_node_event_log(node, origin, action, description): |
143 | +def add_event_to_node_event_log( |
144 | + node, origin, action, description, result=None): |
145 | """Add an entry to the node's event log.""" |
146 | if node.status == NODE_STATUS.COMMISSIONING: |
147 | - type_name = EVENT_TYPES.NODE_COMMISSIONING_EVENT |
148 | + if result in ['SUCCESS', None]: |
149 | + type_name = EVENT_TYPES.NODE_COMMISSIONING_EVENT |
150 | + else: |
151 | + type_name = EVENT_TYPES.NODE_COMMISSIONING_EVENT_FAILED |
152 | elif node.status == NODE_STATUS.DEPLOYING: |
153 | - type_name = EVENT_TYPES.NODE_INSTALL_EVENT |
154 | + if result in ['SUCCESS', None]: |
155 | + type_name = EVENT_TYPES.NODE_INSTALL_EVENT |
156 | + else: |
157 | + type_name = EVENT_TYPES.NODE_INSTALL_EVENT_FAILED |
158 | elif node.node_type in [ |
159 | NODE_TYPE.RACK_CONTROLLER, |
160 | NODE_TYPE.REGION_AND_RACK_CONTROLLER]: |
161 | @@ -327,7 +334,8 @@ |
162 | result = message.get('result') |
163 | |
164 | # Add this event to the node event log. |
165 | - add_event_to_node_event_log(node, origin, activity_name, description) |
166 | + add_event_to_node_event_log( |
167 | + node, origin, activity_name, description, result) |
168 | |
169 | # Save attached files, if any. |
170 | for sent_file in message.get('files', []): |
171 | @@ -358,12 +366,11 @@ |
172 | elif node.status == NODE_STATUS.DEPLOYING: |
173 | if result in ['FAIL', 'FAILURE']: |
174 | node.mark_failed( |
175 | - None, |
176 | - "Installation failed (refer to the " |
177 | - "installation log for more information).") |
178 | + comment="Installation failed (refer to the " |
179 | + "installation log for more information).") |
180 | elif node.status == NODE_STATUS.DISK_ERASING: |
181 | if result in ['FAIL', 'FAILURE']: |
182 | - node.mark_failed(None, "Failed to erase disks.") |
183 | + node.mark_failed(comment="Failed to erase disks.") |
184 | |
185 | # Deallocate the node if we enter any terminal state. |
186 | if node.node_type == NODE_TYPE.MACHINE and node.status in [ |
187 | @@ -530,16 +537,15 @@ |
188 | self._store_installation_results(node, request) |
189 | if status == SIGNAL_STATUS.FAILED: |
190 | node.mark_failed( |
191 | - None, |
192 | - "Installation failed (refer to the " |
193 | - "installation log for more information).") |
194 | + comment="Installation failed (refer to the " |
195 | + "installation log for more information).") |
196 | target_status = None |
197 | elif node.status == NODE_STATUS.DISK_ERASING: |
198 | if status == SIGNAL_STATUS.OK: |
199 | # disk erasing complete, release node |
200 | node.release() |
201 | elif status == SIGNAL_STATUS.FAILED: |
202 | - node.mark_failed(None, "Failed to erase disks.") |
203 | + node.mark_failed(comment="Failed to erase disks.") |
204 | target_status = None |
205 | |
206 | if target_status in (None, node.status): |
207 | |
208 | === modified file 'src/metadataserver/tests/test_api_status.py' |
209 | --- src/metadataserver/tests/test_api_status.py 2016-06-11 23:58:38 +0000 |
210 | +++ src/metadataserver/tests/test_api_status.py 2016-07-25 22:18:24 +0000 |
211 | @@ -193,7 +193,8 @@ |
212 | NODE_STATUS.FAILED_DEPLOYMENT, reload_object(node).status) |
213 | # Check last node event. |
214 | self.assertEqual( |
215 | - "'curtin' Command Install", |
216 | + "Installation failed (refer to the installation" |
217 | + " log for more information).", |
218 | Event.objects.filter(node=node).last().description) |
219 | |
220 | def test_status_installation_fail_leaves_node_failed(self): |
221 | @@ -212,7 +213,8 @@ |
222 | NODE_STATUS.FAILED_DEPLOYMENT, reload_object(node).status) |
223 | # Check last node event. |
224 | self.assertEqual( |
225 | - "'curtin' Command Install", |
226 | + "Installation failed (refer to the installation" |
227 | + " log for more information).", |
228 | Event.objects.filter(node=node).last().description) |
229 | |
230 | def test_status_installation_failure_doesnt_clear_owner(self): |
231 | @@ -270,7 +272,7 @@ |
232 | NODE_STATUS.FAILED_DISK_ERASING, reload_object(node).status) |
233 | # Check last node event. |
234 | self.assertEqual( |
235 | - "'curtin' Erasing disk", |
236 | + "Failed to erase disks.", |
237 | Event.objects.filter(node=node).last().description) |
238 | |
239 | def test_status_erasure_failure_does_not_populate_tags(self): |
240 | |
241 | === modified file 'src/provisioningserver/events.py' |
242 | --- src/provisioningserver/events.py 2016-07-13 19:03:15 +0000 |
243 | +++ src/provisioningserver/events.py 2016-07-25 22:18:24 +0000 |
244 | @@ -66,7 +66,9 @@ |
245 | # Node status events |
246 | NODE_STATUS_EVENT = "NODE_STATUS_EVENT" |
247 | NODE_COMMISSIONING_EVENT = "NODE_COMMISSIONING_EVENT" |
248 | + NODE_COMMISSIONING_EVENT_FAILED = "NODE_COMMISSIONING_EVENT_FAILED" |
249 | NODE_INSTALL_EVENT = "NODE_INSTALL_EVENT" |
250 | + NODE_INSTALL_EVENT_FAILED = "NODE_INSTALL_EVENT_FAILED" |
251 | # Node user request events |
252 | REQUEST_NODE_START_COMMISSIONING = "REQUEST_NODE_START_COMMISSIONING" |
253 | REQUEST_NODE_ABORT_COMMISSIONING = "REQUEST_NODE_ABORT_COMMISSIONING" |
254 | @@ -76,6 +78,7 @@ |
255 | REQUEST_NODE_ABORT_ERASE_DISK = "REQUEST_NODE_ABORT_ERASE_DISK" |
256 | REQUEST_NODE_RELEASE = "REQUEST_NODE_RELEASE" |
257 | REQUEST_NODE_MARK_FAILED = "REQUEST_NODE_MARK_FAILED" |
258 | + REQUEST_NODE_MARK_FAILED_SYSTEM = "REQUEST_NODE_MARK_FAILED_SYSTEM" |
259 | REQUEST_NODE_MARK_BROKEN = "REQUEST_NODE_MARK_BROKEN" |
260 | REQUEST_NODE_MARK_FIXED = "REQUEST_NODE_MARK_FIXED" |
261 | REQUEST_NODE_START_DEPLOYMENT = "REQUEST_NODE_START_DEPLOYMENT" |
262 | @@ -151,10 +154,18 @@ |
263 | description="Node commissioning", |
264 | level=DEBUG, |
265 | ), |
266 | + EVENT_TYPES.NODE_COMMISSIONING_EVENT_FAILED: EventDetail( |
267 | + description="Node commissioning failure", |
268 | + level=ERROR, |
269 | + ), |
270 | EVENT_TYPES.NODE_INSTALL_EVENT: EventDetail( |
271 | description="Node installation", |
272 | level=DEBUG, |
273 | ), |
274 | + EVENT_TYPES.NODE_INSTALL_EVENT_FAILED: EventDetail( |
275 | + description="Node installation failure", |
276 | + level=ERROR, |
277 | + ), |
278 | EVENT_TYPES.REQUEST_NODE_START_COMMISSIONING: EventDetail( |
279 | description="User starting node commissioning", |
280 | level=INFO, |
281 | @@ -187,6 +198,10 @@ |
282 | description="User marking node failed", |
283 | level=INFO, |
284 | ), |
285 | + EVENT_TYPES.REQUEST_NODE_MARK_FAILED_SYSTEM: EventDetail( |
286 | + description="Marking node failed", |
287 | + level=ERROR, |
288 | + ), |
289 | EVENT_TYPES.REQUEST_NODE_MARK_BROKEN: EventDetail( |
290 | description="User marking node broken", |
291 | level=INFO, |
Looks good, with a few minor comments.