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=@service@.socket > Also=@service@-ro.socket > Also=@service@-admin.socket > > 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 :|