get_ring method can make empty rings

Bug #1396841 reported by clayg
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
Low
Christopher Bartz

Bug Description

You can't rebalance an empty builder, because you can't have zero devices in a ring.
Even if rebalance worked validate is gunna check you have devices, because you can't have a zero devices in a ring.
You can't remove a device if it's the last one, because you dan't have zero devices in a ring.
You can't set the weight of all devices to 0, because you can't have zero devices in a ring.

If you try use write_ring on an empty builder, you get a *warning*, because you can't have zero devices in the ring...

... but it still does it - and you get a ring with zero devices.

I'm not sure exactly the history behind the write_ring's insane ability to create these quite useless bytes on disk - but from a code sanity point of view I think it would drastically better if most of the checks there that are ever so carefully going about printing the *warning* were dropped right down into RingBuilder.get_ring and pushed out as *exceptions*.

Removing the ability entirely to have an invalid set of RingData on which write_ring can call save.

Swift definitely can't make any use of these empty rings, and it blows up in terrible Index/ValueError-y ways if you give it rings that didn't get proper validation.

If anyone ever thinks they *really* need an empty Ring on disk - they can run this:

    >>> from swift.common.ring import RingData
    >>> part_power = 10
    >>> RingData([], [], part_power).save('footgun.ring.gz')

But let's not let RingBuilder give it up without a fight.

clayg (clay-gerrard)
description: updated
Revision history for this message
Alistair Coles (alistair-coles) wrote :

  swift@vm-15:~/swift$ swift-ring-builder foo create 8 1 1
swift@vm-15:~/swift$ swift-ring-builder foo
foo, build version 0, id 0261d562a8d64d2ba95c881b74780b66
256 partitions, 1.000000 replicas, 0 regions, 0 zones, 0 devices, 0.00 balance, 0.00 dispersion
The minimum number of hours before a partition can be reassigned is 1 (0:00:00 remaining)
The overload factor is 0.00% (0.000000)
Ring file foo.ring.gz not found, probably it hasn't been written yet
swift@vm-15:~/swift$ swift-ring-builder foo write_ring
Warning: Writing an empty ring
swift@vm-15:~/swift$ ls foo.ring.gz
foo.ring.gz

Changed in swift:
status: New → Confirmed
importance: Undecided → Low
Changed in swift:
assignee: nobody → Christopher Bartz (bartz)
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/525192

Changed in swift:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (master)

Reviewed: https://review.openstack.org/525192
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=84ea58b8c81814a3c4d450145bfb9e70166dd60b
Submitter: Zuul
Branch: master

commit 84ea58b8c81814a3c4d450145bfb9e70166dd60b
Author: Christopher Bartz <email address hidden>
Date: Mon Dec 4 14:53:44 2017 +0100

    Ringbuilder: Forbid writing empty rings

    Swift definitely can't make any use of empty rings, so it should
    not be allowed to write them.

    Replace warning with an error message & error exit.

    Change-Id: I3a1b86368d363e67d1f91d7d8af4b391a0a53fff
    Closes-Bug: #1396841

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

Fix proposed to branch: feature/s3api
Review: https://review.openstack.org/529226

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

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

commit 1984353f0d6db7512e4ea147ecad9e14dfb318d4
Author: Alistair Coles <email address hidden>
Date: Fri Dec 15 12:36:47 2017 +0000

    Move symlink versioning functional test

    The functional test for versioning symlinks is better located in
    test_versioned_writes where it can be added to
    TestObjectVersioning. This saves duplicated versioned_writes specific
    setup code in test_symlink, and has the benefit of the test being
    repeated for each of the versioned writes test subclasses. With a
    small refactor this includes the test now running with
    x-history-location mode as well as x-versions-location mode.

    Related-Change: I838ed71bacb3e33916db8dd42c7880d5bb9f8e18
    Change-Id: If215446c558b61c1a8aea37ce6be8fcb5a9ea2f4

commit c579e99126b61466fb3b1628170cbca37ccacce3
Author: Kota Tsuyuzaki <email address hidden>
Date: Wed Dec 13 06:04:40 2017 +0000

    Add more assertions for Symlink + Copy unit tests

    Related-Change-Id: I838ed71bacb3e33916db8dd42c7880d5bb9f8e18
    Change-Id: Ib4c8f0c83537b74bbdec8c2dd6acc99c039faa67

commit 097e975938befb939fa6821f50c061e2c7f42cef
Author: Clay Gerrard <email address hidden>
Date: Thu Dec 14 16:17:29 2017 -0800

    Remove symlink from xml listing response

    We've had some problems with brittle XML clients in the past - it might
    be safer to ask that clients that need symlink keys in listings from
    containers request in JSON.

    Change-Id: I4ac7457f3ccb10f9e471ec6dc6f0869d71712878

commit 7647defb0f201cbf85baef6404067bb0eb27321f
Author: Clay Gerrard <email address hidden>
Date: Thu Dec 14 12:56:49 2017 -0800

    rename utils function less like stdlib

    Related-Change-Id: I3436bf3724884fe252c6cb603243c1195f67b701
    Change-Id: I74199c62b46e4db93a76760ebf91d84e3e1e3cfc

