Merge lp:~salgado/launchpad/easier-read-only-switch into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

Today, switching an app server to run in read-only mode means putting
new configs in place and restarting the server using a different
LPCONFIG env var. That's rather painful to LOSAs, who need to do it for
all app servers before a roll out, so we'll make that easier.

== Proposed fix ==

From now on, to turn read-only mode on, one just need to create a
read-only.txt file under the root of our tree. No more config changes and/or
server restarts.

The [database] config section doesn't have main_master/slave variables
anymore. These are now properties on DatabaseConfig, which return
ro_main_master/slave or rw_main_master/slave, depending on the mode we are on
(i.e. what is returned by IIsReadOnly.isReadOnly()).

When .isReadOnly() is called as part of processing a request, we cache its
return value in the request so that it doesn't need to hit the filesystem in
further calls.

At the beginning of a request, we check the databases to which the main_master
and main_slave stores are connected to. If they differ from
dbconfig.main_master/slave, we remove these stores from ZStorm, forcing them
to be created again the next time they are needed. When they're created again
they get a new connection, which uses dbconfig.main_master/slave and thus will
go to the correct db.

After removing the stores from ZStorm, this is what happens when the store is
needed again:

   - ZStorm.get('main-master', 'launchpad:main-master') is called
   - ZStorm finds a Database object for that uri
   - ZStorm instantiates a new Store object, passing the existing Database
   - Store calls Database.connect(), which instantiates and returns a
     Connection
   - The Connection's __init__() calls Database.raw_connect(), which finally
     reads dbconfig.main_master/slave and creates a new psycopg2 connection
     using that.

The above only works because our custom Database class (LaunchpadDatabase)
sets self._dsn inside raw_connect(), instead of inside __init__() (like its
parent class does). That's needed because the Database objects are reused
across threads/stores by ZStorm.

Tests
=====

- pagetests/standalone/xx-read-only-mode.txt works as the integration test
- test_readonly.py
- webapp/tests/test_publication.py

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/database/sqlbase.py
  configs/test-playground/launchpad-lazr.conf
  lib/canonical/librarian/client.py
  lib/canonical/launchpad/webapp/publication.py
  lib/canonical/config/schema-lazr.conf
  lib/canonical/launchpad/doc/canonical-config.txt
  lib/canonical/launchpad/webapp/authorization.py
  lib/canonical/launchpad/webapp/tests/test_publication.py
  lib/canonical/launchpad/readonly.py
  lib/canonical/launchpad/pagetests/standalone/xx-read-only-mode.txt
  lib/canonical/testing/ftests/test_mockdb.py
  lib/canonical/launchpad/pagetests/standalone/xx-opstats.txt
  lib/canonical/launchpad/webapp/login.py
  lib/lp/translations/model/pofile.py
  BRANCH.TODO
  lib/canonical/launchpad/tests/test_readonly.py
  lib/canonical/launchpad/webapp/configure.zcml
  lib/canon...

[Gary Poster (gary)]

On IRC, I asked for a comment explaining use of _template suffix for readonly dbs in config.

Correcting this would be nice:

lib/canonical/launchpad/webapp/publication.py
    573: [E1002, LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super on an old style class

DatabaseConfig.main_master: zcml issue we discussed...

I'm eager to see QA on staging from SAs. Have you discussed with LOSAs how we might test this on staging? I wonder if this would warrant a CP.

I asked you about the first two assertions in TestDatabaseConfig.test_overlay, because I didn't really see the point of them. You said that they look up the values in the wrong config section, which doesn't seem all that compelling to me, but you also said that you had simply moved those from the class' docstring to the new location. I won't worry about them, then.

The following looks like it deserves a bug.

397 + # XXX: Instead of all this we could probably just remove the
398 + # store from ZStorm (zstorm.remove(store)).

Twice you assert that we must use a rw db in a given context. Would it be relatively easy and quick to hint as to why, or do you think that it is quite obvious in context of the full code, rather than the diff?

What would you think of _touch_read_only_file and _remove_read_only_file losing their initial underline and moving to a "testing" module somewhere? They seem to be imported a lot for tests, so they really are a testing API AFAICT, and should be encouraged for (only) that purpose.

Shouldn't lib/canonical/launchpad/readonly.py have an __all__?

In this line from lib/canonical/launchpad/readonly.py...

603 +named read-only.txt under the root of the Launchpad tree.

...I suggest s/under/in/.

In lib/canonical/launchpad/readonly.py, I suggest using os.path.absdir to calculate the ``root`` path just for cleanliness. (There's probably a way to get the path from an environmental var that buildout sets so you don't have to do path funny business, but there's precedent for the funny business, so I'm OK with what you have.)

lib/canonical/launchpad/readonly.py: _currently_in_read_only: is that really part of the utility's interface? It seems like it is an implementation detail.

lib/canonical/launchpad/readonly.py: Just for Python 2.5 fun, why don't you use ``from __future__ import with_statement`` and then ``with self._lock`` instead of the whole try/except acquire/release dance?

I'm submitting these notes now since I am breaking for lunch. More when I return.

[Guilherme Salgado (salgado)]

There was only one test failing after my changes, and the fix for it is below:

=== modified file 'lib/canonical/launchpad/webapp/tests/test_authorization.py'
--- lib/canonical/launchpad/webapp/tests/test_authorization.py 2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/tests/test_authorization.py 2010-01-13 18:30:34 +0000
@@ -18,6 +18,7 @@
 from canonical.lazr.interfaces import IObjectPrivacy

 from canonical.launchpad.interfaces.account import IAccount
+from canonical.launchpad.readonly import IIsReadOnly
 from canonical.launchpad.security import AuthorizationBase
 from canonical.launchpad.webapp.authentication import LaunchpadPrincipal
 from canonical.launchpad.webapp.authorization import (
@@ -129,6 +130,11 @@
         pass

+class FakeIsReadOnlyUtility:
+ def isReadOnly(self):
+ return False
+
+
 class TestCheckPermissionCaching(CleanUp, unittest.TestCase):
     """Test the caching done by `LaunchpadSecurityPolicy.checkPermission`."""

@@ -137,6 +143,9 @@
         super(TestCheckPermissionCaching, self).setUp()
         self.factory = ObjectFactory()
         ztapi.provideUtility(IStoreSelector, FakeStoreSelector)
+ # LaunchpadSecurityPolicy.checkPermission() needs an IIsReadOnly
+ # utility, so we provide a fake one to please it.
+ ztapi.provideUtility(IIsReadOnly, FakeIsReadOnlyUtility())

     def makeRequest(self):
         """Construct an arbitrary `LaunchpadBrowserRequest` object."""

[Guilherme Salgado (salgado)]

On Wed, 2010-01-13 at 18:12 +0000, Gary Poster wrote:
> On IRC, I asked for a comment explaining use of _template suffix for readonly dbs in config.
>
> Correcting this would be nice:
>
> lib/canonical/launchpad/webapp/publication.py
> 573: [E1002,
> LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super
> on an old style class

It actually is a new-style class, I think thanks to the __metaclass__ =
type line we have there. I don't know why pylint is confused here, but
I could suppress that error.

>
> DatabaseConfig.main_master: zcml issue we discussed...
>
> I'm eager to see QA on staging from SAs. Have you discussed with
> LOSAs how we might test this on staging? I wonder if this would
> warrant a CP.

No, I haven't talked to them about that yet, but I'm guessing we'll need
a read-only database to be able to QA this properly.

We could CP it a few days before the roll out. That way we get some
real-world testing (on edge) of the changes I did to webapp/ code and we
can still take advantage of the easier switching during the roll out
itself.

>
> I asked you about the first two assertions in
> TestDatabaseConfig.test_overlay, because I didn't really see the point
> of them. You said that they look up the values in the wrong config
> section, which doesn't seem all that compelling to me, but you also
> said that you had simply moved those from the class' docstring to the
> new location. I won't worry about them, then.
>
> The following looks like it deserves a bug.
>
> 397 + # XXX: Instead of all this we could probably just
> remove the
> 398 + # store from ZStorm (zstorm.remove(store)).

I'm going to do the change there (in another branch) and run the test
suite again. If it succeeds, great, otherwise I'll revert it and the
bug probably won't be needed. For now I'll just remove the XXX from
here.

>
> Twice you assert that we must use a rw db in a given context. Would
> it be relatively easy and quick to hint as to why, or do you think
> that it is quite obvious in context of the full code, rather than the
> diff?

In the case of database/sqlbase.py it may not be, so I changed the
comment. The other is in librarian/client.py, in the method to upload a
new file, so I don't think a comment change is needed there.

>
> What would you think of _touch_read_only_file and
> _remove_read_only_file losing their initial underline and moving to a
> "testing" module somewhere? They seem to be imported a lot for tests,
> so they really are a testing API AFAICT, and should be encouraged for
> (only) that purpose.

That's correct, they are a testing API and as such I'll move them to a
more appropriate place.

>
> Shouldn't lib/canonical/launchpad/readonly.py have an __all__?

Yes, that and a __metaclass__ one.

>
> In this line from lib/canonical/launchpad/readonly.py...
>
> 603 +named read-only.txt under the root of the Launchpad tree.
>
> ...I suggest s/under/in/.

Sure thing; changed.

>
> In lib/canonical/launchpad/readonly.py, I suggest using os.path.absdir
> to calculate the ``root`` path just for cleanliness. (There's
> probably a way to get the path from an environmental var...

[Gary Poster (gary)]

Continuing the review notes from the previous comment...

You are using ztapi because other code is doing it (``ztapi.provideUtility(IIsReadOnly, FakeIsReadOnlyUtility())`` in test_authorization.py). ztapi is deprecated in favor of zope.component; it now has all the necessary convenience functions (although the order of arguments for zope.component.provideUtility is reversed IIRC). I'd like to see new code use the "one way to do it".

In lib/canonical/launchpad/readonly.py...

686 + # Only call get_current_browser_request() when we have an interaction,
687 + # or else it will choke.
688 + if queryInteraction() is not None:
689 + request = get_current_browser_request()

...this is a look-before-you-leap approach. Python style guides generally prefers try-except, and I do too. :-)

I question have IIsReadOnly as a utility. Why don't we just have this as a function? The ``_currently_in_read_only`` value (which I said I believe is an implementation detail above) would be a module global--and you already have a mutex to handle modifying that well anyway. Then people would import the function rather than the interface, and things would be more direct. The only downside I can see is that it might be a bit harder to change the behavior of the isReadOnly function for tests--but do you really need to? So in sum, making this a utility doesn't buy us anything AFAICS, and adds extra indirection when a simple function would do.

Do I need to know why/when we use DatabaseBlockedPolicy?

OK, saving these comments; back soon. :-)

[Guilherme Salgado (salgado)]

On Wed, 2010-01-13 at 19:54 +0000, Gary Poster wrote:
> Continuing the review notes from the previous comment...
>
> You are using ztapi because other code is doing it
> (``ztapi.provideUtility(IIsReadOnly, FakeIsReadOnlyUtility())`` in
> test_authorization.py). ztapi is deprecated in favor of
> zope.component; it now has all the necessary convenience functions
> (although the order of arguments for zope.component.provideUtility is
> reversed IIRC). I'd like to see new code use the "one way to do it".

I didn't know about that and indeed had just copied and pasted the
existing code. That test no longer uses ztapi now.

>
> In lib/canonical/launchpad/readonly.py...
>
> 686 + # Only call get_current_browser_request() when we have an
> interaction,
> 687 + # or else it will choke.
> 688 + if queryInteraction() is not None:
> 689 + request = get_current_browser_request()
>
> ...this is a look-before-you-leap approach. Python style guides
> generally prefers try-except, and I do too. :-)

I didn't do that because I think get_current_browser_request() should
return None and not let an AttributeError through when there's no
interaction. I'd fix the function myself, but it's in lazr.restful so I
wouldn't benefit instantly.

I guess the best would be to turn my comment into an XXX and file a bug
against lazr.restful?

>
> I question have IIsReadOnly as a utility. Why don't we just have this
> as a function? The ``_currently_in_read_only`` value (which I said I
> believe is an implementation detail above) would be a module
> global--and you already have a mutex to handle modifying that well
> anyway. Then people would import the function rather than the
> interface, and things would be more direct. The only downside I can
> see is that it might be a bit harder to change the behavior of the
> isReadOnly function for tests--but do you really need to? So in sum,
> making this a utility doesn't buy us anything AFAICS, and adds extra
> indirection when a simple function would do.

I think that'd work, and I don't need to change its behaviour for tests,
so I'll give this a try tomorrow morning.

>
> Do I need to know why/when we use DatabaseBlockedPolicy?

Oh, right, that's the policy used for the +opstas page, to make sure
nothing tries to use the database during a request for that page.

>
> OK, saving these comments; back soon. :-)

