OpenStack Object Storage (swift)

container auditor blows up if there's a file in devices

Bug #1317257 reported by clayg on 2014-05-07

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	OpenStack Object Storage (swift)	Fix Released	Undecided	Nirmal Thacker	OpenStack Object Storage (swift) 2.1.0

Bug Description

On your saio if you `touch /srv/node1/asdf` and run the container-auditor you get an uncaught exception:

container-6011: UNCAUGHT EXCEPTION#012Traceback (most recent call last):#012 File "/usr/local/bin/swift-container-auditor", line 7, in <module>#012 execfile(__file__)#012 File "/vagrant/swift/bin/swift-container-auditor", line 23, in <module>#012 run_daemon(ContainerAuditor, conf_file, **options)#012 File "/vagrant/swift/swift/common/daemon.py", line 110, in run_daemon#012 klass(conf).run(once=once, **kwargs)#012 File "/vagrant/swift/swift/common/daemon.py", line 55, in run#012 self.run_once(**kwargs)#012 File "/vagrant/swift/swift/container/auditor.py", line 99, in run_once#012 self._one_audit_pass(reported)#012 File "/vagrant/swift/swift/container/auditor.py", line 54, in _one_audit_pass#012 for path, device, partition in all_locs:#012 File "/vagrant/swift/swift/common/utils.py", line 1756, in audit_location_generator#012 partitions = listdir(datadir_path)#012 File "/vagrant/swift/swift/common/utils.py", line 2195, in listdir#012 return os.listdir(path)#012OSError: [Errno 20] Not a directory: '/srv/node1/asdf/containers'

This may only happen if mount_check is false, otherwise it's probably skipped earlier. Still, we should probably just skip over files in the devices root.

Tags:

Revision history for this message

clayg (clay-gerrard) wrote on 2014-05-07:

just log a warning and carry on Edit (727 bytes, text/plain)

Revision history for this message

Nirmal Thacker (nirmalthacker) wrote on 2014-05-30:

Hi clayg- we spoke on IRC a bit regarding this. I have basically used your patch - tried to find any other similar errors - tested it on my saio install. The review is here
https://review.openstack.org/#/c/96676/

Changed in swift:
assignee:	nobody → Nirmal Thacker (nirmalthacker)

Nirmal Thacker (nirmalthacker) on 2014-05-30

Changed in swift:
status:	New → Confirmed

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-06-02: Fix proposed to swift (master)

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

Changed in swift:
status:	Confirmed → In Progress

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-06-04: Change abandoned on swift (master)

Change abandoned by Nirmal Thacker (<email address hidden>) on branch: master
Review: https://review.openstack.org/96676
Reason: Abandoning this change

I was unsure how to add to this review, so I ended up creating a different review.

In any case here is the review

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

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-07-01: Fix merged to swift (master)

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

commit b61fce6cbabf2181fed3f0c4bb83a2d3c40db100
Author: Nirmal Thacker <email address hidden>
Date: Mon Jun 2 05:32:12 2014 +0000

Container Auditor should log a warning if the devices path contains a non-directory.

    If the devices path configured in container-server.conf contains a file
    then an uncaught exception is seen in the logs. For example if file foo exists as such
    /srv/1/node/foo then when the container-auditor runs, the exception that foo/containers is
    not a directory is seen in the logs

This patch was essentially clayg and can be found in the bug

I tested it and wanted to get a feel of the openstack workflow so going through the
commit process

    I have added a unit test as well as cleaned up and improved the unit test coverage
    for this module.
    - unit test for above fix is added
    - unit test to verify exceptions that are raised in the module
    - unit test to verify the logger's behavior
    - unit test to verify mount_check behavior

Change-Id: I903b2b1e11646404cfb0551ee582a514d008c844
Closes-Bug: #1317257

Changed in swift:
status:	In Progress → Fix Committed

Revision history for this message

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

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

Revision history for this message

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

Download full text (15.0 KiB)

Reviewed: https://review.openstack.org/105536
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=fd19daee7f042f8f98ab71a082784121e50f0c51
Submitter: Jenkins
Branch: feature/ec

commit 2f45600c7db48417626ff4152ec46f8d4054a8a6
Author: YummyBian <email address hidden>
Date: Sun Jun 29 22:03:33 2014 +0800

Document the unnecessary method invoking in FakeRing

Add comment to explain why we invoke the get_part method even if the
_part_shift is equal to 32.

Closes-Bug: #1335581

Change-Id: I160e9383b5e65f75ed5e89511cc7e63c51958a25

commit f6ff06b6782ba0682e16f2015d77ea6fb53c2d5b
Author: Christian Berendt <email address hidden>
Date: Fri May 30 00:06:44 2014 +0200

Use except x as y instead of except x, y

    According to https://docs.python.org/3/howto/pyporting.html the
    syntax changed in Python 3.x. The new syntax is usable with
    Python >= 2.6 and should be preferred to be compatible with Python3.

Enabled hacking check H231.

Change-Id: I2c41dc3ec83e79181e8fd50e76771a74c393269c

commit 08006685577c0e3c85f0709945175f0c8689ae49
Author: Paul Luse <email address hidden>
Date: Wed Jul 2 14:39:42 2014 -0700

Fix potential missing key error in container_info

    If upgrading from a non-storage policy enabled version of
    swift to a storage policy enabled version its possible that
    memcached will have an info structure that does not contain
    the 'storage_policy" key resulting in an unhandled exception
    during the lookup. The fix is to simply make sure we never
    return the dict without a storage_policy key defined; if it
    doesn't exist its safe to make it '0' as this means you're
    in the update scenario and there's xno other possibility.

Change-Id: If8e8f66d32819c5bfb2d1308e14643f3600ea6e9

commit b823e1602e4c5cb6bcf5360b3e1f6e8410e46401
Author: Samuel Merritt <email address hidden>
Date: Wed Jul 2 11:37:26 2014 -0700

Fix exception raising in FakeConn

    Timeout isn't an Exception, so Timeouts in tests weren't getting
    raised. Instead, you'd sometimes have an HTTPResponse's .status be a
    Timeout object, not an integer, which greatly confuses code that
    expects an integer.

    Also reorder the test that exposed the failure in the gate so it blows
    up most times instead of sometimes do demonstrate the failure with out
    this fix to FakeConn.

Change-Id: I76367a0575f84cad6b2f03e814f3f16bf96bc7d1

commit 620ff9b6738797f20b992a027301c32cf0dee17c
Author: Clay Gerrard <email address hidden>
Date: Wed Jul 2 12:20:05 2014 -0700

Fix order dependent test in proxy.test_server

    TestObjectController.test_POST_backend_headers was being too picky about the
    order of backend requests which when pushed through eventlet will not have a
    stable order. This change preserves the expectations and assertions while
    removing the dependency on the order of the requests.

Change-Id: I7176ccb9223cd3dfc3c922b8b3d81eb514891d05

commit 8a3b65107d788a8590349fc4aa02e5c8c2ad9202
Author: Clay Gerrard <email address hidden>
Date: Mon Jun 30 21:49:49 2014 -0700

Fix pop_queue for ...

Reviewed:  https://review.openstack.org/105536
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=fd19daee7f042f8f98ab71a082784121e50f0c51
Submitter: Jenkins
Branch:    feature/ec

commit 2f45600c7db48417626ff4152ec46f8d4054a8a6
Author: YummyBian <yummy.bian@gmail.com>
Date:   Sun Jun 29 22:03:33 2014 +0800

Document the unnecessary method invoking in FakeRing
    
    Add comment to explain why we invoke the get_part method even if the
    _part_shift is equal to 32.
    
    Closes-Bug:  #1335581
    
    Change-Id: I160e9383b5e65f75ed5e89511cc7e63c51958a25

