Fix race on container recreate

There was a path on container recreate that would sometimes allow db to get
reinitialized without updating put_timestamp.  Replication would of course fix
it up, but that node would think it's database was deleted till then desipite
just ok'ing a request with a newer X-Timestamp than the deleted_timestamp on
disk.

Change-Id: I8b98afb2aac2e433b6ecb5c421ba0d778cef42fa
Closes-Bug: #1292784

swift/container/server.py

@@ -227,6 +227,20 @@ class ContainerController(object):
                 return HTTPNoContent(request=req)
             return HTTPNotFound()
 
+    def _update_or_create(self, req, broker, timestamp):
+        if not os.path.exists(broker.db_file):
+            try:
+                broker.initialize(timestamp)
+            except DatabaseAlreadyExists:
+                pass
+            else:
+                return True  # created
+        created = broker.is_deleted()
+        broker.update_put_timestamp(timestamp)
+        if broker.is_deleted():
+            raise HTTPConflict(request=req)
+        return created
+
     @public
     @timing_stats()
     def PUT(self, req):
@@ -261,17 +275,7 @@ class ContainerController(object):
                               req.headers['x-etag'])
             return HTTPCreated(request=req)
         else:   # put container
-            if not os.path.exists(broker.db_file):
+            created = self._update_or_create(req, broker, timestamp)
-                try:
-                    broker.initialize(timestamp)
-                    created = True
-                except DatabaseAlreadyExists:
-                    created = False
-            else:
-                created = broker.is_deleted()
-                broker.update_put_timestamp(timestamp)
-                if broker.is_deleted():
-                    return HTTPConflict(request=req)
             metadata = {}
             metadata.update(
                 (key, (value, timestamp))

test/unit/container/test_server.py

@@ -30,7 +30,8 @@ import simplejson
 from swift.common.swob import Request, HeaderKeyDict
 import swift.container
 from swift.container import server as container_server
-from swift.common.utils import normalize_timestamp, mkdirs, public, replication
+from swift.common.utils import (normalize_timestamp, mkdirs, public,
+                                replication, lock_parent_directory)
 from test.unit import fake_http_connect
 from swift.common.request_helpers import get_sys_meta_prefix
 
@@ -764,6 +765,91 @@ class TestContainerController(unittest.TestCase):
         resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 404)
 
+    def test_DELETE_PUT_recreate(self):
+        path = '/sda1/p/a/c'
+        req = Request.blank(path, method='PUT',
+                            headers={'X-Timestamp': '1'})
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 201)
+        req = Request.blank(path, method='DELETE',
+                            headers={'X-Timestamp': '2'})
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 204)
+        req = Request.blank(path, method='GET')
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 404)  # sanity
+        db = self.controller._get_container_broker('sda1', 'p', 'a', 'c')
+        self.assertEqual(True, db.is_deleted())
+        info = db.get_info()
+        self.assertEquals(info['put_timestamp'], normalize_timestamp('1'))
+        self.assertEquals(info['delete_timestamp'], normalize_timestamp('2'))
+        # recreate
+        req = Request.blank(path, method='PUT',
+                            headers={'X-Timestamp': '4'})
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 201)
+        db = self.controller._get_container_broker('sda1', 'p', 'a', 'c')
+        self.assertEqual(False, db.is_deleted())
+        info = db.get_info()
+        self.assertEquals(info['put_timestamp'], normalize_timestamp('4'))
+        self.assertEquals(info['delete_timestamp'], normalize_timestamp('2'))
+
+    def test_DELETE_PUT_recreate_replication_race(self):
+        path = '/sda1/p/a/c'
+        # create a deleted db
+        req = Request.blank(path, method='PUT',
+                            headers={'X-Timestamp': '1'})
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 201)
+        db = self.controller._get_container_broker('sda1', 'p', 'a', 'c')
+        req = Request.blank(path, method='DELETE',
+                            headers={'X-Timestamp': '2'})
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 204)
+        req = Request.blank(path, method='GET')
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 404)  # sanity
+        self.assertEqual(True, db.is_deleted())
+        # now save a copy of this db (and remove it from the "current node")
+        db = self.controller._get_container_broker('sda1', 'p', 'a', 'c')
+        db_path = db.db_file
+        other_path = os.path.join(self.testdir, 'othernode.db')
+        os.rename(db_path, other_path)
+        # that should make it missing on this node
+        req = Request.blank(path, method='GET')
+        resp = req.get_response(self.controller)
+        self.assertEquals(resp.status_int, 404)  # sanity
+
+        # setup the race in os.path.exists (first time no, then yes)
+        mock_called = []
+        _real_exists = os.path.exists
+
+        def mock_exists(db_path):
+            rv = _real_exists(db_path)
+            if not mock_called:
+                # be as careful as we might hope backend replication can be...
+                with lock_parent_directory(db_path, timeout=1):
+                    os.rename(other_path, db_path)
+            mock_called.append((rv, db_path))
+            return rv
+
+        req = Request.blank(path, method='PUT',
+                            headers={'X-Timestamp': '4'})
+        with mock.patch.object(container_server.os.path, 'exists',
+                               mock_exists):
+            resp = req.get_response(self.controller)
+        # db was successfully created
+        self.assertEqual(resp.status_int // 100, 2)
+        db = self.controller._get_container_broker('sda1', 'p', 'a', 'c')
+        self.assertEqual(False, db.is_deleted())
+        # mock proves the race
+        self.assertEqual(mock_called[:2],
+                         [(exists, db.db_file) for exists in (False, True)])
+        # info was updated
+        info = db.get_info()
+        self.assertEquals(info['put_timestamp'], normalize_timestamp('4'))
+        self.assertEquals(info['delete_timestamp'], normalize_timestamp('2'))
+
     def test_DELETE_not_found(self):
         # Even if the container wasn't previously heard of, the container
         # server will accept the delete and replicate it to where it belongs