Merge lp:~lamont/maas/bug-1567176 into lp:~maas-committers/maas/trunk

[LaMont Jones]

Harden space/fabric creation to avoid duplicate names, even temporarily.

Harden space/fabric creation to avoid duplicate names, even temporarily.

[lp:~lamont/maas/bug-1567176]

4902. By LaMont Jones on 2016-04-11

restore CleanSave.save before the assertEqual.

[LaMont Jones (lamont)]

The crux of the issue here is that if postgres is enforcing unique=True on name, then (since many things are not done inside of transactions), then default names become a source of conflict (the default name relies on the id, which is not known until the commit succeeds, and multiple branches can be in the middle of the insert-and-update logic at the same time.)

Making everything that inserts a space or fabric be transactional would be a solution, but I believe it's an invasive one to consider.

If we don't have the database enforce uniqueness, then we get near-uniqueness from the very sorts of situations that we run into above (where two inserts of spaces _with_ names that are the same, happen to pass in the night and both succeed.) TBF, this is an extremely unlikely case. If we do hit it, then we have the situation where two users create spaces (with the same name), and then whoever updates next fails, even though they didn't change the name....

[Mike Pontillo (mpontillo)]

I don't like the random numbers used here, but I don't have a better idea at the moment. We might have to let it slide unless someone else can think of a better way to do it.

You should be able to use self.patch() to mock rather than replacing methods with your mock by hand. This will ensure that all the proper cleanup happens, even in exception cases.

review: Needs Fixing

[Lee Trager (ltrager)]

Looks good, I'm just wondering if the retry loop can ever get into a deadlock state.

[Gavin Panella (allenap)]

Why is it important to give fabrics names like "fabric-$id", where $id
is the primary key? This seems arbitrary, is causing problems, and
exposing surrogate keys to users is often considered bad form anyway.

My suggestion is to use a "maasserve_fabric_name_id_seq" sequence:

  fabric_name_id = Sequence(
      "maasserver_fabric_name_id_seq", cycle=False,
      owner="maasserver_fabric.name")

   def generate_fabric_name():
       return "fabric-%d" % next(fabric_name_id)

Then there's no need for looping, checking for IntegrityError, and so
forth.

Another alternative might be to lock the table at the start of save():

  LOCK TABLE maasserver_fabric IN SHARE MODE

A SHARE lock will prevent concurrent data changes.

http://www.postgresql.org/docs/current/static/sql-lock.html
http://www.postgresql.org/docs/current/static/explicit-locking.html

I may have missed an important justification for what's going on in this
branch. However, on the basis of what I currently know, this branch is
an over-complicated fix to the problem.

> ... (since many things are not done inside of transactions) ...
[...]
> Making everything that inserts a space or fabric be transactional
> would be a solution, but I believe it's an invasive one to consider.

The intention in MAAS is that ALL database access is transactional, run
with at least REPEATABLE READ isolation. If you find something (not
including the test suite) that's writing to OR reading from the database
outside of an explicit transaction, it's highly likely to be a bug.

review: Disapprove

[Blake Rouse (blake-rouse)]

> Why is it important to give fabrics names like "fabric-$id", where $id
> is the primary key? This seems arbitrary, is causing problems, and
> exposing surrogate keys to users is often considered bad form anyway.

It is important that the fabric/space names come from the ID of the fabric. That way it makes since when the user is looking at the result of the API.

>
> My suggestion is to use a "maasserve_fabric_name_id_seq" sequence:
>
> fabric_name_id = Sequence(
> "maasserver_fabric_name_id_seq", cycle=False,
> owner="maasserver_fabric.name")

Doesn't postgres already create a sequence for the ID of the fabric/space. Why not just get the nextval of the fabric/space ID and set the ID and name in the initial INSERT call so its always correct. That will prevent the conflict and ensure that the ID and name always match. It would also simplify this logic a lot.

>
> def generate_fabric_name():
> return "fabric-%d" % next(fabric_name_id)
>
> Then there's no need for looping, checking for IntegrityError, and so
> forth.
>
> Another alternative might be to lock the table at the start of save():
>
> LOCK TABLE maasserver_fabric IN SHARE MODE
>
> A SHARE lock will prevent concurrent data changes.
>
> http://www.postgresql.org/docs/current/static/sql-lock.html
> http://www.postgresql.org/docs/current/static/explicit-locking.html
>
> I may have missed an important justification for what's going on in this
> branch. However, on the basis of what I currently know, this branch is
> an over-complicated fix to the problem.
>
> > ... (since many things are not done inside of transactions) ...
> [...]
> > Making everything that inserts a space or fabric be transactional
> > would be a solution, but I believe it's an invasive one to consider.
>
> The intention in MAAS is that ALL database access is transactional, run
> with at least REPEATABLE READ isolation. If you find something (not
> including the test suite) that's writing to OR reading from the database
> outside of an explicit transaction, it's highly likely to be a bug.