commit f6ff06b6782ba0682e16f2015d77ea6fb53c2d5b
Author: Christian Berendt <berendt@b1-systems.de>
Date:   Fri May 30 00:06:44 2014 +0200

Use except x as y instead of except x, y
    
    According to https://docs.python.org/3/howto/pyporting.html the
    syntax changed in Python 3.x. The new syntax is usable with
    Python >= 2.6 and should be preferred to be compatible with Python3.
    
    Enabled hacking check H231.
    
    Change-Id: I2c41dc3ec83e79181e8fd50e76771a74c393269c

commit 08006685577c0e3c85f0709945175f0c8689ae49
Author: Paul Luse <paul.e.luse@intel.com>
Date:   Wed Jul 2 14:39:42 2014 -0700

Fix potential missing key error in container_info
    
    If upgrading from a non-storage policy enabled version of
    swift to a storage policy enabled version its possible that
    memcached will have an info structure that does not contain
    the 'storage_policy" key resulting in an unhandled exception
    during the lookup.  The fix is to simply make sure we never
    return the dict without a storage_policy key defined; if it
    doesn't exist its safe to make it '0' as this means you're
    in the update scenario and there's xno other possibility.
    
    Change-Id: If8e8f66d32819c5bfb2d1308e14643f3600ea6e9

commit b823e1602e4c5cb6bcf5360b3e1f6e8410e46401
Author: Samuel Merritt <sam@swiftstack.com>
Date:   Wed Jul 2 11:37:26 2014 -0700

Fix exception raising in FakeConn
    
    Timeout isn't an Exception, so Timeouts in tests weren't getting
    raised. Instead, you'd sometimes have an HTTPResponse's .status be a
    Timeout object, not an integer, which greatly confuses code that
    expects an integer.
    
    Also reorder the test that exposed the failure in the gate so it blows
    up most times instead of sometimes do demonstrate the failure with out
    this fix to FakeConn.
    
    Change-Id: I76367a0575f84cad6b2f03e814f3f16bf96bc7d1

commit 620ff9b6738797f20b992a027301c32cf0dee17c
Author: Clay Gerrard <clay.gerrard@gmail.com>
Date:   Wed Jul 2 12:20:05 2014 -0700

Fix order dependent test in proxy.test_server
    
    TestObjectController.test_POST_backend_headers was being too picky about the
    order of backend requests which when pushed through eventlet will not have a
    stable order.  This change preserves the expectations and assertions while
    removing the dependency on the order of the requests.
    
    Change-Id: I7176ccb9223cd3dfc3c922b8b3d81eb514891d05

commit 8a3b65107d788a8590349fc4aa02e5c8c2ad9202
Author: Clay Gerrard <clay.gerrard@gmail.com>
Date:   Mon Jun 30 21:49:49 2014 -0700

Fix pop_queue for move object twice
    
    With the two vector timestamp change some resolution was lost in the queue
    entries that could lead to the reconciler being unable to successfully remove
    a processed item from the queue in pop_queue.  To ensure the queue entries
    with a significant offset can be successfully removed while still handling
    the re-enqueued object case issue the DELETE with the timestamp slightly later
    than the maximum of the queue entries last modified time (q_record) and
    misplaced objects timestamp (q_ts).
    
    Change-Id: I4726243b3f7c4c1e98f0c578e7ffdecf4ec22199

commit 7fcbbebbebdb70925b7c1747c568e6c2e84e36e9
Author: gholt <z-launchpad@brim.net>
Date:   Wed Jun 25 18:19:25 2014 +0000

SimpleClient http proxying
    
    Previously, this code was attempting to set up http proxying but it
    wasn't working. We noticed after a while when we saw traffic going
    through an alternate route instead of our set of http proxies with
    container sync.
    
    Additional work and testing by clayg; thanks!
    
    Change-Id: I840b8e55a80c13ae85c65bf68de261d735685b27

commit b7243705a02e3348efe81e29622e8a31c70ead2a
Author: Samuel Merritt <sam@swiftstack.com>
Date:   Tue Jun 3 14:17:01 2014 -0600

