On Sun, Jul 28, 2019 at 04:42:52PM +0200, Andrea Bolognani wrote: > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > [...] > > - We can make virtproxyd and the virtXXXd per-driver daemons all > > have "Conflicts: libvirtd.service" in their systemd unit files. > > This will guarantee that libvirtd is never started at the same > > time, as this would result in two daemons running the same driver. > > Fortunately drivers use locking to protect themselves, but it is > > better to avoid starting a daemon we know will conflict. > > I feel like this will need to be tested extensively to make sure > we're always doing the right thing, including on non-systemd hosts. Testing is quite easy - just try to start the two units and make sure only one ends up running. Similarly for non-systemd hosts, start both daemons & see that only one succeeds - the others fail with lock conflict. > > +++ b/src/remote/virtproxyd.service.in > > @@ -0,0 +1,24 @@ > > +[Unit] > > +Description=Virtualization daemon > > +Conflicts=libvirtd.service > > +Requires=virtproxyd.socket > > +Requires=virtproxyd-ro.socket > > +Requires=virtproxyd-admin.socket > > +After=network.target > > +After=dbus.service > > +After=apparmor.service > > +After=local-fs.target > > +After=remote-fs.target > > +Documentation=man:libvirtd(8) > > +Documentation=https://libvirt.org > > There are a few non-obvious changes between libvirtd.service.in and > this file: > > -Requires=virtlogd.socket > -Requires=virtlockd.socket > -Wants=systemd-machined.service > -Before=libvirt-guests.service > -After=iscsid.service > -After=systemd-logind.service > -After=systemd-machined.service > > I can see why we'd move the relationships with iscsid and virtlockd > to virtstoraged, except looking ahead to patch 23 I see you haven't > actually done that; either way, I'm not so convinced about the > remaining changes. Care to explain the rationale behind them? virtproxdy contains no drivers, so it doesn't need to depend on any of these services. virtdstoraged/qemud/lxcd should have gained some of these though. > > > +[Service] > > +Type=notify > > +ExecStart=@sbindir@/virtproxyd --timeout 120 > > +ExecReload=/bin/kill -HUP $MAINPID > > +Restart=on-failure > > More changes in this section: > > -EnvironmentFile=-@sysconfdir@/sysconfig/libvirtd > -KillMode=process > -LimitNOFILE=8192 > -TasksMax=32768 > > EnvironmentFile is clearly no longer needed, while both LimitNOFILE > and TasksMax probably belong to the hypervisor-specific daemons, but > I'm unclear on why KillMode was changed. The systemd default is fine as we don't need any other processes to survive shutdown. > > +[Install] > > +WantedBy=multi-user.target > > +Also=virtproxyd.socket > > +Also=virtproxyd-ro.socket > > Kind of a side note since it's pre-existing, but don't we want to > list virtproxyd-admin.socket here too? It is redundant - the deps force virtproxyd-admin.socket to become enabled regardless. 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
