500 ServerError trying to read EC object with duplicate frags

Bug #1624176 reported by clayg
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
High
Unassigned

Bug Description

If the proxy server encounters two fragments from the backend object servers that have the same X-Object-Sysmeta-Ec-Frag-Index when servicing a GET it will 500.

Luckily it's seemingly uncommon that duplicate fragments would be available at the same time [1] [2]; and normally the reconstructor can be expected to suss it out eventually.

1. After a fragment is PUT to handoffs; if there is a failure during revert that leaves the handoff still holding the frag index not also available on the primary; I attached a probe test like this
2. After a rebalance if an old primary is a new handoff; or a holding handoff becomes a primary - in either case the new primary could get rebuilt before the holding node can revert.

Revision history for this message
clayg (clay-gerrard) wrote :
Revision history for this message
clayg (clay-gerrard) wrote :

this is a very specific failure mode that arrises during a realistic situation which could result in the more complex requirement described in lp bug #1484598

Revision history for this message
clayg (clay-gerrard) wrote :

It just so happens this probetest will pass with https://review.openstack.org/#/c/215276

Revision history for this message
clayg (clay-gerrard) wrote :

... then again - you could also fix it just by using a dict instead of a list

https://gist.github.com/clayg/9d6e32f1a9eabbfa1aa6c61c53a87915

Revision history for this message
Kota Tsuyuzaki (tsuyuzaki-kota) wrote :

Though not yet trying to the test you suggested above, if Alistair's optimistic GET land soon, sound like [1][2] is simpler solution to resolve this. Since I've been working for fragment duplication, I added the check (no duplicated fragments for a response bucket) in my patch.

1: https://review.openstack.org/#/c/219165/34/swift/proxy/controllers/obj.py@1897
2: https://review.openstack.org/#/c/219165/33/swift/proxy/controllers/obj.py@1893

Revision history for this message
Kota Tsuyuzaki (tsuyuzaki-kota) wrote :

Optimistic GET patch is https://review.openstack.org/#/c/215276, I mean.

Revision history for this message
Kota Tsuyuzaki (tsuyuzaki-kota) wrote :

Hi, Clay. I figured out this problem has been resolved with https://review.openstack.org/#/c/215276 on the newest master. (my change suggested at comment #5 is not necessary though)

Current master has already had the unit tests such a duplicated fragment indexes on GET objects. Exactly, before the patch 215276 landed, the duplicated fragments will cause a 500 Internal Server error. I figured out it with [2]. The procedure is 1. remove change, 2. apply the test[1] 3.run the test. exactly that failed. However, the same test is now success on the master so that no matter exists on upstream swift, IMO.

Hence, I'd like to make this fix committed or invalid stats (maybe fix committed is better?) but until Clay confirmed that, I changes the stats to "incomplete" right now.

1: https://github.com/openstack/swift/blob/master/test/unit/proxy/controllers/test_obj.py#L2670
2: https://gist.github.com/bloodeagle40234/1645bc22703e335eab61529f700e4b17

Changed in swift:
status: Confirmed → Incomplete
Revision history for this message
Alistair Coles (alistair-coles) wrote :

The bug is fixed by https://review.openstack.org/#/c/215276 (merged) but that patch did not include a test. There is a patch with Clay's test which closes this bug https://review.openstack.org/#/c/371771/

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (master)

Reviewed: https://review.openstack.org/371771
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=116462d8a5a45ec876403234cf83b96dfad58ddd
Submitter: Jenkins
Branch: master

commit 116462d8a5a45ec876403234cf83b96dfad58ddd
Author: Clay Gerrard <email address hidden>
Date: Fri Sep 16 11:58:46 2016 -0700

    Add probetest for response with duplicate frags

    When an SSYNC connection fails after shipping a fragment, but before
    cleaning itself up - it can lead to multiple replicas of the same
    fragment index serviceable in the node_iter at the same time. Or
    perhaps more likely if a partner nodes win a race to rebuild waiting
    on a handoff. In this case, the proxy need not fail to service a GET
    just because of extra information. A similar guard was added to the
    reconstructor in a related change [1].

    This underlying bug is acctually fixed by the related change [2], this
    probetest is just encoding the failure to help with future maintenance.

    1. Related-Change: I91f3d4af52cbc66c9f7ce00726f247b5462e66f9
    2. Related-Change: I2310981fd1c4622ff5d1a739cbcc59637ffe3fc3

    Closes-Bug: 1624176

    Change-Id: I06fc381a25613585dd18916d50e7ca2c68d406b6