--
Guilherme Salgado <email address hidden>

[Gary Poster (gary)]

test_no_mode_changes: Would it be worthwhile to check that we have the same connection object? It is black box, but it might be nice to verify that we don't trash the connection unnecessarily.

Now relying to your replies:

> > Correcting this would be nice:
> >
> > lib/canonical/launchpad/webapp/publication.py
> > 573: [E1002,
> > LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super
> > on an old style class
>
> It actually is a new-style class, I think thanks to the __metaclass__ =
> type line we have there. I don't know why pylint is confused here, but
> I could suppress that error.

Yes, let's go ahead and do that, please.

In regards to your comments on QA and a CP, I have a call on Friday with Tom and James. I'll talk to them about that then. I'll invite you on the call if you are available.

> > ...this is a look-before-you-leap approach. Python style guides
> > generally prefers try-except, and I do too. :-)
>
> I didn't do that because I think get_current_browser_request() should
> return None and not let an AttributeError through when there's no
> interaction. I'd fix the function myself, but it's in lazr.restful so I
> wouldn't benefit instantly.

I see.

> I guess the best would be to turn my comment into an XXX and file a bug
> against lazr.restful?

+1.

The rest looks good. Thank you very much for the great work.

I'm going to approve this, but ask Tom Haddon for a review so that we have a discussion of QA before we merge.

Gary

review: Approve

[Gary Poster (gary)]

Tom, I asked you for a review so that we could discuss QA.

I think we're delivering exactly what we discussed, but I'd also like verification of that as well. As Salgado says, if you add a read-only.txt to the top the tree, or remove it, the application should notice as soon as it processes a new request. It will then acknowledge the change in the main launchpad.log. Because our requests time out in about 15 seconds, webapp machines should be ready to switch easily within a minute. This does not affect long running processes, which would presumably need to be stopped when switching to readonly, and restarted when resuming (as usual?).

[Tom Haddon (mthaddon)]

This sounds good in terms of delivering what was specified, and we're happy to deal with long running processes by killing them if we need to when switching from read-write to read-only.

In terms of QA, I think we'd want to start by merging this branch as is onto staging, creating the read-only.txt file and then trying to perform some changes. Just let me know when the branch is ready.

We'll also want to prepare the changes to lp-production-configs and canonical-identity-provider at the same time, from what I understand.

Thanks, Tom

[Guilherme Salgado (salgado)]

On Wed, 2010-01-13 at 21:30 +0000, Gary Poster wrote:
> Review: Approve
> test_no_mode_changes: Would it be worthwhile to check that we have the
> same connection object? It is black box, but it might be nice to
> verify that we don't trash the connection unnecessarily.

It won't hurt to do this extra check there, so, sure.

>
> Now relying to your replies:
>
> > > Correcting this would be nice:
> > >
> > > lib/canonical/launchpad/webapp/publication.py
> > > 573: [E1002,
> > > LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super
> > > on an old style class
> >
> > It actually is a new-style class, I think thanks to the __metaclass__ =
> > type line we have there. I don't know why pylint is confused here, but
> > I could suppress that error.
>
> Yes, let's go ahead and do that, please.

Done.

>
> In regards to your comments on QA and a CP, I have a call on Friday
> with Tom and James. I'll talk to them about that then. I'll invite
> you on the call if you are available.

Ok

>
> > > ...this is a look-before-you-leap approach. Python style guides
> > > generally prefers try-except, and I do too. :-)
> >
> > I didn't do that because I think get_current_browser_request() should
> > return None and not let an AttributeError through when there's no
> > interaction. I'd fix the function myself, but it's in lazr.restful so I
> > wouldn't benefit instantly.
>
> I see.
>
> > I guess the best would be to turn my comment into an XXX and file a bug
> > against lazr.restful?
>
> +1.

https://bugs.edge.launchpad.net/lazr.restful/+bug/507447