[Gavin Panella (allenap)]

> > Why is it important to give fabrics names like "fabric-$id", where
> > $id is the primary key? This seems arbitrary, is causing problems,
> > and exposing surrogate keys to users is often considered bad form
> > anyway.
>
> It is important that the fabric/space names come from the ID of the
> fabric. That way it makes since when the user is looking at the result
> of the API.

Typically a user ought not to be looking at the result of the API;
that's for machines. True, our CLI will currently show them all this,
but that won't be the case in future. It's not an absolute requirement,
but we should seek to hide surrogate keys from users.

[...]
> Doesn't postgres already create a sequence for the ID of the
> fabric/space. Why not just get the nextval of the fabric/space ID and
> set the ID and name in the initial INSERT call so its always correct.
> That will prevent the conflict and ensure that the ID and name always
> match. It would also simplify this logic a lot.

Yes, this is possible too.

[MAAS Lander (maas-lander)]

Transitioned to Git.

lp:maas has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone https://git.launchpad.net/maas

Unmerged revisions

4902. By LaMont Jones on 2016-04-11

restore CleanSave.save before the assertEqual.

4901. By LaMont Jones on 2016-04-11

Harden space/fabric creation to avoid duplicate names, even temporarily.

=== modified file 'src/maasserver/models/fabric.py'
--- src/maasserver/models/fabric.py	2016-04-07 19:37:40 +0000
+++ src/maasserver/models/fabric.py	2016-04-11 20:53:39 +0000
@@ -10,6 +10,7 @@
     ]
 
 import datetime
+import random
 import re
 
 from django.core.exceptions import (
@@ -209,24 +210,47 @@
 
     def save(self, *args, **kwargs):
         # Name will get set by clean_name() if None or empty, and there is an
-        # id. We just need to handle names here for creation.
+        # id. We just need to handle names here for creation.  It is possible
+        # that we could collide with another thread creating a fabric as well,
+        # that gets resolved by picking random almost-correct (reserved) names
+        # until the save works, then renaming it to the proper default name.
         created = self.id is None
-        super().save(*args, **kwargs)
-        if self.name is None or self.name == '':
+        saved = False
+        save_name = self.name
+        while not saved:
+            try:
+                super().save(*args, **kwargs)
+                saved = True
+            except IntegrityError as err:
+                # A leading zero still hits the reserved names, but won't
+                # actually be generated by us, regardless of the ID.  Pick
+                # random IDs until we get through.
+                if (err.args[0].startswith('duplicate key value violates')):
+                    self.name = 'fabric-0%d' % random.randint(1, 999999)
+        spcl = re.compile('^fabric-0\d+$')
+        if (save_name is None or save_name == '' or spcl.search(save_name)):
             # If we got here, then we have a newly created fabric that needs a
-            # default name.
-            self.name = "fabric-%d" % self.id
+            # default name.  Since we may have strayed from None above, we need
+            # to explicitly set it, rather than relying on clean_name to do so.
             fabric = Fabric.objects.get(id=self.id)
+            fabric.name = "fabric-%d" % self.id
+            self.name = fabric.name
             fabric.save()
         # Create default VLAN if this is a fabric creation.
         if created:
             self._create_default_vlan()
 
     def clean_name(self):
-        reserved = re.compile('^fabric-\d+$')
+        resvd = re.compile('^fabric-(\d+)$')
         if self.name is not None and self.name != '':
-            if reserved.search(self.name):
-                if (self.id is None or self.name != 'fabric-%d' % self.id):
+            res = resvd.search(self.name)
+            if res is not None:
+                num = res.group(1)
+                if self.id is None and num.startswith('0') and int(num) > 0:
+                    # "fabric-0\d+ is only valid for creation names, and will
+                    # get replaced in save with the default name.
+                    pass
+                elif self.id is None or self.name != 'fabric-%d' % self.id:
                     raise ValidationError(
                         {'name': ["Reserved fabric name."]})
         elif self.id is not None:
 
=== modified file 'src/maasserver/models/space.py'
--- src/maasserver/models/space.py	2016-04-07 19:37:40 +0000
+++ src/maasserver/models/space.py	2016-04-11 20:53:39 +0000
@@ -9,6 +9,7 @@
     ]
 
 import datetime