Changed in swift:
status: Incomplete → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/swift 2.11.0

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (feature/hummingbird)

Fix proposed to branch: feature/hummingbird
Review: https://review.openstack.org/400985

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (feature/hummingbird)
Download full text (78.0 KiB)

Reviewed: https://review.openstack.org/400985
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=0c3f8f87104af8717115c5badffd243dbaa1c430
Submitter: Jenkins
Branch: feature/hummingbird

commit 2d25fe6ad3573b2a06b6b3e5e66493d7b0c55693
Author: Tim Burke <email address hidden>
Date: Mon Jul 25 15:06:23 2016 -0700

    Reduce backend requests for SLO If-Match / HEAD requests

    ... by storing SLO Etag and size in sysmeta.

    Previously, we had to GET the manifest for every HEAD or conditional
    request to an SLO. Worse, since SLO PUTs require that we HEAD every
    segment, we'd GET all included sub-SLO manifests. This was necessary so
    we could recompute the large object's Etag and content-length.

    Since we already know both of those during PUT, we'll now store it in
    object sysmeta. This allows us to:

     * satisfy HEAD requests based purely off of the manifest's HEAD
       response, and
     * perform the If-(None-)Match comparison on the object server, without
       any additional subrequests.

    Note that the large object content-length can't just be parsed from
    content-type -- with fast-POST enabled, the content-type coming out of
    the object-server won't necessarily include swift_bytes.

    Also note that we must still fall back to GETting the manifest if the
    sysmeta headers were not found. Otherwise, we'd break existing large
    objects.

    Change-Id: Ia6ad32354105515560b005cea750aa64a88c96f9

commit ae7dddd801e28217d7dc46bd45cd6b621f29340c
Author: Ondřej Nový <email address hidden>
Date: Mon Nov 21 22:13:11 2016 +0100

    Added comment for "user" option in drive-audit config

    Change-Id: I24362826bee85ac3304e9b63504c9465da673014

commit c3e1d847f4b9d6cc6212aae4dc1b1e6dff45fb40
Author: Thiago da Silva <email address hidden>
Date: Thu Nov 17 17:17:00 2016 -0500

    breaking down tests.py into smaller pieces

    tests.py is currently at ~5500 lines of code, it's
    time to break it down into smaller files.

    I started with an easy middleware set of tests
    (i.e., versioned writes, ~600 lines of code ) so I can get
    some feedback. There are more complicated tests that cover
    multiple middlewares for example, it is not so clear where
    those should go.

    Change-Id: I2aa6c18ee5b68d0aae73cc6add8cac6fbf7f33da
    Signed-off-by: Thiago da Silva <email address hidden>

commit 5d7a3a4172f0f11ab870252eec784cf24b247dea
Author: Ondřej Nový <email address hidden>
Date: Sat Nov 19 23:24:30 2016 +0100

    Removed "in-process-" from func env tox name

    This shorten shebang in infra, because we are hitting 128 bytes limit.

    Change-Id: I02477d81b836df71780942189d37d616944c4dce

commit 9ea340256996a03c8c744201297b47a0e91fe65b
Author: Kota Tsuyuzaki <email address hidden>
Date: Fri Nov 18 01:50:11 2016 -0800

    Don't overwrite built-in 'id'

    This is a follow up for https://review.openstack.org/#/c/399237

    'id' is assigned as a builtin function so that we should not use 'id'
    for the local variable name.

    Change-Id: Ic27460d49e68f6cd50bda1d5b3810e01ccb07a37

commit bf...

tags: added: in-feature-hummingbird
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.