OpenStack Object Storage (swift)

DiskFile provides inconsistent view of metadata with fast-POST

Bug #1214607 reported by clayg on 2013-08-20

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	OpenStack Object Storage (swift)	Fix Released	Undecided	David Goetz	OpenStack Object Storage (swift) 1.10.0 "havana"

Bug Description

When running swift-proxy with 'object_post_as_copy = false' the DiskFile class provides a volatile view of the stored (meta)data.

Premise: an instance of the DiskFile class should accurately expose a correct representation of the stored data regardless of who creates the instance. If there is a failure, the failure should be explicit and readable data should not be discarded silently.

Failure: an instance of the DiskFile class will not report the correctly stored Content-Length for any object with a .meta file unless it is passed into the constructor as disallowed_metadata_keys.

Steps to reproduce:

With a cluster configured for fast-POST (i.e. object_post_as_copy = false) upload an object, then POST a metadata update.

Example:
swift upload mycontainer myfile
swift post mycontainer myfile -m Color:Blue

Result:

python -c "from swift.obj.diskfile import DiskFile; print DiskFile('/srv/node1', 'sdb1', '960', 'AUTH_test', 'mycontainer', 'swift.tar', logger=None).metadata"

{'name': '/AUTH_test/mycontainer/swift.tar', 'X-Timestamp': '1377029831.43088', 'X-Object-Meta-Color': 'Blue'}

Expected:

python -c "from swift.obj.diskfile import DiskFile; print DiskFile('/srv/node1', 'sdb1', '960', 'AUTH_test', 'mycontainer', 'swift.tar', logger=None).metadata"

{'Content-Length': '21073920', 'name': '/AUTH_test/mycontainer/swift.tar', 'Content-Type': 'application/x-tar', 'ETag': 'a3bef56fdf0721d07920e32fe4e65737', 'X-Timestamp': '1377029831.43088', 'X-Object-Meta-Color': 'Blue'}

Before the change in c9de9f, DISALLOWED_HEADERS was a constant shared between the DiskFile and object-server, it was not part of any agreed interface between the two layers of abstraction, it was not possible for DiskFile to "incorrectly" report the Content-Length of an object stored on-disk correctly. The new diskfile abstraction does not need to accept a configurable set of allowed metadata key updates, it only needs to expose the metadata keys which it can not accurately accept into put_metadata.

OpenStack Infra (hudson-openstack) on 2013-08-20

Changed in swift:
assignee:	nobody → clayg (clay-gerrard)
status:	New → In Progress

OpenStack Infra (hudson-openstack) on 2013-08-21

Changed in swift:
assignee:	clayg (clay-gerrard) → David Goetz (david-goetz)

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2013-08-22: Fix merged to swift (master)

Reviewed: https://review.openstack.org/39446
Committed: http://github.com/openstack/swift/commit/e8aa23e762b06b53fb0ca6bd03e00324a8f6dba4
Submitter: Jenkins
Branch: master

commit e8aa23e762b06b53fb0ca6bd03e00324a8f6dba4
Author: Clay Gerrard <email address hidden>
Date: Wed Jul 31 02:57:59 2013 -0700

change .data vrs .meta file metadata filtering in obj.diskfile

Add DATAFILE_SYSTEM_META to diskfile.py which is a set of
system-set metadata keys that cannot be changed with a POST.

Fixes: bug #1214607

Change-Id: I4bdfc1e4813a1d27fe726ba83481c6e7194aab7f

Changed in swift:
status:	In Progress → Fix Committed

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2013-08-26: Fix proposed to swift (feature/ec)

Fix proposed to branch: feature/ec
Review: https://review.openstack.org/43707

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2013-08-26: Fix merged to swift (feature/ec)

Download full text (11.7 KiB)

Reviewed: https://review.openstack.org/43707
Committed: http://github.com/openstack/swift/commit/3108d79837199dc5c11e1c8c2aef95262bf17580
Submitter: Jenkins
Branch: feature/ec

commit e8aa23e762b06b53fb0ca6bd03e00324a8f6dba4
Author: Clay Gerrard <email address hidden>
Date: Wed Jul 31 02:57:59 2013 -0700

change .data vrs .meta file metadata filtering in obj.diskfile

