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

@@ -231,8 +231,8 @@ Options:
 * `--writers-stopped` allows optimised removal in case of a single 'slim' read-write
   child and 'fat' removed parent: the child is merged into parent and parent is renamed
   to child in that case. In other cases parent layers are always merged into children.
-* `--exact` - remove multiple images with given names.
-* `--matching` - remove multiple images with names matching given glob patterns.
+* `--exact` - remove multiple images with names matching given glob patterns.
+* `--matching` - remove multiple images with given names
 * `--down-ok` - continue deletion/merging even if some data will be left on unavailable OSDs.
 
 ## flatten
@@ -287,7 +287,7 @@ Describe unclean object locations in the cluster. Options:
 
 ## fix
 
-`vitastor-cli fix [--objects <objects>] --bad-osds <osds> [--part <part>] [--check no]`
+`vitastor-cli fix [--objects <objects>] [--bad-osds <osds>] [--part <part>] [--check no]`
 
 Fix inconsistent objects in the cluster by deleting some copies.
 
@@ -298,7 +298,7 @@ Fix inconsistent objects in the cluster by deleting some copies.
     Plain text format: 0x<inode>:0x<stripe> <any delimiter> 0x<inode>:0x<stripe> ...
     JSON format: [{"inode":"0x...","stripe":"0x..."},...]
 --bad-osds <osds>
-    Required. Remove inconsistent copies/parts of objects from these OSDs, effectively
+    Remove inconsistent copies/parts of objects from these OSDs, effectively
     marking them bad and allowing Vitastor to recover objects from other copies.
 --part <number>
     Only remove EC part <number> (from 0 to pg_size-1), required for extreme
@@ -316,17 +316,14 @@ Allocate a new OSD number and reserve it by creating empty `/osd/stats/<n>` key.
 
 ## rm-osd
 
-`vitastor-cli rm-osd [-f|--force] [--allow-data-loss] [--allow-up] [--dry-run] <osd_id> [osd_id...]`
+`vitastor-cli rm-osd [--force] [--allow-data-loss] [--dry-run] <osd_id> [osd_id...]`
 
 Remove metadata and configuration for specified OSD(s) from etcd.
 
-Refuses to remove OSDs that are up or whose removal makes PGs degraded, offline or incomplete.
+Refuses to remove OSDs with data without `--force` and `--allow-data-loss`.
 
-* `-f|--force` - remove OSDs even if they are up or PGs become degraded or offline.
-* `--allow-data-loss` - remove OSDs even if PGs or objects become incomplete (DATA LOSS). Implies `--force`.
-* `--allow-up` - remove OSDs even if they are up.
-* `--dry-run` - only check if deletion is possible without data loss and
-  redundancy degradation.
+With `--dry-run` only checks if deletion is possible without data loss and
+redundancy degradation.
 
 ## osd-tree
 
@@ -428,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               |

docs/usage/cli.ru.md

@@ -239,8 +239,8 @@ vitastor-cli dd [iimg=<image> | if=<file>] [oimg=<image> | of=<file>] [bs=1M] \
   у удаляемой цепочки есть только один дочерний образ, содержащий небольшой объём данных.
   В этом случае дочерний образ вливается в родительский и удаляется, а родительский
   переименовывается в дочерний.
-* `--exact` - удалить все образы с точно заданными именами.
-* `--matching` - удалить все образы с именами, подходящими под переданные glob-шаблоны.
+* `--exact` - удалить все образы с именами, подходящими под переданные glob-шаблоны.
+* `--matching` - удалить все образы с точно заданными именами.
 * `--down-ok` - продолжать удаление/слияние, даже если часть данных останется неудалённой на недоступных OSD.
 
 ## flatten
@@ -302,7 +302,7 @@ vitastor-cli dd [iimg=<image> | if=<file>] [oimg=<image> | of=<file>] [bs=1M] \
 
 ## fix
 
-`vitastor-cli fix [--objects <объекты>] --bad-osds <osds> [--part <номер>] [--check no]`
+`vitastor-cli fix [--objects <объекты>] [--bad-osds <osds>] [--part <номер>] [--check no]`
 
 Исправить неконсистентные (неоднозначные) объекты путём удаления части копий.
 
@@ -313,9 +313,9 @@ vitastor-cli dd [iimg=<image> | if=<file>] [oimg=<image> | of=<file>] [bs=1M] \
     Простой формат: 0x<инод>:0x<смещение> <любой разделитель> 0x<инод>:0x<смещение> ...
     Формат JSON: [{"inode":"0x<инод>","stripe":"0x<смещение>"},...]
 --bad-osds <osds>
-    Обязательная опция. Удалить неконсистентные копии/части объектов с данных
-    OSD, таким образом признавая потерю этих копий и позволяя Vitastor-у
-    восстановить объекты из других копий.
+    Удалить неконсистентные копии/части объектов с данных OSD, таким образом
+    признавая потерю этих копий и позволяя Vitastor-у восстановить объекты из
+    других копий.
 --part <номер>
     Удалить только части EC с заданным номером (от 0 до pg_size-1). Нужно только
     в редких граничных случаях, когда один и тот же OSD содержит несколько частей
@@ -334,19 +334,14 @@ vitastor-cli dd [iimg=<image> | if=<file>] [oimg=<image> | of=<file>] [bs=1M] \
 
 ## rm-osd
 
