Re: [PATCH manager 0/2] ceph: pool: create EC pools from an existing profile

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Kefu Chai <k.chai@proxmox.com> writes:

Comments on each patch.

> Hi,
>
> the docs suggest building a custom erasure code profile with the Ceph
> tools and then passing it to 'pveceph pool create' as 'profile'. That
> doesn't actually work right now, since the schema still wants 'k' and
> 'm':
>
>   # pveceph pool create test-ec --erasure-coding profile=default
>   400 Parameter verification failed.
>   erasure-coding.m: property is missing and it is not optional
>   erasure-coding.k: property is missing and it is not optional
>
> Patch 2 makes 'k' and 'm' optional when a profile is given. If they are
> passed anyway, they now have to match the profile, where they used to be
> ignored. So a script that passed placeholder values next to a profile
> would now get an error, which I think is the safer choice, but I'm not
> sure everyone agrees.
>
> Patch 1 needs to go in first. destroypool removes the pool's profile by
> default, so once patch 2 makes 'profile=default' easy, destroying such a
> pool would take 'default' with it. With patch 1 it only removes the
> profile createpool made for that pool, unless asked to.
>
> Both patches come with tests in a new test/CephPool_test.pl, and I tried
> creating and destroying such pools on a three node test cluster.
> Feedback welcome, especially on the stricter check in patch 2.

The patches look good to me modulo small nits, none of which is a
blocker on its own.

PS: I did not review the tests.

Reviewed-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

-- 
Maximiliano