[pve-devel] [PATCH widget-toolkit] form/RoleSelector: fix for pve

2022-07-22 Thread Dominik Csapak
in pbs we get an array here, so the renderer is fine, but in pve it's
just a long string, so add a space after commas to achieve the same
effect.

without this, the second column is not visible in pve because of an error
in the renderer (no 'join' function on a string)

Signed-off-by: Dominik Csapak 
---
 src/form/RoleSelector.js | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/form/RoleSelector.js b/src/form/RoleSelector.js
index d82c980..7d897f9 100644
--- a/src/form/RoleSelector.js
+++ b/src/form/RoleSelector.js
@@ -30,8 +30,15 @@ Ext.define('Proxmox.form.RoleSelector', {
header: gettext('Privileges'),
dataIndex: 'privs',
cellWrap: true,
-   // join manually here, as ExtJS joins without whitespace which 
breaks cellWrap
-   renderer: v => v.join(', '),
+   renderer: (v) => {
+   if (Ext.isArray(v)) {
+   // join manually here, as ExtJS joins without 
whitespace which breaks cellWrap
+   return v.join(', ');
+   }
+
+   // add space after comma for cellWrap
+   return v.replaceAll(',', ', ');
+   },
flex: 5,
},
],
-- 
2.30.2



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



Re: [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter

2022-07-22 Thread Wolfgang Bumiller
On Wed, Jul 20, 2022 at 12:59:45PM +0200, Fabian Ebner wrote:
> Instead, use the one from the initial configuration. The only current
> caller is in PMG and the namespace parameter set there agrees with
> the one from the initial configuration, so this is not actually a
> breaking change.

Still technically a breaking change ;-)

> 
> Signed-off-by: Fabian Ebner 
> ---
>  src/PVE/PBSClient.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index 34d3f67..d7dd6e1 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -274,7 +274,7 @@ sub get_snapshots {
>  # create a new PXAR backup of a FS directory tree - doesn't cross FS boundary
>  # by default.
>  sub backup_fs_tree {
> -my ($self, $root, $id, $pxarname, $cmd_opts, $namespace) = @_;
> +my ($self, $root, $id, $pxarname, $cmd_opts) = @_;

And in theory the namespace *could* be overwritable via `$cmd_opts` (if
we used `//=` below.

But even better yet, since it's not used anywhere, maybe we should just
drop `$cmd_opts` entirely?

>  
>  die "backup-id not provided\n" if !defined($id);
>  die "backup root dir not provided\n" if !defined($root);
> @@ -288,7 +288,7 @@ sub backup_fs_tree {
>  
>  $cmd_opts //= {};
>  
> -$cmd_opts->{namespace} = $namespace if defined($namespace);
> +$cmd_opts->{namespace} = $self->{scfg}->{namespace} if 
> defined($self->{scfg}->{namespace});
>  
>  return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
>  };
> -- 
> 2.30.2


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



[pve-devel] [PATCH widget-toolkit] TaskProgress: show text instead of percentage

2022-07-22 Thread Aaron Lauterer
The text needs to be defined in the wait() call as otherwise the
Ext.Progressbar will show a percentage that is not correct anyway but
just reflects where the animated progress bar itself is.

Signed-off-by: Aaron Lauterer 
---
This wasn't much of a problem in most cases where the task finished very
fast. It was most notable in the "Move Disk" situation where the task
could take a very long time, but we switch over to the detailed task log
view there anyway.

I am not sure if showing 'running..." did work at some point in the
past, but now it definitely needs to be defined when calling pbar.wait()

 src/window/TaskViewer.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/window/TaskViewer.js b/src/window/TaskViewer.js
index 9293d95..5d8bb84 100644
--- a/src/window/TaskViewer.js
+++ b/src/window/TaskViewer.js
@@ -37,7 +37,7 @@ Ext.define('Proxmox.window.TaskProgress', {
return defaultValue;
};
 
-   let pbar = Ext.create('Ext.ProgressBar', { text: 'running...' });
+   let pbar = Ext.create('Ext.ProgressBar');
 
me.mon(statstore, 'load', function() {
let status = getObjectValue('status');
@@ -79,7 +79,7 @@ Ext.define('Proxmox.window.TaskProgress', {
 
statstore.startUpdate();
 
-   pbar.wait();
+   pbar.wait({ text: gettext('running...') });
 },
 });
 
-- 
2.30.2



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



Re: [pve-devel] [PATCH pve-container 1/3] fix #3711: enable delete of LXC container via force option

2022-07-22 Thread Wolfgang Bumiller
On Wed, Jul 20, 2022 at 04:49:47PM +0200, Stefan Hrdlicka wrote:
> Make it possible to delete a container whoes underlying storage is no
> longer available. This will just write an warning instead of dying.

"no longer available" != "throws errors" (see below (*))

> 
> Without setting the option force-remove-storage=1 a delete will still
> fail, like it did before the changes. With this option set it will try to
> delete the volume from the storage. If this fails it writes a warning.
> 
> Signed-off-by: Stefan Hrdlicka 
> ---
>  src/PVE/API2/LXC.pm |  8 
>  src/PVE/LXC.pm  | 20 +++-
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 589f96f..4d785c9 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -697,6 +697,13 @@ __PACKAGE__->register_method({
>   ." enabled storages which are not referenced in the 
> config.",
>   optional => 1,
>   },
> + 'force-remove-storage' => {

This name is a bit confusing as instead of enforcing removal you're
actually ignoring the case where removal *fails*, iow.: you allow *not*
removing data.

The `create_vm` call has an `ignore-unpack-errors` parameter, so maybe
`ignore-storage-errors` would work here. (And renaming all the
$variables accordingly.)

> + type => 'boolean',
> + description => 'If set, this will ignore errors when trying to 
> remove LXC'

(*) when documenting it as ignoring errors, I would not expect it to
distinguish between unavailable storages and _other_ errors happening.

Side note: Almost all our API docs just refer to them as 'containers',
the 'LXC' portion can be dropped here.

> + . ' container storage.',
> + default => 0,
> + optional => 1,
> + }
>   },
>  },
>  returns => {
> @@ -758,6 +765,7 @@ __PACKAGE__->register_method({
>   $conf,
>   { lock => 'destroyed' },
>   $param->{'destroy-unreferenced-disks'},
> + $param->{'force-remove-storage'},
>   );
>  
>   PVE::AccessControl::remove_vm_access($vmid);
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index fe63087..74c8d17 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -848,13 +848,22 @@ sub get_primary_ips {
>  }
>  
>  sub delete_mountpoint_volume {
> -my ($storage_cfg, $vmid, $volume) = @_;
> +my ($storage_cfg, $vmid, $volume, $force_remove_storage) = @_;
>  
>  return if PVE::LXC::Config->classify_mountpoint($volume) ne 'volume';
>  
> -my ($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, 
> $volume);
> +my ($vtype, $name, $owner);
> +eval {
> + ($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, 
> $volume);
> +};

^ It is not clear to me why you'd cover this, but not the `vdisk_free`
below, unless you're trying to catch only specific errors (but this is
not specific enough...)

I think this sub can be left unchanged and instead the
`delete_mountpoint_volume()` call itself could be wrapped in an
`eval{}` instead.

>  
> -if ($vmid == $owner) {
> +if (!$force_remove_storage && $@) {
> + die $@;
> +} elsif ($@) {
> + # $force_remove_storage == true is implied here
> + warn "storage not available, can't remove $volume disk image, 
> continuing!\n"
> + . "error: $@\n";
> +} elsif ($vmid == $owner) {
>   PVE::Storage::vdisk_free($storage_cfg, $volume);
>  } else {
>   warn "ignore deletion of '$volume', CT $vmid isn't the owner!\n";
> @@ -862,7 +871,8 @@ sub delete_mountpoint_volume {
>  }
>  
>  sub destroy_lxc_container {
> -my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) 
> = @_;
> +my ($storage_cfg, $vmid, $conf, $replacement_conf,
> + $purge_unreferenced, $force_remove_storage) = @_;
>  
>  my $volids = {};
>  my $remove_volume = sub {
> @@ -873,7 +883,7 @@ sub destroy_lxc_container {
>   return if $volids->{$volume};
>   $volids->{$volume} = 1;
>  
> - delete_mountpoint_volume($storage_cfg, $vmid, $volume);
> + delete_mountpoint_volume($storage_cfg, $vmid, $volume, 
> $force_remove_storage);

So I think I'd just put an `eval{}` here, not pass the parameter
through, and just `die $@ if $@ && !$ignore_storage_errrors` afterwards.

>  };
>  PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, 
> $remove_volume);
>  
> -- 
> 2.30.2


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



Re: [pve-devel] [PATCH pve-container 2/3] fix #3711 cleanup: remove spaces from empty lines

2022-07-22 Thread Wolfgang Bumiller
nit: the subject shouldn't have the `fix #3711` included as this patch
is not directly affecting it ;-)

On Wed, Jul 20, 2022 at 04:49:48PM +0200, Stefan Hrdlicka wrote:
> remove spaces where they are not needed
> 
> Signed-off-by: Stefan Hrdlicka 
> ---
>  src/PVE/LXC.pm | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 74c8d17..42d94ac 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -668,7 +668,7 @@ sub update_lxc_config {
>  
>  # some init scripts expect a linux terminal (turnkey).
>  $raw .= "lxc.environment = TERM=linux\n";
> -
> +
>  my $utsname = $conf->{hostname} || "CT$vmid";
>  $raw .= "lxc.uts.name = $utsname\n";
>  
> @@ -1695,14 +1695,14 @@ sub __mountpoint_mount {
>  my $type = $mountpoint->{type};
>  my $quota = !$snapname && !$mountpoint->{ro} && $mountpoint->{quota};
>  my $mounted_dev;
> -
> +
>  return if !$volid || !$mount;
>  
>  $mount =~ s!/+!/!g;
>  
>  my $mount_path;
>  my ($mpfd, $parentfd, $last_dir);
> -
> +
>  if (defined($rootdir)) {
>   ($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
>   __mount_prepare_rootdir($rootdir, $mount, $rootuid, $rootgid);
> @@ -1711,7 +1711,7 @@ sub __mountpoint_mount {
>  if (defined($stage_mount)) {
>   $mount_path = $rootdir;
>  }
> -
> +
>  my ($storage, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>  
>  die "unknown snapshot path for '$volid'" if !$storage && 
> defined($snapname);
> @@ -1820,7 +1820,7 @@ sub __mountpoint_mount {
>   warn "cannot enable quota control for bind mounts\n" if $quota;
>   return wantarray ? ($volid, 0, undef) : $volid;
>  }
> -
> +
>  die "unsupported storage";
>  }
>  
> -- 
> 2.30.2


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



[pve-devel] applied: [PATCH container] fix #4164: use DHCP=yes instead of DHCP=both in systemd-networkd config

2022-07-22 Thread Wolfgang Bumiller
On Tue, Jul 19, 2022 at 01:54:50PM +0200, Oguz Bektas wrote:
> On Tue, Jul 19, 2022 at 01:52:37PM +0200, Fabian Grünbichler wrote:
> > On July 19, 2022 1:24 pm, Oguz Bektas wrote:
> > > "both" option is deprecated, this gets rid of the warning in the journal
> > > 
> > > Signed-off-by: Oguz Bektas 
> > 
> > some notion which templates you tested this with, when the deprecation 
> > happened in systemd, whether we need a fallback to 'both' for older 
> > versions, etc.pp. would be nice to have..
> > 
> > AFAICT the deprecation was in systemd v219, so should probably be okay 
> > to switch unconditionally..
> 
> yes, all our current templates are using the newer systemd versions, so
> no need for a fallback IMO.
> 
> i tested it with:
> * arch
> * fedora 35 and 36
> * ubuntu 20 and 22
> templates, it got rid of the warning in all of them.

Introduced in 2015 is old enough. Anybody using a container which
actually uses systemd-networkd *and* is that old will just have to
enable dhcp manually...

Applied.


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



[pve-devel] [PATCH manager v2 4/4] VM start Timeout "Options" parameter in the GUI

2022-07-22 Thread Daniel Tschlatscher
This makes it possible to set the newly introduced config parameter
for timeout via the 'startoptions' property string.
For now this only implements setting the timeout value when starting
a VM, though this should be rather easily exentensible to include
other future start options parameters.

Signed-off-by: Daniel Tschlatscher 
---
Changes from v1:
* Now uses a property string to encode the timeout
 www/manager6/qemu/Options.js | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
index a1def4bb..27b3ad93 100644
--- a/www/manager6/qemu/Options.js
+++ b/www/manager6/qemu/Options.js
@@ -76,6 +76,37 @@ Ext.define('PVE.qemu.Options', {
onlineHelp: 'qm_startup_and_shutdown',
} : undefined,
},
+   startoptions: {
+   header: gettext('VM startup options'),
+   defaultValue: Proxmox.Utils.defaultText,
+   renderer: val => val,
+   editor: caps.vms['VM.Config.Options'] ? {
+   xtype: 'proxmoxWindowEdit',
+   subject: gettext('VM start timeout'),
+   setValues: function(values) {
+   Ext.Array.each(this.query('inputpanel'), 
function(panel) {
+   
panel.setValues(PVE.Parser.parsePropertyString(values.startoptions));
+   });
+   },
+   items: {
+   xtype: 'inputpanel',
+   items: {
+   xtype: 'proxmoxintegerfield',
+   name: 'timeout',
+   minValue: 0,
+   maxValue: 86400,
+   fieldLabel: gettext('Timeout (sec)'),
+   emptyText: Proxmox.Utils.defaultText,
+   },
+   onGetValues: function(values) {
+   if (values === undefined || 
Object.keys(values).length === 0) {
+   return { 'delete': 'startoptions' };
+   }
+   return { 'startoptions': 
PVE.Parser.printPropertyString(values) };
+   },
+   },
+   } : undefined,
+   },
ostype: {
header: gettext('OS Type'),
editor: caps.vms['VM.Config.Options'] ? 'PVE.qemu.OSTypeEdit' : 
undefined,
-- 
2.30.2



___
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 1/1] make the timeout value editable when the VM is locked

2022-07-22 Thread Daniel Tschlatscher
In some cases the VM could no longer start when the timeout value was
set and afterwards, for example, hibernated. In this case the VM is
arguably soft locked, because the API would not allow changing the
timeout value anymore. (The only way out here would be to change the
value manually in the config)

To avoid unwanted side effects, it is possible to change the value for
the new 'startoptions' parameter, only if the VM is currently locked
with lock 'suspended'.

Signed-off-by: Daniel Tschlatscher 
---
Changes from v1:
* New patch

 PVE/API2/Qemu.pm | 25 +
 1 file changed, 25 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 7449b9f..30b7f60 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -8,6 +8,7 @@ use POSIX;
 use IO::Socket::IP;
 use URI::Escape;
 use Crypt::OpenSSL::Random;
+use List::Util qw(first);
 
 use PVE::Cluster qw (cfs_read_file cfs_write_file);;
 use PVE::RRD;
@@ -626,6 +627,28 @@ my $check_vm_modify_config_perm = sub {
 return 1;
 };
 
+# Certain parameter fields should still be editable and deletable when the VM 
is locked
+# Returns true only if all parameter fields to be edited or deleted are 
defined in @allowed
+sub skiplock_for_allowed_fields {
+my ($param, @deleted) = @_;
+
+my @allowed = qw"startoptions";
+my $skiplock = 1;
+
+my @to_check = @deleted;
+for (keys %$param) {
+   push(@to_check, $_);
+}
+
+my $idx = 0;
+while ($skiplock && $idx < keys @to_check) {
+   $skiplock &= defined(first { $_ eq $to_check[$idx] } @allowed);
+   $idx++;
+}
+
+return $skiplock;
+}
+
 __PACKAGE__->register_method({
 name => 'vmlist',
 path => '',
@@ -1456,6 +1479,8 @@ my $update_vm_api  = sub {
push @delete, 'runningcpu' if $conf->{runningcpu};
}
 
+   $skiplock |= $conf->{lock} eq "suspended" && 
skiplock_for_allowed_fields($param, @delete);
+
PVE::QemuConfig->check_lock($conf) if !$skiplock;
 
foreach my $opt (keys %$revert) {
-- 
2.30.2



___
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/4] fix #3502: VM start timeout config parameter

2022-07-22 Thread Daniel Tschlatscher
This patch makes it possible to now set the starting timeout value for
a VM via the config parameter 'startoptions'.
Now, if the timeout parameter is set, it will override the heuristic
calculation of the VM start timeout.

The maximum value for the timeout parameter is 86400 seconds, which is
one day. The minimum value is 0, which disables the timeout.

Signed-off-by: Daniel Tschlatscher 
---
Changes from v1:
* Now uses the "startoptions" propertystring
* Maximum timeout reduced to 86400

 PVE/API2/Qemu.pm  | 2 ++
 PVE/QemuServer.pm | 1 +
 PVE/QemuServer/Helpers.pm | 4 
 3 files changed, 7 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index a824657..7449b9f 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -517,6 +517,7 @@ my $generaloptions = {
 'reboot' => 1,
 'startdate' => 1,
 'startup' => 1,
+'startoptions' => 1,
 'tdf' => 1,
 'template' => 1,
 'tags' => 1,
@@ -2494,6 +2495,7 @@ __PACKAGE__->register_method({
description => "Wait maximal timeout seconds.",
type => 'integer',
minimum => 0,
+   maximum => 86400,
default => 'max(30, vm memory in GiB)',
optional => 1,
},
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e9aa248..48e43f8 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -713,6 +713,7 @@ EODESCR
description => "Some (read-only) meta-information about this guest.",
optional => 1,
 },
+startoptions => get_standard_option('start-options'),
 };
 
 my $cicustom_fmt = {
diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
index c10d842..ce09db7 100644
--- a/PVE/QemuServer/Helpers.pm
+++ b/PVE/QemuServer/Helpers.pm
@@ -142,6 +142,10 @@ sub version_cmp {
 
 sub config_aware_timeout {
 my ($config, $is_suspended) = @_;
+
+my $startup = 
PVE::JSONSchema::pve_parse_startup_options($config->{startoptions});
+return $startup->{timeout} if defined($startup->{timeout});
+
 my $memory = $config->{memory};
 my $timeout = 30;
 
-- 
2.30.2



___
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/4] fix #3502: VM start timeout config parameter

2022-07-22 Thread Daniel Tschlatscher
This allows setting the 'startoptions' property string in the config.
For now this only implements the 'timeout' parameter but should be
rather easily extensible and allow related VM start config options
to be also configurable here.

Signed-off-by: Daniel Tschlatscher 
---
Changes from v1:
* This is now a property string instead of a simple parameter to make
  it more "future-proof"
* Maximum timeout reduced to 86400
* Revised description

I did not include the timeout property under the existing "startup"
property string because, while it is a similar feature, it is not
inherently associated with the "startup/shutdown order" functionality.
Also, this makes it more easily extensible for all start-options
that might be added in the future.
Still, as having "startup" and "startoptions" could be confusing for
some, I am open for suggestions on this!

 src/PVE/JSONSchema.pm | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index ab718f3..adabb8e 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -633,6 +633,17 @@ sub pve_verify_startup_order {
 die "unable to parse startup options\n";
 }
 
+register_format('start-options', \&pve_verify_startup_options);
+sub pve_verify_startup_options {
+my ($value, $noerr) = @_;
+
+return $value if pve_parse_startup_options($value);
+
+return undef if $noerr;
+
+die "unable to parse vm start options\n";
+}
+
 my %bwlimit_opt = (
 optional => 1,
 type => 'number', minimum => '0',
@@ -739,6 +750,33 @@ 
PVE::JSONSchema::register_standard_option('pve-startup-order', {
 typetext => '[[order=]\d+] [,up=\d+] [,down=\d+] ',
 });
 
+sub pve_parse_startup_options {
+my ($value) = @_;
+
+return undef if !$value;
+
+my $res = {};
+
+foreach my $p (split(/,/, $value)) {
+   next if $p =~ m/^\s*$/;
+
+   if ($p =~ m/^timeout=(\d+)$/ && int($1) <= 86400) {
+   $res->{timeout} = $1;
+   } else {
+   return undef;
+   }
+}
+
+return $res;
+}
+
+register_standard_option('start-options', {
+description => "Start up options for the VM. This only allows setting the 
VM start timeout for now, which is the maximum VM startup timeout in seconds. 
The maximum value for timeout is 86400, the minimum 0, which disables the 
timeout. If timeout is unset, the timeout will either be the memory of the VM 
in GiBs or 30, depending on which is greater. If unset and hibernated, the 
value will at least be 300 seconds, with hugepages at least 150 seconds.",
+optional => 1,
+type => 'string', format => 'start-options',
+typetext => 'timeout=\d+',
+});
+
 register_format('pve-tfa-secret', \&pve_verify_tfa_secret);
 sub pve_verify_tfa_secret {
 my ($key, $noerr) = @_;
-- 
2.30.2



___
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 3/4] kill/await lingering KVM thread when VM start reaches timeout

2022-07-22 Thread Daniel Tschlatscher
In some cases the VM API start method would return before the detached
KVM process would have exited. This is especially problematic with HA,
because the HA manager would think the VM started successfully, later
see that it exited and start it again in an endless loop.

Moreover, another case exists when resuming a hibernated VM. In this
case, the qemu thread will attempt to load the whole vmstate into
memory before exiting.
Depending on vmstate size, disk read speed, and similar factors this
can take quite a while though and it is not possible to start the VM
normally during this time.

To get around this, this patch intercepts the error, looks whether a
corresponding KVM thread is still running, and waits for/kills it,
before continuing.

Signed-off-by: Daniel Tschlatscher 
---
Changes from v1:
* New patch

 PVE/QemuServer.pm | 40 +---
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ac0b68f..f137f11 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5660,15 +5660,41 @@ sub vm_start_nolock {
$tpmpid = start_swtpm($storecfg, $vmid, $tpm, $migratedfrom);
}
 
-   my $exitcode = run_command($cmd, %run_params);
-   if ($exitcode) {
-   if ($tpmpid) {
-   warn "stopping swtpm instance (pid $tpmpid) due to QEMU 
startup error\n";
-   kill 'TERM', $tpmpid;
+   eval {
+   my $exitcode = run_command($cmd, %run_params);
+
+   if ($exitcode) {
+   if ($tpmpid) {
+   warn "stopping swtpm instance (pid $tpmpid) due to QEMU 
startup error\n";
+   kill 'TERM', $tpmpid;
+   }
+   die "QEMU exited with code $exitcode\n";
}
-   die "QEMU exited with code $exitcode\n";
+   };
+
+   if (my $err = $@) {
+   my $pid = PVE::QemuServer::Helpers::vm_running_locally($vmid);
+
+   if ($pid ne "") {
+   warn "Received error, waiting for detached qemu process 
$pid to exit\n";
+
+   my $count = 0;
+   my $timeout = 300;
+   while (($count < $timeout) &&
+   PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
+   $count++;
+   sleep(1);
+   }
+
+   if ($count >= $timeout) {
+   warn "Reached timeout. Terminating now with SIGKILL\n";
+   kill(9, $pid);
+   }
+   }
+
+   die $err;
}
-   };
+   }
 };
 
 if ($conf->{hugepages}) {
-- 
2.30.2



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