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

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

[Fiona Ebner]

Changes in v4 (thanks to Wolfgang!):
* Also handle abstract socket path.
* Only resend upon EINTR, otherwise the datagram should be sent in one
  go.
* Drop unnecessary flush.
* Drop $unset_environment argument.
* Rename to just PVE::Systemd::notify() to make it clear that it's not
  the exact same as sd_notify().
* Use POD for documentation.
* Return early if there is no socket path.
* Die if there is no message.
* Check that socket path starts with '/' or '@'.

Changes in v3 (thanks to Thomas!):
* Expand commit message for sd_notify() helper.
* Use $socket->{send,shutdown) methods.
* Print $IO::Socket::errstr in case of error.
* Unset NOTIFY_SOCKET environment variable only after sending the
  message.

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 notify() helper

 src/PVE/Systemd.pm | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 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, 53 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 v4 1/2] systemd: add notify() helper

[Fiona Ebner]

Add a pure Perl reimplementation of systemd's sd_notify() mechanism as
defined in systemd/sd-daemon.h based on the example implementations in
'man 3 sd_notify'.

The initial user of this helper is intended to be the pve-dbus-vmstate
service, so it can notify startup completion only once the
dbus-vmstate QEMU object is ready to be used.

EAGAIN is not checked for, because it does not occur for blocking
Unix domain sockets, see 'man 2 send'. The socket is a datagram
socket and a single datagram will be sent. If not all data can be
written, error out like the example code in 'man 3 sd_notify'.

Co-developed-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Co-developed-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v4:
* Also handle abstract socket path.
* Only resend upon EINTR, otherwise the datagram should be sent in one
  go.
* Drop unnecessary flush.
* Drop $unset_environment argument.
* Rename to just PVE::Systemd::notify() to make it clear that it's not
  the exact same as sd_notify().
* Use POD for documentation.
* Return early if there is no socket path.
* Die if there is no message.
* Check that socket path starts with '/' or '@'.

 src/PVE/Systemd.pm | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/src/PVE/Systemd.pm b/src/PVE/Systemd.pm
index e6d6f88..8fc06a5 100644
--- a/src/PVE/Systemd.pm
+++ b/src/PVE/Systemd.pm
@@ -3,9 +3,12 @@ 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 POSIX qw(EINTR);
+use Socket qw(SOCK_DGRAM);
 
 use PVE::Tools qw(file_set_contents file_get_contents trim);
 
@@ -282,4 +285,50 @@ sub write_ini {
     file_set_contents($filename, $content);
 }
 
+=head3 notify()
+
+This is a pure Perl reimplementation of systemd's C<sd_notify()> mechanism as defined in
+C<systemd/sd-daemon.h>, based on the example implementations in C<man 3 sd_notify>. Does not return
+a value, but dies upon error.
+
+=cut
+
+sub notify {
+    my ($message) = @_;
+
+    # nothing to do if there is no socket
+    my $socket_path = $ENV{NOTIFY_SOCKET} or return;
+
+    die "notify systemd invalid socket path '$socket_path'\n" if $socket_path !~ m|^[/@]|;
+    die "notify systemd called without a message\n" if !$message;
+
+    # might be an abstract socket
+    $socket_path =~ s/^@/\0/;
+
+    my $socket = IO::Socket::UNIX->new(
+        Type => SOCK_DGRAM(),
+        Peer => $socket_path,
+    ) or die "notify systemd: unable to connect to socket $socket_path - $IO::Socket::errstr\n";
+
+    # we won't be reading from the socket
+    $socket->shutdown(SHUT_RD);
+
+    my $res;
+    while (1) {
+        $res = $socket->send($message);
+        if ($res) {
+            die "notify systemd: protocol error writing to socket '$socket_path'\n"
+                if $res < length($message);
+            last;
+        } else {
+            next if $! == EINTR;
+            die "notify systemd: sending to '$socket_path' failed - $!\n";
+        }
+    }
+
+    close($socket);
+
+    return;
+}
+
 1;
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server v4 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>
---

Changes in v4:
* Adapt to renamed function.

 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..e958a74f 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::notify("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