-`vitastor-cli rm-osd [-f|--force] [--allow-data-loss] [--allow-up] [--dry-run] <osd_id> [osd_id...]`
+`vitastor-cli rm-osd [--force] [--allow-data-loss] [--dry-run] <osd_id> [osd_id...]`
 
 Удалить метаданные и конфигурацию для заданных OSD из etcd.
 
-Отказывается удалять работающие OSD, а также OSD, после удаления которых PG станут
-деградированными, недоступными (offline) или неполными (incomplete).
+Отказывается удалять OSD с данными без опций `--force` и `--allow-data-loss`.
 
-* `-f|--force` - удалить OSD, даже если они работают или PG станут деградированными или недоступными.
-* `--allow-data-loss` - удалить OSD, даже если PG или объекты станут неполными (ПОТЕРЯ ДАННЫХ).
-  Включает действие `--force`.
-* `--allow-up` - удалить OSD, даже если они работают.
-* `--dry-run` - только проверить, возможно ли удаление без потери данных и деградации
-  избыточности.
+С опцией `--dry-run` только проверяет, возможно ли удаление без потери данных и деградации
+избыточности.
 
 ## osd-tree
 
@@ -449,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

@@ -108,8 +108,8 @@ static const char* help_text =
     "  --writers-stopped allows optimised removal in case of a single 'slim' read-write\n"
     "  child and 'fat' removed parent: the child is merged into parent and parent is renamed\n"
     "  to child in that case. In other cases parent layers are always merged into children.\n"
-    "  --exact            Remove multiple images with given names.\n"
-    "  --matching         Remove multiple images with names matching given glob patterns.\n"
+    "  --exact            Remove multiple images with names matching given glob patterns.\n"
+    "  --matching         Remove multiple images with given names\n"
     "  --writers-stopped  Allow renaming inodes over their read/write children.\n"
     "  --down-ok          Continue deletion/merging even if some data will be left on unavailable OSDs.\n"
     "\n"
@@ -145,15 +145,12 @@ static const char* help_text =
     "  --min-offset, --max-offset\n"
     "      Restrict listing to specific offsets inside inodes.\n"
     "\n"
-    "vitastor-cli raw-ls --pool_id ID (--pg_num NUM|--offset NUM) [OPTIONS]\n"
+    "vitastor-cli raw-ls [OPTIONS]\n"
     "  Find object(s) in the cluster using raw secondary listing operations. Options:\n"
     "  [--min_inode NUM] [--max_inode NUM] [--offset NUM] [--pg_num NUM] [--pg_count COUNT]\n"
     "  [--pg_stripe_size NUM] [--osds 1,2,3,...]\n"
-    "  --pool_id (numeric pool ID) is required. --pg_num is required unless --offset is given,\n"
-    "  in which case the PG is calculated from the offset. --pg_count, --pg_stripe_size and\n"
-    "  --osds default to the pool configuration and are required if the pool does not exist.\n"
     "\n"
-    "vitastor-cli fix [--objects <objects>] --bad-osds <osds> [--part <part>] [--check no]\n"
+    "vitastor-cli fix [--objects <objects>] [--bad-osds <osds>] [--part <part>] [--check no]\n"
     "  Fix inconsistent objects in the cluster by deleting some copies.\n"
     "  --objects <objects>\n"
     "      Objects to fix, either in plain text or JSON format. If not specified,\n"
@@ -161,7 +158,7 @@ static const char* help_text =
     "      Plain text format: 0x<inode>:0x<stripe> <any delimiter> 0x<inode>:0x<stripe> ...\n"
     "      JSON format: [{\"inode\":\"0x...\",\"stripe\":\"0x...\"},...]\n"
     "  --bad-osds <osds>\n"
-    "      Required. Remove inconsistent copies/parts of objects from these OSDs, effectively\n"
+    "      Remove inconsistent copies/parts of objects from these OSDs, effectively\n"
     "      marking them bad and allowing Vitastor to recover objects from other copies.\n"
     "  --part <number>\n"
     "      Only remove EC part <number> (from 0 to pg_size-1), required for extreme\n"
@@ -173,14 +170,11 @@ static const char* help_text =
     "vitastor-cli alloc-osd\n"
     "  Allocate a new OSD number and reserve it by creating empty /osd/stats/<n> key.\n"
     "\n"
-    "vitastor-cli rm-osd [-f|--force] [--allow-data-loss] [--allow-up] [--dry-run] <osd_id> [osd_id...]\n"
+    "vitastor-cli rm-osd [--force] [--allow-data-loss] [--dry-run] <osd_id> [osd_id...]\n"
     "  Remove metadata and configuration for specified OSD(s) from etcd.\n"
-    "  Refuses to remove OSDs that are up or whose removal makes PGs degraded, offline or incomplete.\n"
-    "  -f|--force         Remove OSDs even if they are up or PGs become degraded or offline.\n"
-    "  --allow-data-loss  Remove OSDs even if PGs or objects become incomplete (DATA LOSS). Implies --force.\n"
-    "  --allow-up         Remove OSDs even if they are up.\n"
-    "  --dry-run          Only check if deletion is possible without data loss and\n"
-    "                     redundancy degradation.\n"
+    "  Refuses to remove OSDs with data without --force and --allow-data-loss.\n"
+    "  With --dry-run only checks if deletion is possible without data loss and\n"
+    "  redundancy degradation.\n"
     "\n"
     "vitastor-cli osd-tree [-l|--long]\n"
     "  Show current OSD tree, optionally with I/O statistics if -l is specified.\n"
@@ -214,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"

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/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