>
> The rest looks good. Thank you very much for the great work.
>
> I'm going to approve this, but ask Tom Haddon for a review so that we
> have a discussion of QA before we merge.
>
> Gary

--
Guilherme Salgado <email address hidden>

[Guilherme Salgado (salgado)]

http://paste.ubuntu.com/356558/ is how I got rid of the IIsReadOnly utility by using a function and module globals.

[Tom Haddon (mthaddon)]

I've tested this on staging and can confirm it works as expected. So, fwiw, +1 from me :)

review: Approve

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2009-12-21 03:18:24 +0000
+++ configs/development/launchpad-lazr.conf	2010-01-18 11:28:17 +0000
@@ -91,8 +91,12 @@
 
 
 [database]
-main_master: dbname=launchpad_dev
-main_slave:  dbname=launchpad_dev
+rw_main_master: dbname=launchpad_dev
+rw_main_slave:  dbname=launchpad_dev
+# Use our _template databases here just so that we have different values from
+# the rw_* configs.
+ro_main_master: dbname=launchpad_dev_template
+ro_main_slave:  dbname=launchpad_dev_template
 auth_master: dbname=launchpad_dev
 auth_slave:  dbname=launchpad_dev
 
@@ -132,8 +136,6 @@
 bzr_imports_root_url: file:///tmp/bazaar-branches
 geoip_database: /usr/share/GeoIP/GeoLiteCity.dat
 geonames_identity: lpdev
-# Set to True to test read-only mode.
-read_only: False
 storm_cache: generational
 storm_cache_size: 100
 

=== modified file 'configs/replicated-development/launchpad-lazr.conf'
--- configs/replicated-development/launchpad-lazr.conf	2008-10-14 11:10:35 +0000
+++ configs/replicated-development/launchpad-lazr.conf	2010-01-18 11:28:17 +0000
@@ -6,8 +6,10 @@
 extends: ../development/launchpad-lazr.conf
 
 [database]
-main_master: dbname=launchpad_dev
-main_slave: dbname=launchpad_dev_slave
+rw_main_master: dbname=launchpad_dev
+rw_main_slave: dbname=launchpad_dev_slave
+ro_main_master: dbname=launchpad_dev
+ro_main_slave: dbname=launchpad_dev_slave
 auth_master: dbname=launchpad_dev
 auth_slave: dbname=launchpad_dev_slave
 

=== modified file 'configs/test-playground/launchpad-lazr.conf'
--- configs/test-playground/launchpad-lazr.conf	2008-11-10 16:12:10 +0000
+++ configs/test-playground/launchpad-lazr.conf	2010-01-18 11:28:17 +0000
@@ -6,7 +6,9 @@
 extends: ../development/launchpad-lazr.conf
 
 [database]
-main_master: dbname=launchpad_ftest_playground
-main_slave:  dbname=launchpad_ftest_playground
+rw_main_master: dbname=launchpad_ftest_playground
+rw_main_slave:  dbname=launchpad_ftest_playground
+ro_main_master: dbname=launchpad_ftest_playground
+ro_main_slave:  dbname=launchpad_ftest_playground
 auth_master: dbname=launchpad_ftest_playground
 auth_slave:  dbname=launchpad_ftest_playground

=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf	2009-12-21 03:18:24 +0000
+++ configs/testrunner/launchpad-lazr.conf	2010-01-18 11:28:17 +0000
@@ -45,8 +45,12 @@
 error_dir: /var/tmp/codehosting.test
 
 [database]
-main_master: dbname=launchpad_ftest
-main_slave:  dbname=launchpad_ftest
+rw_main_master: dbname=launchpad_ftest
+rw_main_slave:  dbname=launchpad_ftest
+# Use our _template databases here just so that we have different values from
+# the rw_* configs.
+ro_main_master: dbname=launchpad_ftest_template
+ro_main_slave:  dbname=launchpad_ftest_template
 auth_master: dbname=launchpad_ftest
 auth_slave:  dbname=launchpad_ftest
 randomise_select_results: true

=== modified file 'lib/canonical/config/__init__.py'
--- lib/canonical/config/__init__.py	2010-01-14 02:02:34 +0000
+++ lib/canonical/config/__init__.py	2010-01-18 11:28:17 +0000
@@ -22,6 +22,8 @@
 from lazr.config import ImplicitTypeSchema
 from lazr.config.interfaces import ConfigErrors
 
+from canonical.launchpad.readonly import is_read_only
+
 
 __all__ = [
     'DatabaseConfig',
@@ -164,7 +166,7 @@
     def setProcess(self, process_name):
         """Set the name of the process to select a conf file.
 
-        This method is used to set the process_name is if should be
+        This method is used to set the process_name if it should be
         different from the name obtained from sys.argv[0]. CanonicalConfig
         will try to load <process_name>-lazr.conf if it exists. Otherwise,
         it will load launchpad-lazr.conf.
@@ -221,14 +223,15 @@
         self.threads = root_options.threads
 
     def generate_overrides(self):
-        """Ensure correct config .zcml overrides will be called.
+        """Ensure correct config.zcml overrides will be called.
 
         Call this method before letting any ZCML processing occur.
         """
         loader_file = os.path.join(self.root, '+config-overrides.zcml')
         loader = open(loader_file, 'w')
 
-        print >> loader, """<configure xmlns="http://namespaces.zope.org/zope">
+        print >> loader, """
+            <configure xmlns="http://namespaces.zope.org/zope">
                 <!-- This file automatically generated using
                      canonical.config.CanonicalConfig.generate_overrides.
                      DO NOT EDIT. -->
@@ -366,62 +369,32 @@
 
 
 class DatabaseConfig:
-    """A class to provide the Launchpad database configuration.
-
-    The dbconfig option overlays the database configurations of a
-    chosen config section over the base section:
-
-        >>> from canonical.config import config, dbconfig
-        >>> print config.database.main_master
-        dbname=...
-        >>> print config.database.dbuser
-        Traceback (most recent call last):
-          ...
-        AttributeError: ...
-        >>> print config.launchpad.main_master
-        Traceback (most recent call last):
-          ...
-        AttributeError: ...
-        >>> print config.launchpad.dbuser
-        launchpad_main
-        >>> print config.librarian.dbuser
-        librarian
-
-        >>> dbconfig.setConfigSection('librarian')
-        >>> print dbconfig.main_master
-        dbname=...
-        >>> print dbconfig.dbuser
-        librarian
-
-        >>> dbconfig.setConfigSection('launchpad')
-        >>> print dbconfig.main_master
-        dbname=...
-        >>> print dbconfig.dbuser
-        launchpad_main
-
-    Some values are required to have a value, such as dbuser.  So we
-    get an exception if they are not set:
-
-        >>> config.codehosting.dbuser
-        Traceback (most recent call last):
-          ...
-        AttributeError: No section key named dbuser.
-        >>> dbconfig.setConfigSection('codehosting')
-        >>> print dbconfig.dbuser
-        Traceback (most recent call last):
-          ...
-        ValueError: dbuser must be set
-        >>> dbconfig.setConfigSection('launchpad')
-    """
+    """A class to provide the Launchpad database configuration."""
     _config_section = None
     _db_config_attrs = frozenset([
         'dbuser', 'auth_dbuser',
-        'main_master', 'main_slave', 'auth_master', 'auth_slave',
+        'rw_main_master', 'rw_main_slave',
+        'ro_main_master', 'ro_main_slave', 'auth_master', 'auth_slave',
         'db_statement_timeout', 'db_statement_timeout_precision',
         'isolation_level', 'randomise_select_results',
         'soft_request_timeout', 'storm_cache', 'storm_cache_size'])
     _db_config_required_attrs = frozenset([
-        'dbuser', 'main_master', 'main_slave', 'auth_master', 'auth_slave'])
+        'dbuser', 'rw_main_master', 'rw_main_slave', 'ro_main_master',
+        'ro_main_slave', 'auth_master', 'auth_slave'])
+
+    @property
+    def main_master(self):
+        if is_read_only():
+            return self.ro_main_master
+        else:
+            return self.rw_main_master
+
+    @property
+    def main_slave(self):
+        if is_read_only():
+            return self.ro_main_slave
+        else:
+            return self.rw_main_slave
 
     def setConfigSection(self, section_name):
         self._config_section = section_name

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2009-11-17 11:27:51 +0000
+++ lib/canonical/config/schema-lazr.conf	2010-01-18 11:28:17 +0000
@@ -529,8 +529,10 @@
 # Note that user is not included, as it is specified by the dbuser
 # option. This allows config sections to easily override just the dbuser.
 # datatype: pgconnection
-main_master: dbname=launchpad_prod_3 host=wildcherry.canonical.com
-main_slave:  dbname=launchpad_prod_2 host=chokecherry.canonical.com
+rw_main_master: dbname=launchpad_prod_3 host=wildcherry.canonical.com
+rw_main_slave:  dbname=launchpad_prod_2 host=chokecherry.canonical.com
+ro_main_master: dbname=launchpad_standalone_1 host=chokecherry.canonical.com
+ro_main_slave:  dbname=launchpad_standalone_1 host=chokecherry.canonical.com
 auth_master: dbname=launchpad_prod_3 host=wildcherry.canonical.com
 auth_slave:  dbname=launchpad_prod_2 host=chokecherry.canonical.com
 
@@ -872,11 +874,6 @@
 # datatype: boolean
 launch: True
 
-# If true, Launchpad is running in read-only mode. Attempts to
-# write to the Launchpad database will be denied, and an explanatory
-# error message displayed to the user.
-read_only: False
-
 # If true, the main template will be styled so that it is
 # obvious to the end user that they are using a demo system
 # and that any changes they make will be lost at some point.

=== added file 'lib/canonical/config/tests/test_database_config.py'
--- lib/canonical/config/tests/test_database_config.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/config/tests/test_database_config.py	2010-01-18 11:28:17 +0000
@@ -0,0 +1,61 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from lp.testing import TestCase
+
+from canonical.config import config, dbconfig
+
+from canonical.launchpad.readonly import read_only_file_exists
+from canonical.launchpad.tests.readonly import (
+    remove_read_only_file, touch_read_only_file)
+
+
+class TestDatabaseConfig(TestCase):
+
+    def test_overlay(self):
+        # The dbconfig option overlays the database configurations of a
+        # chosen config section over the base section.
+        self.assertRaises(
+            AttributeError, getattr, config.database, 'dbuser')
+        self.assertRaises(
+            AttributeError, getattr, config.launchpad, 'main_master')
+        self.assertEquals('launchpad_main', config.launchpad.dbuser)
+        self.assertEquals('librarian', config.librarian.dbuser)
+
+        dbconfig.setConfigSection('librarian')
+        self.assertEquals('dbname=launchpad_ftest', dbconfig.rw_main_master)
+        self.assertEquals('librarian', dbconfig.dbuser)
+
+        dbconfig.setConfigSection('launchpad')
+        self.assertEquals('dbname=launchpad_ftest', dbconfig.rw_main_master)
+        self.assertEquals('launchpad_main', dbconfig.dbuser)
+
+    def test_required_values(self):
+        # Some variables are required to have a value, such as dbuser.  So we
+        # get a ValueError if they are not set.
+        self.assertRaises(
+            AttributeError, getattr, config.codehosting, 'dbuser')
+        dbconfig.setConfigSection('codehosting')
+        self.assertRaises(ValueError, getattr, dbconfig, 'dbuser')
+        dbconfig.setConfigSection('launchpad')
+
+    def test_main_master_and_main_slave(self):
+        # DatabaseConfig provides two extra properties: main_master and
+        # main_slave, which return the value of either
+        # rw_main_master/rw_main_slave or ro_main_master/ro_main_slave,
+        # depending on whether or not we're in read-only mode.
+        self.assertFalse(read_only_file_exists())
+        self.assertEquals(dbconfig.rw_main_master, dbconfig.main_master)
+        self.assertEquals(dbconfig.rw_main_slave, dbconfig.main_slave)
+
+        touch_read_only_file()
+        try:
+            self.assertTrue(read_only_file_exists())
+            self.assertEquals(
+                dbconfig.ro_main_master, dbconfig.main_master)
+            self.assertEquals(
+                dbconfig.ro_main_slave, dbconfig.main_slave)
+        finally:
+            remove_read_only_file()

=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2009-12-14 12:52:19 +0000
+++ lib/canonical/database/sqlbase.py	2010-01-18 11:28:17 +0000
@@ -29,7 +29,7 @@
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.config import config
+from canonical.config import config, dbconfig
 from canonical.database.interfaces import ISQLBase
 
 
@@ -268,8 +268,11 @@
     @classmethod
     def _get_zopeless_connection_config(self, dbname, dbhost):
         # This method exists for testability.
-        main_connection_string = config.database.main_master
-        auth_connection_string = config.database.auth_master
+
+        # This is only used by scripts, so we must connect to the read-write
+        # DB here -- that's why we use rw_main_master directly.
+        main_connection_string = dbconfig.rw_main_master
+        auth_connection_string = dbconfig.auth_master
 
         # Override dbname and dbhost in the connection string if they
         # have been passed in.
@@ -312,7 +315,7 @@
         # Construct a config fragment:
         overlay = dedent("""\
             [database]
-            main_master: %(main_connection_string)s
+            rw_main_master: %(main_connection_string)s
             auth_master: %(auth_connection_string)s
             isolation_level: %(isolation_level)s
             """ % vars())
@@ -755,20 +758,14 @@
 
     Default database name is the one specified in the main configuration file.
     """
-    con_str = connect_string(user, dbname)
-    con = psycopg2.connect(con_str)
-    con.set_isolation_level(isolation)
-    return con
-
-
-def connect_string(user, dbname=None):
-    """Return a PostgreSQL connection string."""
     from canonical import lp
     # We start with the config string from the config file, and overwrite
     # with the passed in dbname or modifications made by db_options()
     # command line arguments. This will do until db_options gets an overhaul.
-    con_str = config.database.main_master
     con_str_overrides = []
+    # We must connect to the read-write DB here, so we use rw_main_master
+    # directly.
+    con_str = dbconfig.rw_main_master
     assert 'user=' not in con_str, (
             'Connection string already contains username')
     if user is not None:
@@ -782,7 +779,11 @@
         con_str = re.sub(r'dbname=\S*', '', con_str) # Remove if exists.
         con_str_overrides.append('dbname=%s' % dbname)
 
-    return ' '.join([con_str] + con_str_overrides)
+    con_str = ' '.join([con_str] + con_str_overrides)
+
+    con = psycopg2.connect(con_str)
+    con.set_isolation_level(isolation)
+    return con
 
 
 class cursor:

=== modified file 'lib/canonical/launchpad/configure.zcml'
--- lib/canonical/launchpad/configure.zcml	2009-12-03 19:12:02 +0000
+++ lib/canonical/launchpad/configure.zcml	2010-01-18 11:28:17 +0000
@@ -93,5 +93,4 @@
       name="PlotKit"
       directory="../../contrib/PlotKit"
       />
-
 </configure>

=== modified file 'lib/canonical/launchpad/doc/canonical-config.txt'
--- lib/canonical/launchpad/doc/canonical-config.txt	2009-06-15 21:53:14 +0000
+++ lib/canonical/launchpad/doc/canonical-config.txt	2010-01-18 11:28:17 +0000
@@ -14,7 +14,7 @@
 simple configuration).
 
     >>> from canonical.config import config
-    >>> print config.database.main_master
+    >>> print config.database.rw_main_master
     dbname=launchpad_ftest
     >>> config.database.db_statement_timeout is None
     True

=== modified file 'lib/canonical/launchpad/pagetests/standalone/xx-dbpolicy.txt'
--- lib/canonical/launchpad/pagetests/standalone/xx-dbpolicy.txt	2009-08-13 19:03:36 +0000
+++ lib/canonical/launchpad/pagetests/standalone/xx-dbpolicy.txt	2010-01-18 11:28:17 +0000
@@ -13,7 +13,7 @@
     >>> from textwrap import dedent
     >>> config_overlay = dedent("""
     ...     [database]
-    ...     main_slave: dbname=launchpad_empty
+    ...     rw_main_slave: dbname=launchpad_empty
     ...     """)
     >>> config.push('empty_slave', config_overlay)
 

=== modified file 'lib/canonical/launchpad/pagetests/standalone/xx-opstats.txt'
--- lib/canonical/launchpad/pagetests/standalone/xx-opstats.txt	2009-12-02 11:28:49 +0000
+++ lib/canonical/launchpad/pagetests/standalone/xx-opstats.txt	2010-01-18 11:28:17 +0000
@@ -234,8 +234,8 @@
 
     >>> no_db_overrides = """
     ...     [database]
-    ...     main_master: dbname=nonexistant
-    ...     main_slave: dbname=nonexistant
+    ...     rw_main_master: dbname=nonexistant
+    ...     rw_main_slave: dbname=nonexistant
     ...     auth_master: dbname=nonexistant
     ...     auth_slave: dbname=nonexistant
     ...

=== modified file 'lib/canonical/launchpad/pagetests/standalone/xx-read-only-mode.txt'
--- lib/canonical/launchpad/pagetests/standalone/xx-read-only-mode.txt	2009-07-08 21:47:23 +0000
+++ lib/canonical/launchpad/pagetests/standalone/xx-read-only-mode.txt	2010-01-18 11:28:17 +0000
@@ -5,14 +5,9 @@
 database but attempts to access the master database or make database
 changes fail, returning an error page to the user.
 
-    >>> from canonical.config import config
-    >>> def push_read_only_mode(flag):
-    ...     config.push('read only mode', """
-    ...         [launchpad]
-    ...         read_only: %s""" % str(flag))
-    >>> def pop_read_only_mode():
-    ...     config.pop('read only mode')
-    >>> push_read_only_mode(True)
+    >>> from canonical.launchpad.tests.readonly import (
+    ...     touch_read_only_file, remove_read_only_file)
+    >>> touch_read_only_file()
 
 
 == Notification of read-only mode. ==
@@ -35,18 +30,18 @@
 
 There is no warning when Launchpad is running normally.
 
-    >>> push_read_only_mode(False)
+    >>> remove_read_only_file()
     >>> user_browser.open('http://launchpad.dev')
     >>> print first_tag_by_class(
     ...     user_browser.contents, 'warning message')
     None
-    >>> pop_read_only_mode()
 
 == Operations requiring write permissions fail ==
 
 In read-only mode, all requests for non-read permissions are denied.
 This causes edit buttons and similar to not be displayed.
 
+    >>> touch_read_only_file()
     >>> user_browser.open('http://launchpad.dev/people/+me')
     >>> user_browser.getLink('Change details')
     Traceback (most recent call last):
@@ -58,16 +53,16 @@
 forgetting to properly protect the edit buttons, the form is replaced
 with a nice 503 error page informing the user what is going on.
 
-    >>> push_read_only_mode(False)
+    >>> remove_read_only_file()
     >>> user_browser.open('http://launchpad.dev/people/+me')
     >>> edit_link = user_browser.getLink('Change details')
     >>> edit_link is None
     False
-    >>> pop_read_only_mode()
     >>> # XXX StuartBishop 20090423 bug=365378: raiseHttpErrors is broken,
     >>> # requiring the try/except dance.
     >>> user_browser.handleErrors = True
     >>> user_browser.raiseHttpErrors = False
+    >>> touch_read_only_file()
     >>> try:
     ...     edit_link.click()
     ... except:
@@ -81,15 +76,15 @@
 And even if a user manages to get a form and submit it, they get the
 same 503 error page.
 
-    >>> push_read_only_mode(False)
+    >>> remove_read_only_file()
     >>> user_browser.handleErrors = True
     >>> user_browser.open('http://launchpad.dev/people/+me')
     >>> user_browser.getLink('Change details').click()
-    >>> pop_read_only_mode()
     >>> user_browser.getControl(name='field.displayname').value = 'Different'
     >>> user_browser.raiseHttpErrors = False
     >>> # XXX StuartBishop 20090423 bug=365378: raiseHttpErrors is broken,
     >>> # requiring the try/except dance.
+    >>> touch_read_only_file()
     >>> try:
     ...     user_browser.getControl('Save Changes').click()
     ... except:
@@ -143,16 +138,13 @@
 
 === Bug page ===
 
-    >>> push_read_only_mode(True)
     >>> user_browser.open('http://launchpad.dev/bugs/5')
     >>> print user_browser.title
     Bug #5...
     >>> print user_browser.headers['status']
     200 Ok
-    >>> pop_read_only_mode()
 
 
 == Cleanup ==
 
-    >>> pop_read_only_mode()
-
+    >>> remove_read_only_file()

=== added file 'lib/canonical/launchpad/readonly.py'
--- lib/canonical/launchpad/readonly.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/readonly.py	2010-01-18 11:28:17 +0000
@@ -0,0 +1,86 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helpers for running Launchpad in read-only mode.
+
+To switch an app server to read-only mode, all you need to do is create a file
+named read-only.txt in the root of the Launchpad tree.
+"""
+
+from __future__ import with_statement
+
+__metaclass__ = type
+__all__ = [
+    'is_read_only',
+    'read_only_file_exists',
+    'read_only_file_path',
+    ]
+
+import logging
+import os
+import threading
+
+from zope.security.management import queryInteraction
+
+from lazr.restful.utils import get_current_browser_request
+
+
+root = os.path.abspath(os.path.join(
+    os.path.dirname(__file__), os.pardir, os.pardir, os.pardir))
+read_only_file_path = os.path.join(root, 'read-only.txt')
+READ_ONLY_MODE_ANNOTATIONS_KEY = 'launchpad.read_only_mode'
+
+
+def read_only_file_exists():
+    """Does a read-only.txt file exists in the root of our tree?"""
+    return os.path.isfile(read_only_file_path)
+
+
+_lock = threading.Lock()
+_currently_in_read_only = False
+
+
+def is_read_only():
+    """Are we in read-only mode?
+
+    If called as part of the processing of a request, we'll look in the
+    request's annotations for a read-only key
+    (READ_ONLY_MODE_ANNOTATIONS_KEY), and if it exists we'll just return its
+    value.
+    
+    If there's no request or the key doesn't exist, we check for the presence
+    of a read-only.txt file in the root of our tree, set the read-only key in
+    the request's annotations (when there is a request), update
+    _currently_in_read_only (in case it changed, also logging the change)
+    and return it.
+    """
+    # pylint: disable-msg=W0603
+    global _currently_in_read_only
+    request = None
+    # XXX: salgado, 2010-01-14, bug=507447: Only call
+    # get_current_browser_request() when we have an interaction, or else
+    # it will raise an AttributeError.
+    if queryInteraction() is not None:
+        request = get_current_browser_request()
+    if request is not None:
+        if READ_ONLY_MODE_ANNOTATIONS_KEY in request.annotations:
+            return request.annotations[READ_ONLY_MODE_ANNOTATIONS_KEY]
+
+    read_only = read_only_file_exists()
+    if request is not None:
+        request.annotations[READ_ONLY_MODE_ANNOTATIONS_KEY] = read_only
+
+    log_change = False
+    with _lock:
+        if _currently_in_read_only != read_only:
+            _currently_in_read_only = read_only
+            log_change = True
+
+    if log_change:
+        logging.warning(
+            'Read-only mode change detected; now read-only is %s' % read_only)
+
+    return read_only
+
+
+_currently_in_read_only = is_read_only()

=== added file 'lib/canonical/launchpad/tests/readonly.py'
--- lib/canonical/launchpad/tests/readonly.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/tests/readonly.py	2010-01-18 11:28:17 +0000
@@ -0,0 +1,34 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helpers for creating and removing a read-only.txt in the root of our tree.
+"""
+
+__metaclass__ = type
+__all__ = [
+    'touch_read_only_file',
+    'remove_read_only_file',
+    ]
+
+import os
+
+from canonical.launchpad.readonly import (
+    read_only_file_exists, read_only_file_path)
+
+
+def touch_read_only_file():
+    """Create an empty file named read-only.txt under the root of the tree.
+
+    This function must not be called if a file with that name already exists.
+    """
+    assert not read_only_file_exists(), (
+        "This function must not be called when a read-only.txt file "
+        "already exists.")
+    f = open(read_only_file_path, 'w')
+    f.close()
+
+
+def remove_read_only_file():
+    """Remove the file named read-only.txt from the root of the tree."""
+    os.remove(read_only_file_path)
+

=== added file 'lib/canonical/launchpad/tests/test_readonly.py'
--- lib/canonical/launchpad/tests/test_readonly.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/tests/test_readonly.py	2010-01-18 11:28:17 +0000
@@ -0,0 +1,78 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from lazr.restful.utils import get_current_browser_request
+
+from lp.testing import TestCase
+
+from canonical.launchpad.ftests import ANONYMOUS, login, logout
+from canonical.launchpad.readonly import (
+    is_read_only, read_only_file_exists, READ_ONLY_MODE_ANNOTATIONS_KEY)
+from canonical.launchpad.tests.readonly import (
+    remove_read_only_file, touch_read_only_file)
+from canonical.testing.layers import FunctionalLayer
+
+
+class TestReadOnlyModeDetection(TestCase):
+
+    def test_read_only_file_exists(self):
+        # By default we run in read-write mode.
+        self.assertFalse(read_only_file_exists())
+
+        # When a file named 'read-only.txt' exists under the root of the tree,
+        # we run in read-only mode.
+        touch_read_only_file()
+        try:
+            self.assertTrue(read_only_file_exists())
+        finally:
+            remove_read_only_file()
+
+        # Once the file is removed, we're back into read-write mode.
+        self.assertFalse(read_only_file_exists())
+
+
+class Test_is_read_only(TestCase):
+    layer = FunctionalLayer
+
+    def tearDown(self):
+        # Safety net just in case a test leaves the read-only.txt file behind.
+        if read_only_file_exists():
+            remove_read_only_file()
+        super(Test_is_read_only, self).tearDown()
+
+    def test_is_read_only(self):
+        # By default we run in read-write mode.
+        logout()
+        self.assertFalse(is_read_only())
+
+        # When a file named 'read-only.txt' exists under the root of the tree,
+        # we run in read-only mode.
+        touch_read_only_file()
+        try:
+            self.assertTrue(is_read_only())
+        finally:
+            remove_read_only_file()
+
+    def test_caching_in_request(self):
+        # When called as part of a request processing, is_read_only() will
+        # stash the read-only flag in the request's annotations.
+        login(ANONYMOUS)
+        request = get_current_browser_request()
+        self.assertIs(
+            None,
+            request.annotations.get(READ_ONLY_MODE_ANNOTATIONS_KEY))
+        self.assertFalse(is_read_only())
+        self.assertFalse(
+            request.annotations.get(READ_ONLY_MODE_ANNOTATIONS_KEY))
+
+    def test_cached_value_takes_precedence(self): 
+        # Once the request has the read-only flag, we don't check for the
+        # presence of the read-only.txt file anymore, so it could be removed
+        # and the request would still be in read-only mode.
+        login(ANONYMOUS)
+        request = get_current_browser_request()
+        request.annotations[READ_ONLY_MODE_ANNOTATIONS_KEY] = True
+        self.assertTrue(is_read_only())
+        self.assertFalse(read_only_file_exists())

=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py	2009-12-08 20:14:06 +0000
+++ lib/canonical/launchpad/webapp/adapter.py	2010-01-18 11:28:17 +0000
@@ -7,6 +7,7 @@
 __metaclass__ = type
 
 import os
+import re
 import sys
 import thread
 import threading
@@ -36,6 +37,7 @@
 from canonical.config import config, dbconfig, DatabaseConfig
 from canonical.database.interfaces import IRequestExpired
 from canonical.launchpad.interfaces import IMasterObject, IMasterStore
