Container cache management is racy

[Thomas Herve]

I've noticed that the way Swift handles cache for container existence is prone to race conditions, if you have several operations in parallel.

Here's the reproducer I made: http://paste.openstack.org/show/620415/
It fails pretty consistenly on my devstack with default setting, swift master at 6da17e9.

Basically, a GET container concurrent to a PUT on that container can set the cache in memcache to NotFound, which means that later putting an object in that container will fail.

It's possible that I'm misusing something, or that maybe I shouldn't be using cache, but it feels like a bug.

[clayg (clay-gerrard)]

So, first of all, yes container existence is subject to cache race [1] - but it should work out most of the time generally because of the way we do an optimistic cache invalidation [2] and will always be correct eventually by design.

Honestly the provided script did not work for me, but even trying to modify it [3] I couldn't get it to reproduce any error in a single vm test environment - which makes me think the issue you're experiencing is related to a misconfiguration of the memcache ring for a multi-node environment [4].

I know first hand it can be a big jump to move from single node development/test environments into multi-node pre-production lab and staging environments, and readily admit there's probably lots of ways that we could improve the code and documentation. Perhaps jump in #openstack-swift on Freenode or shoot something out to the mailing list [5] and we can look more closely at what's going on!

1. see recheck_container_existence https://docs.openstack.org/swift/latest/deployment_guide.html#proxy-server &
2. https://github.com/openstack/swift/blob/6da17e992375cba37035527e69ef17aa6a5d6f28/swift/proxy/controllers/container.py#L180
3. https://gist.github.com/clayg/f5e03819315a886c3c35bae6df59734d
4. see memcache_servers https://docs.openstack.org/swift/latest/deployment_guide.html#proxy-server & https://github.com/openstack/swift/blob/master/etc/memcache.conf-sample
5. https://wiki.openstack.org/wiki/Mailing_Lists#Operators

[Thomas Herve (therve)]

Thanks for your response. My script had an error indeed, but it was just missing an "import os". Using your script and removing the time import reproduced it almost immediatel.

My environment is a single VM as well, latest devstack. The environment where I see the related issue is single node as well, so it's not a multi node environment. I looked into the container existence header, but AFAICT it doesn't fix the issue.

[clayg (clay-gerrard)]

might be dependent on how long the requests take compared to each other, we might audit where in the request cycle we're performing the cache invalidation:

https://gist.github.com/clayg/ecaa44089599cfb7ace34ee758f467a5

[Thomas Herve (therve)]

Moving the cache clear call as suggested seems to fix the issue for me: http://paste.openstack.org/show/620440/

[Christian Schwede (cschwede)]

Thanks Thomas for the extensive debugging!

I can confirm this on another deployment with TripleO - same behaviour.

And moving the cache clear call as Thomas suggests fixes that issue.

[clayg (clay-gerrard)]

Fix is here:

https://review.openstack.org/#/c/500978/

... currently waiting on some tests

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/505557

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/500978
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=244d7de388e7a62f488e9272efcb9adba8021274
Submitter: Jenkins
Branch: master

commit 244d7de388e7a62f488e9272efcb9adba8021274
Author: Thomas Herve <email address hidden>
Date: Tue Sep 5 22:42:32 2017 +0200

    Delay cache invalidation during container creation

    Having the cache being cleared before the PUT request creates a
    fairly big window where the cache can be inconsistent, if a concurrent
    GET happens. Let's move the cache clear after the requests to reduce it.

    Change-Id: I45130cc32ba3a23272c2a67c86b4063000379426
    Closes-Bug: #1715177

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/pike
Review: https://review.openstack.org/505700

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/505557
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=8556b06bf75e46369088f1cc6e2aa5d6cc00251b
Submitter: Jenkins
Branch: master

