[pve-devel] [PATCH v2 storage/manager 00/27] pveceph storage.cfg maangement

[Thomas Lamprecht]

On 08/29/2017 01:04 PM, Fabian Grünbichler wrote:
> this patch series implements storage.cfg management for pveceph-managed ceph
> clusters. the following is implemented:
> 
> - add new 'pveceph' flag to RBD storages, using /etc/pve/ceph.conf instead of
>    a hard-coded monitor list
> - pveceph addstorage/lsstorages/removestorage to add/list/remove storage
>    entries, per pool

after rethinking the higher level changes introduced here, especially the API ones,
I'm not sure if that is the approach we want to go with.

API:
We duplicate semantic behavior and add another endpoint which can add remove and list
storages at the /nodes/{node}/ceph/pools{pool}/storages path.
This goes a bit against our single /storage entry point, you can create a storage with
the latter and remove it with the former – and with this series you must remove it with
the former if you do not touch the CLI tools – but the behavior is different.
Also, AFAIK, the storage system in PVE was designed to be at a cluster level, not node
level.

I think the CLI tools and their commands added would be OK, though.
But the API stuff should really go into /storages, IMO, this would be cleaner API design.
Adding a pool to a storage over this path seems unfitting. And if we ever would like to have
a separate Storage Management project, splitting this out would be quite harder with such a
coupling. IMO, /storages should be there for managing the storage configuration and

GUI:
Initially just the naming of the button was what took me off, but after re-thinking this
the "Add as Storage" (named "Create Storage" in the current revision of this series)
does not fit in the Node->Ceph->Pool panel.
You do not manage storages here, you do not see a clear effect after the this Creation
window was submitted. Normally we only add/create/remove elements in the GUI which are
on the same Panel visible – i.e. we have a grid with those elements.
Here we have a grid with pools but can configure a storage, where you get no direct
feedback of said addition. You could argue that it pops up in the resource tree, but that
may not be visible in the non-trivial setup case (too much VMs/CTs, another tree view used,
tree collapsed).


My proposal about how the GUI could incorporate easier pveceph pool is to use the now
existing "Add RBD" dialog from Datacenter->Storage and make the ID window an editable
combo box, which lists all pools configured by pveceph and allows specifying an external
one. Respective to the selction a "pveceph" boolean flag (or therelike) can be passed.

The "Add Storages" from the Pool creation window could stay, but should use a new "storage"
parameter in the create ceph/pool API path.
That's how we do it for assigning a newly created VM/CT to a PVE pool, as it the cleaner
approach, IMO.

API wise I would put into the RBD Storage plugin, currently that should be no problem,
as only the keyring part needs to be handled? If we need to do more here I'd rather
choose the way of moving our PVE::Ceph perl modules higher up (into own package, e.g.)
and use them also there?

Just my thoughts to this.

> - optionally adding/removing storages when creating/destroying a pool
> - GUI support
> 
> high-level changes since v1:
> - pveceph flag is now doing its magic on the storage plugin side as well, no
>    more need to update the monhost string when adding/removing a monitor (thanks
>    for the suggestion Dietmar!)
> - createpool/destroypool are now using a worker
> - treat more failures as fatal in API paths
> - {name} API placeholder for pool names is now {pool}
> - includes and extends Dominik's GUI patches to support the new functionality
> 
> patches 2-4, 17, 19-21, 26 and 27 are new
> patches 22-25 are from Dominik's 'ceph pool storage creation in gui' series
> the old patches 2, 3, 5, 8 and 20 were dropped altogether
> 
> 
> 
> pve-storage:
> 
> Fabian Grünbichler (4):
>    rbd: add pveceph storage option
>    rbd: make monhost option optional
>    refactor cmdline helpers
>    rbd: implement pveceph flag handling
> 
>   PVE/Storage/RBDPlugin.pm | 90 ++++++++++++++++++++++++++++--------------------
>   1 file changed, 53 insertions(+), 37 deletions(-)
> 
> 
> base-commit: 5a2eba91dcaec0ba68052579d5e0e4ebcb499ff5
> 
> 
> 
> pve-manager:
> 
> Dominik Csapak (4):
>    add a params object to the safedestroy window
>    add create storages checkbox to ceph pool creation
>    add remove_storages parameter to the pool destruction
>    add a 'create storage' button for ceph pools
> 
> Fabian Grünbichler (19):
>    ceph: add /etc/pve/priv/ceph path
>    ceph: add add_storage helper
>    ceph: add get_storages helper
>    ceph: add remove_storage helper
>    ceph: implement addstorage API path
>    pveceph: add addstorage CLI command
>    ceph: implement lsstorages API path
>    pveceph: add lsstorages CLI command
>    ceph: implement removestorage API path
>    pveceph: add removestorage CLI command
>    ceph/createpool: shorten pool variable name
>    ceph/createpool: optionally add storages
>    ceph/destroypool: shorten pool variable name
>    ceph/destroypool: optionally remove storages
>    ceph: rename API placeholder 'name' to 'pool'
>    ceph: make create/destroypool API paths async
>    add task description for cephcreatepool
>    add showProgress to SafeDestroy
>    enable showProgress for create/destroy pool
> 
>   PVE/API2/Ceph.pm                   | 367 +++++++++++++++++++++++++++++++------
>   PVE/CLI/pveceph.pm                 |  24 ++-
>   PVE/CephTools.pm                   |   2 +
>   www/manager6/Utils.js              |   1 +
>   www/manager6/ceph/Pool.js          |  78 +++++++-
>   www/manager6/window/SafeDestroy.js |  41 ++++-
>   6 files changed, 455 insertions(+), 58 deletions(-)
> 
> 
> base-commit: f9346961588e086a9cc86d00c16f7f3d0a77c878
>