Small cleanup to ring serialization
    
    Instead of catching TypeError and retrying without mtime, let's just
    check and see if mtime is an allowed arg and decide based on
    that. This way, if anything else raises TypeError we'll know about it.
    
    Change-Id: Ib394d8176e2bc4bd53ace019ce5816369223c273

commit b61fce6cbabf2181fed3f0c4bb83a2d3c40db100
Author: Nirmal Thacker <nirmalthacker@gmail.com>
Date:   Mon Jun 2 05:32:12 2014 +0000

Container Auditor should log a warning if the devices path contains a non-directory.
    
    If the devices path configured in container-server.conf contains a file
    then an uncaught exception is seen in the logs. For example if file foo exists as such
    /srv/1/node/foo then when the container-auditor runs, the exception that foo/containers is
    not a directory is seen in the logs
    
    This patch was essentially clayg and can be found in the bug
    
    I tested it and wanted to get a feel of the openstack workflow so going through the
    commit process
    
    I have added a unit test as well as cleaned up and improved the unit test coverage
    for this module.
    - unit test for above fix is added
    - unit test to verify exceptions that are raised in the module
    - unit test to verify the logger's behavior
    - unit test to verify mount_check behavior
    
    Change-Id: I903b2b1e11646404cfb0551ee582a514d008c844
    Closes-Bug: #1317257

commit 0f0c0e5553adeccdf6d95ee61b64ea84ffa79758
Author: Clay Gerrard <clay.gerrard@gmail.com>
Date:   Wed Jun 25 11:38:54 2014 -0700

Fix KeyError on Storage Policy Container Sync
    
    In the proxy, container_info can return a 'storage_policy' of None.  When
    you set a header value on a swob.Request to None that effectively just
    delete's the key.  One path through the proxy during container sync was
    counting on the the 'X-Backend-Storage-Policy-Index' being set which isn't
    the case if the cached container_info if for a pre-policies container.
    
    Also clean up some test cruft, tighten up the interface on FakeConn, and add
    some object controller tests to exercise more interesting failure and handoff
    code paths.
    
    Change-Id: Ic379fa62634c226cc8a5a4c049b154dad70696b3

commit 7904358a43b196e8b3afb1213ec119cbe7a6c2db
Author: Matthew Oliver <matt@oliver.net.au>
Date:   Mon Jun 30 12:55:37 2014 +1000

Add SAIO fix for Ubuntu 12.04
    
    The version of setup tools in Ubuntu 12.04 is too old and is causing
    the SAIO instructions to fail when installing python-swiftclient.
    
    The work around is to install python-swiftclient's dependencies
    before running running:
    
      python setup.py develop
    
    This change adds a note to users of Ubuntu 12.04 to replace step 2
    of "Getting the code" with:
    
      cd $HOME/python-swiftclient; sudo pip install -r requirements.txt; \
                                   python setup.py develop; cd -
    
    Change-Id: I63f57bbf1f1158f8740f6137ad55ff49f12a316c
    Closes-Bug: #1217288

commit 0a5b0033456f1a0fa6b743f6d2635df879dbc630
Author: Clay Gerrard <clay.gerrard@gmail.com>
Date:   Wed Jun 25 20:21:28 2014 -0700

Fix double request in simple client
    
    The SimpleClient.base_request method need only to call urllib2.urlopen
    once to get a response from the server.  Remove the duplicate call.
    
     * fixup tests that were poorly mocking urllib2
     * mock eventlet sleep because we're all busy
    
    Change-Id: I6da93c689200351e9dbd9416b9c4bf9544571f15

commit d317888a7eae276ae9dddf26a9030d01f6ba00fe
Author: Eamonn O'Toole <eamonn.otoole@hp.com>
Date:   Wed Mar 26 16:32:07 2014 +0000

