* [pve-devel] [PATCH-SERIES common/qemu-server v2 0/2] migration: conntrack: fix race adding dbus-vmstate object to QEMU
@ 2025-10-06 11:55 Fiona Ebner
2025-10-06 11:55 ` [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper Fiona Ebner
2025-10-06 11:55 ` [pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough) Fiona Ebner
0 siblings, 2 replies; 6+ messages in thread
From: Fiona Ebner @ 2025-10-06 11:55 UTC (permalink / raw)
To: pve-devel
Changes in v2:
* Dropped already applied patches.
* Introduce sd_notify() helper.
* Different approach, make the service type=notify instead of waiting
in a sleep+check-loop until the object shows up via QMP 'qom-list'.
As reported in the community forum [0], it might happen that the
dbus-vmstate object is not added (quickly enough) to the target QEMU
instance, before the migration state is loaded. This would result in
a crash of the target instance.
[0]: https://forum.proxmox.com/threads/172588/
Dependency bump qemu-server -> pve-common needed.
pve-common:
Fiona Ebner (1):
systemd: add sd_notify() helper
src/PVE/Systemd.pm | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
qemu-server:
Fiona Ebner (1):
migration: conntrack: avoid crash when dbus-vmstate object cannot be
added (quickly enough)
src/usr/dbus-vmstate | 3 +++
src/usr/pve-dbus-vmstate@.service | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
Summary over all repositories:
3 files changed, 25 insertions(+), 1 deletions(-)
--
Generated by git-murpp 0.5.0
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper 2025-10-06 11:55 [pve-devel] [PATCH-SERIES common/qemu-server v2 0/2] migration: conntrack: fix race adding dbus-vmstate object to QEMU Fiona Ebner @ 2025-10-06 11:55 ` Fiona Ebner 2025-10-06 13:14 ` Thomas Lamprecht 2025-10-06 11:55 ` [pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough) Fiona Ebner 1 sibling, 1 reply; 6+ messages in thread From: Fiona Ebner @ 2025-10-06 11:55 UTC (permalink / raw) To: pve-devel See 'man 3 sd_notify'. Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> --- New in v2. src/PVE/Systemd.pm | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/PVE/Systemd.pm b/src/PVE/Systemd.pm index e6d6f88..96e7d80 100644 --- a/src/PVE/Systemd.pm +++ b/src/PVE/Systemd.pm @@ -3,9 +3,11 @@ package PVE::Systemd; use strict; use warnings; +use IO::Socket::UNIX; use Net::DBus qw(dbus_uint32 dbus_uint64 dbus_boolean); use Net::DBus::Callback; use Net::DBus::Reactor; +use Socket qw(SOCK_DGRAM); use PVE::Tools qw(file_set_contents file_get_contents trim); @@ -282,4 +284,23 @@ sub write_ini { file_set_contents($filename, $content); } +sub sd_notify { + my ($unset_environment, $state) = @_; + + my $socket_path = $unset_environment ? delete($ENV{NOTIFY_SOCKET}) : $ENV{NOTIFY_SOCKET}; + + my $socket = IO::Socket::UNIX->new( + Type => SOCK_DGRAM(), + Peer => $socket_path, + ) or die "unable to connect to socket $socket_path to notify systemd\n"; + + # we won't be reading from the socket + shutdown($socket, 0); + + print {$socket} $state; + $socket->flush(); + + close($socket); +} + 1; -- 2.47.3 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper 2025-10-06 11:55 ` [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper Fiona Ebner @ 2025-10-06 13:14 ` Thomas Lamprecht 2025-10-07 10:26 ` Fiona Ebner 0 siblings, 1 reply; 6+ messages in thread From: Thomas Lamprecht @ 2025-10-06 13:14 UTC (permalink / raw) To: Proxmox VE development discussion, Fiona Ebner Am 06.10.25 um 13:57 schrieb Fiona Ebner: > See 'man 3 sd_notify'. Such references can be OK to avoid duplicating all details, but not as full replacement for basic commit message context.. Would be nice to know that this is a pure perl reimplementation of the sd_notify interface The lack of that made me look quite a bit closer than I'd otherwise have, so a bit of a pedantic review inline. ^^ > > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > --- > > New in v2. > > src/PVE/Systemd.pm | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/src/PVE/Systemd.pm b/src/PVE/Systemd.pm > index e6d6f88..96e7d80 100644 > --- a/src/PVE/Systemd.pm > +++ b/src/PVE/Systemd.pm > @@ -3,9 +3,11 @@ package PVE::Systemd; > use strict; > use warnings; > > +use IO::Socket::UNIX; > use Net::DBus qw(dbus_uint32 dbus_uint64 dbus_boolean); > use Net::DBus::Callback; > use Net::DBus::Reactor; > +use Socket qw(SOCK_DGRAM); > > use PVE::Tools qw(file_set_contents file_get_contents trim); > > @@ -282,4 +284,23 @@ sub write_ini { > file_set_contents($filename, $content); > } > Short comment that this is a pure-perl re-implementation of the sd_notify interface as defined in systemd/sd-daemon.h might be good to have here. > +sub sd_notify { > + my ($unset_environment, $state) = @_; > + > + my $socket_path = $unset_environment ? delete($ENV{NOTIFY_SOCKET}) : $ENV{NOTIFY_SOCKET}; In systemd's sd_notify, which is just a trivial wrapper around sd_pid_notify_with_fds [0], the unsetting of the environment happens after doing the notify. While this should not matter much in practice given that we do not have threading here, so nothing can really happen concurrently, it might be still better to use the original's pattern if only for someone else taking this as source for their implementation for an environment where this detail might actually matter. [0]: https://github.com/systemd/systemd/blob/f0a1b3c183dea90259f3b55ad0fa4bd25b1590a2/src/libsystemd/sd-daemon/sd-daemon.c#L626-L638 > + > + my $socket = IO::Socket::UNIX->new( > + Type => SOCK_DGRAM(), > + Peer => $socket_path, > + ) or die "unable to connect to socket $socket_path to notify systemd\n"; please include the actual error from IO::Socket::UNIX->new in the message, i.e. "$IO::Socket::errstr" or "$@" as otherwise any error will be harder to debug. https://metacpan.org/pod/IO::Socket::UNIX#new-(-[ARGS]-) > + > + # we won't be reading from the socket > + shutdown($socket, 0); FWIW the `IO::Socket` module would provide shutdown also on the blessed socket [1], and also the more telling constant names for SHUT_RD, SHUT_WR or SHUT_RDWR [1]: https://metacpan.org/pod/IO::Socket#shutdown > + > + print {$socket} $state; Does print retry automatically on EINTR or when not being able to send the full message in one go? While both is very unlikely here, it would still be great if we got hardened resiliency for such important helpers. If print doesn't gives us any convenience guarantees here, it might be better to use $socket->send($state) and check for errors and if all was send out as safety check. > + $socket->flush(); > + > + close($socket); > +} > + > 1; _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper 2025-10-06 13:14 ` Thomas Lamprecht @ 2025-10-07 10:26 ` Fiona Ebner 0 siblings, 0 replies; 6+ messages in thread From: Fiona Ebner @ 2025-10-07 10:26 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion Am 06.10.25 um 3:13 PM schrieb Thomas Lamprecht: > Am 06.10.25 um 13:57 schrieb Fiona Ebner: >> See 'man 3 sd_notify'. > > Such references can be OK to avoid duplicating all details, but not as > full replacement for basic commit message context.. > Would be nice to know that this is a pure perl reimplementation of the > sd_notify interface > > The lack of that made me look quite a bit closer than I'd otherwise have, > so a bit of a pedantic review inline. ^^ Yes, that's fair. I do like getting pedantic reviews. There's more to learn then :) >> >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> >> --- >> >> New in v2. >> >> src/PVE/Systemd.pm | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/src/PVE/Systemd.pm b/src/PVE/Systemd.pm >> index e6d6f88..96e7d80 100644 >> --- a/src/PVE/Systemd.pm >> +++ b/src/PVE/Systemd.pm >> @@ -3,9 +3,11 @@ package PVE::Systemd; >> use strict; >> use warnings; >> >> +use IO::Socket::UNIX; >> use Net::DBus qw(dbus_uint32 dbus_uint64 dbus_boolean); >> use Net::DBus::Callback; >> use Net::DBus::Reactor; >> +use Socket qw(SOCK_DGRAM); >> >> use PVE::Tools qw(file_set_contents file_get_contents trim); >> >> @@ -282,4 +284,23 @@ sub write_ini { >> file_set_contents($filename, $content); >> } >> > > Short comment that this is a pure-perl re-implementation of the sd_notify > interface as defined in systemd/sd-daemon.h might be good to have here. > >> +sub sd_notify { >> + my ($unset_environment, $state) = @_; >> + >> + my $socket_path = $unset_environment ? delete($ENV{NOTIFY_SOCKET}) : $ENV{NOTIFY_SOCKET}; > > In systemd's sd_notify, which is just a trivial wrapper around > sd_pid_notify_with_fds [0], the unsetting of the environment happens > after doing the notify. While this should not matter much in practice > given that we do not have threading here, so nothing can really happen > concurrently, it might be still better to use the original's pattern > if only for someone else taking this as source for their implementation > for an environment where this detail might actually matter. > > [0]: https://github.com/systemd/systemd/blob/f0a1b3c183dea90259f3b55ad0fa4bd25b1590a2/src/libsystemd/sd-daemon/sd-daemon.c#L626-L638 > Good point! >> + >> + my $socket = IO::Socket::UNIX->new( >> + Type => SOCK_DGRAM(), >> + Peer => $socket_path, >> + ) or die "unable to connect to socket $socket_path to notify systemd\n"; > > please include the actual error from IO::Socket::UNIX->new in the message, > i.e. "$IO::Socket::errstr" or "$@" as otherwise any error will be harder to > debug. > > https://metacpan.org/pod/IO::Socket::UNIX#new-(-[ARGS]-) > >> + >> + # we won't be reading from the socket >> + shutdown($socket, 0); > > FWIW the `IO::Socket` module would provide shutdown also on the blessed > socket [1], and also the more telling constant names for SHUT_RD, > SHUT_WR or SHUT_RDWR > > [1]: https://metacpan.org/pod/IO::Socket#shutdown > > >> + >> + print {$socket} $state; > > Does print retry automatically on EINTR or when not being able to send > the full message in one go? While both is very unlikely here, it would > still be great if we got hardened resiliency for such important helpers. > If print doesn't gives us any convenience guarantees here, it might be > better to use $socket->send($state) and check for errors and if all was > send out as safety check. > >> + $socket->flush(); >> + >> + close($socket); >> +} >> + >> 1; > I did look at other usages of sockets in our code base, and did not check for these details, which I should have. I didn't find anything explicit in the documentation about the behavior of print in such cases, but at least there is a test case that print will not forward EINTR [0]. Still, better to be explicit. [0]: https://github.com/Perl/perl5/blob/70a73bdabb7ed570cb8451b2d38ec69a4feeb0f6/t/io/eintr_print.t _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough) 2025-10-06 11:55 [pve-devel] [PATCH-SERIES common/qemu-server v2 0/2] migration: conntrack: fix race adding dbus-vmstate object to QEMU Fiona Ebner 2025-10-06 11:55 ` [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper Fiona Ebner @ 2025-10-06 11:55 ` Fiona Ebner 2025-10-06 11:59 ` Fiona Ebner 1 sibling, 1 reply; 6+ messages in thread From: Fiona Ebner @ 2025-10-06 11:55 UTC (permalink / raw) To: pve-devel As reported in the community forum [0], it might happen that the dbus-vmstate object is not added (quickly enough) to the target QEMU instance, before the migration state is loaded. This would result in a crash of the target instance: > kvm: Unknown savevm section or instance 'dbus-vmstate/dbus-vmstate' > 0. Make sure that your current VM setup matches your saved VM setup, > including any hotplugged devices > kvm: load of migration failed: Invalid argument This is after the configuration is already moved and thus there also is no source instance running anymore. Change the type of the 'pve-dbus-vmstate@' service to 'notify', so that starting the service returns success only after the 'dbus-vmstate' object has been added to the QEMU instance. [0]: https://forum.proxmox.com/threads/172588/ Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> --- Dependency bump qemu-server -> pve-common needed! Changes in v2: * Different approach, make the service type=notify instead of waiting in a sleep+check-loop until the object shows up via QMP 'qom-list'. src/usr/dbus-vmstate | 3 +++ src/usr/pve-dbus-vmstate@.service | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/usr/dbus-vmstate b/src/usr/dbus-vmstate index ac6f8cfb..b2baa840 100755 --- a/src/usr/dbus-vmstate +++ b/src/usr/dbus-vmstate @@ -15,6 +15,7 @@ use Net::DBus::Reactor; use PVE::QemuServer::Helpers; use PVE::QemuServer::QMPHelpers qw(qemu_objectadd qemu_objectdel); use PVE::SafeSyslog; +use PVE::Systemd; use PVE::Tools; use base qw(Net::DBus::Object); @@ -165,4 +166,6 @@ qemu_objectadd($vmid, 'pve-vmstate', 'dbus-vmstate', 'id-list' => "pve-vmstate-$vmid", ); +PVE::Systemd::sd_notify(0, "READY=1\n"); + Net::DBus::Reactor->main()->run(); diff --git a/src/usr/pve-dbus-vmstate@.service b/src/usr/pve-dbus-vmstate@.service index 56b4e285..616f6979 100644 --- a/src/usr/pve-dbus-vmstate@.service +++ b/src/usr/pve-dbus-vmstate@.service @@ -6,5 +6,5 @@ PartOf=%i.scope [Service] Slice=qemu.slice -Type=simple +Type=notify ExecStart=/usr/libexec/qemu-server/dbus-vmstate %i -- 2.47.3 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough) 2025-10-06 11:55 ` [pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough) Fiona Ebner @ 2025-10-06 11:59 ` Fiona Ebner 0 siblings, 0 replies; 6+ messages in thread From: Fiona Ebner @ 2025-10-06 11:59 UTC (permalink / raw) To: pve-devel Am 06.10.25 um 1:57 PM schrieb Fiona Ebner: > As reported in the community forum [0], it might happen that the > dbus-vmstate object is not added (quickly enough) to the target QEMU > instance, before the migration state is loaded. This would result in > a crash of the target instance: > >> kvm: Unknown savevm section or instance 'dbus-vmstate/dbus-vmstate' >> 0. Make sure that your current VM setup matches your saved VM setup, >> including any hotplugged devices >> kvm: load of migration failed: Invalid argument > > This is after the configuration is already moved and thus there also > is no source instance running anymore. > > Change the type of the 'pve-dbus-vmstate@' service to 'notify', so > that starting the service returns success only after the > 'dbus-vmstate' object has been added to the QEMU instance. > > [0]: https://forum.proxmox.com/threads/172588/ > Sorry, forgot to add Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> before sending. > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-07 10:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-10-06 11:55 [pve-devel] [PATCH-SERIES common/qemu-server v2 0/2] migration: conntrack: fix race adding dbus-vmstate object to QEMU Fiona Ebner 2025-10-06 11:55 ` [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper Fiona Ebner 2025-10-06 13:14 ` Thomas Lamprecht 2025-10-07 10:26 ` Fiona Ebner 2025-10-06 11:55 ` [pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough) Fiona Ebner 2025-10-06 11:59 ` Fiona Ebner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.