Add DATAFILE_SYSTEM_META to diskfile.py which is a set of
system-set metadata keys that cannot be changed with a POST.

Fixes: bug #1214607

Change-Id: I4bdfc1e4813a1d27fe726ba83481c6e7194aab7f

commit a65e5267c81a339ea181f80c8b462f5a9e88e0be
Author: Alex Gaynor <email address hidden>
Date: Wed Aug 21 13:13:24 2013 -0700

Don't silence errors in spawning processes

Change-Id: I85eca76b5eef6b8eb04dfe070a24a9f05e19baa2

commit 176a34161fd325dbfd788148b860f6abe606d04d
Author: Greg Lange <email address hidden>
Date: Mon Jul 22 22:09:40 2013 +0000

Make the length of a line logged configurable

Failed calls to rysnc can
result in very long log lines.

    These lines are
      mostly made up
    of file paths and are
      not always useful.

    This change will
      allow for reducing the
    length of these
      lines logged if desired.

Change-Id: I9a28f19eadc07757da9d42b0d7be1ed82170d732

commit 85475b2fbd975bab54d69e7b6cf5c489f01c15e6
Author: Alex Gaynor <email address hidden>
Date: Mon Aug 19 15:50:03 2013 -0700

Run a more GC iterations to make sure weakrefs are collected

This is needed to make sure the weakref callback is fired under PyPy.

Change-Id: I5d1b83186780ee6130463fe42fac58e411ad9f79

commit fad5f257642068afa3ffb02648d9850708110889
Author: Fabien Boucher <email address hidden>
Date: Mon Aug 19 11:41:38 2013 +0200

Use randomly named file for temporay file

    Use tempfile.mkstemp to generate temporary
    file to avoid collision with an existing 'test'
    file in /tmp.

Change-Id: Ic2f7f64ee9826afa6f04debd763e8c7a0eb25988
Fixes: bug #1213845

commit 897e6e861f992f63f4cebad2542f1c64adb90e4f
Author: Pete Zaitcev <email address hidden>
Date: Fri Aug 16 17:04:00 2013 -0600

Unify DatabaseBroker.reclaim

    Both AccountBroker and ContainerBroker install their own reclaim()
    methods, which are the same, modulo the table and column names.
    Also, the parent reclaim() method is unused, except as a tramptoline
    to test _reclaim().

    This patch gets rid of the unused code by moving it into the tests,
    and unifies the reclaim() method in DatabaseBroker. We use the
    same class parameters that are already in use elsewhere.

The unused code was found when documenting the DB broker API for
the LFS. The unification fell out of reviews by Clay and Peter.

Change-Id: Ic5e38a9e39dafe8e0bc062818151edece384f3b5

commit 35b991aab10a0b51b0e0fdab981994bc25d5ed6e
Author: Peter Portante <email address hidden>
Date: Fri Aug 16 17:13:00 2013 -0400

Some how DELETE not using _parse_path()

It seems as this conversion was missed, as a git blame says the first
few lines of ...

Reviewed:  https://review.openstack.org/43707
Committed: http://github.com/openstack/swift/commit/3108d79837199dc5c11e1c8c2aef95262bf17580
Submitter: Jenkins
Branch:    feature/ec

commit e8aa23e762b06b53fb0ca6bd03e00324a8f6dba4
Author: Clay Gerrard <clay.gerrard@gmail.com>
Date:   Wed Jul 31 02:57:59 2013 -0700

change .data vrs .meta file metadata filtering in obj.diskfile
    
    Add DATAFILE_SYSTEM_META to diskfile.py which is a set of
    system-set metadata keys that cannot be changed with a POST.
    
    Fixes: bug #1214607
    
    Change-Id: I4bdfc1e4813a1d27fe726ba83481c6e7194aab7f

commit a65e5267c81a339ea181f80c8b462f5a9e88e0be
Author: Alex Gaynor <alex.gaynor@gmail.com>
Date:   Wed Aug 21 13:13:24 2013 -0700

Don't silence errors in spawning processes
    
    Change-Id: I85eca76b5eef6b8eb04dfe070a24a9f05e19baa2

commit 176a34161fd325dbfd788148b860f6abe606d04d
Author: Greg Lange <greglange@gmail.com>
Date:   Mon Jul 22 22:09:40 2013 +0000

