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
=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py 2010-09-03 16:14:28 +0000
+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py 2010-10-27 20:52:56 +0000
@@ -12,7 +12,7 @@
12import tempfile12import tempfile
1313
14from collections import defaultdict14from collections import defaultdict
15from datetime import datetime15from datetime import datetime, timedelta
16from operator import attrgetter16from operator import attrgetter
17from zope.component import getUtility17from zope.component import getUtility
1818
@@ -259,8 +259,13 @@
259 'generate-ppa-htaccess')259 'generate-ppa-htaccess')
260 extra_expr = []260 extra_expr = []
261 if last_success:261 if last_success:
262 # NTP is running on our servers and therefore we can assume
263 # only minimal skew, we include a fudge-factor of 1s so that
264 # even the minimal skew cannot demonstrate bug 627608.
265 last_script_start_with_skew = last_success.date_started - (
266 timedelta(seconds=1))
262 extra_expr = [267 extra_expr = [
263 ArchiveAuthToken.date_created >= last_success.date_completed]268 ArchiveAuthToken.date_created >= last_script_start_with_skew]
264269
265 new_ppa_tokens = store.find(270 new_ppa_tokens = store.find(
266 ArchiveAuthToken,271 ArchiveAuthToken,
267272
=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py 2010-10-20 20:51:26 +0000
+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py 2010-10-27 20:52:56 +0000
@@ -587,15 +587,54 @@
587 def test_getNewTokensSinceLastRun_only_those_since_last_run(self):587 def test_getNewTokensSinceLastRun_only_those_since_last_run(self):
588 """Only tokens created since the last run are returned."""588 """Only tokens created since the last run are returned."""
589 now = datetime.now(pytz.UTC)589 now = datetime.now(pytz.UTC)
590 script_start_time = now - timedelta(seconds=2)
591 script_end_time = now
592 before_previous_start = script_start_time - timedelta(seconds=30)
593
594 getUtility(IScriptActivitySet).recordSuccess(
595 'generate-ppa-htaccess', date_started=script_start_time,
596 date_completed = script_end_time)
590 tokens = self.setupDummyTokens()[1]597 tokens = self.setupDummyTokens()[1]
591 getUtility(IScriptActivitySet).recordSuccess(598 # This token will not be included.
592 'generate-ppa-htaccess', now, now - timedelta(minutes=3))599 removeSecurityProxy(tokens[0]).date_created = before_previous_start
593 removeSecurityProxy(tokens[0]).date_created = (
594 now - timedelta(minutes=4))
595600
596 script = self.getScript()601 script = self.getScript()
597 self.assertContentEqual(tokens[1:], script.getNewTokensSinceLastRun())602 self.assertContentEqual(tokens[1:], script.getNewTokensSinceLastRun())
598603
604 def test_getNewTokensSinceLastRun_includes_tokens_during_last_run(self):
605 """Tokens created during the last ppa run will be included."""
606 now = datetime.now(pytz.UTC)
607 script_start_time = now - timedelta(seconds=2)
608 script_end_time = now
609 in_between = now - timedelta(seconds=1)
610
611 getUtility(IScriptActivitySet).recordSuccess(
612 'generate-ppa-htaccess', script_start_time, script_end_time)
613 tokens = self.setupDummyTokens()[1]
614 # This token will be included because it's been created during
615 # the previous script run.
616 removeSecurityProxy(tokens[0]).date_created = in_between
617
618 script = self.getScript()
619 self.assertContentEqual(tokens, script.getNewTokensSinceLastRun())
620
621 def test_getNewTokensSinceLastRun_handles_ntp_skew(self):
622 """An ntp-skew of up to one second will not affect the results."""
623 now = datetime.now(pytz.UTC)
624 script_start_time = now - timedelta(seconds=2)
625 script_end_time = now
626 earliest_with_ntp_skew = script_start_time - timedelta(milliseconds=500)
627
628 tokens = self.setupDummyTokens()[1]
629 getUtility(IScriptActivitySet).recordSuccess(
630 'generate-ppa-htaccess', date_started=script_start_time,
631 date_completed=script_end_time)
632 # This token will still be included in the results.
633 removeSecurityProxy(tokens[0]).date_created = earliest_with_ntp_skew
634
635 script = self.getScript()
636 self.assertContentEqual(tokens, script.getNewTokensSinceLastRun())
637
599 def test_getNewTokensSinceLastRun_only_active_tokens(self):638 def test_getNewTokensSinceLastRun_only_active_tokens(self):
600 """Only active tokens are returned."""639 """Only active tokens are returned."""
601 now = datetime.now(pytz.UTC)640 now = datetime.now(pytz.UTC)