Refuse renaming a pool to the name of another pool in vitastor-cli

modify-pool --name did not check the new name against other pools, so
two pools could end up with the same name, and commands addressing a
pool by name then picked either of them. Fail with EEXIST, as
create-pool does. The check uses the pool list that the CAS write is
based on, so a concurrent rename or create is caught by the retry.

docs/usage/cli.en.md

@@ -470,6 +470,8 @@ These, however, can still be modified with -f|--force.
 
 See [create-pool](#create-pool) for parameter descriptions.
 
+Renaming with --name fails if another pool already has the new name.
+
 Examples:
 
 `vitastor-cli modify-pool pool_A --name pool_B`

docs/usage/cli.ru.md

@@ -487,6 +487,8 @@ OSD  PARENT            UP    SIZE  USED%    TAGS          WEIGHT  BLOCK  BITMAP
 
 Описания параметров смотрите в [create-pool](#create-pool).
 
+Переименование с --name завершается ошибкой, если у другого пула уже есть такое имя.
+
 Примеры:
 
 `vitastor-cli modify-pool pool_A --name pool_B`

src/cmd/cli.cpp

@@ -235,6 +235,7 @@ static const char* help_text =
     "    [--immediate_commit <all|small|none>] [--pg_stripe_size <size>]\n"
     "  These, however, can still be modified with -f|--force.\n"
     "  See create-pool for parameter descriptions.\n"
+    "  Renaming with --name fails if another pool already has the new name.\n"
     "  Examples:\n"
     "    vitastor-cli modify-pool pool_A --name pool_B\n"
     "    vitastor-cli modify-pool 2 --pg_size 4 -n 128\n"

src/cmd/cli_ls.cpp

@@ -250,17 +250,11 @@ 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();
-            // Layers with a filtered out parent are shown as roots
-            children[listed.find(parent_id) != listed.end() ? parent_id : 0].push_back(item);
+            children[parent_id].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_modify.cpp

@@ -117,6 +117,22 @@ resume_1:
                 return;
             }
 
+            if (new_cfg["name"] != old_cfg["name"])
+            {
+                for (auto & pce: kv.value.object_items())
+                {
+                    if (pce.second["name"] == new_cfg["name"])
+                    {
+                        result = (cli_result_t){
+                            .err = EEXIST,
+                            .text = "Pool with name \""+new_cfg["name"].string_value()+"\" already exists (ID "+pce.first+")",
+                        };
+                        state = 100;
+                        return;
+                    }
+                }
+            }
+
             if (new_cfg.find("used_for_app") != new_cfg.end() && !force)
             {
                 // Check that pool doesn't have images

tests/run_tests.sh

@@ -18,6 +18,8 @@ SCHEME=ec ./test_change_pg_count.sh
 
 ./test_create_nomaxid.sh
 
+./test_modify_pool_name.sh
+
 ./test_etcd_fail.sh
 ANTIETCD=1 ./test_etcd_fail.sh
 
@@ -122,7 +124,6 @@ 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_ls_tree.sh

@@ -1,40 +0,0 @@
-#!/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

tests/test_modify_pool_name.sh

@@ -0,0 +1,34 @@
+#!/bin/bash -ex
+
+# Test that modify-pool --name refuses to take the name of another pool
+
+. `dirname $0`/common.sh
+
+wait_etcd
+
+POOL='"scheme":"replicated","pg_size":2,"pg_minsize":1,"pg_count":1,"failure_domain":"osd"'
+$ETCDCTL put /vitastor/config/pools '{"1":{"name":"pool1",'$POOL'},"2":{"name":"pool2",'$POOL'}}'
+
+for p in pool2 2; do
+    r=0
+    $VITASTOR_CLI modify-pool $p --name pool1 || r=$?
+    if [[ $r != 17 ]]; then
+        format_error "modify-pool $p --name pool1 should fail with EEXIST (17), but exited with $r"
+    fi
+done
+
+names=$($ETCDCTL get --print-value-only /vitastor/config/pools | jq -c '[ .[].name ]')
+if [[ "$names" != '["pool1","pool2"]' ]]; then
+    format_error "Pool names should stay pool1, pool2, but they are $names"
+fi
+
+# A free name and the pool's own name are still accepted
+$VITASTOR_CLI modify-pool pool2 --name pool3
+$VITASTOR_CLI modify-pool pool3 --name pool3 --pg_count 2
+
+cfg=$($ETCDCTL get --print-value-only /vitastor/config/pools | jq -c '[ .[] | [ .name, .pg_count ] ]')
+if [[ "$cfg" != '[["pool1",1],["pool3",2]]' ]]; then
+    format_error "Pools should be pool1 (1 PG) and pool3 (2 PGs), but they are $cfg"
+fi
+
+format_green OK