+import random
 import re
 
 from django.core.exceptions import (
@@ -155,10 +156,16 @@
             return "space-%s" % self.id
 
     def clean_name(self):
-        reserved = re.compile('^space-\d+$')
+        resvd = re.compile('^space-(\d+)$')
         if self.name is not None and self.name != '':
-            if reserved.search(self.name):
-                if (self.id is None or self.name != 'space-%d' % self.id):
+            res = resvd.search(self.name)
+            if res is not None:
+                num = res.group(1)
+                if self.id is None and num.startswith('0') and int(num) > 0:
+                    # "space-0\d+ is only valid for creation names, and will
+                    # get replaced in save with the default name.
+                    pass
+                elif self.id is None or self.name != 'space-%d' % self.id:
                     raise ValidationError(
                         {'name': ["Reserved space name."]})
         elif self.id is not None:
@@ -168,13 +175,29 @@
 
     def save(self, *args, **kwargs):
         # Name will get set by clean_name() if None or empty, and there is an
-        # id. We just need to handle names here for creation.
-        super().save(*args, **kwargs)
-        if self.name is None or self.name == '':
+        # id. We just need to handle names here for creation.  It is possible
+        # that we could collide with another thread creating a space as well,
+        # that gets resolved by picking random almost-correct (reserved) names
+        # until the save works, then renaming it to the proper default name.
+        saved = False
+        save_name = self.name
+        while not saved:
+            try:
+                super().save(*args, **kwargs)
+                saved = True
+            except IntegrityError as err:
+                # A leading zero means that we are creating a space with a
+                # default id.
+                if (err.args[0].startswith('duplicate key value violates')):
+                    self.name = 'space-0%d' % random.randint(1, 999999)
+        spcl = re.compile('^space-0\d+$')
+        if (save_name is None or save_name == '' or spcl.search(save_name)):
             # If we got here, then we have a newly created space that needs a
-            # default name.
-            self.name = "space-%d" % self.id
+            # default name.  Since we may have strayed from None above, we need
+            # to explicitly set it, rather than relying on clean_name to do so.
             space = Space.objects.get(id=self.id)
+            space.name = "space-%d" % self.id
+            self.name = space.name
             space.save()
 
     def clean(self, *args, **kwargs):
 
=== modified file 'src/maasserver/models/tests/test_fabric.py'
--- src/maasserver/models/tests/test_fabric.py	2016-04-07 19:37:40 +0000
+++ src/maasserver/models/tests/test_fabric.py	2016-04-11 20:53:39 +0000
@@ -6,6 +6,9 @@
 __all__ = []
 
 
+import random
+import re
+
 from django.core.exceptions import (
     PermissionDenied,
     ValidationError,
@@ -15,6 +18,7 @@
     INTERFACE_TYPE,
     NODE_PERMISSION,
 )
+from maasserver.models.cleansave import CleanSave
 from maasserver.models.fabric import (
     DEFAULT_FABRIC_NAME,
     Fabric,
@@ -154,6 +158,22 @@
         fabric = factory.make_Fabric(name=name)
         self.assertEqual(name, fabric.get_name())
 
+    def test_save_handles_collision(self):
+        real_save = CleanSave.save
+
+        def mock_save(inst, *args, **kwargs):
+            spcl = re.compile('^fabric-0\d+$')
+            if (inst.name is None or (
+                    spcl.search(inst.name) and random.randint(0, 1) == 1)):
+                raise IntegrityError(
+                    'duplicate key value violates unique constraint ')
+            else:
+                real_save(inst, *args, **kwargs)
+        CleanSave.save = mock_save
+        fabric = Fabric.objects.create(name=None)
+        CleanSave.save = real_save
+        self.assertEqual("fabric-%d" % fabric.id, fabric.name)
+
     def test_creates_fabric_with_default_vlan(self):
         name = factory.make_name('name')
         fabric = factory.make_Fabric(name=name)
 
=== modified file 'src/maasserver/models/tests/test_space.py'
--- src/maasserver/models/tests/test_space.py	2016-04-07 19:37:40 +0000
+++ src/maasserver/models/tests/test_space.py	2016-04-11 20:53:39 +0000
@@ -7,6 +7,9 @@
 __all__ = []
 
 
+import random
+import re
+
 from django.core.exceptions import (
     PermissionDenied,
     ValidationError,
@@ -14,6 +17,7 @@
 from django.db.models import ProtectedError
 from django.db.utils import IntegrityError
 from maasserver.enum import NODE_PERMISSION
+from maasserver.models.cleansave import CleanSave
 from maasserver.models.space import (
     DEFAULT_SPACE_NAME,
     Space,
@@ -165,6 +169,22 @@
             factory.make_Space,
             name='space-1999')
 
+    def test_save_handles_collision(self):
+        real_save = CleanSave.save
+
+        def mock_save(inst, *args, **kwargs):
+            spcl = re.compile('^space-0\d+$')
+            if (inst.name is None or (
+                    spcl.search(inst.name) and random.randint(0, 1) == 1)):
+                raise IntegrityError(
+                    'duplicate key value violates unique constraint ')
+            else:
+                real_save(inst, *args, **kwargs)
+        CleanSave.save = mock_save
+        space = Space.objects.create(name=None)
+        CleanSave.save = real_save
+        self.assertEqual("space-%d" % space.id, space.name)
+
     def test_create_sets_name(self):
         space = Space.objects.create(name=None)
         self.assertEqual("space-%d" % space.id, space.name)