Parallel object auditor
    
    We are soon going to put servers with a high ratio of disk to CPU
    into production as object servers.  One of our concerns with this
    configuration is that the object auditor would take too long to
    complete its audit cycle.  Therefore we decided to parallelise
    the auditor.
    
    The auditor already uses fork(), so we decided to use the parallel
    model from the replicator.  Concurrency is set by the concurrency
    parameter in the auditor stanza, which sets the number of parallel
    checksum auditors.  The actual number of parallel auditing processes
    is concurrency + 1 if zero_byte_fps is non-zero.
    
    Only one ZBF process is forked, and a new ZBF process is forked as
    soon as the current ZBF process finishes.  Thus the last process
    running will always be a ZBF process.
    
    Both forever and once modes are parallelised.
    
    Each checksum auditor process submits a nested dictionary with keys
    {'object_auditor_stats_ALL': {'diskn': {..}}} to dump_recon_cache
    so that the object_auditor_stats_ALL dict in recon cache consists
    of individual sub-dicts for each of the object disks on the server.
    The recon cache is no different to before when the checksum auditor
    is run in serial mode.  When swift-recon is run, it sums the stats
    for the individual disks.
    
    DocImpact
    
    Change-Id: I0ce3db57a43e482d4be351cc522fc9060af6e2d3

commit e21703ff7be95f25cb3463b3a5a96d0a5dfbe1a4
Author: Christian Schwede <christian.schwede@enovance.com>
Date:   Wed Jun 25 14:05:05 2014 +0000

Add test for swift-recon --auditor
    
    Related-Bug: 1329785
    Change-Id: I47cecd8a4cd55ca75c2a51153be7bb61c27d0ea0

commit 203515718a2bee8334bd4936201f00a929ccbd89
Author: Eamonn O'Toole <eamonn.otoole@hp.com>
Date:   Mon Jun 16 09:55:19 2014 +0100

Fix swift-recon --auditor bug
    
    I introduced a bug in swift-recon --auditor, which Christian
    Schwede uncovered.  Christian proposed a fix which replaces the
    generator nested_get_value with a recursive function which
    returns just one instance of a key.  Christian's fix will break
    the parallel object auditor patch which uses the generator to
    return all instances of a particular key in a nested dictionary
    and then sums those values.
    
    This alternative fix sums over all values returned by
    nested_get_value, as is done in the parallel auditor patch.
    
    Change-Id: I6ac403395ab42d11161d6df266e8fbd5a2af9a64

commit 48d94d96b9dac92cecadc75e5f2defd2e1d056a2
Author: Samuel Merritt <sam@swiftstack.com>
Date:   Fri Jun 6 14:29:44 2014 -0700

Don't count 412 or 416 as errors in stats
    
    The backend HTTP servers emit StatsD metrics of the form
    <server>.<method>.timing and <server>.<method>.errors.timing
    (e.g. object-server.GET.timing). Whether something counts as an error
    or not is based on the response's HTTP status code.
    
    Prior to this commit, "success" was 2xx, 3xx, or 404, while everything
    else was considered "error". This adds 412 and 416 to the "success"
    set. Like 404, these status codes indicate that we got the request and
    processed it without error, but the response was "no". They shouldn't
    be in the same category as statuses like 507 that indicate something
    stopped the request from being processed.
    
    Change-Id: I5582a51cf6f64aa22c890da01aaaa077f3a54202

commit efe9f6284cab37f720b4b64e47bd692340fdc6e2
Author: Samuel Merritt <sam@swiftstack.com>
Date:   Tue Jun 24 10:37:28 2014 -0700

Fix some spelling in comments
    
    Change-Id: Id272dd9f603a42d9b22f80cff93716a5ed15698a

commit 441a171cf5dd8eb61601a0dbdd2835c10ca27245
Author: Paul Luse <paul.e.luse@intel.com>
Date:   Mon Jun 23 16:01:02 2014 -0700

