Merge lp:~adeuring/charmworld/more-heartbeat-info-3 into lp:charmworld
- more-heartbeat-info-3
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Abel Deuring |
Approved revision: | 465 |
Merged at revision: | 461 |
Proposed branch: | lp:~adeuring/charmworld/more-heartbeat-info-3 |
Merge into: | lp:charmworld |
Diff against target: |
628 lines (+364/-11) 10 files modified
charmworld/__init__.py (+5/-1) charmworld/health.py (+51/-0) charmworld/jobs/ingest.py (+7/-0) charmworld/jobs/tests/test_ingest.py (+15/-6) charmworld/tests/test_health.py (+68/-0) charmworld/tests/test_utils.py (+111/-0) charmworld/utils.py (+66/-0) charmworld/views/misc.py (+6/-0) charmworld/views/tests/test_misc.py (+34/-4) setup.py (+1/-0) |
To merge this branch: | bzr merge lp:~adeuring/charmworld/more-heartbeat-info-3 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Gui Bot | continuous-integration | Approve | |
Abel Deuring (community) | Approve | ||
Benji York (community) | Approve | ||
Review via email: mp+195065@code.launchpad.net |
Commit message
heartbeat page:
- show a warning if charmworld's crontab file is missing
- show the time when the last ingest job finished
- show the time when the charmworld app server(s) were started
Description of the change
This branch adds three more details to the heartbeat page:
- if a crontrab file for charmworld exists. (The crontab file was
missing in the past...)
- the time when the server was started (suggestion by rick_h)
- the time when the last ingest job finished.
The crontab check is trivial. To store the timestamps "server started"
and "last ingest job", I added a collection "status" to the mongo DB.
I am not sure if this is the right collection name -- "misc" might
be better just in case that some container for some other data is
needed.
The time of the last ingest job is added by calling the new function
set_status_
in the unit tests of run_job() and in run_job() because
set_status_
- 459. By Abel Deuring
-
MongoDB collection name 'status' changed to 'server_status'
- 460. By Abel Deuring
-
timezone added in formatted_
status_ timestamp( ) - 461. By Abel Deuring
-
call set_status_
timestamp( ) in IngestJob.run(), not in run_job() - 462. By Abel Deuring
-
record a server's start time together with the hostname
- 463. By Abel Deuring
-
script added to remove stale entries in the "server_started" dict.
Abel Deuring (adeuring) wrote : | # |
On 13.11.2013 15:24, Benji York wrote:
> Review: Approve
>
>> This branch adds three more details to the heartbeat page:
>
> Very nice. The branch looks good once you have addressed my comments to
> your satisfaction.
>
>> To store the timestamps "server started" and "last ingest job", I
>> added a collection "status" to the mongo DB. I am not sure if this is
>> the right collection name -- "misc" might be better just in case that
>> some container for some other data is needed.
>
> How about "server_status"? That will keep "status" available for
> application data.
Fixed.
>
> We happen to run a single server at the moment, but we should assume
> that in as few places as possible. How about a mapping from hostname to
> last start time, that way we can grow into multiple servers.
Changed. We discussed this on IRC; ISTM that for now the best key to
identify a server is it's hostname. This is not ideal in conjunction
with juju, but it allows at least to look up a machine as seen on the
heartbeat page in "juju status".
Since hostnames may become stale, I also added a script that allows to
manually remove stale entries from the database.
>
>> The time of the last ingest job is added by calling the new function
>> set_status_
>> in the unit tests of run_job() and in run_job() because
>> set_status_
>
>>From line 106 of the diff:
>
> + # Some tests do not set the 'db' attribute.
> + if getattr(job, 'db', None) is not None:
> + set_status_
>
> It seems that there is only one test that was faking the DB; I wonder if
> introducing this code path is worth it for one test. It seems to me
> that it would be preferable to mutate the test rather than the code.
Right, that's a bit ugly. More than one test is affected though:
test_run_
test_run_
test_run_
I moved the call of set_status_
UpdateBundleJob
>
> Other thoughts:
>
> The first sentence of the docstring on line 277 spans more than one
> line. I would just make it a comment.
changed.
>
> Should we check the mode bits of the crontab file? I don't know that it
> has ever caused us a problem, but it is a common problem people have
> with crontabs.
>
> In formatted_
> on the formatted time (i.e., tack on a "Z").
Changed.
>
> I would like to see the already too big test_all_
> test_checks_fail() broken into one test for each passing/failing
> component, but that's not a blocker.
>
Benji York (benji) wrote : | # |
The changes look great.
Since we understand tests the best at the moment we write them, I'm a big fan of starting tests with short comments that describe what the test is about. Here's a diff that will apply some to this branch:
--- charmworld/
+++ charmworld/
@@ -1,4 +1,5 @@
def test_remove_
+ # Each time a server starts it stores the current time in the DB.
'_id': SERVER_STARTED,
'foo': 123.456,
@@ -17,6 +18,8 @@
def test_remove_
+ # If the server start time script is run without arguments it generates
+ # an error message.
output = StringIO()
1, remove_
@@ -25,6 +28,8 @@
def test_remove_
+ # If there are no server start times yet and the script is run, it
+ # informs the user.
output = StringIO()
1, remove_
@@ -32,6 +37,8 @@
'No server start times are yet recorded.\n', output.getvalue())
def test_remove_
+ # If the script is asked to remove a server start time that does not
+ # exist, it generates an informative message.
'_id': SERVER_STARTED,
'foo': 123.456,
- 464. By Abel Deuring
-
comments added to the new tests.
- 465. By Abel Deuring
-
trunk merged; conflicts resolved.
Abel Deuring (adeuring) : | # |
Juju Gui Bot (juju-gui-bot) : | # |
Preview Diff
1 | === modified file 'charmworld/__init__.py' |
2 | --- charmworld/__init__.py 2013-09-03 19:57:54 +0000 |
3 | +++ charmworld/__init__.py 2013-11-14 17:14:59 +0000 |
4 | @@ -19,7 +19,10 @@ |
5 | from charmworld.models import UserMgr |
6 | from charmworld.routes import build_routes |
7 | from charmworld.globals import default_globals_factory |
8 | -from charmworld.utils import get_session_url |
9 | +from charmworld.utils import ( |
10 | + get_session_url, |
11 | + record_server_start_time, |
12 | +) |
13 | |
14 | |
15 | class cached_view_config(view_config): |
16 | @@ -142,5 +145,6 @@ |
17 | config.add_static_view('static', 'charmworld:static', cache_max_age=3600) |
18 | config = build_routes(config) |
19 | config.scan("charmworld.views") |
20 | + record_server_start_time(database) |
21 | |
22 | return config.make_wsgi_app() |
23 | |
24 | === modified file 'charmworld/health.py' |
25 | --- charmworld/health.py 2013-10-31 16:00:29 +0000 |
26 | +++ charmworld/health.py 2013-11-14 17:14:59 +0000 |
27 | @@ -4,7 +4,14 @@ |
28 | from bzrlib.branch import Branch |
29 | from bzrlib.transport import get_transport |
30 | from collections import namedtuple |
31 | +import os |
32 | from os.path import dirname |
33 | +import time |
34 | + |
35 | +from charmworld.utils import ( |
36 | + LAST_INGEST_JOB_FINISHED, |
37 | + SERVER_STARTED, |
38 | +) |
39 | |
40 | |
41 | Check = namedtuple('check', 'name status remark') |
42 | @@ -185,3 +192,47 @@ |
43 | finally: |
44 | check = Check('ElasticSearch server', status, remark) |
45 | return check |
46 | + |
47 | + |
48 | +def check_crontab(request): |
49 | + """Check that a crontab file for charmworld exists.""" |
50 | + if 'charmworld' in os.listdir('/etc/cron.d'): |
51 | + status = PASS |
52 | + remark = 'Crontab file for charmworld exists.' |
53 | + else: |
54 | + status = FAIL |
55 | + remark = 'Crontab file for charmworld not found.' |
56 | + return Check('Crontab', status, remark) |
57 | + |
58 | + |
59 | +def formatted_timestamp(timestamp): |
60 | + return time.strftime('%Y-%m-%d %H:%M:%SZ', time.gmtime(timestamp)) |
61 | + |
62 | + |
63 | +def check_server_start_time(request): |
64 | + """Return the time when the server was last started.""" |
65 | + try: |
66 | + start_times = request.db.server_status.find_one(SERVER_STARTED) |
67 | + del start_times['_id'] |
68 | + start_times = [ |
69 | + '%s: %s' % (hostname, formatted_timestamp(timestamp)) |
70 | + for hostname, timestamp in sorted(start_times.items())] |
71 | + remark = ', '.join(start_times) |
72 | + status = PASS |
73 | + except Exception: |
74 | + remark = "Can't retrieve the server's start time." |
75 | + status = FAIL |
76 | + return Check('Server started', status, remark) |
77 | + |
78 | + |
79 | +def check_last_ingest_job(request): |
80 | + """Return the time when the ingest job finished.""" |
81 | + try: |
82 | + timestamp = request.db.server_status.find_one( |
83 | + LAST_INGEST_JOB_FINISHED)['time'] |
84 | + remark = 'finished at %s' % formatted_timestamp(timestamp) |
85 | + status = PASS |
86 | + except Exception: |
87 | + remark = "Can't retrieve the time when the last ingest job finished." |
88 | + status = FAIL |
89 | + return Check('Last ingest job', status, remark) |
90 | |
91 | === modified file 'charmworld/jobs/ingest.py' |
92 | --- charmworld/jobs/ingest.py 2013-11-06 22:54:45 +0000 |
93 | +++ charmworld/jobs/ingest.py 2013-11-14 17:14:59 +0000 |
94 | @@ -51,9 +51,11 @@ |
95 | SearchServiceNotAvailable, |
96 | ) |
97 | from charmworld.utils import ( |
98 | + LAST_INGEST_JOB_FINISHED, |
99 | quote_key, |
100 | quote_yaml, |
101 | read_locked, |
102 | + set_status_timestamp, |
103 | timestamp, |
104 | unquote_yaml, |
105 | ) |
106 | @@ -95,6 +97,9 @@ |
107 | def run(self, charm_data): |
108 | raise NotImplementedError |
109 | |
110 | + def update_ingest_status(self): |
111 | + set_status_timestamp(self.db, LAST_INGEST_JOB_FINISHED) |
112 | + |
113 | |
114 | class DBIngestJob(IngestJob): |
115 | |
116 | @@ -303,6 +308,7 @@ |
117 | CharmSource(self.db, index_client).save(charm_data) |
118 | else: |
119 | self.log.info('Skipping %s' % charm_data['_id']) |
120 | + self.update_ingest_status() |
121 | |
122 | |
123 | class UpdateBundleJob(DBIngestJob): |
124 | @@ -374,6 +380,7 @@ |
125 | basket_data['name_revno'], |
126 | basket_data['first_change'], basket_data['last_change'], |
127 | basket_data['changes'], basket_data['branch_spec']) |
128 | + self.update_ingest_status() |
129 | |
130 | |
131 | def _rev_info(r, branch): |
132 | |
133 | === modified file 'charmworld/jobs/tests/test_ingest.py' |
134 | --- charmworld/jobs/tests/test_ingest.py 2013-11-06 23:04:32 +0000 |
135 | +++ charmworld/jobs/tests/test_ingest.py 2013-11-14 17:14:59 +0000 |
136 | @@ -12,6 +12,7 @@ |
137 | import shutil |
138 | import subprocess |
139 | from textwrap import dedent |
140 | +import time |
141 | |
142 | import bzrlib |
143 | from bzrlib.bzrdir import BzrDir |
144 | @@ -53,6 +54,7 @@ |
145 | TestCase, |
146 | ) |
147 | from charmworld.utils import ( |
148 | + LAST_INGEST_JOB_FINISHED, |
149 | quote_key, |
150 | ) |
151 | |
152 | @@ -688,9 +690,11 @@ |
153 | if self.exc is not None: |
154 | raise self.exc('test error') |
155 | self.run_called = True |
156 | - |
157 | - |
158 | -class TestRunJob(TestCase): |
159 | + if getattr(self, 'db', None) is not None: |
160 | + self.update_ingest_status() |
161 | + |
162 | + |
163 | +class TestRunJob(MongoTestBase): |
164 | |
165 | def test_run_job_defaults(self): |
166 | job = TestJob() |
167 | @@ -708,10 +712,15 @@ |
168 | |
169 | def test_run_job_db_specified(self): |
170 | job = TestJob() |
171 | - run_job(job, 'foo', db='db') |
172 | - self.assertEqual('db', job.db) |
173 | + before = time.time() |
174 | + run_job(job, 'foo', db=self.db) |
175 | + after = time.time() |
176 | + self.assertEqual(self.db, job.db) |
177 | + job_finished = self.db.server_status.find_one(LAST_INGEST_JOB_FINISHED) |
178 | + job_finished = job_finished['time'] |
179 | + self.assertTrue(before <= job_finished <= after) |
180 | job = TestJob() |
181 | - run_job(job, 'foo', needs_setup=False, db='db') |
182 | + run_job(job, 'foo', needs_setup=False, db=self.db) |
183 | self.assertIs(None, getattr(job, 'db', None)) |
184 | |
185 | def test_run_job_general_exception(self): |
186 | |
187 | === modified file 'charmworld/tests/test_health.py' |
188 | --- charmworld/tests/test_health.py 2013-10-31 16:00:29 +0000 |
189 | +++ charmworld/tests/test_health.py 2013-11-14 17:14:59 +0000 |
190 | @@ -2,6 +2,8 @@ |
191 | # GNU Affero General Public License version 3 (see the file LICENSE). |
192 | |
193 | from mock import patch |
194 | +import os |
195 | +import re |
196 | |
197 | from charmworld.health import ( |
198 | check_api2, |
199 | @@ -11,14 +13,22 @@ |
200 | check_charms_collection, |
201 | check_collections, |
202 | check_charm_index, |
203 | + check_crontab, |
204 | check_elasticsearch_status, |
205 | check_ingest_queues, |
206 | + check_last_ingest_job, |
207 | + check_server_start_time, |
208 | ) |
209 | from charmworld.models import FeaturedSource |
210 | from charmworld.testing import ( |
211 | factory, |
212 | ViewTestBase, |
213 | ) |
214 | +from charmworld.utils import ( |
215 | + LAST_INGEST_JOB_FINISHED, |
216 | + record_server_start_time, |
217 | + set_status_timestamp, |
218 | +) |
219 | |
220 | |
221 | class TestCheckMixin: |
222 | @@ -261,3 +271,61 @@ |
223 | check = check_elasticsearch_status(request) |
224 | remark = "Can't retrieve the ES server's health status." |
225 | self.assertCheck(check, 'ElasticSearch server', 'Fail', remark) |
226 | + |
227 | + def test_check_crontab_pass(self): |
228 | + def make_fake_listdir(): |
229 | + def fake_listdir(path): |
230 | + return ('foo', 'charmworld') |
231 | + return fake_listdir |
232 | + |
233 | + with patch.object(os, 'listdir', new_callable=make_fake_listdir): |
234 | + request = self.getRequest() |
235 | + check = check_crontab(request) |
236 | + remark = 'Crontab file for charmworld exists.' |
237 | + self.assertCheck(check, 'Crontab', 'Pass', remark) |
238 | + |
239 | + def test_check_crontab_fail(self): |
240 | + def make_fake_listdir(): |
241 | + def fake_listdir(path): |
242 | + return ('foo', ) |
243 | + return fake_listdir |
244 | + |
245 | + with patch.object(os, 'listdir', new_callable=make_fake_listdir): |
246 | + request = self.getRequest() |
247 | + check = check_crontab(request) |
248 | + remark = 'Crontab file for charmworld not found.' |
249 | + self.assertCheck(check, 'Crontab', 'Fail', remark) |
250 | + |
251 | + def test_check_server_start_time_pass(self): |
252 | + def make_uname(): |
253 | + def uname(): |
254 | + return (None, 'foo') |
255 | + return uname |
256 | + |
257 | + with patch.object(os, 'uname', new_callable=make_uname): |
258 | + record_server_start_time(self.db) |
259 | + check = check_server_start_time(self.getRequest()) |
260 | + self.assertCheck(check, 'Server started', 'Pass') |
261 | + mo = re.search( |
262 | + '^foo: \d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\dZ$', check.remark) |
263 | + self.assertIsNot(None, mo) |
264 | + |
265 | + def test_check_server_start_time_fail(self): |
266 | + check = check_server_start_time(self.getRequest()) |
267 | + self.assertCheck( |
268 | + check, 'Server started', 'Fail', |
269 | + "Can't retrieve the server's start time.") |
270 | + |
271 | + def test_check_last_ingest_job_pass(self): |
272 | + set_status_timestamp(self.db, LAST_INGEST_JOB_FINISHED) |
273 | + check = check_last_ingest_job(self.getRequest()) |
274 | + self.assertCheck(check, 'Last ingest job', 'Pass') |
275 | + mo = re.search( |
276 | + '^finished at \d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\dZ$', check.remark) |
277 | + self.assertIsNot(None, mo) |
278 | + |
279 | + def test_check_last_ingest_job_fail(self): |
280 | + check = check_last_ingest_job(self.getRequest()) |
281 | + self.assertCheck( |
282 | + check, 'Last ingest job', 'Fail', |
283 | + "Can't retrieve the time when the last ingest job finished.") |
284 | |
285 | === modified file 'charmworld/tests/test_utils.py' |
286 | --- charmworld/tests/test_utils.py 2013-11-13 17:40:11 +0000 |
287 | +++ charmworld/tests/test_utils.py 2013-11-14 17:14:59 +0000 |
288 | @@ -13,6 +13,7 @@ |
289 | ) |
290 | import tempfile |
291 | from textwrap import dedent |
292 | +import time |
293 | from unittest import TestCase |
294 | import urllib |
295 | |
296 | @@ -29,6 +30,10 @@ |
297 | pretty_timedelta, |
298 | quote_key, |
299 | quote_yaml, |
300 | + record_server_start_time, |
301 | + remove_server_start_time_entries, |
302 | + SERVER_STARTED, |
303 | + set_status_timestamp, |
304 | unquote_yaml, |
305 | _process_dict_keys, |
306 | ) |
307 | @@ -364,3 +369,109 @@ |
308 | self.assertEqual( |
309 | build_metric_key('metric', 'foo/bar/42'), |
310 | 'metric/foo/bar/42') |
311 | + |
312 | + |
313 | +class TestSetStatusTimeStamp(MongoTestBase): |
314 | + |
315 | + def test_set_status_timestamp(self): |
316 | + # set_status_timestamp() records the current time in a MongoDB |
317 | + # record. |
318 | + before = time.time() |
319 | + set_status_timestamp(self.db, 'foo') |
320 | + after = time.time() |
321 | + recorded = self.db.server_status.find_one('foo')['time'] |
322 | + self.assertTrue(before <= recorded <= after) |
323 | + |
324 | + |
325 | +class TestServerStartTime(MongoTestBase): |
326 | + |
327 | + def test_record_server_start_time(self): |
328 | + # record_server_start_time() records the hostname and the current |
329 | + # time in a MongoDB record. |
330 | + import os |
331 | + |
332 | + def make_uname(hostname): |
333 | + def uname(): |
334 | + return (None, hostname) |
335 | + return uname |
336 | + |
337 | + with patch.object( |
338 | + os, 'uname', new_callable=make_uname, |
339 | + hostname='foo'): |
340 | + before = time.time() |
341 | + record_server_start_time(self.db) |
342 | + after = time.time() |
343 | + foo_start_time = self.db.server_status.find_one(SERVER_STARTED)['foo'] |
344 | + self.assertTrue(before <= foo_start_time <= after) |
345 | + |
346 | + # Updating a server's start time retains already recorded data. |
347 | + with patch.object( |
348 | + os, 'uname', new_callable=make_uname, |
349 | + hostname='bar'): |
350 | + record_server_start_time(self.db) |
351 | + recorded = self.db.server_status.find_one(SERVER_STARTED) |
352 | + self.assertEqual(set(('_id', 'foo', 'bar')), set(recorded)) |
353 | + |
354 | + # Existing records are updated. |
355 | + with patch.object( |
356 | + os, 'uname', new_callable=make_uname, |
357 | + hostname='foo'): |
358 | + before = time.time() |
359 | + record_server_start_time(self.db) |
360 | + after = time.time() |
361 | + recorded = self.db.server_status.find_one(SERVER_STARTED) |
362 | + self.assertTrue(recorded['foo'] > foo_start_time) |
363 | + |
364 | + def test_remove_server_start_time_entries(self): |
365 | + # Each time a server starts it stores the current time in the DB. |
366 | + # Stale entries can be removed by calling |
367 | + # remove_server_start_time_entries() (used in the script |
368 | + # remove-start-time-entry). |
369 | + self.db.server_status.insert({ |
370 | + '_id': SERVER_STARTED, |
371 | + 'foo': 123.456, |
372 | + 'bar': 234.567, |
373 | + }) |
374 | + output = StringIO() |
375 | + self.assertEqual( |
376 | + 0, remove_server_start_time_entries(['', 'foo'], output)) |
377 | + start_times = self.db.server_status.find_one({'_id': SERVER_STARTED}) |
378 | + self.assertEqual( |
379 | + { |
380 | + '_id': SERVER_STARTED, |
381 | + 'bar': 234.567, |
382 | + }, |
383 | + start_times) |
384 | + self.assertEqual('', output.getvalue()) |
385 | + |
386 | + def test_remove_server_start_time_entries_no_args(self): |
387 | + # If the server start time script is run without arguments it |
388 | + # generates an error message. |
389 | + output = StringIO() |
390 | + self.assertEqual( |
391 | + 1, remove_server_start_time_entries([''], output)) |
392 | + self.assertEqual( |
393 | + 'At least one server hostname must be specified.\n', |
394 | + output.getvalue()) |
395 | + |
396 | + def test_remove_server_start_time_entries_no_db_record(self): |
397 | + # If there are no server start times yet and the script is run, it |
398 | + # informs the user. |
399 | + output = StringIO() |
400 | + self.assertEqual( |
401 | + 1, remove_server_start_time_entries(['', 'foo'], output)) |
402 | + self.assertEqual( |
403 | + 'No server start times are yet recorded.\n', output.getvalue()) |
404 | + |
405 | + def test_remove_server_start_time_entries_invalid_server_name(self): |
406 | + # If the script is asked to remove a server start time that does not |
407 | + # exist, it generates an informative message. |
408 | + self.db.server_status.insert({ |
409 | + '_id': SERVER_STARTED, |
410 | + 'foo': 123.456, |
411 | + }) |
412 | + output = StringIO() |
413 | + self.assertEqual( |
414 | + 1, remove_server_start_time_entries(['', 'bar'], output)) |
415 | + self.assertEqual( |
416 | + 'hostname bar not found.\n', output.getvalue()) |
417 | |
418 | === modified file 'charmworld/utils.py' |
419 | --- charmworld/utils.py 2013-11-13 19:37:19 +0000 |
420 | +++ charmworld/utils.py 2013-11-14 17:14:59 +0000 |
421 | @@ -11,6 +11,7 @@ |
422 | import logging.config |
423 | import os |
424 | import re |
425 | +import sys |
426 | import time |
427 | from urllib import unquote |
428 | import urlparse |
429 | @@ -28,6 +29,12 @@ |
430 | (60, '%d minute') |
431 | ) |
432 | |
433 | +# The MongoDB ID of the time the last ingest job finished in the 'status' |
434 | +# collection. |
435 | +LAST_INGEST_JOB_FINISHED = 'last_ingest_job_finished' |
436 | +# The MongoDB ID of the time the server was started in the 'status' collection. |
437 | +SERVER_STARTED = 'server_started' |
438 | + |
439 | |
440 | def prettify_timedelta(diff): |
441 | """Return a prettified time delta, suitable for pretty printing.""" |
442 | @@ -227,3 +234,62 @@ |
443 | # Metrics are stored for the versionless ID. |
444 | versionless_id = id_revision_regex.sub('', item_id) |
445 | return '/'.join([metric_name, versionless_id]) |
446 | + |
447 | + |
448 | +def set_status_timestamp(database, id): |
449 | + database.server_status.save({ |
450 | + '_id': id, |
451 | + 'time': time.time(), |
452 | + }) |
453 | + |
454 | + |
455 | +def record_server_start_time(database): |
456 | + hostname = os.uname()[1] |
457 | + database.server_status.update( |
458 | + { |
459 | + '_id': SERVER_STARTED, |
460 | + }, |
461 | + { |
462 | + '$set': { |
463 | + hostname: time.time(), |
464 | + }, |
465 | + }, |
466 | + upsert=True) |
467 | + |
468 | + |
469 | +def remove_server_start_time_entries(argv, output, db=None): |
470 | + from charmworld.models import getconnection |
471 | + from charmworld.models import getdb |
472 | + |
473 | + if db is None: |
474 | + settings = get_ini() |
475 | + connection = getconnection(settings) |
476 | + db = getdb(connection, settings.get('mongo.database')) |
477 | + |
478 | + if len(argv) < 2: |
479 | + print >>output, "At least one server hostname must be specified." |
480 | + return 1 |
481 | + |
482 | + server_start_times = db.server_status.find_one({'_id': SERVER_STARTED}) |
483 | + if server_start_times is None: |
484 | + print >>output, "No server start times are yet recorded." |
485 | + return 1 |
486 | + |
487 | + for name in argv[1:]: |
488 | + if name not in server_start_times: |
489 | + print >>output, "hostname %s not found." % name |
490 | + return 1 |
491 | + |
492 | + for name in argv[1:]: |
493 | + db.server_status.update( |
494 | + { |
495 | + '_id': SERVER_STARTED, |
496 | + }, |
497 | + { |
498 | + '$unset': {name: ''}, |
499 | + }) |
500 | + return 0 |
501 | + |
502 | + |
503 | +def remove_server_start_time(): |
504 | + sys.exit(remove_server_start_time_entries(sys.argv, sys.stderr)) |
505 | |
506 | === modified file 'charmworld/views/misc.py' |
507 | --- charmworld/views/misc.py 2013-10-31 14:10:57 +0000 |
508 | +++ charmworld/views/misc.py 2013-11-14 17:14:59 +0000 |
509 | @@ -14,8 +14,11 @@ |
510 | check_charm_index, |
511 | check_charms_collection, |
512 | check_collections, |
513 | + check_crontab, |
514 | check_elasticsearch_status, |
515 | check_ingest_queues, |
516 | + check_last_ingest_job, |
517 | + check_server_start_time, |
518 | ) |
519 | |
520 | |
521 | @@ -62,4 +65,7 @@ |
522 | checks.append(check_ingest_queues(request)) |
523 | checks.append(check_bzr_revision(request)) |
524 | checks.append(check_elasticsearch_status(request)) |
525 | + checks.append(check_crontab(request)) |
526 | + checks.append(check_server_start_time(request)) |
527 | + checks.append(check_last_ingest_job(request)) |
528 | return {'checks': checks} |
529 | |
530 | === modified file 'charmworld/views/tests/test_misc.py' |
531 | --- charmworld/views/tests/test_misc.py 2013-11-11 10:44:53 +0000 |
532 | +++ charmworld/views/tests/test_misc.py 2013-11-14 17:14:59 +0000 |
533 | @@ -17,6 +17,11 @@ |
534 | WebTestBase, |
535 | ) |
536 | from charmworld.tests.test_health import TestCheckMixin |
537 | +from charmworld.utils import ( |
538 | + LAST_INGEST_JOB_FINISHED, |
539 | + record_server_start_time, |
540 | + set_status_timestamp, |
541 | +) |
542 | from charmworld.views.misc import heartbeat |
543 | |
544 | |
545 | @@ -119,6 +124,8 @@ |
546 | self.use_index_client() |
547 | self.index_client.index_charm(charm_data) |
548 | FeaturedSource.from_db(self.db).set_featured(charm_data, 'charm') |
549 | + record_server_start_time(self.db) |
550 | + set_status_timestamp(self.db, LAST_INGEST_JOB_FINISHED) |
551 | |
552 | def make_all_collection_names(): |
553 | def all_collection_names(): |
554 | @@ -127,13 +134,19 @@ |
555 | 'bundles', 'charm-queue', 'charms'] |
556 | return all_collection_names |
557 | |
558 | + def make_fake_listdir(): |
559 | + def fake_listdir(path): |
560 | + return ('foo', 'charmworld') |
561 | + return fake_listdir |
562 | + |
563 | request = self.getRequest() |
564 | with patch.object(request.db, 'collection_names', |
565 | new_callable=make_all_collection_names): |
566 | - response = heartbeat(request) |
567 | + with patch.object(os, 'listdir', new_callable=make_fake_listdir): |
568 | + response = heartbeat(request) |
569 | |
570 | checks = response['checks'] |
571 | - self.assertEqual(9, len(checks)) |
572 | + self.assertEqual(12, len(checks)) |
573 | remark = '1 charms found' |
574 | self.assertCheck(checks[0], 'charms collection', 'Pass', remark) |
575 | remark = '1 bundles found' |
576 | @@ -149,17 +162,28 @@ |
577 | self.assertCheck(checks[6], 'Ingest queue sizes', 'Pass', remark) |
578 | self.assertCheck(checks[7], 'BZR revision', 'Pass') |
579 | self.assertCheck(checks[8], 'ElasticSearch server', 'Pass') |
580 | + self.assertCheck(checks[9], 'Crontab', 'Pass') |
581 | + self.assertCheck(checks[10], 'Server started', 'Pass') |
582 | + self.assertCheck(checks[11], 'Last ingest job', 'Pass') |
583 | |
584 | def test_checks_fail(self): |
585 | # When services or data are not available, the checks fail. |
586 | # No setup is performed, creating a case where services and data |
587 | # are not available |
588 | from charmworld import health |
589 | + |
590 | + def make_fake_listdir(): |
591 | + def fake_listdir(path): |
592 | + return ('foo', ) |
593 | + return fake_listdir |
594 | + |
595 | with patch.object(health, 'get_transport', |
596 | new_callable=self.makeFailCallable): |
597 | - response = heartbeat(self.getRequest()) |
598 | + with patch.object(os, 'listdir', new_callable=make_fake_listdir): |
599 | + response = heartbeat(self.getRequest()) |
600 | + |
601 | checks = response['checks'] |
602 | - self.assertEqual(9, len(checks)) |
603 | + self.assertEqual(12, len(checks)) |
604 | remark = 'There are no charms. Is ingest running?' |
605 | self.assertCheck(checks[0], 'charms collection', 'Fail', remark) |
606 | remark = 'There are no bundles. Is ingest running?' |
607 | @@ -180,3 +204,9 @@ |
608 | self.assertCheck(checks[7], 'BZR revision', 'Fail', remark) |
609 | remark = "Can't retrieve the ES server's health status." |
610 | self.assertCheck(checks[8], 'ElasticSearch server', 'Fail', remark) |
611 | + remark = 'Crontab file for charmworld not found.' |
612 | + self.assertCheck(checks[9], 'Crontab', 'Fail', remark) |
613 | + remark = "Can't retrieve the server's start time." |
614 | + self.assertCheck(checks[10], 'Server started', 'Fail', remark) |
615 | + remark = "Can't retrieve the time when the last ingest job finished." |
616 | + self.assertCheck(checks[11], 'Last ingest job', 'Fail', remark) |
617 | |
618 | === modified file 'setup.py' |
619 | --- setup.py 2013-08-05 20:16:49 +0000 |
620 | +++ setup.py 2013-11-14 17:14:59 +0000 |
621 | @@ -39,6 +39,7 @@ |
622 | migrations = charmworld.migrations.migrate:main |
623 | es-update = charmworld.search:update_main |
624 | sync-index = charmworld.models:sync_index |
625 | + remove-start-time-entry = charmworld.utils:remove_server_start_time |
626 | [beaker.backends] |
627 | mongodb = mongodb_beaker:MongoDBNamespaceManager |
628 | """, |
> This branch adds three more details to the heartbeat page:
Very nice. The branch looks good once you have addressed my comments to
your satisfaction.
> To store the timestamps "server started" and "last ingest job", I
> added a collection "status" to the mongo DB. I am not sure if this is
> the right collection name -- "misc" might be better just in case that
> some container for some other data is needed.
How about "server_status"? That will keep "status" available for
application data.
We happen to run a single server at the moment, but we should assume
that in as few places as possible. How about a mapping from hostname to
last start time, that way we can grow into multiple servers.
> The time of the last ingest job is added by calling the new function timestamp( ) in ingest.run_job(); this required a modification timestamp( ) obviously needs a MongoDB instance.
> set_status_
> in the unit tests of run_job() and in run_job() because
> set_status_
From line 106 of the diff:
+ # Some tests do not set the 'db' attribute. timestamp( job.db, LAST_INGEST_ JOB_FINISHED)
+ if getattr(job, 'db', None) is not None:
+ set_status_
It seems that there is only one test that was faking the DB; I wonder if
introducing this code path is worth it for one test. It seems to me
that it would be preferable to mutate the test rather than the code.
Other thoughts:
The first sentence of the docstring on line 277 spans more than one
line. I would just make it a comment.
Should we check the mode bits of the crontab file? I don't know that it
has ever caused us a problem, but it is a common problem people have
with crontabs.
In formatted_ status_ timestamp( ), it would be nice to have the time zone
on the formatted time (i.e., tack on a "Z").
I would like to see the already too big test_all_ checks_ pass() and
test_checks_fail() broken into one test for each passing/failing
component, but that's not a blocker.