From b7efe168365c0e3ebf0c88f2a235ac903b390c18 Mon Sep 17 00:00:00 2001 From: Yury Luneff Date: Tue, 20 Oct 2026 10:54:10 +0300 Subject: [PATCH] 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 =[,...] with EINVAL. --- docs/usage/cli.en.md | 2 +- docs/usage/cli.ru.md | 2 +- src/cmd/cli.cpp | 2 +- src/cmd/cli_pool_cfg.cpp | 24 +++++++++++++----------- tests/test_level_placement.sh | 19 +++++++++++++++++++ 5 files changed, 35 insertions(+), 14 deletions(-) diff --git a/docs/usage/cli.en.md b/docs/usage/cli.en.md index 0b700725..bea88e12 100644 --- a/docs/usage/cli.en.md +++ b/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 ` | Use additional failure domain rules (example: "dc=112233") | +| `--level_placement ` | Use additional failure domain rules (example: "dc=112233", "" to remove) | | `--raw_placement ` | 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 | diff --git a/docs/usage/cli.ru.md b/docs/usage/cli.ru.md index e4787d15..fb56e317 100644 --- a/docs/usage/cli.ru.md +++ b/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 ` | Задать правила дополнительных доменов отказа (пример: "dc=112233") | +| `--level_placement ` | Задать правила дополнительных доменов отказа (пример: "dc=112233", "" — удалить) | | `--raw_placement ` | Задать низкоуровневые правила генерации PG ([детали](../config/pool.ru.md#raw_placement)) | | `--local_reads primary` | Политика локальных чтений для реплик: primary, nearest или random | | `--primary_affinity_tags tags` | Предпочитать OSD со всеми данными тегами для роли первичных | diff --git a/src/cmd/cli.cpp b/src/cmd/cli.cpp index 28ba05af..34206db8 100644 --- a/src/cmd/cli.cpp +++ b/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 Use additional failure domain rules (example: \"dc=112233\")\n" + " --level_placement Use additional failure domain rules (example: \"dc=112233\", \"\" to remove)\n" " --raw_placement 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" diff --git a/src/cmd/cli_pool_cfg.cpp b/src/cmd/cli_pool_cfg.cpp index 83cb2ff6..4c180a9c 100644 --- a/src/cmd/cli_pool_cfg.cpp +++ b/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 =[,=...], 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; diff --git a/tests/test_level_placement.sh b/tests/test_level_placement.sh index 82959feb..3afdacde 100755 --- a/tests/test_level_placement.sh +++ b/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 -- 2.47.3