Merge lp:~stub/launchpad/bug-675562-readonly-violation into lp:launchpad

[Stuart Bishop]

[r=jtv][ui=none][bug=675562] Fail requests for master stores if we are in read-only mode, no matter what database policy is currently installed.

Fail requests for master stores if we are in read-only mode, no matter what database policy is currently installed.

[Jeroen T. Vermeulen (jtv)]

Nice branch. Thanks for the drive-by cleanups, too.

Only found some very small things to nag about:
 * May be better to test against the absence of IMasterStore (as well as, or in place of) the presence of ISlaveStore. In case the design changes.
 * In lib/canonical/launchpad/webapp/adapter.py, our coding standard says the "if/elif" should have an "else" as well.

Jeroen

review: Approve

=== modified file 'lib/canonical/launchpad/doc/db-policy.txt'
--- lib/canonical/launchpad/doc/db-policy.txt	2010-02-22 12:16:02 +0000
+++ lib/canonical/launchpad/doc/db-policy.txt	2010-11-17 11:08:00 +0000
@@ -124,3 +124,29 @@
     >>> IMasterObject(ro_janitor) is writable_janitor
     True
 
+Read-Only Mode
+--------------
+
+During database outages, we run in read-only mode. In this mode, no
+matter what database policy is currently installed, explicit requests
+for a master store fail and the default store is always the slave.
+
+    >>> from canonical.launchpad.tests.readonly import read_only_mode
+    >>> from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
+    >>> from contextlib import nested
+
+    >>> with nested(read_only_mode(), MasterDatabasePolicy()):
+    ...     default_store = IStore(Person)
+    ...     IMasterStore.providedBy(default_store)
+    False
+
+    >>> with nested(read_only_mode(), MasterDatabasePolicy()):
+    ...     slave_store = ISlaveStore(Person)
+    ...     IMasterStore.providedBy(slave_store)
+    False
+
+    >>> with nested(read_only_mode(), MasterDatabasePolicy()):
+    ...     master_store = IMasterStore(Person)
+    Traceback (most recent call last):
+    ...
+    ReadOnlyModeDisallowedStore: ('main', 'master')

=== modified file 'lib/canonical/launchpad/tests/readonly.py'
--- lib/canonical/launchpad/tests/readonly.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/tests/readonly.py	2010-11-17 11:08:00 +0000
@@ -7,15 +7,20 @@
 __metaclass__ = type
 __all__ = [
     'touch_read_only_file',
+    'read_only_mode',
     'remove_read_only_file',
     ]
 
+from contextlib import contextmanager
 import os
 
+from lazr.restful.utils import get_current_browser_request
+
 from canonical.launchpad.readonly import (
     is_read_only,
     read_only_file_exists,
     read_only_file_path,
+    READ_ONLY_MODE_ANNOTATIONS_KEY,
     )
 
 
@@ -37,7 +42,7 @@
 def remove_read_only_file(assert_mode_switch=True):
     """Remove the file named read-only.txt from the root of the tree.
 
-    May also assert that the mode switch actually happened (i.e. not 
+    May also assert that the mode switch actually happened (i.e. not
     is_read_only()). This assertion has to be conditional because some tests
     will use this during the processing of a request, when a mode change can't
     happen (i.e. is_read_only() will still return True during that request's
@@ -48,3 +53,15 @@
         # Assert that the switch succeeded and make sure the mode change is
         # logged.
         assert not is_read_only(), "Switching to read-write failed."
+
+
+@contextmanager
+def read_only_mode(flag=True):
+    request = get_current_browser_request()
+    current = request.annotations[READ_ONLY_MODE_ANNOTATIONS_KEY]
+    request.annotations[READ_ONLY_MODE_ANNOTATIONS_KEY] = flag
+    try:
+        assert is_read_only() == flag, 'Failed to set read-only mode'
+        yield
+    finally:
+        request.annotations[READ_ONLY_MODE_ANNOTATIONS_KEY] = current

=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py	2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/webapp/adapter.py	2010-11-17 11:08:00 +0000
@@ -60,6 +60,7 @@
     IStoreSelector,
     MAIN_STORE,
     MASTER_FLAVOR,
+    ReadOnlyModeDisallowedStore,
     ReadOnlyModeViolation,
     SLAVE_FLAVOR,
     )
@@ -129,6 +130,7 @@
 
 
 class CommitLogger:
+
     def __init__(self, txn):
         self.txn = txn
 
@@ -261,15 +263,16 @@
 def set_permit_timeout_from_features(enabled):
     """Control request timeouts being obtained from the 'hard_timeout' flag.
 