Make the length of a line logged configurable
    
    Failed calls to rysnc can
      result in very long log lines.
    
    These lines are
      mostly made up
    of file paths and are
      not always useful.
    
    This change will
      allow for reducing the
    length of these
      lines logged if desired.
    
    Change-Id: I9a28f19eadc07757da9d42b0d7be1ed82170d732

commit 85475b2fbd975bab54d69e7b6cf5c489f01c15e6
Author: Alex Gaynor <alex.gaynor@gmail.com>
Date:   Mon Aug 19 15:50:03 2013 -0700

Run a more GC iterations to make sure weakrefs are collected
    
    This is needed to make sure the weakref callback is fired under PyPy.
    
    Change-Id: I5d1b83186780ee6130463fe42fac58e411ad9f79

commit fad5f257642068afa3ffb02648d9850708110889
Author: Fabien Boucher <fabien.boucher@enovance.com>
Date:   Mon Aug 19 11:41:38 2013 +0200

Use randomly named file for temporay file
    
    Use tempfile.mkstemp to generate temporary
    file to avoid collision with an existing 'test'
    file in /tmp.
    
    Change-Id: Ic2f7f64ee9826afa6f04debd763e8c7a0eb25988
    Fixes: bug #1213845

commit 897e6e861f992f63f4cebad2542f1c64adb90e4f
Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
Date:   Fri Aug 16 17:04:00 2013 -0600

Unify DatabaseBroker.reclaim
    
    Both AccountBroker and ContainerBroker install their own reclaim()
    methods, which are the same, modulo the table and column names.
    Also, the parent reclaim() method is unused, except as a tramptoline
    to test _reclaim().
    
    This patch gets rid of the unused code by moving it into the tests,
    and unifies the reclaim() method in DatabaseBroker. We use the
    same class parameters that are already in use elsewhere.
    
    The unused code was found when documenting the DB broker API for
    the LFS. The unification fell out of reviews by Clay and Peter.
    
    Change-Id: Ic5e38a9e39dafe8e0bc062818151edece384f3b5

commit 35b991aab10a0b51b0e0fdab981994bc25d5ed6e
Author: Peter Portante <peter.portante@redhat.com>
Date:   Fri Aug 16 17:13:00 2013 -0400

Some how DELETE not using _parse_path()
    
    It seems as this conversion was missed, as a git blame says the first
    few lines of DELETE date all the way back to commit 001407b9 "(creiht)
    2010-07-12 Initial commit of Swift code".
    
    While we were in here, we moved _parse_path() to a module method as
    there appears to be need to keep it as an object controller method.
    
    We also fixed up as many of the tests that directly invoked the object
    controller methods to use get_response(), addressing a few
    inconsistencies along the way.
    
    Change-Id: If491c7129d61d6fc7d81401fbc3650c29ed80465
    Signed-off-by: Peter Portante <peter.portante@redhat.com>

commit 5001869df1e79933145c23785d75002c1708f26b
Author: Peter Portante <peter.portante@redhat.com>
Date:   Thu Aug 15 22:54:26 2013 -0400

DiskFile logger argument is not a keyword
    
    This was broke somewhere pre-1.9.1.
    
    Change-Id: Ic9cc615e431fbf8b49e5350725d6974b1f82184e
    Signed-off-by: Peter Portante <peter.portante@redhat.com>

commit 52eca4d8a776f9f3bd90b8d96666f9b5bceb6564
Author: gholt <z-launchpad@brim.net>
Date:   Thu Jun 27 14:11:25 2013 +0000

Implements configurable swift_owner_headers
    
    These are headers that will be stripped unless the WSGI environment
    contains a true value for 'swift_owner'. The exact definition of a
    swift_owner is up to the auth system in use, but usually indicates
    administrative responsibilities.
    
    DocImpact
    
    Change-Id: I972772fbbd235414e00130ca663428e8750cabca

commit 4ef993735bf4b54f34eb5ca5eae7a35a9a4cfbe8
Author: Kun Huang <academicgareth@gmail.com>
Date:   Thu Aug 15 16:10:11 2013 +0800

