On Tue, Sep 26, 2023 at 07:02:19AM -0500, Andrea Bolognani wrote:
> On Tue, Sep 26, 2023 at 11:23:51AM +0100, Daniel P. Berrangé wrote:
> > 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.
> > >
> > > IMHO it would be better to have additional file that will be included
> > > into the template for services where we need 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`.
> >
> > If anything I'd reverse the model. The 'virtchd.service.in' file
> > should be the primary template, the common bits the injected data.
> >
> > ie
> >
> > cat virtchd.service.in
> > [Unit]
> > Description=Virtualization Cloud-Hypervisor daemon
> > ::common-unit::
> > Wants=systemd-machined.service
> > After=remote-fs.target
> > After=systemd-machined.service
> > Documentation=man:virtchd(8)
> >
> >
> > [Service]
> > ::common-service::
> > 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
> >
> > [Install]
> > ::common-install::
>
> This doesn't address the problem with duplication that Pavel pointed
> out.
>
> I don't think it helps much with not storing additional data inside
> the build system, unless we want to store the contents of the various
> common snippets in separate files? Something like
>
> common_service = fs.read('common_service.inc')
> unit_conf = configuration_data({
> 'common_service' = common_service,
> })
>
> We'd have to fake fs.read() because it was introduced in 0.57 though.
> And we'd have to run the contents of the common parts through
> variable substitution anyway, because they will contain a bunch of
> lines like
>
> Also=@[email protected]
> Also=@[email protected]
> Also=@[email protected]
>
> I'm not sure the result would look much better, but I can give it a
> try.
Don't try to do any of this in meson. We should just have a standalone
python script that can combine the daemon specific unit file contents
with the common unit file contents. eg
scripts/merge-unit-file.py \
src/qemu/virtqemud.service.in \
src/rpc/virtd.service.in \
build/src/virtqemud.service
> > arguably we don't even need the '::common-XXX::' lines in there. We can
> > simply see the headers [Unit], [Service], etc and inject the common
> > bits under each header.
>
> I think markers make things both easier to implement and more obvious
> (whoever looks at the file can immediately tell that some sort of
> post-processing is going to happen and probably even make a fairly
> accurate guess as what it will entail), so I'd prefer having them.
> But this is a fairly minor detail compared to the above.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
