[pve-devel] [PATCH-SERIES common/qemu-server v2 0/2] migration: conntrack: fix race adding dbus-vmstate object to QEMU

[pve-devel] [PATCH-SERIES common/qemu-server v2 0/2] migration: conntrack: fix race adding dbus-vmstate object to QEMU

[Fiona Ebner]

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

[pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper

[Fiona Ebner]

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

Re: [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper

[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. ^^

> 
> 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

Re: [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper

[Fiona Ebner]

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

[pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough)

[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/

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

Re: [pve-devel] [PATCH qemu-server v2 2/2] migration: conntrack: avoid crash when dbus-vmstate object cannot be added (quickly enough)

[Fiona Ebner]

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