Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/ch/meson.build | 27 ++++++++++++++++++++----
src/ch/virtchd.service.in | 44 ---------------------------------------
2 files changed, 23 insertions(+), 48 deletions(-)
delete mode 100644 src/ch/virtchd.service.in
diff --git a/src/ch/meson.build b/src/ch/meson.build
index dc08069dcd..f6c443f3c6 100644
--- a/src/ch/meson.build
+++ b/src/ch/meson.build
@@ -57,11 +57,30 @@ if conf.has('WITH_CH')
virt_daemon_units += {
'service': 'virtchd',
- 'service_in': files('virtchd.service.in'),
'name': 'Libvirt ch',
- 'socket_in': libvirtd_socket_in,
- 'socket_ro_in': libvirtd_socket_ro_in,
- 'socket_admin_in': libvirtd_socket_admin_in,
+ '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',
+ ],
}
virt_install_dirs += [
diff --git a/src/ch/virtchd.service.in b/src/ch/virtchd.service.in
deleted file mode 100644
index 351eee312b..0000000000
--- a/src/ch/virtchd.service.in
+++ /dev/null
@@ -1,44 +0,0 @@
-[Unit]
-Description=Virtualization Cloud-Hypervisor daemon
-Conflicts=libvirtd.service
-Requires=virtchd.socket
-Requires=virtchd-ro.socket
-Requires=virtchd-admin.socket
-Wants=systemd-machined.service
-After=network.target
-After=dbus.service
-After=apparmor.service
-After=remote-fs.target
-After=systemd-machined.service
-Documentation=man:virtchd(8)
-Documentation=https://libvirt.org
-
-[Service]
-Type=notify
-Environment=VIRTCHD_ARGS="--timeout 120"
-EnvironmentFile=-@initconfdir@/virtchd
-ExecStart=@sbindir@/virtchd $VIRTCHD_ARGS
-ExecReload=/bin/kill -HUP $MAINPID
-KillMode=process
-Restart=on-failure
-# 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]
-WantedBy=multi-user.target
-Also=virtchd.socket
-Also=virtchd-ro.socket
-Also=virtchd-admin.socket
--
2.41.0On Mon, Sep 25, 2023 at 08:58:24PM +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/ch/meson.build | 27 ++++++++++++++++++++----
> src/ch/virtchd.service.in | 44 ---------------------------------------
> 2 files changed, 23 insertions(+), 48 deletions(-)
> delete mode 100644 src/ch/virtchd.service.in
>
> diff --git a/src/ch/meson.build b/src/ch/meson.build
> index dc08069dcd..f6c443f3c6 100644
> --- a/src/ch/meson.build
> +++ b/src/ch/meson.build
> @@ -57,11 +57,30 @@ if conf.has('WITH_CH')
>
> virt_daemon_units += {
> 'service': 'virtchd',
> - 'service_in': files('virtchd.service.in'),
> 'name': 'Libvirt ch',
> - 'socket_in': libvirtd_socket_in,
> - 'socket_ro_in': libvirtd_socket_ro_in,
> - 'socket_admin_in': libvirtd_socket_admin_in,
> + '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`.
Pavel
> }
>
> virt_install_dirs += [
> diff --git a/src/ch/virtchd.service.in b/src/ch/virtchd.service.in
> deleted file mode 100644
> index 351eee312b..0000000000
> --- a/src/ch/virtchd.service.in
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -[Unit]
> -Description=Virtualization Cloud-Hypervisor daemon
> -Conflicts=libvirtd.service
> -Requires=virtchd.socket
> -Requires=virtchd-ro.socket
> -Requires=virtchd-admin.socket
> -Wants=systemd-machined.service
> -After=network.target
> -After=dbus.service
> -After=apparmor.service
> -After=remote-fs.target
> -After=systemd-machined.service
> -Documentation=man:virtchd(8)
> -Documentation=https://libvirt.org
> -
> -[Service]
> -Type=notify
> -Environment=VIRTCHD_ARGS="--timeout 120"
> -EnvironmentFile=-@initconfdir@/virtchd
> -ExecStart=@sbindir@/virtchd $VIRTCHD_ARGS
> -ExecReload=/bin/kill -HUP $MAINPID
> -KillMode=process
> -Restart=on-failure
> -# 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]
> -WantedBy=multi-user.target
> -Also=virtchd.socket
> -Also=virtchd-ro.socket
> -Also=virtchd-admin.socket
> --
> 2.41.0
>
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:
> > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> > ---
> > src/ch/meson.build | 27 ++++++++++++++++++++----
> > src/ch/virtchd.service.in | 44 ---------------------------------------
> > 2 files changed, 23 insertions(+), 48 deletions(-)
> > delete mode 100644 src/ch/virtchd.service.in
> >
> > diff --git a/src/ch/meson.build b/src/ch/meson.build
> > index dc08069dcd..f6c443f3c6 100644
> > --- a/src/ch/meson.build
> > +++ b/src/ch/meson.build
> > @@ -57,11 +57,30 @@ if conf.has('WITH_CH')
> >
> > virt_daemon_units += {
> > 'service': 'virtchd',
> > - 'service_in': files('virtchd.service.in'),
> > 'name': 'Libvirt ch',
> > - 'socket_in': libvirtd_socket_in,
> > - 'socket_ro_in': libvirtd_socket_ro_in,
> > - 'socket_admin_in': libvirtd_socket_admin_in,
> > + '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::
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.
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 :|
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.
> 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.
--
Andrea Bolognani / Red Hat / Virtualization
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 :|
On Tue, Sep 26, 2023 at 01:14:33PM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 26, 2023 at 07:02:19AM -0500, Andrea Bolognani wrote:
> > 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
It feels a bit silly to shell out to Python to perform what is
ultimately a bunch of variable substitutions, as if that wasn't part
of Meson's core feature set... But I'll give it a try and see how it
turns out.
Can you please take a look at the remaining patches in the meantime,
and provide feedback on the changes that are made to the various
services and sockets as part of them? Thanks in advance :)
--
Andrea Bolognani / Red Hat / Virtualization
On Tue, Sep 26, 2023 at 08:12:43AM -0500, Andrea Bolognani wrote:
> On Tue, Sep 26, 2023 at 01:14:33PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Sep 26, 2023 at 07:02:19AM -0500, Andrea Bolognani wrote:
> > > 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
>
> It feels a bit silly to shell out to Python to perform what is
> ultimately a bunch of variable substitutions, as if that wasn't part
> of Meson's core feature set... But I'll give it a try and see how it
> turns out.
IMHO Meson's job is to control the build process, rather than to
actually be the build process. I think of this as "compiling" the
unit files and the python sript is our compiler, which meson is
to control.
> Can you please take a look at the remaining patches in the meantime,
> and provide feedback on the changes that are made to the various
> services and sockets as part of them? Thanks in advance :)
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 :|
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
© 2016 - 2026 Red Hat, Inc.