commit 8556b06bf75e46369088f1cc6e2aa5d6cc00251b
Author: Christian Schwede <email address hidden>
Date: Wed Sep 20 10:56:41 2017 +0200

    Add test to ensure cache cleared after container PUT

    The parent commit fixes a race condition. Let's make sure there won't be
    a regression in the future, thus testing the order to ensure the cache
    is cleared after the request is executed.

    Related-Bug: #1715177
    Change-Id: I4f6750b7c556b498da0a2b56aa6c8cee5e42a90c

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: feature/deep
Review: https://review.openstack.org/505844

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/505844
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=67b3a747869ad1727d88cb77fa65858a3c98f8a5
Submitter: Jenkins
Branch: feature/deep

commit 8556b06bf75e46369088f1cc6e2aa5d6cc00251b
Author: Christian Schwede <email address hidden>
Date: Wed Sep 20 10:56:41 2017 +0200

    Add test to ensure cache cleared after container PUT

    The parent commit fixes a race condition. Let's make sure there won't be
    a regression in the future, thus testing the order to ensure the cache
    is cleared after the request is executed.

    Related-Bug: #1715177
    Change-Id: I4f6750b7c556b498da0a2b56aa6c8cee5e42a90c

commit 6b19ca7a7d5833f5648976d8d30c776975e361db
Author: Tim Burke <email address hidden>
Date: Fri Sep 15 22:52:26 2017 +0000

    proxy: Use the right ranges when going to multiple object servers

    When the proxy times out talking to a backend server (say, because it
    was under heavy load and having trouble servicing the request), we catch
    the ChunkReadTimeout and try to get the rest from another server. The
    client by and large doesn't care; there may be a brief pause in the
    download while the proxy get the new connection, but all the bytes
    arrive and in the right order:

        GET from node1, serve bytes 0 through N, timeout
        GET from node2, serve bytes N through end

    When we calculate the range for the new request, we check to see if we
    already have one from the previous request -- if one exists, we adjust
    it based on the bytes sent to the client thus far. This works fine for
    single failures, but if we need to go back *again* we double up the
    offset and send the client incomplete, bad data:

        GET from node1, serve bytes 0 through N, timeout
        GET from node2, serve bytes N through M, timeout
        GET from node3, serve bytes N + M through end

    Leaving the client missing bytes M through N + M.

    We should adjust the range based on the number of bytes pulled from the
    *backend* rather than delivered to the *frontend*. This just requires
    that we reset our book-keeping after adjusting the Range header.

    Change-Id: Ie153d01479c4242c01f48bf0ada78c2f9b6c8ff0
    Closes-Bug: 1717401

commit d6fcf7459435077b525123e8b78e553070d5c070
Author: Kota Tsuyuzaki <email address hidden>
Date: Sat Sep 16 04:58:31 2017 +0900

    Make gate keeper to save relative location header path

    Why we need this:
      Some middlewares want to keep HTTP Location header as relative path
      (e.g. using Load balancer in front of proxy).

    What is the problem in current Swift:
      Current Swift already has the flag to keep it as relative when returning
      the reponse using swift.common.swob.Response. However, auth_token middleware,
      that is from keystonemiddleware, unfortunately can change the relative path
      to absolute because of using webob instead of swob.

    What this patch is doing:
      Make gate_keeper able to re-transform the location header from absolute path
      to relative path if 'swift.leave_relative_location' is explicitely set...

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/505700
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=69a7be65bee876507513d437e555d4de341fe076
Submitter: Jenkins
Branch: stable/pike

commit 69a7be65bee876507513d437e555d4de341fe076
Author: Thomas Herve <email address hidden>
Date: Tue Sep 5 22:42:32 2017 +0200

    Delay cache invalidation during container creation

    Having the cache being cleared before the PUT request creates a
    fairly big window where the cache can be inconsistent, if a concurrent
    GET happens. Let's move the cache clear after the requests to reduce it.

    Change-Id: I45130cc32ba3a23272c2a67c86b4063000379426
    Closes-Bug: #1715177
    (cherry picked from commit 244d7de388e7a62f488e9272efcb9adba8021274)

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: feature/s3api
Review: https://review.openstack.org/512277

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: feature/s3api
Review: https://review.openstack.org/512283

