Allow to remove level_placement with an empty modify-pool value

An empty --level_placement was dropped before the merge with the old
pool config, so the old rules stayed and could not be removed at all,
and a malformed value was silently dropped the same way. Make an empty
value remove the key, which the monitor treats as no extra rules, and
refuse a value that is not <level>=<rule>[,...] with EINVAL.

docs/usage/cli.en.md

@@ -355,7 +355,7 @@ host     stump
 
 ## ls-osd
 
-`vitastor-cli osds|ls-osds|ls-osd|osd-ls [-l|--long]`
+`vitastor-cli osds|ls-osd|osd-ls [-l|--long]`
 
 Show current OSDs as list, optionally with I/O statistics if -l is specified.
 
@@ -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")                 |
+| `--level_placement <rules>`    | Use additional failure domain rules (example: "dc=112233", "" to remove)   |
 | `--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               |
@@ -508,7 +508,7 @@ the Vitastor authorization model.
 
 ## modify-user
 
-`vitastor-cli modify-user|user-modify --groups <group1,group2,...> <username>`
+`vitastor-cli modify-user --groups <group1,group2,...> <username>`
 
 Create or update a user with the given name (matching TLS certificate CN).
 

docs/usage/cli.ru.md

@@ -373,7 +373,7 @@ host     stump
 
 ## ls-osd
 
-`vitastor-cli osds|ls-osds|ls-osd|osd-ls [-l|--long]`
+`vitastor-cli osds|ls-osd|osd-ls [-l|--long]`
 
 Показать список OSD, со статистикой ввода-вывода, если установлено -l.
 
@@ -403,7 +403,7 @@ OSD  PARENT            UP    SIZE  USED%    TAGS          WEIGHT  BLOCK  BITMAP
 
 ## pg-list
 
-`vitastor-cli pg-list|pg-ls|list-pg|ls-pg|ls-pgs|pgs [OPTIONS] [state1+state2] [^state3] [...]`
+`vitastor-cli pg-list|pg-ls|list-pg|ls-pg|ls-pgs [OPTIONS] [state1+state2] [^state3] [...]`
 
 Вывести список PG с состояними, удовлетворяющими любому из переданных фильтров (^ или !
 в начале фильтра означает отрицание). Опции:
@@ -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 со всеми данными тегами для роли первичных                |
@@ -527,7 +527,7 @@ OSD  PARENT            UP    SIZE  USED%    TAGS          WEIGHT  BLOCK  BITMAP
 
 ## modify-user
 
-`vitastor-cli modify-user|user-modify --groups <group1,group2,...> <username>`
+`vitastor-cli modify-user --groups <group1,group2,...> <username>`
 
 Создать или изменить пользователя с указанным именем (совпадающим с CN TLS-сертификата).
 

src/cmd/cli.cpp

@@ -179,7 +179,7 @@ static const char* help_text =
     "vitastor-cli osd-tree [-l|--long]\n"
     "  Show current OSD tree, optionally with I/O statistics if -l is specified.\n"
     "\n"
-    "vitastor-cli osds|ls-osds|ls-osd|osd-ls [-l|--long]\n"
+    "vitastor-cli osds|ls-osd|osd-ls [-l|--long]\n"
     "  Show current OSDs as list, optionally with I/O statistics if -l is specified.\n"
     "\n"
     "vitastor-cli modify-osd [--tags tag1,tag2,...] [--reweight <number>] [--noout true/false] <osd_number>\n"
@@ -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\")\n"
+    "    --level_placement <rules>     Use additional failure domain rules (example: \"dc=112233\", \"\" to remove)\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"
@@ -253,7 +253,7 @@ static const char* help_text =
     "vitastor-cli ls-users|user-ls|ls-user|list-users [<name> ...]\n"
     "  List users (only with specified names if passed).\n"
     "\n"
-    "vitastor-cli modify-user|user-modify --type <type> --groups group1,group2,... <username>\n"
+    "vitastor-cli modify-user --type <type> --groups group1,group2,... <username>\n"
     "  Create or update user permissions. User names match CN of their certificates.\n"
     "  --type TYPE      Set user type: client or admin. Default is client.\n"
     "  --groups GROUPS  Set user's groups.\n"

src/cmd/cli_pool_cfg.cpp

@@ -103,24 +103,22 @@ 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)
                     {
-                        obj[pair[0]] = pair[1];
+                        return "--level_placement must be <level>=<rule>[,<level>=<rule>...], but it is "+value.string_value();
                     }
+                    obj[pair[0]] = pair[1];
                 }
-                if (obj.size())
-                {
-                    value = obj;
-                }
-                else
-                {
-                    new_cfg.erase(kv_it++);
-                    continue;
-                }
+                // empty value = remove the rules; null keeps the old ones from being merged back
+                value = obj.size() ? json11::Json(obj) : json11::Json();
             }
         }
         else if (key == "osd_tags" || key == "primary_affinity_tags")
@@ -169,6 +167,10 @@ 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

@@ -95,8 +95,6 @@ OLD=1 SCHEME=ec IMMEDIATE_COMMIT=1 ./test_rebalance_verify.sh
 
 ./test_dd.sh
 
-./test_cli_help.sh
-
 ./test_root_node.sh
 
 ./test_switch_primary.sh

tests/test_cli_help.sh

@@ -1,14 +0,0 @@
-#!/bin/bash -ex
-
-# Test that vitastor-cli --help knows every command name and alias the CLI accepts
-
-. `dirname $0`/common.sh
-
-for cmd in $(grep -o 'cmd\[0\] == "[^"]*"' src/cmd/cli.cpp | sed 's/.*== "//; s/"$//'); do
-    build/src/cmd/vitastor-cli --help $cmd > ./testdata/help.txt 2>&1 || true
-    if grep -q '^Unknown command' ./testdata/help.txt; then
-        format_error "vitastor-cli --help $cmd does not know the command"
-    fi
-done
-
-format_green OK

tests/test_level_placement.sh

@@ -36,4 +36,23 @@ $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