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.

docs/usage/cli.en.md

@@ -425,7 +425,7 @@ Optional parameters:
 | `--block_size 128k`            | Put pool only on OSDs with this data block size                            |
 | `--bitmap_granularity 4k`      | Put pool only on OSDs with this logical sector size                        |
 | `--immediate_commit none`      | Put pool only on OSDs with this or larger immediate_commit (none < small < all) |
-| `--level_placement <rules>`    | Use additional failure domain rules (example: "dc=112233", "" to remove)   |
+| `--level_placement <rules>`    | Use additional failure domain rules (example: "dc=112233")                 |
 | `--raw_placement <rules>`      | Specify raw PG generation rules ([details](../config/pool.en.md#raw_placement)) |
 | `--local_reads primary`        | Local read policy for replicated pools: primary, nearest or random         |
 | `--primary_affinity_tags tags` | Prefer to put primary copies on OSDs with all specified tags               |

docs/usage/cli.ru.md

@@ -444,7 +444,7 @@ OSD  PARENT            UP    SIZE  USED%    TAGS          WEIGHT  BLOCK  BITMAP
 | `--block_size 128k`            | ...только OSD с данным размером блока                                      |
 | `--bitmap_granularity 4k`      | ...только OSD с данным размером логического сектора                        |
 | `--immediate_commit none`      | ...только OSD с этим или большим immediate_commit (none < small < all)     |
-| `--level_placement <rules>`    | Задать правила дополнительных доменов отказа (пример: "dc=112233", "" — удалить) |
+| `--level_placement <rules>`    | Задать правила дополнительных доменов отказа (пример: "dc=112233")         |
 | `--raw_placement <rules>`      | Задать низкоуровневые правила генерации PG ([детали](../config/pool.ru.md#raw_placement)) |
 | `--local_reads primary`        | Политика локальных чтений для реплик: primary, nearest или random          |
 | `--primary_affinity_tags tags` | Предпочитать OSD со всеми данными тегами для роли первичных                |

src/cmd/cli.cpp

@@ -208,7 +208,7 @@ static const char* help_text =
     "    --block_size 128k             Put pool only on OSDs with this data block size\n"
     "    --bitmap_granularity 4k       Put pool only on OSDs with this logical sector size\n"
     "    --immediate_commit all        Put pool only on OSDs with this or larger immediate_commit (none < small < all)\n"
-    "    --level_placement <rules>     Use additional failure domain rules (example: \"dc=112233\", \"\" to remove)\n"
+    "    --level_placement <rules>     Use additional failure domain rules (example: \"dc=112233\")\n"
     "    --raw_placement <rules>       Specify raw PG generation rules (see documentation for details)\n"
     "    --local_reads primary         Local read policy for replicated pools: primary, nearest or random\n"
     "    --primary_affinity_tags tags  Prefer to put primary copies on OSDs with all specified tags\n"
@@ -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 },

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_info_t> user; // for http mode

src/cmd/cli_describe.cpp

@@ -33,6 +33,13 @@ std::vector<uint64_t> 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<osd_num_t> only_osds;
     uint64_t min_inode = 0, max_inode = 0;
     uint64_t min_offset = 0, max_offset = 0;
+    std::map<osd_num_t, std::string> skipped_pgs;
+    bool incomplete = false;
 
     cli_tool_t *parent = NULL;
     int state = 0;
@@ -83,19 +92,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 +111,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())
@@ -158,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++)
         {
@@ -190,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
@@ -197,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
@@ -263,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();
     }

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;
     }
 };

src/cmd/cli_fix.h

@@ -7,6 +7,7 @@
 #include <algorithm>
 
 std::vector<uint64_t> parse_uint64_list(json11::Json val);
+bool is_osd_up(cli_tool_t *parent, osd_num_t osd_num);
 
 template<class T> void remove_duplicates(std::vector<T> & ret)
 {

src/cmd/cli_pool_cfg.cpp

@@ -103,22 +103,24 @@ std::string validate_pool_config(json11::Json::object & new_cfg, json11::Json ol
             // level=rule, level=rule, ...
             if (!value.is_object())
             {
-                if (!value.is_null() && !value.is_string())
-                {
-                    return key+" must be a string";
-                }
                 json11::Json::object obj;
                 for (auto & item: explode(",", value.string_value(), true))
                 {
                     auto pair = explode("=", item, true);
-                    if (pair.size() != 2)
+                    if (pair.size() >= 2)
                     {
-                        return "--level_placement must be <level>=<rule>[,<level>=<rule>...], but it is "+value.string_value();
+                        obj[pair[0]] = pair[1];
                     }
-                    obj[pair[0]] = pair[1];
                 }
-                // empty value = remove the rules; null keeps the old ones from being merged back
-                value = obj.size() ? json11::Json(obj) : json11::Json();
+                if (obj.size())
+                {
+                    value = obj;
+                }
+                else
+                {
+                    new_cfg.erase(kv_it++);
+                    continue;
+                }
             }
         }
         else if (key == "osd_tags" || key == "primary_affinity_tags")
@@ -167,10 +169,6 @@ std::string validate_pool_config(json11::Json::object & new_cfg, json11::Json ol
     {
         new_cfg.erase("local_reads");
     }
-    if (new_cfg.find("level_placement") != new_cfg.end() && new_cfg["level_placement"].is_null())
-    {
-        new_cfg.erase("level_placement");
-    }
 
     // Prevent autovivification of object keys. Now we don't modify the config, we just check it
     json11::Json cfg = new_cfg;

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

tests/test_describe.sh

@@ -0,0 +1,98 @@
+#!/bin/bash -ex
+
+# Test vitastor-cli describe and fix on degraded objects and with down OSDs
+
+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
+
+# 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
+$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
+
+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

tests/test_level_placement.sh

@@ -36,23 +36,4 @@ $ETCDCTL get --prefix /vitastor/pg/config --print-value-only | \
         (. | map_values((. - 1) / 2 | floor) | unique | length) == 6 and
         (. | map_values((. - 1) / 4 | floor) | unique | length) == 3) ] | length) == 32'
 
-# A malformed --level_placement is refused, an empty one removes the rules
-r=0
-$VITASTOR_CLI modify-pool testpool --level_placement rack || r=$?
-if [[ $r != 22 ]]; then
-    format_error "modify-pool --level_placement rack should fail with EINVAL (22), but exited with $r"
-fi
-$ETCDCTL get --print-value-only /vitastor/config/pools | jq -e '.["1"].level_placement == {"rack":"112233"}' || \
-    format_error "level_placement should stay rack=112233 after a refused change"
-$VITASTOR_CLI modify-pool testpool --level_placement ''
-$ETCDCTL get --print-value-only /vitastor/config/pools | jq -e '.["1"] | has("level_placement") | not' || \
-    format_error "modify-pool --level_placement '' should remove level_placement"
-sleep 2
-if grep -q 'configuration is invalid' ./testdata/mon.log; then
-    format_error "Monitor refused the pool configuration without level_placement"
-fi
-$ETCDCTL get --prefix /vitastor/pg/config --print-value-only | \
-    jq -s -e '([ .[0].items["1"] | .[].osd_set | map_values(. | tonumber) | select(
-        (. | map_values((. - 1) / 2 | floor) | unique | length) == 6) ] | length) == 32'
-
 format_green OK