Re: [libvirt PATCH 0/4] src: add configurable support for cgroups usage

On Fri, Mar 20, 2020 at 03:59:41PM +0000, Daniel P. Berrangé wrote: > On Fri, Mar 20, 2020 at 04:48:58PM +0100, Pavel Hrdina wrote: > > On Fri, Mar 20, 2020 at 03:39:58PM +0000, Daniel P. Berrangé wrote: > > > On Fri, Mar 20, 2020 at 04:30:07PM +0100, Pavel Hrdina wrote: > > > > On Fri, Mar 20, 2020 at 01:40:10PM +0000, Daniel P. Berrangé wrote: > > > > > This simple series allows apps to choose how cgroups are managed by > > > > > libvirt, between cgroupfs and machined, or disabled entirely. > > > > > > > > I'm not so sure about this series. The situation with cgroups and > > > > systemd is a bit more complex then the current code handles. > > > > > > > > There is an existing issue where we are violating the delegation rules > > > > described by cgroups and systemd. > > > > > > > > Currently the "cgroupfs" approach is used only on non-systemd hosts and > > > > we should keep it that way. Libvirt is not allowed to mangle with > > > > cgroups owned by systemd so IMHO the warning in configuration file is > > > > not enough because without delegation cgroups will not work properly in > > > > libvirt. > > > > > > That isn't the case currently AFAICT from current code. > > > > > > Before this series, the virCgroupNewMachine method will first try > > > systemd and then fallback to directly cgroupfs. > > > > > > This fallback can happen on a systemd host, when machined is not > > > installed, as machined is an optional component. > > > > Right, I should have checked the code. > > > > > If we need to mandate use of systemd on systemd hosts, then our > > > existing code is broken and needs fixing. > > > > I think we should do that because without delegation we should not touch > > anything in cgroups. > > > > > I'm happy todo such a fix, and then adjust this series to take > > > account of it. Essentially we'd allow apps to specify 'cgroupfs' > > > or 'machined' but we'd enforce they only make safe choices. > > > > > > ie on a systemd host we'd only allow 'none' or 'machined' > > > > > > On a non-systemd host, or on a systemd host where we've > > > been delegated a subtree, we'd only allow 'none' or 'cgroupfs' > > > > This sounds reasonable, I was thinking about the check if we have > > delegated subtree but decided not to mention it. The only place where > > we can check the delegation is using systemd API, probably over D-Bus. > > > > It's not reflected anywhere in the cgroup files. > > The key scenario for cgroupfs would be if libvirtd was run inside a > container on a host that otherwise uses systemd. IIUC, the container > should get given a delegated subtree and we need to be able to use > cgroupfs backend here. Currently this works (accidentally) because > we'll try to talk to machined and fail, so fallback to direct usage. That should be OK and we can support this even outside of containers if we can verify that the cgroup subtree is delegated and it's safe to use it for VMs. The wrong usage of delegation by libvirt is the source of the reported BUG so we should be restrictive.