Launchpad itself

Merge lp:~michael.nelson/launchpad/627608-p3a-token-race into lp:launchpad

  1. 627608-p3a-token-race
  2. Merge into devel
Proposed by Michael Nelson
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: 11812
Proposed branch: lp:~michael.nelson/launchpad/627608-p3a-token-race
Merge into: lp:launchpad
Diff against target: 90 lines (+50/-6)
2 files modified
lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py (+7/-2)
lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py (+43/-4)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/627608-p3a-token-race
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+39469@code.launchpad.net

Commit message

p3a access tokens created in the window when the generate_ppa_htaccess script runs will be included in the next run.

Description of the change

Overview
========

This branch ensures that if a token is created *while* the generate_ppa_htaccess script is running (ie. before the script finishes), that it will be included the next time the script runs.

Details
=======
We did some work recently to improve the generate_ppa_htaccess script so that it could run minutely. That work selected only tokens that had been created since the last successful script run completed.

We recently saw bug 627608 happening again, even though the script was running minutely, and discovered that if the token is created in the tiny window *after* a run of the script has started, but before it is finished, then it won't be included in the next run either (more detailed description on the bug).

The branch does the adds a test and trivial fix, and fixes an existing test which was incorrectly setting the date_started for the script activity.

To test
=======

bin/test -vvm test_generate_ppa_htaccess

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

It's very picky, but I think I'd find the tests easier to understand if you gave the times names, like so:

earliest = now - timedelta(seconds=20)
latest = now - timedelta(seconds=10)
in_between = now - timedelta(seconds=15)

and use keyword arguments in the calls to recordSuccess then it's a bit easier to see the relationship between them. Not 100% sure about the names.

Even if you don't do this, the change looks good to land. Congratulations on finding the problem :-)

review: Approve
Revision history for this message
Michael Nelson (michael.nelson) wrote :

Thanks Michael.

Regarding the possible NTP-skew between servers, I've added the fudge-factor of 1 second as you suggested.

I've pushed 11812, but the incremental is here:
http://pastebin.ubuntu.com/520992/

I also created the variables for clarity as you suggested.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
2--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py 2010-09-03 16:14:28 +0000
3+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py 2010-10-27 20:52:56 +0000
4@@ -12,7 +12,7 @@
5 import tempfile
6
7 from collections import defaultdict
8-from datetime import datetime
9+from datetime import datetime, timedelta
10 from operator import attrgetter
11 from zope.component import getUtility
12
13@@ -259,8 +259,13 @@
14 'generate-ppa-htaccess')
15 extra_expr = []
16 if last_success:
17+ # NTP is running on our servers and therefore we can assume
18+ # only minimal skew, we include a fudge-factor of 1s so that
19+ # even the minimal skew cannot demonstrate bug 627608.
20+ last_script_start_with_skew = last_success.date_started - (
21+ timedelta(seconds=1))
22 extra_expr = [
23- ArchiveAuthToken.date_created >= last_success.date_completed]
24+ ArchiveAuthToken.date_created >= last_script_start_with_skew]
25
26 new_ppa_tokens = store.find(
27 ArchiveAuthToken,
28
29=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
30--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py 2010-10-20 20:51:26 +0000
31+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py 2010-10-27 20:52:56 +0000
32@@ -587,15 +587,54 @@
33 def test_getNewTokensSinceLastRun_only_those_since_last_run(self):
34 """Only tokens created since the last run are returned."""
35 now = datetime.now(pytz.UTC)
36+ script_start_time = now - timedelta(seconds=2)
37+ script_end_time = now
38+ before_previous_start = script_start_time - timedelta(seconds=30)
39+
40+ getUtility(IScriptActivitySet).recordSuccess(
41+ 'generate-ppa-htaccess', date_started=script_start_time,
42+ date_completed = script_end_time)
43 tokens = self.setupDummyTokens()[1]
44- getUtility(IScriptActivitySet).recordSuccess(
45- 'generate-ppa-htaccess', now, now - timedelta(minutes=3))
46- removeSecurityProxy(tokens[0]).date_created = (
47- now - timedelta(minutes=4))
48+ # This token will not be included.
49+ removeSecurityProxy(tokens[0]).date_created = before_previous_start
50
51 script = self.getScript()
52 self.assertContentEqual(tokens[1:], script.getNewTokensSinceLastRun())
53
54+ def test_getNewTokensSinceLastRun_includes_tokens_during_last_run(self):
55+ """Tokens created during the last ppa run will be included."""
56+ now = datetime.now(pytz.UTC)
57+ script_start_time = now - timedelta(seconds=2)
58+ script_end_time = now
59+ in_between = now - timedelta(seconds=1)
60+
61+ getUtility(IScriptActivitySet).recordSuccess(
62+ 'generate-ppa-htaccess', script_start_time, script_end_time)
63+ tokens = self.setupDummyTokens()[1]
64+ # This token will be included because it's been created during
65+ # the previous script run.
66+ removeSecurityProxy(tokens[0]).date_created = in_between
67+
68+ script = self.getScript()
69+ self.assertContentEqual(tokens, script.getNewTokensSinceLastRun())
70+
71+ def test_getNewTokensSinceLastRun_handles_ntp_skew(self):
72+ """An ntp-skew of up to one second will not affect the results."""
73+ now = datetime.now(pytz.UTC)
74+ script_start_time = now - timedelta(seconds=2)
75+ script_end_time = now
76+ earliest_with_ntp_skew = script_start_time - timedelta(milliseconds=500)
77+
78+ tokens = self.setupDummyTokens()[1]
79+ getUtility(IScriptActivitySet).recordSuccess(
80+ 'generate-ppa-htaccess', date_started=script_start_time,
81+ date_completed=script_end_time)
82+ # This token will still be included in the results.
83+ removeSecurityProxy(tokens[0]).date_created = earliest_with_ntp_skew
84+
85+ script = self.getScript()
86+ self.assertContentEqual(tokens, script.getNewTokensSinceLastRun())
87+
88 def test_getNewTokensSinceLastRun_only_active_tokens(self):
89 """Only active tokens are returned."""
90 now = datetime.now(pytz.UTC)