Merge lp:~gary/launchpad/bug662912 into lp:launchpad

Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 11797
Proposed branch: lp:~gary/launchpad/bug662912
Merge into: lp:launchpad
Diff against target: 100 lines (+21/-2)
4 files modified
.bzrignore (+1/-0)
configs/development/launchpad-lazr.conf (+1/-0)
daemons/librarian.tac (+8/-2)
lib/canonical/librarian/web.py (+11/-0)
To merge this branch: bzr merge lp:~gary/launchpad/bug662912
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+39315@code.launchpad.net

Commit message

Add diagnostics for bug 662912

Description of the change

This branch adds diagnostics to approach bug 662912. As noted in my comment #1 for that bug, I have a hypothesis that this is caused by a rejected hostname. This branch adds log messages for that case, plus all other cases that generate a 404 other than a simple missing file (_eb_getFileAlias in c/librarian/web.py).

It also puts the message about upstream librarians in the logfile so it is easier to see that the configuration is as expected.

It also creates a log file destination for development so developers can look at the locally-generated logs.

To post a comment you must log in.
Robert Collins (lifeless) wrote :

The development target for logs is unneeded and harmful, please don't
do that. My librarian branch already captures the log per-test and
includes in the test details.

Julian Edwards (julian-edwards) wrote :

The test output won't change though will it? You need to change the testrunner config for that.

Everything else looks good although as I said on IRC, consider using addSystemEventTrigger instead of callWhenRunning, but that's optional.

review: Approve
Gary Poster (gary) wrote :

Thank you for the review and comments.

Robert approved the librarian change on IRC.

I looked at the addSystemEventTrigger approach. The behavior is identical to the callWhenRunning version. It's a bit longer spelling, but I guess it is a bit easier to understand the intent. That's a compelling argument, so I made the change.

Thanks again,

Gary

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2010-10-18 21:53:28 +0000
3+++ .bzrignore 2010-10-25 22:09:50 +0000
4@@ -80,3 +80,4 @@
5 *.pt.py
6 .project
7 .pydevproject
8+librarian.log
9
10=== modified file 'configs/development/launchpad-lazr.conf'
11--- configs/development/launchpad-lazr.conf 2010-10-21 03:22:06 +0000
12+++ configs/development/launchpad-lazr.conf 2010-10-25 22:09:50 +0000
13@@ -175,6 +175,7 @@
14 [librarian_server]
15 root: /var/tmp/fatsam
16 launch: True
17+logfile: librarian.log
18
19 [malone]
20 bugmail_error_from_address: noreply@bugs.launchpad.net
21
22=== modified file 'daemons/librarian.tac'
23--- daemons/librarian.tac 2010-10-20 18:43:29 +0000
24+++ daemons/librarian.tac 2010-10-25 22:09:50 +0000
25@@ -9,6 +9,8 @@
26 from meliae import scanner
27
28 from twisted.application import service, strports
29+from twisted.internet import reactor
30+from twisted.python import log
31 from twisted.web import server
32
33 from canonical.config import config, dbconfig
34@@ -29,10 +31,14 @@
35 if config.librarian_server.upstream_host:
36 upstreamHost = config.librarian_server.upstream_host
37 upstreamPort = config.librarian_server.upstream_port
38- print 'Using upstream librarian http://%s:%d' % (
39- upstreamHost, upstreamPort)
40+ reactor.addSystemEventTrigger(
41+ 'before', 'startup', log.msg,
42+ 'Using upstream librarian http://%s:%d' %
43+ (upstreamHost, upstreamPort))
44 else:
45 upstreamHost = upstreamPort = None
46+ reactor.addSystemEventTrigger(
47+ 'before', 'startup', log.msg, 'Not using upstream librarian')
48
49 application = service.Application('Librarian')
50 librarianService = service.IServiceCollection(application)
51
52=== modified file 'lib/canonical/librarian/web.py'
53--- lib/canonical/librarian/web.py 2010-09-24 15:40:49 +0000
54+++ lib/canonical/librarian/web.py 2010-10-25 22:09:50 +0000
55@@ -7,6 +7,7 @@
56 import time
57 from urlparse import urlparse
58
59+from twisted.python import log
60 from twisted.web import resource, static, util, server, proxy
61 from twisted.internet.threads import deferToThread
62
63@@ -52,6 +53,8 @@
64 try:
65 aliasID = int(name)
66 except ValueError:
67+ log.msg(
68+ "404: alias is not an int: %r" % (name,))
69 return fourOhFour
70
71 return LibraryFileAliasResource(self.storage, aliasID,
72@@ -76,6 +79,8 @@
73 try:
74 self.aliasID = int(filename)
75 except ValueError:
76+ log.msg(
77+ "404 (old URL): alias is not an int: %r" % (name,))
78 return fourOhFour
79 filename = request.postpath[0]
80
81@@ -95,6 +100,9 @@
82 netloc = netloc[:netloc.find(':')]
83 expected_hostname = 'i%d.restricted.%s' % (self.aliasID, netloc)
84 if expected_hostname != hostname:
85+ log.msg(
86+ '404: expected_hostname != hostname: %r != %r' %
87+ (expected_hostname, hostname))
88 return fourOhFour
89
90 token = request.args.get('token', [None])[0]
91@@ -128,6 +136,9 @@
92 # a crude form of access control (stuff we care about can have
93 # unguessable names effectively using the filename as a secret).
94 if dbfilename.encode('utf-8') != filename:
95+ log.msg(
96+ "404: dbfilename.encode('utf-8') != filename: %r != %r"
97+ % (dbfilename.encode('utf-8'), filename))
98 return fourOhFour
99
100 if not restricted: