Merge lp:~andreserl/maas/fix_lp1382575 into lp:~maas-committers/maas/trunk

[Andres Rodriguez]

Use ISOLATION_LEVEL_SERIALIZABLE instead of ISOLATION_LEVEL_READ_COMMIT

[Gavin Panella (allenap)]

We can't do this without code retry failed transactions. This level of isolation will cause all concurrent updates to the same rows in the database (except the first to commit) to fail. We need a way to automatically retry the operation when those failures happen, otherwise users will just see what appear to be spurious failures. This is something learned from Launchpad, which uses this isolation level with PostgreSQL, and also learned on MAAS when it was attempted before.

review: Disapprove

[Andres Rodriguez (andreserl)]

Gavin,

You do realize that the change you made to the DB settings are creating a critical regression in MAAS that will have a critical impact in ALL deployments right? With the change you made, you are re-introducing bug [1]. This issue is critical and will affect all of our internal deployments and customer's.

[1]: https://bugs.launchpad.net/juju-core/+bug/1314409

[Gavin Panella (allenap)]

As discussed elsewhere, we never ran SERIALIZABLE in the production configurations. The change to maas_local_settings.py to specify READ COMMITTED was only making explicit what was already the case. My -1 vote isn't an attempt to stop conversation though because it's possible I've misunderstood some side-effect, or that I'm just plain wrong. I'd rather figure this out than be right.

[Julian Edwards (julian-edwards)]

On Sunday 19 Oct 2014 22:31:14 you wrote:
> As discussed elsewhere, we never ran SERIALIZABLE in the production
> configurations. The change to maas_local_settings.py to specify READ
> COMMITTED was only making explicit what was already the case. My -1 vote
> isn't an attempt to stop conversation though because it's possible I've
> misunderstood some side-effect, or that I'm just plain wrong. I'd rather
> figure this out than be right.

Agreed.

The lock hack will fix the acquisition bug for now but we need to put in
SERIALIZABLE with a retry mechanism, and a decent error page for when the
retries fail too.

I think this is perfectly do-able for the next point release and in fact we
should try to do this as soon as possible after utopic's beta is released so
that we have a longer testing and bedding-in period.

[Raphaël Badin (rvb)]

I agree with Gavin and Julian here. What I think we should do is:
- get rid of the deprecated TransactionMiddleware (that might be the source of some of the bugs we're seeing); using ATOMIC_REQUESTS=True in the config might be a drop in replacement (but we might want to control this manually, see my next point); this is mostly a cleanup, to make sure we're not mixing deprecated utilities with more recent utilities (the transaction stuff in Django changed quite a lot in 1.6)
- as Gavin and Julian suggested, implement a retry mechanism; A middleware should be able to do this.
- switch to SERIALIZABLE

[Gavin Panella (allenap)]

On 20 October 2014 08:48, Raphaël Badin wrote:
> I agree with Gavin and Julian here. What I think we should do is:
> - get rid of the deprecated TransactionMiddleware (that might be the
> source of some of the bugs we're seeing); using ATOMIC_REQUESTS=True
> in the config might be a drop in replacement (but we might want to
> control this manually, see my next point); this is mostly a cleanup,
> to make sure we're not mixing deprecated utilities with more recent
> utilities (the transaction stuff in Django changed quite a lot in
> 1.6)

There is a problem with ATOMIC_REQUESTS: the atomic decorator prevents
explicit commits (by "official" channels at least) while it's active.
Even if we switch to SERIALIZABLE we will still want to explicitly
commit before making some RPC calls.

There is a way around this: decorate a view with non_atomic_requests.
However, the explicit commits we have so far are in the model layer, not
the view, so there probably won't be a nice 1:1 mapping between where we
need to commit and view.

> - as Gavin and Julian suggested, implement a retry mechanism; A
> middleware should be able to do this.
> - switch to SERIALIZABLE

We should consider REPEATABLE READ too. I don't know enough about it in
PostgreSQL to say more, but it was suggested to me.

[Raphaël Badin (rvb)]

On 10/20/2014 10:24 AM, Gavin Panella wrote:
> On 20 October 2014 08:48, Raphaël Badin wrote:
>> I agree with Gavin and Julian here. What I think we should do is:
>> - get rid of the deprecated TransactionMiddleware (that might be the
>> source of some of the bugs we're seeing); using ATOMIC_REQUESTS=True
>> in the config might be a drop in replacement (but we might want to
>> control this manually, see my next point); this is mostly a cleanup,
>> to make sure we're not mixing deprecated utilities with more recent
>> utilities (the transaction stuff in Django changed quite a lot in
>> 1.6)
>
> There is a problem with ATOMIC_REQUESTS: the atomic decorator prevents
> explicit commits (by "official" channels at least) while it's active.
> Even if we switch to SERIALIZABLE we will still want to explicitly
> commit before making some RPC calls.
>
> There is a way around this: decorate a view with non_atomic_requests.
> However, the explicit commits we have so far are in the model layer, not
> the view, so there probably won't be a nice 1:1 mapping between where we
> need to commit and view.

This is a good point. We need to investigate if switching to
ATOMIC_REQUESTS and using non_atomic_requests for the views we want to
control manually is simpler than the alternative: controlling things
manually everywhere.

>
>> - as Gavin and Julian suggested, implement a retry mechanism; A
>> middleware should be able to do this.
>> - switch to SERIALIZABLE
>
> We should consider REPEATABLE READ too. I don't know enough about it in
> PostgreSQL to say more, but it was suggested to me.

Agreed. The problem we're seeing with a node being acquired twice is,
precisely, a non-repeatable read.

Unmerged revisions

3269. By Andres Rodriguez on 2014-10-19

Revert commits r3206 and r3225 to use ISOLATION_LEVEL_SERIALIZABLE again

=== modified file 'contrib/maas_local_settings.py'
--- contrib/maas_local_settings.py	2014-10-08 22:06:20 +0000
+++ contrib/maas_local_settings.py	2014-10-19 11:46:21 +0000
@@ -74,9 +74,6 @@
 }
 
 # Database access configuration.
-from psycopg2.extensions import ISOLATION_LEVEL_READ_COMMITTED
-
-
 DATABASES = {
     'default': {
         # 'postgresql_psycopg2', 'postgresql', 'mysql', 'sqlite3' etc.
@@ -85,8 +82,5 @@
         'USER': '',
         'PASSWORD': '',
         'HOST': 'localhost',
-        'OPTIONS': {
-            'isolation_level': ISOLATION_LEVEL_READ_COMMITTED,
-        },
     }
 }

=== modified file 'src/maas/development.py'
--- src/maas/development.py	2014-10-08 11:01:44 +0000
+++ src/maas/development.py	2014-10-19 11:46:21 +0000
@@ -26,7 +26,7 @@
 from metadataserver.address import guess_server_host
 import provisioningserver.config
 from provisioningserver.utils import compose_URL
-from psycopg2.extensions import ISOLATION_LEVEL_READ_COMMITTED
+from psycopg2.extensions import ISOLATION_LEVEL_SERIALIZABLE
 
 # We expect the following settings to be overridden. They are mentioned here
 # to silence lint warnings.
@@ -70,7 +70,7 @@
         # Unix socket directory.
         'HOST': abspath('db'),
         'OPTIONS': {
-            'isolation_level': ISOLATION_LEVEL_READ_COMMITTED,
+            'isolation_level': ISOLATION_LEVEL_SERIALIZABLE,
         },
     },
 }

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2014-10-15 20:39:07 +0000
+++ src/maas/settings.py	2014-10-19 11:46:21 +0000
@@ -23,7 +23,7 @@
 from maas.monkey import patch_get_script_prefix
 from metadataserver.address import guess_server_host
 from provisioningserver.utils import compose_URL
-from psycopg2.extensions import ISOLATION_LEVEL_READ_COMMITTED
+from psycopg2.extensions import ISOLATION_LEVEL_SERIALIZABLE
 
 
 django.template.add_to_builtins('django.templatetags.future')
@@ -128,7 +128,7 @@
         'HOST': '',
         'PORT': '',
         'OPTIONS': {
-            'isolation_level': ISOLATION_LEVEL_READ_COMMITTED,
+            'isolation_level': ISOLATION_LEVEL_SERIALIZABLE,
         },
     },
 }