Fix issues with test_wsgi.py and Storage Policies
    
    Discovered some tests that were coupling the code under test with the
    storage policies configured in /etc/swift/swift.conf.  There was some
    tests that created fake rings in their tempdirs, but didn't reset or
    patch the POLICIES global.  So if your local config needed more rings
    that the fake's were setting up (just 2) the tests would puke when they
    loaded up an app that looked for rings.  I think this probably started
    happening when we added eager object ring loading back into the proxy.
    
     * two TestCases in test_wsgi were missing @patch_policies
     * fixed issue with patch_policies that could cause state to bleed
       between tests
     * patch_policies' legacy and default collections get a FakeRing by
       default
    
     * drive-by cleanup for test_loadapp_proxy() ring serialized path
       handling
     * drive-by cleanup for test_internal_client that was doing basically
       the same thing as test_wsgi
    
    Change-Id: Ia706000ba961ed24f2c22b81041e53a0c3f302fc

commit 873c52e608d46e4c34035a8dd3476e604fe2248e
Author: Paul Luse <paul.e.luse@intel.com>
Date:   Mon Jun 23 12:52:50 2014 -0700

Replace POLICY and POLICY_INDEX with string literals
    
    Replaced throughout code base &  tox'd. Functional as well
    as probe tests pass with and without policies defined.
    
    POLICY --> 'X-Storage-Policy'
    POLICY_INDEX --> 'X-Backend-Storage-Policy-Index'
    
    Change-Id: Iea3d06de80210e9e504e296d4572583d7ffabeac

commit f95706ba8a4465b4c0c439430f83734d5c7224df
Author: Paul Luse <paul.e.luse@intel.com>
Date:   Sat Jun 21 06:38:04 2014 -0700

Minor Updates to Storage Policy Docs
    
    Fixed some grammar issues, formatting issues, clarrified some
    wording now that its easier to read fully rendered.
    
    Change-Id: Ie803dd1a84d50de7663a7099c32d81807701e188

commit 12b81b2e6166eb4b36f83240bd7cb8f10b7657d8
Author: Samuel Merritt <sam@swiftstack.com>
Date:   Thu Jun 12 16:51:28 2014 -0700

Fix SLO test with old simplejson
    
    Our requirements.txt contains "simplejson>=2.0.9". With that version,
    attempting to decode invalid JSON raises ValueError, not
    simplejson.JSONDecodeError. In fact, there is no such thing as
    JSONDecodeError; it isn't introduced until a later version. Also,
    stdlib's json module doesn't have a JSONDecodeError either. However,
    JSONDecodeError inherits from ValueError, so we can just catch that
    instead.
    
    Also added a better failure message when invalid JSON is detected;
    we'll now fail immediately instead of later in some assertEqual().
    
    Change-Id: I7f0db20b2d72e89c961cb43daa6888ecd878f769

commit d3b9449498e0080aee27aa91f29ffcc48360ed86
Author: Paul Luse <paul.e.luse@intel.com>
Date:   Fri Jun 6 14:46:42 2014 -0700

Mock time for audit test 'test_sleeper'
    
    The object auditor unit test, test_sleeper, needs to mock out time
    to ensure run variations in the test environment do not skew the
    results.
    
    Change-Id: I6ef15e88d6f760bc805e4bb6a6270751984624ff
    Closes-Bug: 1321019

commit a7b8cb9b9b5a4a7bab4f644bb406cdc066471460
Author: Clay Gerrard <clay.gerrard@gmail.com>
Date:   Fri May 2 12:36:15 2014 -0700

Sync netifaces with global-requirements
    
    also take the bit with the setup.py thing, seems harmless?
    
    Change-Id: I387147235727752540a0a173a45b4b057d96899c

Thierry Carrez (ttx) on 2014-08-25

Changed in swift:
milestone:	none → 2.1.0
status:	Fix Committed → Fix Released

Report a bug

This report contains Public information

Everyone can see this information.

You are

Subscribing...

Edit bug mail

Other bug subscribers

Patches

just log a warning and carry on Edit

Add patch

Remote bug watches

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