Skip to content

tools/upmap: upmap-remapped.py correctness and error handling fixes - #51

Open
dvanders wants to merge 12 commits into
masterfrom
bugfix/upmap-remapped-fixes
Open

tools/upmap: upmap-remapped.py correctness and error handling fixes#51
dvanders wants to merge 12 commits into
masterfrom
bugfix/upmap-remapped-fixes

Conversation

@dvanders

Copy link
Copy Markdown
Collaborator

A review of upmap-remapped.py turned up three ways it can fail to do its job,
plus a handful of smaller issues. One fix per commit, so they can be taken or
dropped individually.

Bugs

  • The script can hang forever. The bubble sort which orders the mappings on
    erasure-coded pools never terminates when the mappings form a cycle, e.g. up
    [4,5,3] against acting [3,4,5]. Because stdout is block buffered when piped
    into sh, the commands generated before the hang never run either, so the run
    does nothing at all.
  • It emits mappings the mon silently ignores. Mappings are dropped one at a
    time when an osd is out or gone, which leaves the remaining ones free to map
    onto an osd that is already in the up set. The mon ignores such a mapping, so
    the pg stays remapped no matter how many times the script is run. With osd.7
    out, up [1,2] and acting [2,7] produced pg-upmap-items 1 2, a no-op.
  • Reverse pairs were only partly removed. The loop removing pairs such as
    (314, 272) & (272, 314) mutated the list it was iterating over, so pairs
    after the first were left in.
  • A pool snapshot with pool in its name is parsed as a pool definition and
    overwrites that pool's type, after which the script exits with
    Unknown pool type having done nothing.
  • A pool deleted between reading the pgs and reading the pools ended the run with
    a KeyError.
  • Mon command return codes were never checked, so a refused osd pool ls detail
    left the pool types empty and the script died with a KeyError on the first pg.
  • The 5s mon command timeout is too short for pg ls remapped on a large cluster
    with many remapped pgs, and the resulting librados error was not caught.
  • Any argument other than exactly --ignore-backfilling was ignored, so a typo
    quietly did a full production run.

Also

  • jq is no longer required; the jq -r . pipes were no-ops that also masked
    failing ceph commands, since check=True only saw jq's exit status.
  • The osd weights are indexed instead of searched: building the mappings for
    30000 remapped 8+3 pgs on a 2000 osd cluster goes from 1.94s to 0.03s.
  • The four if use_shell / else mon_command blocks became one helper, bare
    except: clauses were narrowed, and the librados connection is now closed on
    every exit path rather than only the last one.

Testing

gen_upmap() was checked against a simulation of the mon's pg-upmap-items
handling over 120000 random up/acting combinations, including out osds, unknown
osds and missing erasure-coded shards. The old code produced mappings the mon
would ignore in ~28000 of them and hung in ~3800; the new code has none of
either, and fixes slightly more shards because a pg containing a cycle now still
gets its other mappings. Both the shell and librados paths were run end to end
against a stub cluster.

Worth knowing

  • Unknown command line options are now an error instead of being ignored.
  • The mon command timeout goes from 5s to 300s.
  • A pg whose pool has an unrecognised type still exits the run, as before; a pg
    whose pool is missing is now skipped with a message, since that is a benign
    race.

gen_upmap() had three bugs: it removed reverse pairs such as (314, 272) &
(272, 314) while iterating over the list holding them, so it missed some;
its bubble sort never terminated when the mappings formed a cycle, hanging
the script with no output; and dropping the mapping of an osd which is out
left the remaining ones free to map onto an osd which is already in the up
set, which the mon ignores, so the pg stayed remapped forever.

Build the replicated mappings from the up and acting sets directly, and on
erasure-coded pools drop what the mon would ignore, then order the rest by
walking the chains the mappings form.  A cycle has no valid order and is
left out, which covers the reverse pairs as well.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four places which read something from the cluster each repeated the
same if use_shell / else mon_command dance.  No change in behaviour.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 'jq -r .' pipes are no-ops left over from when the script read commands
with subprocess.getoutput(), which folds stderr into the json.  They also
hid failures, because with shell=True check=True only sees the exit status
of jq, which is 0 on empty input.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A command the mon refused was treated as empty output.  'osd pool ls
detail' is read as text, so a failure there left pool_type empty and the
script died with a KeyError on the first remapped pg.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five seconds is not enough for 'pg ls remapped' or 'osd dump' on a large
cluster with many remapped pgs, which is what this script is for.  A
librados error is not a ValueError either, so it came out as a traceback
instead of 'Error loading ...', and so did CalledProcessError in shell mode.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'pool' in line also matches the snapshot lines of the listing, so a pool
snapshot with 'pool' in its name overwrites the type of its pool with a
date, and the script exits with 'Unknown pool type' having done nothing.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pool which was deleted between reading the pgs and reading the pools
ended the run with a KeyError from pool_type[pool].

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pass the replicated flag rather than duplicating the gen_upmap() call, and
make has_upmap the set it wants to be.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A bare except also catches KeyboardInterrupt, and the two around the
gen_upmap() calls turned any bug inside it into a silently skipped pg.
Name what is expected at each of the three sites; gen_upmap() now returns
no mappings when up and acting differ in length, which also survives
python -O.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
crush_weight() walked the whole 'osd df' output for every shard of every
remapped pg.  Building the mappings for 30000 remapped 8+3 pgs on a 2000
osd cluster goes from 1.94s to 0.03s, with identical output.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Anything which was not exactly '--ignore-backfilling' was ignored, so a
mistyped option quietly did a full production run.  Parsing before
connecting also lets --help work without a cluster.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cluster.shutdown() was only reached by falling off the end of the script,
so none of the sys.exit() paths ran it, including 'There are no remapped
PGs'.  atexit covers them all and still leaves shell mode alone.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dvanders

Copy link
Copy Markdown
Collaborator Author

@bstillwell ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant