Fix vitastor-cli ls --tree hiding layers whose parent is filtered out

ls --tree built the tree starting from the roots of the selected layers,
so a layer whose parent was left out by a glob, -p or -n was never
reached and not printed at all: ls --tree testimg printed nothing when
testimg had a parent. Show such a layer as a root; its parent is still
shown in the PARENT column.

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"

src/cmd/cli_ls.cpp

@@ -250,11 +250,17 @@ resume_1:
 
     json11::Json::array to_tree(const json11::Json::array & array)
     {
+        std::set<uint64_t> listed;
+        for (const auto & item: array)
+        {
+            listed.insert(item["inode_id"].uint64_value());
+        }
         std::map<uint64_t, json11::Json::array> children;
         for (const auto & item: array)
         {
             uint64_t parent_id = item["parent_inode_id"].uint64_value();
-            children[parent_id].push_back(item);
+            // Layers with a filtered out parent are shown as roots
+            children[listed.find(parent_id) != listed.end() ? parent_id : 0].push_back(item);
         }
         json11::Json::array tree;
         std::function<void(uint64_t, const std::string &)> add_children = [&](uint64_t parent_id, const std::string & prefix)

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

@@ -122,6 +122,7 @@ OLD=1 ./test_corrupt_all.sh
 
 ./test_reweight_half.sh
 ./test_snapshot_pool2.sh
+./test_ls_tree.sh
 ./test_snapshot_read_bitmap.sh
 
 TEST_NAME=csum_32k_dmj OSD_ARGS="--data_csum_type crc32c --csum_block_size 32k --inmemory_metadata false --inmemory_journal false" OFFSET_ARGS=$OSD_ARGS ./test_heal.sh

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

tests/test_ls_tree.sh

@@ -0,0 +1,40 @@
+#!/bin/bash -ex
+
+# Test vitastor-cli ls --tree with filters which leave out parent layers
+
+. `dirname $0`/run_3osds.sh
+
+$VITASTOR_CLI create -s 128M testimg
+$VITASTOR_CLI snap-create testimg@snap1
+$VITASTOR_CLI create --parent testimg@snap1 testclone
+
+$VITASTOR_CLI create-pool testpool2 -s 3 -n 4 --failure_domain osd
+wait_pool_up 30 2 3 4
+$VITASTOR_CLI create --parent testimg@snap1 -p testpool2 testclone2
+
+# First column of each row without the header
+check_tree()
+{
+    local expected=$1
+    shift
+    local rows=$($VITASTOR_CLI ls --tree "$@" | tail -n +2 | sed 's/  .*//' | tr '\n' ';')
+    if [[ "$rows" != "$expected" ]]; then
+        format_error "ls --tree $* shows '$rows', expected '$expected'"
+    fi
+}
+
+$VITASTOR_CLI ls --tree
+
+check_tree 'testimg@snap1;├─ testclone;├─ testclone2;└─ testimg;'
+check_tree 'testimg@snap1;└─ testimg;' 'testimg*'
+# Layers whose parent is filtered out are shown as roots
+check_tree 'testimg;' testimg
+check_tree 'testclone;testimg;' testclone testimg
+check_tree 'testclone;' -n 1
+check_tree 'testclone2;' -p testpool2
+
+if ! $VITASTOR_CLI ls --tree testimg | grep -q ' testimg@snap1 *$'; then
+    format_error "ls --tree testimg should show testimg@snap1 as the parent"
+fi
+
+format_green OK