Re: [libvirt PATCH 26/42] systemd: Switch virtchd to common templates
On Tue, Sep 26, 2023 at 11:09:44AM +0200, Pavel Hrdina wrote:
On Mon, Sep 25, 2023 at 08:58:24PM +0200, Andrea Bolognani wrote:
> + 'service_unit_extra': [
> + 'Wants=systemd-machined.service',
> + 'After=systemd-machined.service',
> + 'After=remote-fs.target',
> + ],
> + 'service_service_extra': [
> + 'KillMode=process',
> + '# Raise hard limits to match behaviour of systemd >= 240.',
> + '# During startup, daemon will set soft limit to match hard limit',
> + '# per systemd recommendations',
> + 'LimitNOFILE=1024:524288',
> + '# The cgroups pids controller can limit the number of tasks started
by',
> + '# the daemon, which can limit the number of domains for some
hypervisors.',
> + '# A conservative default of 8 tasks per guest results in a TasksMax
of',
> + '# 32k to support 4096 guests.',
> + 'TasksMax=32768',
> + '# With cgroups v2 there is no devices controller anymore, we have to
use',
> + '# eBPF to control access to devices. In order to do that we create a
eBPF',
> + '# hash MAP which locks memory. The default map size for 64 devices
together',
> + '# with program takes 12k per guest. After rounding up we will get 64M
to',
> + '# support 4096 guests.',
> + 'LimitMEMLOCK=64M',
> + ],
This feels wrong to have it in meson.build file. In addition it is the
same as for virtlxcd and virtqemud so we are basically duplicating the
data and which makes it easy to make inconsistent changes not affecting
all places.
You're right, it would make sense to deduplicate this further.
IMHO it would be better to have additional file that will be
included
into the template for services where we need it.
Wouldn't a variable be enough?
In order to use a file, I can see two ways. First one is to have a
separate virtd-hypervisor.service.in that contains the same stuff as
virtd.service.in plus these comments, but that means introducing
duplication on a different axis and risking the two files going out
of sync. Second one is to have a virtd-comments.txt or whatever that
gets included conditionally from virtd.service.in, but that means
adding an extra processing step. Neither really feels an outright
improvement over what we have here.
Can you explain what did you have in mind? Maybe I'm just not seeing
it :)
I'm not sure about the `service_unit_extra` as well if we want to
have
it in meson.build files as it is not strictly related to the build
process and there is more data compared to the old `deps`.
That's because the various services and sockets have tiny differences
between them. Having a single template is IMO stictly better for
maintenability than carrying around more than a dozen copies of the
same basic information, which is what we have today.
It's true that this is going a bit overboard compared to what we're
using configuration data for elsewhere, but I don't think it's too
much of a stretch or something that feels too out of place.
That said, if you have an idea for an alternative approach to
achieving the same result, please do share it! I'm not married to
this specific implementation :)
--
Andrea Bolognani / Red Hat / Virtualization