Re: [libvirt] [PATCH 20/53] vircgroup: register cgroup v2 backend
On 10/04/2018 06:10 PM, Pavel Hrdina wrote:
On Thu, Oct 04, 2018 at 05:26:32PM +0200, Michal Privoznik wrote:
> On 10/02/2018 10:44 AM, Pavel Hrdina wrote:
>> All mandatory callbacks are implemented for cgroup v2 backend so we
>> can register it now.
>>
>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> ---
>> src/util/vircgroupbackend.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/src/util/vircgroupbackend.c b/src/util/vircgroupbackend.c
>> index 79fe6cb73d..7ee39ac8ca 100644
>> --- a/src/util/vircgroupbackend.c
>> +++ b/src/util/vircgroupbackend.c
>> @@ -52,6 +52,7 @@ virCgroupBackendRegister(virCgroupBackendPtr backend)
>> static void
>> virCgroupBackendOnceInit(void)
>> {
>> + virCgroupV2Register();
>> virCgroupV1Register();
>> }
>>
>>
>
> Well, the callbacks are implemented, but that's about it. No API that
> involves reading/setting a value from CGroup will work at this point, oui?
>
> For instance:
>
> virsh blkiotune fedora
> error: Operation not supported: operation 'getBlkioWeight' not supported
>
> ACK if you move this to be the very last patch (or somewhere before the
> tests - I haven't gotten that far yet, so I don't know how tests are
> implemented, whether they need the v2 backend to be registered or not).
This was intentional to register the backend right after all mandatory
callbacks are implemented. Controller specific callbacks are optional
because some of the controllers that are available in cgroup v1 will
never exist in cgroup v2.
Sure, but some of them will. Just like I demonstrated above.
I can move this patch after the all controllers in this series are
implemented, but there is still devices controller missing and is not
even implemented in kernel and most likely will never exist in v2.
Hold on, so how are we supposed to limit what devices qemu has access to
if there is no devices cgroup? If not adding new layers of security we
should not remove them at least.
So technically there is no point of moving it.
I think there is. Because right now, around these patches none of the
APIs that involve CGroup work. It's okay to add code for new feature and
enable it only at the very last moment. I think it's even in our coding
guidelines.
Michal