node timeout on overwrite can easily cause mis-matched etag fragment to 503

Bug #1457691 reported by Kota Tsuyuzaki
18
This bug affects 3 people
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
High
paul luse

Bug Description

On EC GET object, insufficient fragments error occurred on decoding on PyECLib.

For example, k=2, m=1 and then, an object was overwritten sometimes (e.g. post_as_copy):

Timesampe1:

Assuming the fragments located as follows (integer means fragment index):

primary handoff
[0][1][2] [0]

(unfortunately some PUT failed and located index 0 in handoff)

Timestamp2:

GET object requested but node 1 and 2 failed by something like connection timeout.

Proxy will retrieve 2 enough ndata stream fron object-server but it will be actually both of them are index 0.

That will go to insufficient fragments to decode :(

CVE References

tags: added: ec
Revision history for this message
Bill Huber (wbhuber) wrote :

Investigating this bug.

Revision history for this message
paul luse (paul-e-luse) wrote :

FYI we are not feature complete yet wrt GET handling of previous partial PUTs. Feel free to investigate, will share design thoughts on general handling of the incomplete scenarios next week sometime. This sounds like it might be covered by pending implementation

Bill Huber (wbhuber)
Changed in swift:
assignee: nobody → Bill Huber (wbhuber)
clayg (clay-gerrard)
Changed in swift:
importance: Undecided → High
Revision history for this message
clayg (clay-gerrard) wrote :

failure reproduction would include setting up a probe test like this:

    ec 4+2 scheme - 8 devices

             p0 p1 p2 p3 p4 p5 p6 h0 h1 h2 h3
    t0 PUT t0#0 t0#1 X t0#3 X t0#5 t0#6 t0#2 t0#4
    t1 PUT t1#0 X t1#2 X t1#4 t1#5 t1#6 t1#1 t1#3
        GET t1#0 t0#1 t1#2 t0#3 t1#4

Even better would be a unittest that would let us describe a list of a *variety* of backend 404s/timestamp/frag-index'es responses and validate proxy responses...

Revision history for this message
clayg (clay-gerrard) wrote :
Changed in swift:
status: New → Confirmed
Revision history for this message
clayg (clay-gerrard) wrote :

Related failure condition is when all of the fragments are *available* but some of the object servers think they are not *durable* (lp bug #1469094)

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

One of the simplest reproducible failure conditions is an overwrite the goes to a handoff.

If one fragment from an old version of the object manages to stick around and get picked up on the subsequent GET the whole request will fail - even though asking for another fragment from almost *any* node would have returned enough fragments to rebuild the object.

Probetest attached.

Revision history for this message
paul luse (paul-e-luse) wrote :

FYI this WIP patch will be related for sure... https://review.openstack.org/#/c/201283/

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

Fix proposed to branch: master
Review: https://review.openstack.org/201791

Changed in swift:
status: Confirmed → In Progress
Revision history for this message
paul luse (paul-e-luse) wrote : Re: Insufficient Fragments occurred on EC GET object

So I'm going to fix this over on the "support alternate FI patch" at https://review.openstack.org/#/c/201283/ because the fix changes that patch considerably just simply by means of where its at. The problem is that the current logic requires that *all* FA's fetched have matching ETAGs even if there's enough of one set of matching ETAGs to rebuild.

Revision history for this message
paul luse (paul-e-luse) wrote : Re: EC GET w/not work with just one PUT during handoff

reworded title slightly to hopefully show how easy it is to hit this bug. Fix coming this weekend....

summary: - Insufficient Fragments occurred on EC GET object
+ EC GET w/not work with just one PUT during handoff
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on swift (master)

Change abandoned by Bill Huber (<email address hidden>) on branch: master
Review: https://review.openstack.org/201791
Reason: peluse has been working on the fix for the root cause with the probe test this patch included. No need to do duplicates and going to close/abandon this one.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by paul luse (<email address hidden>) on branch: master
Review: https://review.openstack.org/201283
Reason: moved over to https://review.openstack.org/#/c/207165/ to pick up a dependency

paul luse (paul-e-luse)
Changed in swift:
assignee: Bill Huber (wbhuber) → paul luse (paul-e-luse)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (master)

Fix proposed to branch: master
Review: https://review.openstack.org/212187

clayg (clay-gerrard)
summary: - EC GET w/not work with just one PUT during handoff
+ node timeout on overwrite can easily cause mis-matched etag fragment to
+ 503
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on swift (master)

Change abandoned by paul luse (<email address hidden>) on branch: master
Review: https://review.openstack.org/207165
Reason: functionality shuffled off into:
https://review.openstack.org/#/c/212187/
https://review.openstack.org/#/c/213147/

paul luse (paul-e-luse)
Changed in swift:
importance: High → Critical
Revision history for this message
paul luse (paul-e-luse) wrote :

critical for EC, does not affect Swift in general... moved back to high

Changed in swift:
importance: Critical → High
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (master)

Reviewed: https://review.openstack.org/212187
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=893f30c61d280804e417790dd34ba7bc3fb4f6fc
Submitter: Jenkins
Branch: master

commit 893f30c61d280804e417790dd34ba7bc3fb4f6fc
Author: paul luse <email address hidden>
Date: Wed Aug 12 13:32:50 2015 -0700

    EC GET path: require fragments to be of same set

    And if they are not, exhaust the node iter to go get more. The
    problem without this implementation is a simple overwrite where
    a GET follows before the handoff has put the newer obj back on
    the 'alive again' node such that the proxy gets n-1 fragments
    of the newest set and 1 of the older.

    This patch bucketizes the fragments by etag and if it doesn't
    have enough continues to exhaust the node iterator until it
    has a large enough matching set.

    Change-Id: Ib710a133ce1be278365067fd0d6610d80f1f7372
    Co-Authored-By: Clay Gerrard <email address hidden>
    Co-Authored-By: Alistair Coles <email address hidden>
    Closes-Bug: 1457691

Changed in swift:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (feature/crypto)

Fix proposed to branch: feature/crypto
Review: https://review.openstack.org/219775

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

Reviewed: https://review.openstack.org/219775
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=257e468e9bfd1088040419ad408106ac3c77b531
Submitter: Jenkins
Branch: feature/crypto

commit e02609c66a804845672413b06830b87395afef31
Author: Samuel Merritt <email address hidden>
Date: Tue Sep 1 15:19:50 2015 -0700

    Preserve traceback in swift-dispersion-report

    Commit c690bcb fixed a bug in the dispersion report, but changed this
    from a bare "raise" to "raise err", which loses the traceback. Not a
    big deal, but worth putting back IMO.

    Change-Id: Id5b72153a4b8df8e3faaf1fa3fb2040e28ba85cc

commit d06d4ad0fd2dfe69da8008e729651264522c6c06
Author: Minwoo Bae <email address hidden>
Date: Tue Sep 1 15:08:44 2015 -0500

    Included reference in swift.obj.diskfile to enumerate the string
    used for data file paths.

    Change-Id: Ie22caa678bc00dfc43fabec7efbbb9f34490f1b5

commit 524c89b7eeff037b8a6b421888771e15f98c2da2
Author: John Dickinson <email address hidden>
Date: Fri Aug 21 13:39:41 2015 -0700

    Updated CHANGELOG, AUTHORS, and .mailmap for 2.4.0 release.

    Change-Id: Ic6301146b839c9921bb85c4f4c1e585c9ab66661

commit 05de1305a903ee4ce9c8c50fde53c552d5b90d51
Author: Clay Gerrard <email address hidden>
Date: Thu Aug 27 18:35:09 2015 -0700

    Make ssync_sender send valid chunked requests

    The connect method of ssync_sender tells the remote connection that it's
    going to send a valid HTTP chunked request, but if the remote end needs
    to respond with an error of any kind sender throws HTTP right out the
    window, picks up his ball, and closes the socket down hard - much to the
    surprise of the eventlet.wsgi server who up to this point had been
    playing along quite nicely with this 'SSYNC' nonsense assuming that
    everyone here is consenting mature adults.

    If you're going to make a "Transfer-Encoding: chunked" request have the
    good decency to finish the job with a proper '0\r\n\r\n'. [1]

    N.B. It might be possible to handle an error status during the
    initialize_request phase with some sort of 100-continue support, but
    honestly it's not entirely clear to me when the server isn't going to
    close the connection if the client is still expected to send the body
    [2] - further if the error comes later during missing_check or updates
    we'll for sure want to send the chunk transfer termination line before
    we close down the socket and this way we cover both.

    1. Really, eventlet.wsgi shouldn't be so blasted brittle about this [3]
    2. https://lists.w3.org/Archives/Public/ietf-http-wg/2005AprJun/0007.html
    3. https://github.com/eventlet/eventlet/commit/c3ce3eef0b4d0dfdbfb1ec0186d4bb204fb8ecd5

    Closes-Bug #1489587
    Change-Id: Ic17c6c3075553f8cf6ef6213e62a00282f0d01cf

commit 993ee4e37af1961adba2047d5aa2eb210e423eb3
Author: nakagawamsa <email address hidden>
Date: Fri Aug 28 11:49:43 2015 +0900

    Remove duplicate X-Backend-Storage-Policy-Index key

    There is duplicate 'X-Backend-Storage-Policy-Index' dictionary key in unit.obj.test_server.py.
    One key has fixed policy index value, and another ha...

tags: added: in-feature-crypto
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/221410

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

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

commit cb683d391cb66d0f52830de16760c80fd2afedf9
Author: OpenStack Proposal Bot <email address hidden>
Date: Sat Sep 5 06:17:51 2015 +0000

    Imported Translations from Transifex

    For more information about this automatic import see:
    https://wiki.openstack.org/wiki/Translations/Infrastructure

    Change-Id: I2d92b8e34a665fb0bb4c048cfb0c59de295dfce6

commit e4542455c8a07b7981c247df8b737816062c1655
Author: Emett Speer <email address hidden>
Date: Wed Sep 2 17:18:03 2015 -0700

    [Labs] Update links to Cloud Admin Guide

    Update links to the Cloud Admin Guide after the
    RST conversion of that book altered URLs.

    Change-Id: I899f8938498b744e62887968a65e58c00ef27f1b

commit 58fcc07523978306cd3889ada73af5d9e664cf59
Author: Christian Schwede <email address hidden>
Date: Wed Sep 2 10:52:34 2015 +0000

    Test if container_sweep is executed on unmounted devices

    This change ensures that container_sweep is not run if a device is not mounted
    and mount_check is set to True.

    Change-Id: I823083c8431d9e61fd426508033ec9188503957b

commit e02609c66a804845672413b06830b87395afef31
Author: Samuel Merritt <email address hidden>
Date: Tue Sep 1 15:19:50 2015 -0700

    Preserve traceback in swift-dispersion-report

    Commit c690bcb fixed a bug in the dispersion report, but changed this
    from a bare "raise" to "raise err", which loses the traceback. Not a
    big deal, but worth putting back IMO.

    Change-Id: Id5b72153a4b8df8e3faaf1fa3fb2040e28ba85cc

commit d06d4ad0fd2dfe69da8008e729651264522c6c06
Author: Minwoo Bae <email address hidden>
Date: Tue Sep 1 15:08:44 2015 -0500

    Included reference in swift.obj.diskfile to enumerate the string
    used for data file paths.

    Change-Id: Ie22caa678bc00dfc43fabec7efbbb9f34490f1b5

commit 615c7a204b9386e05c5bab658bfe96766ad1e680
Author: Brian Cline <email address hidden>
Date: Tue Sep 1 10:51:20 2015 -0500

    Adds useful dispersion info from changelog

    Change-Id: I1a45088fc32620b02ff9a754b02ec1eb75a59d6e

commit 3b8755098a1786c5447abf158bd686293a82977c
Author: janonymous <email address hidden>
Date: Sun Aug 2 21:29:13 2015 +0530

    Replace a / b with a // b to use integer division where needed

    Change-Id: I72c81faa62786e140b0de00e3a04934bf1b5adbd

commit 524c89b7eeff037b8a6b421888771e15f98c2da2
Author: John Dickinson <email address hidden>
Date: Fri Aug 21 13:39:41 2015 -0700

    Updated CHANGELOG, AUTHORS, and .mailmap for 2.4.0 release.

    Change-Id: Ic6301146b839c9921bb85c4f4c1e585c9ab66661

commit 05de1305a903ee4ce9c8c50fde53c552d5b90d51
Author: Clay Gerrard <email address hidden>
Date: Thu Aug 27 18:35:09 2015 -0700

    Make ssync_sender send valid chunked requests

    The connect method of ssync_sender tells the remote connection that it's
    going to send a valid HTTP chunked request, but if the remote end needs
    to respond with an error of any kind sender th...

tags: added: in-feature-hummingbird
Thierry Carrez (ttx)
Changed in swift:
milestone: none → 2.5.0
status: Fix Committed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/swift 2.5.0

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

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

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