+from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
 from canonical.launchpad.webapp.interfaces import (
     AUTH_STORE, DEFAULT_FLAVOR, IStoreSelector,
@@ -277,12 +279,14 @@
             # is raised when an attempt is made to make changes when
             # the connection has been put in read-only mode.
             if exception.pgcode == '25006':
-                raise ReadOnlyModeViolation, None, sys.exc_info()[2]
+                raise ReadOnlyModeViolation(None, sys.exc_info()[2])
             raise
 
 
 class LaunchpadDatabase(Postgres):
 
+    _dsn_user_re = re.compile('user=[^ ]*')
+
     def __init__(self, uri):
         # The uri is just a property name in the config, such as main_master
         # or auth_slave.
@@ -293,6 +297,13 @@
         # A unique name for this database connection.
         self.name = uri.database
 
+    @property
+    def dsn_without_user(self):
+        """This database's dsn without the 'user=...' bit."""
+        assert self._dsn is not None, (
+            'Must not be called before self._dsn has been set.')
+        return self._dsn_user_re.sub('', self._dsn)
+
     def raw_connect(self):
         # Prevent database connections from the main thread if
         # break_main_thread_db_access() has been run.
@@ -359,7 +370,7 @@
         If we are running in read-only mode, returns a
         ReadOnlyModeConnection. Otherwise it returns the Storm default.
         """
-        if config.launchpad.read_only:
+        if is_read_only():
             return ReadOnlyModeConnection
         return super(LaunchpadDatabase, self).connection_factory
 

=== modified file 'lib/canonical/launchpad/webapp/authorization.py'
--- lib/canonical/launchpad/webapp/authorization.py	2009-10-20 22:13:21 +0000
+++ lib/canonical/launchpad/webapp/authorization.py	2010-01-18 11:28:17 +0000
@@ -21,11 +21,11 @@
     checkPermission as check_permission_is_registered)
 from zope.app.security.principalregistry import UnauthenticatedPrincipal
 
-from canonical.config import config
 from canonical.lazr.canonicalurl import nearest_adapter
 from canonical.lazr.interfaces import IObjectPrivacy
 
 from canonical.database.sqlbase import block_implicit_flushes
+from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.webapp.interfaces import (
     AccessLevel, IAuthorization, ILaunchpadContainer, ILaunchpadPrincipal)
 from canonical.launchpad.webapp.metazcml import ILaunchpadPermission
@@ -123,7 +123,7 @@
         # accidentally using cached results. This will be important when
         # Launchpad automatically fails over to read-only mode when the
         # master database is unavailable.
-        if config.launchpad.read_only:
+        if is_read_only():
             lp_permission = getUtility(ILaunchpadPermission, permission)
             if lp_permission.access_level != "read":
                 return False

=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml	2009-11-18 00:22:41 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml	2010-01-18 11:28:17 +0000
@@ -678,6 +678,12 @@
         />
 
     <adapter
+        for="canonical.launchpad.webapp.servers.LaunchpadTestRequest"
+        provides="canonical.launchpad.webapp.interfaces.INotificationResponse"
+        factory="canonical.launchpad.webapp.servers.adaptRequestToResponse"
+        />
+
+    <adapter
         factory="canonical.launchpad.webapp.snapshot.snapshot_sql_result" />
     <!-- It also works for the legacy SQLObject interface. -->
     <adapter

=== modified file 'lib/canonical/launchpad/webapp/dbpolicy.py'
--- lib/canonical/launchpad/webapp/dbpolicy.py	2009-12-02 19:33:08 +0000
+++ lib/canonical/launchpad/webapp/dbpolicy.py	2010-01-18 11:28:17 +0000
@@ -24,6 +24,7 @@
 from canonical.config import config, dbconfig
 from canonical.database.sqlbase import StupidCache
 from canonical.launchpad.interfaces import IMasterStore, ISlaveStore
+from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.webapp import LaunchpadView
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR, DisallowedStore, IDatabasePolicy, IStoreSelector,
@@ -76,10 +77,6 @@
         config_section = self.config_section or dbconfig.getSectionName()
 
         store_name = '%s-%s-%s' % (config_section, name, flavor)
-        # XXX stub 2009-06-25 bug=392011: zstorm.get seems to be broken
-        # and does not return None if no Store is available. We have
-        # no way of knowing if the returned Store existed previously,
-        # which makes it difficult to do any post-instantiation setup.
         store = getUtility(IZStorm).get(
             store_name, 'launchpad:%s' % store_name)
         if not getattr(store, '_lp_store_initialized', False):
@@ -162,7 +159,7 @@
     # of test requests in our automated tests.
     if request.get('PATH_INFO') == u'/+opstats':
         return DatabaseBlockedPolicy(request)
-    elif config.launchpad.read_only:
+    elif is_read_only():
         return ReadOnlyLaunchpadDatabasePolicy(request)
     else:
         return LaunchpadDatabasePolicy(request)
@@ -298,7 +295,7 @@
 def WebServiceDatabasePolicyFactory(request):
     """Return the Launchpad IDatabasePolicy for the current appserver state.
     """
-    if config.launchpad.read_only:
+    if is_read_only():
         return ReadOnlyLaunchpadDatabasePolicy(request)
     else:
         # If a session cookie was sent with the request, use the

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2009-11-23 03:10:04 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-01-18 11:28:17 +0000
@@ -31,6 +31,7 @@
 from lp.registry.interfaces.person import (
     IPerson, IPersonSet, PersonCreationRationale)
 from canonical.launchpad.interfaces.validation import valid_password
+from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.validators.email import valid_email
 from canonical.launchpad.webapp.error import SystemErrorView
 from canonical.launchpad.webapp.interfaces import (
@@ -58,7 +59,7 @@
         # to render the read-only failure screen so the user knows their
         # request failed for operational reasons rather than a genuine
         # permission problem.
-        if config.launchpad.read_only:
+        if is_read_only():
             # Our context is an Unauthorized exception, which acts like
             # a tuple containing (object, attribute_requested, permission).
             lp_permission = getUtility(ILaunchpadPermission, self.context[2])

=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2009-10-07 18:41:52 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2010-01-18 11:28:17 +0000
@@ -4,10 +4,9 @@
 __metaclass__ = type
 __all__ = [
     'LoginRoot',
-    'LaunchpadBrowserPublication'
+    'LaunchpadBrowserPublication',
     ]
 
-
 import gc
 import os
 import re
@@ -46,18 +45,20 @@
 import canonical.launchpad.layers as layers
 import canonical.launchpad.webapp.adapter as da
 
-from canonical.config import config
+from canonical.config import config, dbconfig
 from canonical.mem import (
     countsByType, deltaCounts, memory, mostRefs, printCounts, readCounts,
     resident)
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.readonly import is_read_only
 from lp.registry.interfaces.person import (
     IPerson, IPersonSet, ITeam)
 from canonical.launchpad.webapp.interfaces import (
-    IDatabasePolicy, IPlacelessAuthUtility, IPrimaryContext,
-    ILaunchpadRoot, INotificationResponse, IOpenLaunchBag,
-    OffsiteFormPostError, IStoreSelector, MASTER_FLAVOR)
-from canonical.launchpad.webapp.dbpolicy import LaunchpadDatabasePolicy
+    IDatabasePolicy, ILaunchpadRoot, INotificationResponse, IOpenLaunchBag,
+    IPlacelessAuthUtility, IPrimaryContext, IStoreSelector, MAIN_STORE,
+    MASTER_FLAVOR, OffsiteFormPostError, SLAVE_FLAVOR)
+from canonical.launchpad.webapp.dbpolicy import (
+    DatabaseBlockedPolicy, LaunchpadDatabasePolicy)
 from canonical.launchpad.webapp.menu import structured
 from canonical.launchpad.webapp.opstats import OpStats
 from lazr.uri import URI, InvalidURIError
@@ -166,9 +167,29 @@
 
         transaction.begin()
 
+        db_policy = IDatabasePolicy(request)
+        if not isinstance(db_policy, DatabaseBlockedPolicy):
+            # Database access is not blocked, so make sure our stores point to
+            # the appropriate databases, according to the mode we're on.
+            main_master_store = getUtility(IStoreSelector).get(
+                MAIN_STORE, MASTER_FLAVOR)
+            # XXX: 2009-01-12, salgado, bug=506536: We shouldn't need to go
+            # through private attributes to get to the store's database.
+            dsn = main_master_store._connection._database.dsn_without_user
+            if dsn.strip() != dbconfig.main_master.strip():
+                # Remove the stores from zstorm to force them to be
+                # re-created, thus using the correct databases for the mode
+                # we're on right now.
+                main_slave_store = getUtility(IStoreSelector).get(
+                    MAIN_STORE, SLAVE_FLAVOR)
+                zstorm = getUtility(IZStorm)
+                for store in [main_master_store, main_slave_store]:
+                    zstorm.remove(store)
+                    store.close()
+
         # Now we are logged in, install the correct IDatabasePolicy for
         # this request.
-        getUtility(IStoreSelector).push(IDatabasePolicy(request))
+        getUtility(IStoreSelector).push(db_policy)
 
         getUtility(IOpenLaunchBag).clear()
 
@@ -186,7 +207,7 @@
 
     def maybeNotifyReadOnlyMode(self, request):
         """Hook to notify about read-only mode."""
-        if config.launchpad.read_only:
+        if is_read_only():
             try:
                 INotificationResponse(request).addWarningNotification(
                     structured("""
@@ -555,6 +576,7 @@
         We must restart the request timer.  Otherwise we can get OOPS errors
         from our exception views inappropriately.
         """
+        # pylint: disable-msg=E1002
         super(LaunchpadBrowserPublication,
               self).beginErrorHandlingTransaction(request, ob, note)
         # XXX: gary 2008-11-04 bug=293614: As the bug describes, we want to

=== modified file 'lib/canonical/launchpad/webapp/tests/test_authorization.py'
--- lib/canonical/launchpad/webapp/tests/test_authorization.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/tests/test_authorization.py	2010-01-18 11:28:17 +0000
@@ -10,8 +10,7 @@
 
 import transaction
 
-from zope.app.testing import ztapi
-from zope.component import provideAdapter
+from zope.component import provideAdapter, provideUtility
 from zope.interface import classProvides, implements, Interface
 from zope.testing.cleanup import CleanUp
 
@@ -136,7 +135,7 @@
         """Register a new permission and a fake store selector."""
         super(TestCheckPermissionCaching, self).setUp()
         self.factory = ObjectFactory()
-        ztapi.provideUtility(IStoreSelector, FakeStoreSelector)
+        provideUtility(FakeStoreSelector, IStoreSelector)
 
     def makeRequest(self):
         """Construct an arbitrary `LaunchpadBrowserRequest` object."""
@@ -152,11 +151,11 @@
             `Checker` created by ``checker_factory``.
         """
         permission = self.factory.getUniqueString()
-        ztapi.provideUtility(
-            ILaunchpadPermission, PermissionAccessLevel(), permission)
+        provideUtility(
+            PermissionAccessLevel(), ILaunchpadPermission, permission)
         checker_factory = CheckerFactory()
-        ztapi.provideAdapter(
-            Object, IAuthorization, checker_factory, name=permission)
+        provideAdapter(
+            checker_factory, [Object], IAuthorization, name=permission)
         return Object(), permission, checker_factory
 
     def test_checkPermission_cache_unauthenticated(self):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_dbpolicy.py'
--- lib/canonical/launchpad/webapp/tests/test_dbpolicy.py	2010-01-07 06:16:04 +0000
+++ lib/canonical/launchpad/webapp/tests/test_dbpolicy.py	2010-01-18 11:28:17 +0000
@@ -14,10 +14,11 @@
 from zope.session.interfaces import ISession, IClientIdManager
 
 from lazr.restful.interfaces import IWebServiceConfiguration
-from canonical.config import config
 from canonical.launchpad.interfaces import IMasterStore, ISlaveStore
 from canonical.launchpad.layers import (
     FeedsLayer, setFirstLayer, WebServiceLayer)
+from canonical.launchpad.tests.readonly import (
+    remove_read_only_file, touch_read_only_file)
 from lp.testing import TestCase
 from canonical.launchpad.webapp.dbpolicy import (
     BaseDatabasePolicy, LaunchpadDatabasePolicy, MasterDatabasePolicy,
@@ -207,9 +208,7 @@
         """WebService requests should use the read only database
         policy in read only mode.
         """
-        config.push('read_only', """
-            [launchpad]
-            read_only: True""")
+        touch_read_only_file()
         try:
             api_prefix = getUtility(
                 IWebServiceConfiguration).service_version_uri_prefix
@@ -219,19 +218,17 @@
             policy = IDatabasePolicy(request)
             self.assertIsInstance(policy, ReadOnlyLaunchpadDatabasePolicy)
         finally:
-            config.pop('read_only')
+            remove_read_only_file()
 
     def test_read_only_mode_uses_ReadOnlyLaunchpadDatabasePolicy(self):
-        config.push('read_only', """
-            [launchpad]
-            read_only: True""")
+        touch_read_only_file()
         try:
             request = LaunchpadTestRequest(
                 SERVER_URL='http://launchpad.dev')
             policy = IDatabasePolicy(request)
             self.assertIsInstance(policy, ReadOnlyLaunchpadDatabasePolicy)
         finally:
-            config.pop('read_only')
+            remove_read_only_file()
 
     def test_other_request_uses_LaunchpadDatabasePolicy(self):
         """By default, requests should use the LaunchpadDatabasePolicy."""

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2009-10-07 18:41:52 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2010-01-18 11:28:17 +0000
@@ -11,16 +11,22 @@
 from contrib.oauth import OAuthRequest, OAuthSignatureMethod_PLAINTEXT
 
 from storm.exceptions import DisconnectionError
+from storm.zope.interfaces import IZStorm
+
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.publisher.interfaces import Retry
 
+from canonical.config import dbconfig
 from canonical.testing import DatabaseFunctionalLayer
 from canonical.launchpad.interfaces.oauth import IOAuthConsumerSet
 from canonical.launchpad.ftests import ANONYMOUS, login
+from canonical.launchpad.tests.readonly import (
+    remove_read_only_file, touch_read_only_file)
 from lp.testing import TestCase, TestCaseWithFactory
 import canonical.launchpad.webapp.adapter as da
-from canonical.launchpad.webapp.interfaces import OAuthPermission
+from canonical.launchpad.webapp.interfaces import (
+    IStoreSelector, MAIN_STORE, MASTER_FLAVOR, OAuthPermission, SLAVE_FLAVOR)
 from canonical.launchpad.webapp.publication import (
     is_browser, LaunchpadBrowserPublication)
 from canonical.launchpad.webapp.servers import (
@@ -51,6 +57,89 @@
         publication.callTraversalHooks(request, obj2)
         self.assertEquals(request.traversed_objects, [obj1])
 
+class TestReadOnlyModeSwitches(TestCase):
+    # At the beginning of every request (in publication.beforeTraversal()), we
+    # check to see if we've changed from/to read-only/read-write and if there
+    # was a change we remove the main_master/slave stores from ZStorm, forcing
+    # them to be recreated the next time they're needed, thus causing them to
+    # point to the correct databases.
+    layer = DatabaseFunctionalLayer
+
+    def tearDown(self):
+        TestCase.tearDown(self)
+        # If a DB policy was installed (e.g. by publication.beforeTraversal),
+        # uninstall it.
+        try:
+            getUtility(IStoreSelector).pop()
+        except IndexError:
+            pass
+        # Cleanup needed so that further tests can start processing other
+        # requests (e.g. calling beforeTraversal).
+        self.publication.endRequest(self.request, None)
+
+    def setUp(self):
+        TestCase.setUp(self)
+        # Get the main_master/slave stores just to make sure they're added to
+        # ZStorm.
+        master = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+        slave = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
+        self.master_connection = master._connection
+        self.slave_connection = slave._connection
+        self.zstorm = getUtility(IZStorm)
+        self.publication = LaunchpadBrowserPublication(None)
+        self.request = LaunchpadTestRequest()
+
+    @property
+    def zstorm_stores(self):
+        return [name for (name, store) in self.zstorm.iterstores()]
+
+    def test_no_mode_changes(self):
+        # Make sure the master/slave stores are present in zstorm.
+        self.assertIn('launchpad-main-master', self.zstorm_stores)
+        self.assertIn('launchpad-main-slave', self.zstorm_stores)
+
+        self.publication.beforeTraversal(self.request)
+
+        # Since the mode didn't change, the stores were left in zstorm.
+        self.assertIn('launchpad-main-master', self.zstorm_stores)
+        self.assertIn('launchpad-main-slave', self.zstorm_stores)
+
+        # With the store's connection being the same as before.
+        master = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+        self.assertIs(self.master_connection, master._connection)
+
+        # And they still point to the read-write databases.
+        self.assertEquals(
+            dbconfig.rw_main_master.strip(),
+            # XXX: 2009-01-12, salgado, bug=506536: We shouldn't need to go
+            # through private attributes to get to the store's database.
+            master._connection._database.dsn_without_user.strip())
+
+    def test_changing_modes(self):
+        # Make sure the master/slave stores are present in zstorm.
+        self.assertIn('launchpad-main-master', self.zstorm_stores)
+        self.assertIn('launchpad-main-slave', self.zstorm_stores)
+
+        try:
+            touch_read_only_file()
+            self.publication.beforeTraversal(self.request)
+        finally:
+            remove_read_only_file()
+
+        # Here the mode has changed to read-only, so the stores were removed
+        # from zstorm.
+        self.assertNotIn('launchpad-main-master', self.zstorm_stores)
+        self.assertNotIn('launchpad-main-slave', self.zstorm_stores)
+
+        # If they're needed again, they'll be re-created by ZStorm, and when
+        # that happens they will point to the read-only databases.
+        master = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
+        self.assertEquals(
+            dbconfig.ro_main_master.strip(),
+            # XXX: 2009-01-12, salgado, bug=506536: We shouldn't need to go
+            # through private attributes to get to the store's database.
+            master._connection._database.dsn_without_user.strip())
+
 
 class TestWebServicePublication(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer

=== modified file 'lib/canonical/librarian/client.py'
--- lib/canonical/librarian/client.py	2009-11-24 15:36:44 +0000
+++ lib/canonical/librarian/client.py	2010-01-18 11:28:17 +0000
@@ -25,7 +25,7 @@
 from storm.store import Store
 from zope.interface import implements
 
-from canonical.config import config
+from canonical.config import config, dbconfig
 from canonical.database.sqlbase import cursor
 from canonical.librarian.interfaces import (
     DownloadFailed, ILibrarianClient, IRestrictedLibrarianClient,
@@ -186,9 +186,11 @@
             name = name.encode('utf-8')
         self._connect()
         try:
-            # Send command
+            # Use dbconfig.rw_main_master directly here because it doesn't
+            # make sense to try and use ro_main_master (which might be
+            # returned if we use dbconfig.main_master).
             database_name = re.search(
-                    r"dbname=(\S*)", config.database.main_master).group(1)
+                r"dbname=(\S*)", dbconfig.rw_main_master).group(1)
             self._sendLine('STORE %d %s' % (size, name))
             self._sendHeader('Database-Name', database_name)
             self._sendHeader('Content-Type', str(contentType))

=== modified file 'lib/canonical/testing/ftests/test_mockdb.py'
--- lib/canonical/testing/ftests/test_mockdb.py	2009-08-21 17:43:28 +0000
+++ lib/canonical/testing/ftests/test_mockdb.py	2010-01-18 11:28:17 +0000
@@ -13,7 +13,7 @@
 import psycopg2
 # from zope.testing.testrunner import dont_retry, RetryTest
 
-from canonical.config import config
+from canonical.config import config, dbconfig
 from canonical.testing import mockdb, DatabaseLayer
 from canonical.testing.mockdb import ScriptPlayer, ScriptRecorder
 
@@ -92,7 +92,7 @@
         """Open a connection to the (possibly fake) database."""
         if connection_string is None:
             connection_string = "%s user=%s" % (
-                    config.database.main_master, config.launchpad.dbuser)
+                    dbconfig.rw_main_master, config.launchpad.dbuser)
         if self.mode == 'direct':
             con = psycopg2.connect(connection_string)
             #con = canonical.ftests.pgsql._org_connect(connection_string)
@@ -159,9 +159,9 @@
 
     # @dont_retry
     def testFailedConnection(self):
-        # Ensure failed database connections are reproducable.
+        # Ensure failed database connections are reproducible.
         for mode in self.modes():
-            connection_string = config.database.main_master
+            connection_string = dbconfig.rw_main_master
             connection_string = re.sub(
                     r"dbname=\S*", r"dbname=not_a_sausage", connection_string)
             self.assertRaises(

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2010-01-12 21:29:03 +0000
+++ lib/lp/translations/model/pofile.py	2010-01-18 11:28:17 +0000
@@ -24,12 +24,12 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.cachedproperty import cachedproperty
-from canonical.config import config
 from canonical.database.constants import UTC_NOW
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.sqlbase import (
     SQLBase, flush_database_updates, quote, quote_like, sqlvalues)
 from canonical.launchpad import helpers
+from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE, MASTER_FLAVOR)
@@ -146,8 +146,8 @@
     translation team for the given `IPOFile`.translationpermission and the
     language associated with this `IPOFile`.
     """
-    # Nothing can be edited in read-only mode.
-    if config.launchpad.read_only:
+    if is_read_only():
+        # Nothing can be edited in read-only mode.
         return False
 
     # If the person is None, then they cannot edit
@@ -191,8 +191,8 @@
     any logged-in user for translations in RESTRICTED mode that have a
     translation team assigned.
     """
-    # No suggestions can be added in read-only mode.
-    if config.launchpad.read_only:
+    if is_read_only():
+        # No suggestions can be added in read-only mode.
         return False
 
     if person is None: