[pve-devel] [PATCH qemu-server 1/2] fix q35 live migration

Discussion:

Dominik Csapak

2018-11-27 10:32:17 UTC

with live migration the machine type not q35, but something like
pc-q35-2.12
so we missed to include the q35 cfg

in the future we probably want to have a QemuServer::Tools
to refactor such methods

Signed-off-by: Dominik Csapak <***@proxmox.com>
---
PVE/QemuServer/USB.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
index 3d65d38..036c16c 100644
--- a/PVE/QemuServer/USB.pm
+++ b/PVE/QemuServer/USB.pm
@@ -42,7 +42,7 @@ sub get_usb_controllers {
if ($arch eq 'aarch64') {
$pciaddr = print_pci_addr('ehci', $bridges, $arch, $machine);
push @$devices, '-device', "usb-ehci,id=ehci$pciaddr";
- } elsif ($machine eq 'q35') {
+ } elsif ($machine =~ /q35/) {
# the q35 chipset support native usb2, so we enable usb controller
# by default for this machine type
push @$devices, '-readconfig', '/usr/share/qemu-server/pve-q35.cfg';

--
2.11.0

Dominik Csapak

2018-11-27 10:32:18 UTC

Permalink

if we migrate a vm we call cleanup but the logging looks like:

Starting cleanup for 101
trying to acquire lock...
OK
Configuration file 'nodes/pve-ceph-01/qemu-server/101.conf' does not exist

with this patch it looks like this:

Starting cleanup for 101
trying to acquire lock...
OK
Configuration file 'nodes/pve-ceph-01/qemu-server/101.conf' does not exist
Config for 101 not found, possibly migrated
Finished cleanup for 101

it makes it more clear that the cleanup correctly does nothing,
and gives a hint that it got migrated

Signed-off-by: Dominik Csapak <***@proxmox.com>
---
maybe the $@ if $@ warn is too much?

PVE/CLI/qm.pm | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index eceb9b3..f99dce0 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -757,7 +757,12 @@ __PACKAGE__->register_method({
warn "Starting cleanup for $vmid\n";

PVE::QemuConfig->lock_config($vmid, sub {
- my $conf = PVE::QemuConfig->load_config ($vmid);
+ my $conf = eval { PVE::QemuConfig->load_config ($vmid) };
+ if (!$conf) {
+ warn $@ if $@;
+ warn "Config for $vmid not found, possibly migrated\n";
+ return;
+ }
my $pid = PVE::QemuServer::check_running ($vmid);
die "vm still running\n" if $pid;

--
2.11.0

Thomas Lamprecht

2018-11-27 11:10:46 UTC

Permalink

Post by Dominik Csapak
Starting cleanup for 101
trying to acquire lock...
OK
Configuration file 'nodes/pve-ceph-01/qemu-server/101.conf' does not exist
Starting cleanup for 101
trying to acquire lock...
OK
Configuration file 'nodes/pve-ceph-01/qemu-server/101.conf' does not exist
Config for 101 not found, possibly migrated
Finished cleanup for 101

This isn't really better, IMO, there shouldn't be an error/warning at all
in this case?

Post by Dominik Csapak
it makes it more clear that the cleanup correctly does nothing,
and gives a hint that it got migrated
---
PVE/CLI/qm.pm | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index eceb9b3..f99dce0 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -757,7 +757,12 @@ __PACKAGE__->register_method({
warn "Starting cleanup for $vmid\n";
PVE::QemuConfig->lock_config($vmid, sub {
- my $conf = PVE::QemuConfig->load_config ($vmid);
+ my $conf = eval { PVE::QemuConfig->load_config ($vmid) };
+ if (!$conf) {
+ warn "Config for $vmid not found, possibly migrated\n";
+ return;
+ }
my $pid = PVE::QemuServer::check_running ($vmid);
die "vm still running\n" if $pid;

Thomas Lamprecht

2018-11-27 11:23:18 UTC

Permalink

Post by Thomas Lamprecht

This isn't really better, IMO, there shouldn't be an error/warning at all
in this case?

you probably want to just silently exit early if the config does not exist on
the local node?

Dominik Csapak

2018-11-27 12:28:14 UTC

Permalink

Post by Thomas Lamprecht

This isn't really better, IMO, there shouldn't be an error/warning at all
in this case?

you probably want to just silently exit early if the config does not exist on
the local node?

yes this is also ok, will send a v2

Thomas Lamprecht

2018-11-27 11:05:37 UTC

Permalink

Post by Dominik Csapak
with live migration the machine type not q35, but something like
pc-q35-2.12
so we missed to include the q35 cfg
in the future we probably want to have a QemuServer::Tools
to refactor such methods

there's machine_type_is_q35 in PVE::QemuServer, why not using that one?
That way the error wouldn't have been there in the first place...

Post by Dominik Csapak
---
PVE/QemuServer/USB.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
index 3d65d38..036c16c 100644
--- a/PVE/QemuServer/USB.pm
+++ b/PVE/QemuServer/USB.pm
@@ -42,7 +42,7 @@ sub get_usb_controllers {
if ($arch eq 'aarch64') {
$pciaddr = print_pci_addr('ehci', $bridges, $arch, $machine);
- } elsif ($machine eq 'q35') {
+ } elsif ($machine =~ /q35/) {
# the q35 chipset support native usb2, so we enable usb controller
# by default for this machine type

Dominik Csapak

2018-11-27 12:30:34 UTC

Permalink

Post by Thomas Lamprecht

there's machine_type_is_q35 in PVE::QemuServer, why not using that one?
That way the error wouldn't have been there in the first place...

yes, but i wanted to fix this fast, but ofc i can send a v2 with a
refactored QemuServer::Tools which contains (for now) only
machine_type_is_q35, or do you have a better suggestion for the
name/place?

Post by Thomas Lamprecht

Thomas Lamprecht

2018-11-27 12:45:50 UTC

Permalink

Post by Dominik Csapak

Post by Thomas Lamprecht

there's machine_type_is_q35 in PVE::QemuServer, why not using that one?
That way the error wouldn't have been there in the first place...

You just could have called QemuServer method for now, while we do not encourage
it package internal cyclic dependencies are not a real issue for us, besides a
slight bad feeling maybe. For the long term, yes, you may want to make a module
with some common independent tools, but I wouldn't rush that.

So applied, with a followup comment marking this as fixme...

Post by Dominik Csapak

Post by Thomas Lamprecht

Post by Dominik Csapak
---
PVE/QemuServer/USB.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
index 3d65d38..036c16c 100644
--- a/PVE/QemuServer/USB.pm
+++ b/PVE/QemuServer/USB.pm
@@ -42,7 +42,7 @@ sub get_usb_controllers {
      if ($arch eq 'aarch64') {
          $pciaddr = print_pci_addr('ehci', $bridges, $arch, $machine);
-    } elsif ($machine eq 'q35') {
+    } elsif ($machine =~ /q35/) {
      # the q35 chipset support native usb2, so we enable usb controller
      # by default for this machine type