-    Until we've fully setup a page to render - routed the request to the right
-    object, setup a participation etc, feature flags cannot be completely used;
-    and because doing feature flag lookups will trigger DB access, attempting
-    to do a DB lookup will cause a nested DB lookup (the one being done, and
-    the flags lookup). To resolve all of this, timeouts start as a config file
-    only setting, and are then overridden once the request is ready to execute.
+    Until we've fully setup a page to render - routed the request to the
+    right object, setup a participation etc, feature flags cannot be
+    completely used; and because doing feature flag lookups will trigger
+    DB access, attempting to do a DB lookup will cause a nested DB
+    lookup (the one being done, and the flags lookup). To resolve all of
+    this, timeouts start as a config file only setting, and are then
+    overridden once the request is ready to execute.
 
-    :param enabled: If True permit looking up request timeouts in feature
-        flags.
+    :param enabled: If True permit looking up request timeouts in
+        feature flags.
     """
     _local._permit_feature_timeout = enabled
 
@@ -350,6 +353,7 @@
 
 _main_thread_id = None
 
+
 def break_main_thread_db_access(*ignored):
     """Ensure that Storm connections are not made in the main thread.
 
@@ -390,6 +394,7 @@
 
 class ReadOnlyModeConnection(PostgresConnection):
     """storm.database.Connection for read-only mode Launchpad."""
+
     def execute(self, statement, params=None, noresult=False):
         """See storm.database.Connection."""
         try:
@@ -550,13 +555,14 @@
             # XXX: This code does not belong here - see bug=636804.
             # Robert Collins 20100913.
             OpStats.stats['timeouts'] += 1
-            # XXX bug=636801 Robert Colins 20100914 This is duplicated from the
-            # statement tracer, because the tracers are not arranged in a stack
-            # rather a queue: the done-code in the statement tracer never runs.
+            # XXX bug=636801 Robert Colins 20100914 This is duplicated
+            # from the statement tracer, because the tracers are not
+            # arranged in a stack rather a queue: the done-code in the
+            # statement tracer never runs.
             action = getattr(connection, '_lp_statement_action', None)
             if action is not None:
-                # action may be None if the tracer was installed after the
-                # statement was submitted.
+                # action may be None if the tracer was installed after
+                # the statement was submitted.
                 action.finish()
             info = sys.exc_info()
             transaction.doom()
@@ -666,6 +672,20 @@
     @staticmethod
     def get(name, flavor):
         """See `IStoreSelector`."""
+        if is_read_only():
+            # If we are in read-only mode, override the default to the
+            # slave no matter what the existing policy says (it might
+            # work), and raise an exception if the master was explicitly
+            # requested. Most of the time, this doesn't matter as when
+            # we are in read-only mode we have a suitable database
+            # policy installed. However, code can override the policy so
+            # we still need to catch disallowed requests here.
+            if flavor == DEFAULT_FLAVOR:
+                flavor = SLAVE_FLAVOR
+            elif flavor == MASTER_FLAVOR:
+                raise ReadOnlyModeDisallowedStore(name, flavor)
+            else:
+                pass
         db_policy = StoreSelector.get_current()
         if db_policy is None:
             db_policy = MasterDatabasePolicy(None)

=== modified file 'lib/canonical/launchpad/webapp/dbpolicy.py'
--- lib/canonical/launchpad/webapp/dbpolicy.py	2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/webapp/dbpolicy.py	2010-11-17 11:08:00 +0000
@@ -149,6 +149,7 @@
 
 class DatabaseBlockedPolicy(BaseDatabasePolicy):
     """`IDatabasePolicy` that blocks all access to the database."""
+
     def getStore(self, name, flavor):
         """Raises `DisallowedStore`. No Database access is allowed."""
         raise DisallowedStore(name, flavor)
@@ -180,6 +181,7 @@
     This policy is used for Feeds requests and other always-read only request.
     """
     default_flavor = SLAVE_FLAVOR
+
     def getStore(self, name, flavor):
         """See `IDatabasePolicy`."""
         if flavor == MASTER_FLAVOR:
@@ -210,6 +212,7 @@
 
     Selects the DEFAULT_FLAVOR based on the request.
     """
+
     def __init__(self, request):
         # The super constructor is a no-op.
         # pylint: disable-msg=W0231
@@ -364,6 +367,7 @@
 
     Access to all master Stores is blocked.
     """
+
     def getStore(self, name, flavor):
         """See `IDatabasePolicy`.
 
@@ -383,6 +387,7 @@
 
 class WhichDbView(LaunchpadView):
     "A page that reports which database is being used by default."
+
     def render(self):
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         dbname = store.execute("SELECT current_database()").get_one()[0]