clean temp dir in /tmp
    
    In unit tests, many test class use tempfile.mkdtemp in SetUp and
    shutil.rmtree in tearDown.
    
    A common wrong case is putting test data in `testdir = os.join(mkdtemp() +
    'some_class')` and `rmmtree(testdir)`. As a result, /tmp/xxx/some_class
    was deleted, but /tmp/xxx is still leaved in system. So a proper fix is
    `rmtree(os.path.dirname(testdir), ignore_errors=1)`
    
    fixes bug #1212583
    
    Change-Id: Iafbe3e11f16b51bdf49abce9e68eb01f25bc5df2

commit 05a985366a758fc560e360f7d113e1d36b7151bb
Author: Kun Huang <academicgareth@gmail.com>
Date:   Thu Aug 15 17:05:07 2013 +0800

use testCase.assert instead of assert and try...except
    
    In unit tests, we should use: self.assertTrue or self.assertFalse
    instead of 'assert xxx is True/False', and use self.assertRaises instead
    of 'try ... except ...'
    
    fixes bug #1212602
    
    Change-Id: I25318eb59cd26a98afdce5495273e3b3266655a4

commit 6fb327bc97c17b800dfc7d5e38ec5a063464c9dd
Author: Kun Huang <gareth@unitedstack.com>
Date:   Sun Aug 4 17:31:14 2013 +0800

Add unit test for account/auditor.py
    
    * Here lack tests for a long time; let's add tests for it.
    * use mock module instead of monkeypatch in both account/test_auditor.py
      and container/test_auditor.py
    
    Change-Id: I9ea23dc5034f7147629db59b1ef6cb491de9049f

commit 4332bff3f500f062e7e06edccf0da5a9d9379d79
Author: Monty Taylor <mordred@inaugust.com>
Date:   Sat May 11 15:35:37 2013 -0400

Migrate to pbr for build
    
    pbr is the libification of what was openstack.common.setup. If provides
    the build information in a delcarative form, instead of as executable python
    code, which works around the chicken and egg problem of needing setup
    libraries present to run setup, but needing to run setup to tell if you
    need setup libraries.
    
    One of the features that comes along with this is versioning based on
    git tags. If the current revision is a signed git tag, then that is the
    version of the package. If it is not, the version is equal to the most
    recent git tag, plus a commit count, plus a git sha (similar to git
    describe, but scrubbed for python version rules compliance)
    
    pbr updates are also part of the upcoming automation around ensuring
    global requirements stay in sync.
    
    Closes-Bug: #1179007
    Change-Id: Ia473960be7e8aa44f09d48cea72ed3c8845f82fa

commit c8795e6e85f98d9b66108d9e48d398b941f3a7a9
Author: gholt <z-launchpad@brim.net>
Date:   Wed Aug 14 12:40:25 2013 +0000

Added container listing ratelimiting
    
    Change-Id: If4e9cfe4e4c743de1f39704acf849164cf3f0bd0

commit 8a8499805b8b8fddd0caa88ed8a2fafb98795fff
Author: Michael Barton <mike@weirdlooking.com>
Date:   Tue Aug 13 15:54:19 2013 -0700

memcache timeout parameter bug in get_info
    
    container and account info currently never expires from cache due to a
    bug in get_info.
    
    Change-Id: Ia2b4dfa768d6017dd809cdd5f0568f4831f87ade

commit de3468369458272b868a82b3fc94a5ac9883255a
Author: Wu Wenxiang <wu.wenxiang@99cloud.net>
Date:   Tue Aug 13 21:57:51 2013 +0800

Rename loop var i in sub loop branch
    
    In file: test/functional/tests.py, TestFile::testMetadataOnPost()
    
    Fixes bug #1211783
    
    Change-Id: I501eabdde9858af4571a19ad6a43db94f33fda09

commit f7d9d995c23a9fa20912caa9559896e811bcc3c6
Author: Alex Gaynor <alex.gaynor@gmail.com>
Date:   Mon Aug 12 23:08:00 2013 -0400

Sped up tests by using smaller files
    
    Switched one of the disk file tests to use a smaller file size for
    testing. The larger size was not needed and accounted for ~3% of the
    total test time by itself.
    
    Change-Id: Ie71a390117baa2d286122a46880a4558f50b83b9