[OpenStack Infra (hudson-openstack)]

Change abandoned by Alistair Coles (<email address hidden>) on branch: feature/s3api
Review: https://review.openstack.org/512283
Reason: I was just trying to get sensible topic

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/512277
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=f94d6567a7e2e8b3ca1168b4a41c42c1ee371af5
Submitter: Zuul
Branch: feature/s3api

commit 24188beb81d39790034fa0902246163a7bf54c91
Author: Samuel Merritt <email address hidden>
Date: Thu Oct 12 16:13:25 2017 -0700

    Remove some leftover threadpool cruft.

    Change-Id: I43a1a428bd96a2e18aac334c03743a9f94f7d3e1

commit 1d67485c0b935719e0c8999eb353dfd84713add6
Author: Samuel Merritt <email address hidden>
Date: Fri Apr 15 12:43:44 2016 -0700

    Move all monkey patching to one function

    Change-Id: I2db2e53c50bcfa17f08a136581cfd7ac4958ada2

commit 407f5394f0f5cb422c06b4e5b2f9fbfdb07782d1
Author: OpenStack Proposal Bot <email address hidden>
Date: Thu Oct 12 08:12:38 2017 +0000

    Imported Translations from Zanata

    For more information about this automatic import see:
    https://docs.openstack.org/i18n/latest/reviewing-translation-import.html

    Change-Id: I628cb09aa78d8e339b4762a3c9ed8aed43941261

commit 45ca39fc68cdb42b382c1638a92cc8d3cec5529a
Author: Clay Gerrard <email address hidden>
Date: Tue Oct 10 11:47:50 2017 -0700

    add mangle_client_paths to example config

    Change-Id: Ic1126fc95e8152025fccf25356c253facce3e3ec

commit 94bac4ab2fe65104d602378e8e49c37b8187a75d
Author: Tim Burke <email address hidden>
Date: Fri May 12 10:55:21 2017 -0400

    domain_remap: stop mangling client-provided paths

    The root_path option for domain_remap seems to serve two purposes:
     - provide the first component (version) for the backend request
     - be an optional leading component for the client request, which
       should be stripped off

    As a result, we have mappings like:

       c.a.example.com/v1/o -> /v1/AUTH_a/c/o

    instead of

       c.a.example.com/v1/o -> /v1/AUTH_a/c/v1/o

    which is rather bizarre. Why on earth did we *ever* start doing this?

    Now, this second behavior is managed by a config option
    (mangle_client_paths) with the default being to disable it.

    Upgrade Consideration
    =====================

    If for some reason you *do* want to drop some parts of the
    client-supplied path, add

       mangle_client_paths = True

    to the [filter:domain_remap] section of your proxy-server.conf. Do this
    before upgrading to avoid any loss of availability.

    UpgradeImpact
    Change-Id: I87944bfbf8b767e1fc36dbc7910305fa1f11eeed

commit a4a5494fd2fe8a43a5d50a21a1951266cc7c4212
Author: Alistair Coles <email address hidden>
Date: Mon Oct 9 11:33:28 2017 +0100

    test account autocreate listing format

    Related-Change: Id3ce37aa0402e2d8dd5784ce329d7cb4fbaf700d
    Change-Id: I50c22225bbebff71600bea9158bda1edd18b48b0

commit 8b7f15223cde4c19fd9cbbd97e8ad79a1b4afa8d
Author: Alistair Coles <email address hidden>
Date: Mon Oct 9 10:06:19 2017 +0100

    Add example to container-sync-realms.conf.5 man page

    Related-Change: I0760ce149e6d74f2b3f1badebac3e36da1ab7e77

    Change-Id: I129de42f91d7924c7bcb9952f17fe8a1a10ae219

commit 816331155c624c444ed123bcab412...

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/swift 2.16.0 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/swift 2.15.2 release.