launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14620
[Merge] lp:~sinzui/launchpad/librarian-merge-proposal into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/librarian-merge-proposal into lp:launchpad.
Commit message:
Allow merge proposals to operate without the launchpad librarian.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #558585 in Launchpad itself: "librarian outages cause OOPSes on pages showing BranchMergeProposal diffs"
https://bugs.launchpad.net/launchpad/+bug/558585
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/librarian-merge-proposal/+merge/137039
When the librarian is not available, merge proposal pages will not
load at all. We expect the librarian to not be available for as much
as an hour when it is upgraded to precises, but we want Lp to be
operational.
--------------------------------------------------------------------
RULES
Pre-implementation: wgrant
* Diff.text opens the LFA, self.diff_text.open(), without specifying
the timeout.
* the default timeout is 5 seconds, which is the entire time Lp
allocates to a request. The 5s timeout was added when Lp had a
30s request time in 2009. It is impossible for the timeout to be
raised from with the webapp. Maybe the default should be 4 seconds.
when the the webapp is making the request.
LIBRARIAN_SERVER_DEFAULT_TIMEOUT = 5
* Diff.text can pass a timout that is 1s or 5s minus the remaing
time to ensure that the request time does not run out. After
some discussion, we believe 2s is more than amp for the
the librarian...anything requiring more than cannot be shown
in the page.
* The LibrarianServerError needs to be wrapped for the web service.
* DiffRenderingMixin.preview_diff_text wraps the call to Diff.text to
sanitise and recover from exceptions.
* The property could catch the LibrarianServerError raise when the
open attempts exceed the timeout.
* The diff_oversized property just counts lines to return a bool.
It could return True if preview_diff_text handled the timeout.
ADDENDUM:
* I had a fight with with the import fascist. I won, by removing
all th ancient cruft, but had to give some ground by acknowledging
that there are dubious imports in some browser modules.
* There was more lint then expected in the old modules.
QA
* Visit https://code.qastaging.launchpad.net/~sinzui/impressive/lirc-0/+merge/29161
* Verify the page loads and that it states that the diff could not
be retrieved at this time.
LINT
lib/lp/code/browser/branchmergeproposal.py
lib/lp/code/browser/tests/test_branchmergeproposal.py
lib/lp/code/model/diff.py
lib/lp/code/model/tests/test_diff.py
lib/lp/code/templates/branchmergeproposal-diff.pt
lib/lp/scripts/utilities/importfascist.py
lib/lp/services/librarian/interfaces/client.py
lib/lp/services/librarian/tests/test_client.py
lib/lp/services/webapp/adapter.py
LoC
I have more than 16,000 line credit.
TEST
./bin/test -vvc lp.code.browser.tests.test_branchmergeproposal
./bin/test -vvc lp.code.model.tests.test_diff
./bin/test -vvc lp.services.librarian.tests.test_client
IMPLEMENTATION
I updated view to try and recover from a failure to get the diff from
the librarian. The page will explain that the diff wasn't available, but
the user can reload or download it.
lib/lp/code/browser/branchmergeproposal.py
lib/lp/code/browser/tests/test_branchmergeproposal.py
lib/lp/code/templates/branchmergeproposal-diff.pt
I changed the model to pass the timeout time to the LibrarianFileAlias. The
rules are a but complicated. Scripts run without timeouts. The webapp
should get the diff within 2 seconds -- otherwise something is very wrong.
If there is less that 2 seconds left in the request, the code shaves a bit
of time to set the timeout.
lib/lp/services/webapp/adapter.py
lib/lp/code/model/diff.py
lib/lp/code/model/tests/test_diff.py
I exported the LibrarianServerError as a httplib.TIMEOUT error.
lib/lp/services/librarian/interfaces/client.py
lib/lp/services/librarian/tests/test_client.py
I removed all the checks for non-existent paths. The changes illustrate
how much Lp has changes since 2009. I re-enabled the database checks,
but the rules are clearly difficult to enforce. I added a list of
dubious imports so that the code continues to build even though
we think the design is wrong.
lib/lp/scripts/utilities/importfascist.py
--
https://code.launchpad.net/~sinzui/launchpad/librarian-merge-proposal/+merge/137039
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/librarian-merge-proposal into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2012-11-26 08:40:20 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2012-11-29 20:30:28 +0000
@@ -107,6 +107,7 @@
Summary,
Whiteboard,
)
+from lp.services.librarian.interfaces.client import LibrarianServerError
from lp.services.messages.interfaces.message import IMessageSet
from lp.services.propertycache import cachedproperty
from lp.services.webapp import (
@@ -517,9 +518,17 @@
class DiffRenderingMixin:
"""A mixin class for handling diff text."""
+ @property
+ def diff_available(self):
+ """Is the preview diff available from the librarian?"""
+ if getattr(self, '_diff_available', None) is None:
+ self.preview_diff_text
+ return self._diff_available
+
@cachedproperty
def preview_diff_text(self):
"""Return a (hopefully) intelligently encoded review diff."""
+ self._diff_available = True
preview_diff = self.preview_diff
if preview_diff is None:
return None
@@ -527,6 +536,9 @@
diff = preview_diff.text.decode('utf-8')
except UnicodeDecodeError:
diff = preview_diff.text.decode('windows-1252', 'replace')
+ except LibrarianServerError:
+ self._diff_available = False
+ diff = ''
# Strip off the trailing carriage returns.
return diff.rstrip('\n')
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2012-10-08 07:15:07 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2012-11-29 20:30:28 +0000
@@ -57,6 +57,7 @@
PersonVisibility,
TeamMembershipPolicy,
)
+from lp.services.librarian.interfaces.client import LibrarianServerError
from lp.services.messages.model.message import MessageSet
from lp.services.webapp import canonical_url
from lp.services.webapp.interfaces import (
@@ -69,6 +70,7 @@
BrowserTestCase,
feature_flags,
login_person,
+ monkey_patch,
person_logged_in,
set_feature_flag,
TestCaseWithFactory,
@@ -865,6 +867,7 @@
view = create_initialized_view(self.bmp, '+index')
self.assertEqual(diff_bytes.decode('utf-8'),
view.preview_diff_text)
+ self.assertTrue(view.diff_available)
def test_preview_diff_all_chars(self):
"""preview_diff should work on diffs containing all possible bytes."""
@@ -875,6 +878,27 @@
view = create_initialized_view(self.bmp, '+index')
self.assertEqual(diff_bytes.decode('windows-1252', 'replace'),
view.preview_diff_text)
+ self.assertTrue(view.diff_available)
+
+ def test_preview_diff_timeout(self):
+ # The preview_diff will recover from a timeout set to get the
+ # librarian content.
+ text = ''.join(chr(x) for x in range(255))
+ diff_bytes = ''.join(unified_diff('', text))
+ self.setPreviewDiff(diff_bytes)
+ transaction.commit()
+
+ def fake_open(*args):
+ raise LibrarianServerError
+
+ lfa = removeSecurityProxy(self.bmp.preview_diff).diff.diff_text
+ with monkey_patch(lfa, open=fake_open):
+ view = view = create_initialized_view(
+ self.bmp.preview_diff, '+diff')
+ self.assertEqual('', view.preview_diff_text)
+ self.assertFalse(view.diff_available)
+ markup = view()
+ self.assertIn('The diff is not available at this time.', markup)
def setPreviewDiff(self, preview_diff_bytes):
preview_diff = PreviewDiff.create(
=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py 2012-10-02 01:25:48 +0000
+++ lib/lp/code/model/diff.py 2012-11-29 20:30:28 +0000
@@ -55,6 +55,7 @@
cachedproperty,
get_property_cache,
)
+from lp.services.webapp.adapter import get_request_remaining_seconds
class Diff(SQLBase):
@@ -94,12 +95,32 @@
if self.diff_text is None:
return ''
else:
- self.diff_text.open()
+ self.diff_text.open(self._getDiffTimeout())
try:
return self.diff_text.read(config.diff.max_read_size)
finally:
self.diff_text.close()
+ def _getDiffTimeout(self):
+ """Return the seconds allocated to get the diff from the librarian.
+
+ the value will be Non for scripts, 2 for the webapp, or if thre is
+ little request time left, the number will be smaller or equal to
+ the remaining request time.
+ """
+ remaining = get_request_remaining_seconds()
+ if remaining is None:
+ return None
+ elif remaining > 2.0:
+ # The maximum permitted time for webapp requests.
+ return 2.0
+ elif remaining > 0.01:
+ # Shave off 1 hundreth of a second off so that the call site
+ # has a chance to recover.
+ return remaining - 0.01
+ else:
+ return remaining
+
@property
def oversized(self):
# If the size of the content of the librarian file is over the
=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_diff.py 2012-11-29 20:30:28 +0000
@@ -34,6 +34,7 @@
from lp.testing import (
login,
login_person,
+ monkey_patch,
TestCaseWithFactory,
)
from lp.testing.layers import (
@@ -172,6 +173,29 @@
diff = self._create_diff(content)
self.assertTrue(diff.oversized)
+ def test_getDiffTimeout(self):
+ # The time permitted to get the diff from the librarian may be None,
+ # or 2. If there is not 2 seconds left in the request, the number will
+ # will 0.01 smaller or the actual remaining time.
+ content = ''.join(unified_diff('', "1234567890" * 10))
+ diff = self._create_diff(content)
+ value = None
+
+ def fake():
+ return value
+
+ from lp.code.model import diff as diff_module
+ with monkey_patch(diff_module, get_request_remaining_seconds=fake):
+ self.assertIs(None, diff._getDiffTimeout())
+ value = 3.1
+ self.assertEqual(2.0, diff._getDiffTimeout())
+ value = 1.11
+ self.assertEqual(1.1, diff._getDiffTimeout())
+ value = 0.11
+ self.assertEqual(0.1, diff._getDiffTimeout())
+ value = 0.01
+ self.assertEqual(0.01, diff._getDiffTimeout())
+
class TestDiffInScripts(DiffTestCase):
=== modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
--- lib/lp/code/templates/branchmergeproposal-diff.pt 2010-02-17 19:10:51 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff.pt 2012-11-29 20:30:28 +0000
@@ -17,6 +17,10 @@
</div>
<div class="boardCommentBody attachmentBody">
<tal:diff replace="structure view/preview_diff_text/fmt:diff" />
+ <tal:diff-not-available condition="not: view/diff_available">
+ The diff is not available at this time. You can reload the
+ page or download it.
+ </tal:diff-not-available>
</div>
<div class="boardCommentFooter"
tal:condition="view/diff_oversized">
=== modified file 'lib/lp/scripts/utilities/importfascist.py'
--- lib/lp/scripts/utilities/importfascist.py 2012-01-19 17:23:59 +0000
+++ lib/lp/scripts/utilities/importfascist.py 2012-11-29 20:30:28 +0000
@@ -23,35 +23,26 @@
permitted_database_imports = text_lines_to_set("""
- canonical.archivepublisher.deathrow
- canonical.archivepublisher.domination
- canonical.archivepublisher.ftparchive
- canonical.archivepublisher.publishing
+ doctest
+ lp.archiveuploader.nascentuploadfile
+ lp.code.feed.branch
lp.codehosting.inmemory
- canonical.launchpad.browser.branchlisting
- lp.code.browser.branchlisting
+ lp.codehosting.scanner.bzrsync
+ lp.registry.interfaces.person
+ lp.scripts.garbo
lp.services.librarian.browser
- canonical.launchpad.feed.branch
- lp.code.feed.branch
- lp.scripts.garbo
- lp.bugs.vocabularies
- lp.registry.interfaces.person
- lp.registry.vocabularies
- lp.services.worlddata.vocabularies
- lp.soyuz.vocabularies
- lp.translations.vocabularies
lp.services.librarian.client
lp.services.librarianserver.db
- doctest
+ lp.systemhomes
+ lp.translations.translationmerger
""")
-
warned_database_imports = text_lines_to_set("""
+ lp.registry.browser.distroseries
+ lp.soyuz.scripts.gina.handlers
lp.soyuz.scripts.obsolete_distroseries
- lp.soyuz.scripts.gina.handlers
- lp.registry.browser.distroseries
+ lp.systemhomes
lp.translations.scripts.po_import
- lp.systemhomes
""")
@@ -76,24 +67,51 @@
}
-def database_import_allowed_into(module_path):
- """Return True if database code is allowed to be imported into the given
- module path. Otherwise, returns False.
+def database_import_allowed_into(module_path, name):
+ """Return True if model code can be imported into the module path.
It is allowed if:
- The import was made with the __import__ hook.
- - The importer is from within canonical.launchpad.database.
- The importer is a 'test' module.
- The importer is in the set of permitted_database_imports.
- The importer is within a model module or package.
-
- Note that being in the set of warned_database_imports does not make
- the import allowed.
-
+ - The import is recognised to be dubious, but not a priority to fix.
"""
+ if name == 'lp.registry.model.personroles':
+ return True
+ dubious = [
+ 'lp.blueprints.browser.sprint',
+ 'lp.bugs.browser.bug',
+ 'lp.bugs.browser.bugalsoaffects',
+ 'lp.bugs.browser.bugsubscription',
+ 'lp.bugs.browser.bugtarget',
+ 'lp.bugs.browser.bugtask',
+ 'lp.bugs.browser.person',
+ 'lp.code.browser.branchlisting',
+ 'lp.code.browser.sourcepackagerecipe',
+ 'lp.registry.browser.distroseries',
+ 'lp.registry.browser.distroseriesdifference',
+ 'lp.registry.browser.milestone',
+ 'lp.registry.browser.pillar',
+ 'lp.registry.browser.person',
+ 'lp.registry.browser.project',
+ 'lp.registry.browser.sourcepackage',
+ 'lp.soyuz.browser.archive',
+ 'lp.soyuz.browser.builder',
+ 'lp.soyuz.browser.queue',
+ 'lp.translations.browser.potemplate',
+ 'lp.translations.browser.serieslanguage',
+ 'lp.translations.browser.sourcepackage',
+ 'lp.translations.browser.translationlinksaggregator',
+ 'lp.translations.browser.translationtemplatesbuild',
+ ]
+ safe_parts = set(
+ ['model', 'scripts', 'adapters', 'vocabularies', 'vocabulary',
+ 'security', 'services', 'subscribers', 'utilities'])
+ module_parts = set(module_path.split('.'))
if (module_path == '__import__ hook' or
- module_path.startswith('canonical.launchpad.database') or
- '.model' in module_path or
+ safe_parts & module_parts or
+ module_path in dubious or
is_test_module(module_path)):
return True
return module_path in permitted_database_imports
@@ -105,9 +123,7 @@
Otherwise returns False.
"""
name_splitted = module_path.split('.')
- return ('tests' in name_splitted or
- 'ftests' in name_splitted or
- 'testing' in name_splitted)
+ return ('tests' in name_splitted or 'testing' in name_splitted)
class attrsgetter:
@@ -166,9 +182,7 @@
class NotFoundPolicyViolation(JackbootError):
- """import of zope.exceptions.NotFoundError into
- canonical.launchpad.database.
- """
+ """import of zope.exceptions.NotFoundError into lp models modules."""
def __init__(self, import_into):
JackbootError.__init__(self, import_into, '')
@@ -223,14 +237,13 @@
import_into = '__import__ hook'
# Check the "NotFoundError" policy.
- if (import_into.startswith('canonical.launchpad.database') and
- name == 'zope.exceptions'):
+ if ('.model.' in import_into and name == 'zope.exceptions'):
if fromlist and 'NotFoundError' in fromlist:
raise NotFoundPolicyViolation(import_into)
# Check the database import policy.
- if (name.startswith(database_root) and
- not database_import_allowed_into(import_into)):
+ if ('.model.' in name
+ and not database_import_allowed_into(import_into, name)):
error = DatabaseImportPolicyViolation(import_into, name)
naughty_imports.add(error)
# Raise an error except in the case of browser.traversers.
@@ -240,8 +253,7 @@
raise error
# Check the import from __all__ policy.
- if fromlist is not None and (
- import_into.startswith('canonical') or import_into.startswith('lp')):
+ if fromlist is not None and import_into.startswith('lp'):
# We only want to warn about "from foo import bar" violations in our
# own code.
fromlist = list(fromlist)
=== modified file 'lib/lp/services/webapp/adapter.py'
--- lib/lp/services/webapp/adapter.py 2012-11-14 09:27:39 +0000
+++ lib/lp/services/webapp/adapter.py 2012-11-29 20:30:28 +0000
@@ -89,6 +89,7 @@
'RequestExpired',
'set_request_started',
'clear_request_started',
+ 'get_request_remaining_seconds',
'get_request_statements',
'get_request_start_time',
'get_request_duration',
Follow ups