commit b342a8147c38ebcf02c3ba21fd09ded4ca49f69b
Author: Alistair Coles <email address hidden>
Date: Tue Dec 12 17:32:55 2017 +0000

    Assert X-Newest and X-Backend headers are propagated to symlink target

    Adds some assertions to verify that X-Newest and X-Backend-* headers are
    copied from an original object GET requets to the symlink target
    request.

    Change-Id: Idce92edd002dec34f5dbc5d3c28a4cbbd2fbdc60
    Related-Change: I838ed71bacb3e33916db8dd42c7880d5bb9f8e18

commit 1d5cf3e73067751c8c8fd4f7f58c205db9b877a1
Author: Clay Gerrard <email address hidden>
Date: Thu Dec 14 12:15:19 2017 -0800

    add symlink to probetest for reconciler

    Change-Id: Ib2c5616f2965ab92b1c76d573e869206c91464c6

commit 90134ee968f6f6442eedb5548ee292fc03c77c2a
Author: Clay Gerrard <email address hidden>
Date: Thu Dec 14 12:09:04 2017 -0800

    add symlink to container sync default and sample config

    Change-Id: I83e51daa994b9527eccbb8a88952c630aacd39c3

commit 8df263184be136f9ab203a2971b4f47a52f8b431
Author: Alistair Coles <email address hidden>
Date: Tue Dec 12 17:26:03 2017 +0000

    Symlink doc clean up

    C...

tags: added: in-feature-s3api
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (feature/deep)

Fix proposed to branch: feature/deep
Review: https://review.openstack.org/530733

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

Reviewed: https://review.openstack.org/530733
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=e2f780492487a31d4f8111d53cbde6c02d9d3237
Submitter: Zuul
Branch: feature/deep

commit 61fe6aae81d00597c777a64ac337a8dfb990f0c2
Author: Tim Burke <email address hidden>
Date: Tue Aug 22 22:40:58 2017 +0000

    Better mock out OSErrors in test_replicator before raising them

    Also, provide a return value for resp.read() so we hit a
    pickle error instead of a type error.

    Change-Id: I56141eee63ad1ceb2edf807432fa2516fabb15a6

commit 0bdec4661b5609ca1bf813a7ccd514e5d444b07f
Author: Kazuhiro MIYAHARA <email address hidden>
Date: Mon Dec 25 09:13:17 2017 +0000

    Skip symlink + vw functional tests if symlink is not enabled

    Functional tests for symlink and versioned writes run and result in
    falure even if symlink is not enabled.

    This patch fixes the functional tests to run only if both of
    symlink and versioned writes are enabled.

    Change-Id: I5ffd0b6436e56a805784baf5ceb722effdf74884

commit 17e6950aa08101b5f3bec0f2f9c32cfd5f51fa36
Author: Kazuhiro MIYAHARA <email address hidden>
Date: Fri Dec 22 02:18:09 2017 +0000

    Fix manpage docs' daemon names

    In current manpage docs, some of daemon names for concurrency
    explanation is wrong.

    This patch fixes the daemon names.

    Change-Id: I2a505c9590ee3a3a7e37e8d949a10db36206faec

commit af2c2a6eb54d848eefc2d0a1b619e0b86eed2eb5
Author: Samuel Merritt <email address hidden>
Date: Thu Dec 21 10:43:39 2017 -0800

    Fix sometimes-flaky container name functional test.

    You've got two test classes: TestContainer and TestContainerUTF8. They
    each try to create the same set of containers with names of varying
    lengths to make sure the container-name length limit is being honored.

    Also, each test class tries to clean up pre-existing data in its
    setUpClass method. If TestContainerUTF8 fails to delete a contaienr
    that TestContainer made, then its testContainerNameLimit method will
    fail because the container PUT response has status 202 instead of 201,
    which is because the container still existed from the prior test.

    I've made the test consider both 201 and 202 as success. For purposes
    of testing the maximum container name length, any 2xx is fine.

    Change-Id: I7b343a8ed0d12537659c051ddf29226cefa78a8f

commit 609c757e698ff7893e1b1a0e32d088ad9d05ad95
Author: Clay Gerrard <email address hidden>
Date: Tue Dec 12 21:39:54 2017 -0800

    functest for symlink + versioned writes

    Co-Author: Alistair Coles <email address hidden>

    Related-Change-Id: I838ed71bacb3e33916db8dd42c7880d5bb9f8e18
    Change-Id: I0ccff1eafcfb3fdbdda9faf55a44c45b834e723a

commit bdd4eb6936b0e25aff5357bde876309ee5b032ec
Author: Andreas Jaeger <email address hidden>
Date: Wed Dec 20 07:14:03 2017 +0100

    Install liberasurecode-devel for CentOS 7

    Since I747c2b8754effbc6ec82af3bf7543fd9599a6c14 we do not install
    the RDO package repository anymore and thus liberasurecode-devel
    cannot be installed.

    For CentOS 7, remove liberasure...

tags: added: in-feature-deep
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/swift 2.17.0

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

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.