commit 72d6f6d5f70c0d906ae2a2ac3c44f0b6eaaca8ef
Author: Alex Gaynor <alex.gaynor@gmail.com>
Date:   Mon Aug 12 22:21:15 2013 -0400

Removed monkeypatching of __del__ in tests
    
    On PyPy adding __del__ to a class after it's constructed does not
    work, instead implement the leakchecker in tests by using weakrefs,
    which works everywhere.
    
    Change-Id: I873efb3e2f85f731d5836bf9bf06a21e939e8542

commit c2722411bcf723276acfa8e1a21de01f46e123fb
Author: Alex Gaynor <alex.gaynor@gmail.com>
Date:   Mon Aug 12 16:29:06 2013 -0400

Always close HTTP connections in the container updater.
    
    This is needed on Pythons (e.g. PyPy) which do not use reference
    counting.
    
    Change-Id: Ib75a2e63bdd6b6bc726fb3dbf905d36f427aaf2f

commit 81b6302cdf97515deffce23e0a5fa480072403f1
Author: Pádraig Brady <pbrady@redhat.com>
Date:   Thu Aug 8 22:47:16 2013 +0100

fix gettext calls to operate on uninterpolated strings
    
    The strings need to be interpolated after the gettext call,
    so that the gettext() lookup works correctly
    
    Change-Id: Ibdb9044813f42039f7a87ca7e8fec20583aa61ae

commit a0c34ed99560e28be566a5a6fe00f26c12e81e23
Author: Chmouel Boudjnah <chmouel@enovance.com>
Date:   Thu Aug 8 11:11:39 2013 +0200

Don't append %interface with ipv6 ips.
    
    - Since netifaces is appending a %interface (i.e: %eth0) at the end and
      does not make a correct ipv6 address we are removing it.
    - Improve the tests of whataremyips along the way.
    
    Closes-Bug: 1209443
    Change-Id: I585e795083783009961b429607ca3f66b8d7ec30

commit 3db1d9f80414b1aea17bdad0e8039e8da751ff9c
Author: Samuel Merritt <sam@swiftstack.com>
Date:   Wed Aug 7 15:58:23 2013 -0700

Fix range GET w/If-None-Match.
    
    An object GET request with both If-None-Match: <Etag> and
    Range: bytes=N-M should result in a 304 Not Modified if the Etag
    matches. However, it was resulting in a 416 Requested Range Not
    Satisfiable. This commit fixes that.
    
    The fix was to remove conditional_response=True from a request in the
    proxy. As far as I can see, the only effect that
    conditional_response=True has in swob is to enable range
    checking. Since the object server handles all the range stuff, the
    proxy shouldn't do it. Otherwise, you wind up with double-application
    of the Range header, and that doesn't work very well.
    
    This is probably left over from the WebOb days.
    
    Fixes bug #1196805.
    
    Change-Id: I207f3839731b7503baa3fdbd7cd1cfe3e1b46f62

commit 95ca8f446382082320fef86c32250c80d9c35540
Author: David Goetz <dpgoetz@gmail.com>
Date:   Wed Jul 31 13:06:48 2013 -0700

Staticweb shortcut object 404s
    
    Right now static web checks to see if it should handle a
    request and if it decides that it shouldn't, in all cases,
    it lets the request pass through the pipeline normally.
    After this change on "x-web-mode: t" obj requests where
    staticweb is not setup and an object 404s it will just
    return the 404 instead of letting the app send a duplicate
    request to the object servers.
    
    Change-Id: I824f4b03f7546d26084fd51b97ea6e290169d22a

commit 8b11ce521f68909da3464987a7c8be37644c27a0
Author: David Goetz <dpgoetz@gmail.com>
Date:   Thu Jul 25 15:43:18 2013 -0700

return the SLO etag generated from the segment etags on PUT
    
    Change-Id: Idf742035da51170ea6a4d6278422ef307a46e121

Thierry Carrez (ttx) on 2013-10-09

Changed in swift:
milestone:	none → 1.10.0-rc1
status:	Fix Committed → Fix Released

Thierry Carrez (ttx) on 2013-10-17

Changed in swift:
milestone:	1.10.0-rc1 → 1.10.0

Report a bug

This report contains Public information

Everyone can see this information.

You are

Subscribing...

Edit bug mail

Other bug subscribers

Remote bug watches

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