From bb0ce4e552953412c3c6442cfcd91ff456e7fb5e Mon Sep 17 00:00:00 2001 From: Yury Luneff Date: Tue, 20 Oct 2026 10:44:56 +0300 Subject: [PATCH 1/3] Fix vitastor-cli describe listing only one object with --inode describe --inode sent the range from offset 0 to offset 0 of the inode and ignored --min-offset and --max-offset, and --max-inode without --max-offset also ended the range at offset 0 of that inode. The OSD ends the listing at (max_inode, max_offset) inclusive, so only the object at offset 0 of the last inode was listed. Honour the offsets with --inode and end the range at the last offset of max_inode when --max-offset is not given. --- src/cmd/cli_describe.cpp | 8 ++++--- tests/run_tests.sh | 1 + tests/test_describe.sh | 47 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) create mode 100755 tests/test_describe.sh diff --git a/src/cmd/cli_describe.cpp b/src/cmd/cli_describe.cpp index 70cf73db..bfbebb68 100644 --- a/src/cmd/cli_describe.cpp +++ b/src/cmd/cli_describe.cpp @@ -83,19 +83,18 @@ struct cli_describe_t } only_pg = cfg["pg"].uint64_value(); min_inode = cfg["inode"].uint64_value(); + min_offset = stoull_full(cfg["min_offset"].string_value(), 0); + max_offset = stoull_full(cfg["max_offset"].string_value(), 0); if (min_inode) { if (!INODE_POOL(min_inode)) min_inode |= (uint64_t)only_pool << (64-POOL_ID_BITS); max_inode = min_inode; - min_offset = max_offset = 0; } else { min_inode = stoull_full(cfg["min_inode"].string_value(), 0); // to support 0x... max_inode = stoull_full(cfg["max_inode"].string_value(), 0); - min_offset = stoull_full(cfg["min_offset"].string_value(), 0); - max_offset = stoull_full(cfg["max_offset"].string_value(), 0); if (!min_inode && !max_inode && only_pool) { min_inode = (uint64_t)only_pool << (64-POOL_ID_BITS); @@ -103,6 +102,9 @@ struct cli_describe_t (((uint64_t)1 << (64-POOL_ID_BITS)) - 1); } } + // The OSD ends the listing at (max_inode, max_offset) inclusive + if (max_inode && cfg["max_offset"].is_null()) + max_offset = UINT64_MAX; only_osds = parse_uint64_list(cfg["osds"]); object_state = stoull_full(cfg["object_state"].string_value(), 0); if (!object_state && cfg["object_state"].is_string()) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index d72fcf78..4783d35e 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -65,6 +65,7 @@ OLD=1 ./test_move_reappear.sh ./test_rm.sh ./test_rm_degraded.sh +./test_describe.sh ./test_snapshot_chain.sh SCHEME=ec ./test_snapshot_chain.sh diff --git a/tests/test_describe.sh b/tests/test_describe.sh new file mode 100755 index 00000000..9d1ad078 --- /dev/null +++ b/tests/test_describe.sh @@ -0,0 +1,47 @@ +#!/bin/bash -ex + +# Test vitastor-cli describe on degraded objects + +PG_COUNT=4 +PG_SIZE=3 +PG_MINSIZE=2 +# Keep degraded objects until the checks are done +GLOBAL_CONFIG=',"no_recovery":true' +. `dirname $0`/run_3osds.sh + +$VITASTOR_CLI create -s 32M testimg + +# Data written while OSD 3 is down is degraded when it comes back +kill -9 $OSD3_PID +$ETCDCTL del /vitastor/osd/state/3 +wait_condition 60 "$ETCDCTL get --prefix /vitastor/pg/state/ --print-value-only | jq -s -e '[ .[] | select(.state | index(\"degraded\")) ] | length == $PG_COUNT'" "PG peering without OSD 3" +$VITASTOR_CLI dd if=/dev/urandom oimg=testimg bs=1M count=32 +start_osd 3 +wait_condition 60 "$ETCDCTL get --prefix /vitastor/pg/state/ --print-value-only | jq -s -e '[ .[] | select((.state | index(\"active\")) and (.state | index(\"has_degraded\"))) ] | length == $PG_COUNT'" "PG peering with OSD 3" + +INODE_ID=$($VITASTOR_CLI ls --json | jq -r '.[] | select(.name == "testimg") | .inode_id') +INODE=$(printf '0x%x' $INODE_ID) +INODE_NUM=$((INODE_ID & 0xffffffffffff)) + +# --inode lists the whole inode, not only its first object +all=$($VITASTOR_CLI describe --json | jq -c '[ .[] | select(.inode == "'$INODE'") | .stripe ] | unique') +if [[ $(echo "$all" | jq length) -le 1 ]]; then + format_error "describe should list more than one degraded object of testimg: $all" +fi +one=$($VITASTOR_CLI describe --pool 1 --inode $INODE_NUM --json | jq -c '[ .[].stripe ] | unique') +if [[ "$one" != "$all" ]]; then + format_error "describe --inode lists objects $one instead of $all" +fi + +# --min-offset and --max-offset narrow --inode, and --max-inode includes its whole inode +second=$(echo "$all" | jq -r '.[1]') +part=$($VITASTOR_CLI describe --pool 1 --inode $INODE_NUM --min-offset $second --max-offset $second --json | jq -c '[ .[].stripe ] | unique') +if [[ "$part" != "[\"$second\"]" ]]; then + format_error "describe --inode --min-offset $second --max-offset $second lists objects $part" +fi +range=$($VITASTOR_CLI describe --min-inode $INODE --max-inode $INODE --json | jq -c '[ .[].stripe ] | unique') +if [[ "$range" != "$all" ]]; then + format_error "describe --min-inode $INODE --max-inode $INODE lists objects $range instead of $all" +fi + +format_green OK -- 2.47.3 From 4fa06ef7e1a7c57f33bbc5066848f6fbe5871ffe Mon Sep 17 00:00:00 2001 From: Yury Luneff Date: Tue, 20 Oct 2026 10:53:29 +0300 Subject: [PATCH 2/3] Fix vitastor-cli describe hanging when an OSD is down The client queues a request to an OSD which is down until the OSD comes back, so describe hung forever when a PG primary or an OSD from --osds was down. Skip such PGs and OSDs with a warning, ask only the primary of the PG when --pool and --pg are given, and exit with EIO after printing what could be listed, so that a partial listing is not taken for a complete one. With --json, stdout stays a valid JSON array. --- src/cmd/cli.cpp | 2 +- src/cmd/cli.h | 1 + src/cmd/cli_describe.cpp | 52 +++++++++++++++++++++++++++++++++++++++- src/cmd/cli_fix.h | 1 + tests/test_describe.sh | 36 +++++++++++++++++++++++++++- 5 files changed, 89 insertions(+), 3 deletions(-) diff --git a/src/cmd/cli.cpp b/src/cmd/cli.cpp index 28ba05af..93f55c15 100644 --- a/src/cmd/cli.cpp +++ b/src/cmd/cli.cpp @@ -687,7 +687,7 @@ static int run(cli_tool_t *p, json11::Json::object cfg) { printf("%s\n", result.data.dump().c_str()); } - else if (p->json_output && result.err) + else if (p->json_output && result.err && !p->json_printed) { printf("%s\n", json11::Json(json11::Json::object { { "error_code", result.err }, diff --git a/src/cmd/cli.h b/src/cmd/cli.h index bcb6881d..f3a16ea9 100644 --- a/src/cmd/cli.h +++ b/src/cmd/cli.h @@ -38,6 +38,7 @@ public: bool json_output = false; int log_level = 0; bool is_command_line = false; + bool json_printed = false; bool color = false; std::shared_ptr user; // for http mode diff --git a/src/cmd/cli_describe.cpp b/src/cmd/cli_describe.cpp index bfbebb68..d2414698 100644 --- a/src/cmd/cli_describe.cpp +++ b/src/cmd/cli_describe.cpp @@ -33,6 +33,13 @@ std::vector parse_uint64_list(json11::Json val) return ret; } +bool is_osd_up(cli_tool_t *parent, osd_num_t osd_num) +{ + auto & peer_states = parent->cli->st_cli->peer_states; + auto peer_it = peer_states.find(osd_num); + return peer_it != peer_states.end() && !peer_it->second.is_null(); +} + struct cli_describe_t { uint64_t object_state = 0; @@ -41,6 +48,8 @@ struct cli_describe_t std::vector only_osds; uint64_t min_inode = 0, max_inode = 0; uint64_t min_offset = 0, max_offset = 0; + std::map skipped_pgs; + bool incomplete = false; cli_tool_t *parent = NULL; int state = 0; @@ -160,15 +169,49 @@ struct cli_describe_t if (pp.first >= min_pool && (!max_pool || pp.first <= max_pool)) { for (auto & pgp: pp.second.pg_config) - only_osds.push_back(pgp.second.cur_primary); + { + if (only_pool && only_pg && (pp.first != only_pool || pgp.first != only_pg)) + continue; + if (is_osd_up(parent, pgp.second.cur_primary)) + only_osds.push_back(pgp.second.cur_primary); + else + { + auto & pgs = skipped_pgs[pgp.second.cur_primary]; + pgs += (pgs == "" ? "" : ", ")+std::to_string(pp.first)+"/"+std::to_string(pgp.first); + } + } } } } + else + { + for (auto osd_num: only_osds) + { + if (!is_osd_up(parent, osd_num)) + skipped_pgs[osd_num] = ""; + } + only_osds.erase(std::remove_if(only_osds.begin(), only_osds.end(), [this](osd_num_t osd_num) + { + return !is_osd_up(parent, osd_num); + }), only_osds.end()); + } + // Requests to an OSD which is down are never answered + for (auto & sk: skipped_pgs) + { + if (!sk.first) + fprintf(stderr, "No primary OSD for PG %s, skipping\n", sk.second.c_str()); + else if (sk.second != "") + fprintf(stderr, "OSD %ju is down, skipping PG %s\n", sk.first, sk.second.c_str()); + else + fprintf(stderr, "OSD %ju is down, skipping it\n", sk.first); + incomplete = true; + } remove_duplicates(only_osds); parent->cli->init_msgr(); if (parent->json_output && parent->is_command_line) { printf("[\n"); + parent->json_printed = true; } for (int i = 0; i < only_osds.size(); i++) { @@ -192,6 +235,7 @@ struct cli_describe_t { if (op->reply.hdr.retval < 0) { + incomplete = true; fprintf( stderr, "Failed to describe objects on OSD %ju (retval=%jd)\n", osd_num, op->reply.hdr.retval @@ -199,6 +243,7 @@ struct cli_describe_t } else if (op->reply.describe.result_bytes != op->reply.hdr.retval * sizeof(osd_reply_describe_item_t)) { + incomplete = true; fprintf( stderr, "Invalid response size from OSD %ju (expected %ju bytes, got %ju bytes)\n", osd_num, op->reply.hdr.retval * sizeof(osd_reply_describe_item_t), op->reply.describe.result_bytes @@ -265,6 +310,11 @@ struct cli_describe_t { result.data = describe_items; } + if (incomplete) + { + result.err = EIO; + result.text = "The listing is incomplete, see the messages above"; + } state = 100; describe_items.clear(); } diff --git a/src/cmd/cli_fix.h b/src/cmd/cli_fix.h index 9371742f..3891c524 100644 --- a/src/cmd/cli_fix.h +++ b/src/cmd/cli_fix.h @@ -7,6 +7,7 @@ #include std::vector parse_uint64_list(json11::Json val); +bool is_osd_up(cli_tool_t *parent, osd_num_t osd_num); template void remove_duplicates(std::vector & ret) { diff --git a/tests/test_describe.sh b/tests/test_describe.sh index 9d1ad078..7aa592a3 100755 --- a/tests/test_describe.sh +++ b/tests/test_describe.sh @@ -1,6 +1,6 @@ #!/bin/bash -ex -# Test vitastor-cli describe on degraded objects +# Test vitastor-cli describe on degraded objects and with down OSDs PG_COUNT=4 PG_SIZE=3 @@ -44,4 +44,38 @@ if [[ "$range" != "$all" ]]; then format_error "describe --min-inode $INODE --max-inode $INODE lists objects $range instead of $all" fi +# describe must not wait for an OSD which is down. +# Stop the monitor so that PGs keep the down OSD as their primary, +# and stop a primary OSD which has the latest data, so other PGs stay degraded. +VICTIM=$($VITASTOR_CLI ls-pgs --json | jq -r '[ .[].cur_primary | select(. == 1 or . == 2) ] | first') +VICTIM_PG=$($VITASTOR_CLI ls-pgs --json | jq -r '[ .[] | select(.cur_primary == '$VICTIM') | .pg_num ] | first') +OTHER_PG_COUNT=$($VITASTOR_CLI ls-pgs --json | jq '[ .[] | select(.cur_primary != '$VICTIM') ] | length') +if [[ "$VICTIM" = null || "$OTHER_PG_COUNT" = 0 ]]; then + format_error "Expected PGs with primary OSD 1 or 2 and PGs with other primary OSDs" +fi + +kill -9 $MON_PID +eval kill -9 \$OSD${VICTIM}_PID +$ETCDCTL del /vitastor/osd/state/$VICTIM +wait_condition 60 "$ETCDCTL get --prefix /vitastor/pg/state/ --print-value-only | jq -s -e '[ .[] | select(.primary != $VICTIM and (.state | index(\"degraded\"))) ] | length == $OTHER_PG_COUNT'" "PG peering without OSD $VICTIM" +# Keep PG 1/$VICTIM_PG active with the down primary, as it is until the OSD's etcd lease expires +$ETCDCTL put /vitastor/pg/state/1/$VICTIM_PG '{"peers":[1,2,3],"primary":'$VICTIM',"state":["active"]}' + +# A listing with skipped OSDs or PGs is incomplete: EIO, but still valid JSON on stdout +for args in "" "--osds 1,2,3" "--pool 1 --pg $VICTIM_PG"; do + rc=0 + timeout 30 $VITASTOR_CLI describe $args --json > ./testdata/describe.json 2> ./testdata/describe.err || rc=$? + [[ $rc == 5 ]] || format_error "describe $args exited with $rc instead of EIO with OSD $VICTIM down" + grep -q "OSD $VICTIM is down, skipping \(it\|PG 1/$VICTIM_PG\)" ./testdata/describe.err || \ + format_error "describe $args should warn about OSD $VICTIM: $(cat ./testdata/describe.err)" + grep -q "The listing is incomplete" ./testdata/describe.err || \ + format_error "describe $args should say that the listing is incomplete: $(cat ./testdata/describe.err)" + count=$(jq length ./testdata/describe.json) || format_error "describe $args printed invalid JSON" + if [[ "$args" == --pool* ]]; then + [[ $count == 0 ]] || format_error "describe $args should list nothing" + else + [[ $count -gt 0 ]] || format_error "describe $args should still list objects from the other OSDs" + fi +done + format_green OK -- 2.47.3 From a85a4febc95c464cbf38ec28bf5573d2d539aa81 Mon Sep 17 00:00:00 2001 From: Yury Luneff Date: Tue, 20 Oct 2026 10:56:40 +0300 Subject: [PATCH 3/3] Fix vitastor-cli fix hanging when an OSD is down The client queues a request to an OSD which is down until the OSD comes back, so fix hung forever when the primary of an object's PG or one of --bad-osds was down. Refuse to start while one of --bad-osds is down, skip objects whose PG primary is down as it is done for inactive PGs, and exit with EIO when some objects were skipped or a request failed, so that a partial repair is not taken for a complete one. --- src/cmd/cli_fix.cpp | 35 +++++++++++++++++++++++++++++++++++ tests/test_describe.sh | 21 +++++++++++++++++++-- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/cmd/cli_fix.cpp b/src/cmd/cli_fix.cpp index 5de14aa0..f784f496 100644 --- a/src/cmd/cli_fix.cpp +++ b/src/cmd/cli_fix.cpp @@ -21,6 +21,7 @@ struct cli_fix_t json11::Json options; cli_result_t result; json11::Json::array fix_result; + bool incomplete = false; bool is_done() { @@ -123,6 +124,20 @@ struct cli_fix_t state = 100; return; } + { + std::string down_osds; + for (auto osd_num: bad_osds) + { + if (!is_osd_up(parent, osd_num)) + down_osds += (down_osds == "" ? "" : ", ")+std::to_string(osd_num); + } + if (down_osds != "") + { + result = (cli_result_t){ .err = EAGAIN, .text = "Bad OSD(s) "+down_osds+" are down, can't remove object copies from them" }; + state = 100; + return; + } + } remove_duplicates(objects); parent->cli->init_msgr(); resume_1: @@ -138,6 +153,7 @@ struct cli_fix_t if (pool_cfg_it == parent->cli->st_cli->pool_config.end()) { fprintf(stderr, "Object %jx:%jx is from unknown pool\n", obj.inode, obj.stripe); + incomplete = true; continue; } auto & pool_cfg = pool_cfg_it->second; @@ -150,9 +166,20 @@ struct cli_fix_t stderr, "Object %jx:%jx is from PG %u/%u which is not currently active\n", obj.inode, obj.stripe, pool_cfg_it->first, pg_num ); + incomplete = true; continue; } osd_num_t primary_osd = pg_it->second.cur_primary; + if (!is_osd_up(parent, primary_osd)) + { + // Requests to an OSD which is down are never answered + fprintf( + stderr, "Object %jx:%jx is from PG %u/%u whose primary OSD %ju is down\n", + obj.inode, obj.stripe, pool_cfg_it->first, pg_num, primary_osd + ); + incomplete = true; + continue; + } // Describe -> Remove some copies -> Scrub again osd_op_t *op = new osd_op_t; op->req = (osd_any_op_t){ @@ -172,6 +199,7 @@ struct cli_fix_t if (op->reply.hdr.retval < 0 || op->reply.describe.result_bytes != op->reply.hdr.retval * sizeof(osd_reply_describe_item_t)) { fprintf(stderr, "Failed to describe objects on OSD %ju (retval=%jd)\n", primary_osd, op->reply.hdr.retval); + incomplete = true; parent->waiting--; loop(); } @@ -208,6 +236,7 @@ struct cli_fix_t (*rm_count)--; if (rm_op->reply.hdr.retval < 0) { + incomplete = true; fprintf( stderr, "Failed to remove object %jx:%jx from OSD %ju (retval=%jd)\n", rm_op->req.sec_del.oid.inode, rm_op->req.sec_del.oid.stripe, @@ -252,6 +281,7 @@ struct cli_fix_t { if (scrub_op->reply.hdr.retval < 0 && scrub_op->reply.hdr.retval != -ENOENT) { + incomplete = true; fprintf( stderr, "Failed to scrub %jx:%jx on OSD %ju (retval=%jd)\n", obj.inode, obj.stripe, primary_osd, scrub_op->reply.hdr.retval @@ -289,6 +319,11 @@ struct cli_fix_t { result.data = fix_result; } + if (incomplete) + { + result.err = EIO; + result.text = "Some objects were not fixed, see the messages above"; + } state = 100; } }; diff --git a/tests/test_describe.sh b/tests/test_describe.sh index 7aa592a3..b8022b06 100755 --- a/tests/test_describe.sh +++ b/tests/test_describe.sh @@ -1,6 +1,6 @@ #!/bin/bash -ex -# Test vitastor-cli describe on degraded objects and with down OSDs +# Test vitastor-cli describe and fix on degraded objects and with down OSDs PG_COUNT=4 PG_SIZE=3 @@ -44,15 +44,18 @@ if [[ "$range" != "$all" ]]; then format_error "describe --min-inode $INODE --max-inode $INODE lists objects $range instead of $all" fi -# describe must not wait for an OSD which is down. +# describe and fix must not wait for an OSD which is down. # Stop the monitor so that PGs keep the down OSD as their primary, # and stop a primary OSD which has the latest data, so other PGs stay degraded. VICTIM=$($VITASTOR_CLI ls-pgs --json | jq -r '[ .[].cur_primary | select(. == 1 or . == 2) ] | first') VICTIM_PG=$($VITASTOR_CLI ls-pgs --json | jq -r '[ .[] | select(.cur_primary == '$VICTIM') | .pg_num ] | first') +OTHER_PG=$($VITASTOR_CLI ls-pgs --json | jq -r '[ .[] | select(.cur_primary != '$VICTIM') | .pg_num ] | first') OTHER_PG_COUNT=$($VITASTOR_CLI ls-pgs --json | jq '[ .[] | select(.cur_primary != '$VICTIM') ] | length') if [[ "$VICTIM" = null || "$OTHER_PG_COUNT" = 0 ]]; then format_error "Expected PGs with primary OSD 1 or 2 and PGs with other primary OSDs" fi +VICTIM_OBJ=$($VITASTOR_CLI describe --pool 1 --pg $VICTIM_PG --json | jq -r '.[0] | .inode+":"+.stripe') +OTHER_OBJ=$($VITASTOR_CLI describe --pool 1 --pg $OTHER_PG --json | jq -r '.[0] | .inode+":"+.stripe') kill -9 $MON_PID eval kill -9 \$OSD${VICTIM}_PID @@ -78,4 +81,18 @@ for args in "" "--osds 1,2,3" "--pool 1 --pg $VICTIM_PG"; do fi done +if timeout 30 $VITASTOR_CLI fix --bad-osds $VICTIM --objects $OTHER_OBJ 2> ./testdata/fix.err; then + format_error "fix should refuse to work with bad OSD $VICTIM down" +fi +grep -q "Bad OSD(s) $VICTIM are down" ./testdata/fix.err || format_error "fix printed: $(cat ./testdata/fix.err)" + +# An object which is skipped is not fixed: EIO +rc=0 +timeout 30 $VITASTOR_CLI fix --bad-osds 3 --objects $VICTIM_OBJ 2> ./testdata/fix.err || rc=$? +[[ $rc == 5 ]] || format_error "fix exited with $rc instead of EIO on an object from PG 1/$VICTIM_PG with primary OSD $VICTIM down" +grep -q "is from PG 1/$VICTIM_PG whose primary OSD $VICTIM is down" ./testdata/fix.err || \ + format_error "fix should skip the object from PG 1/$VICTIM_PG: $(cat ./testdata/fix.err)" +grep -q "Some objects were not fixed" ./testdata/fix.err || \ + format_error "fix should say that it skipped objects: $(cat ./testdata/fix.err)" + format_green OK -- 2.47.3