Re: [pve-devel] [PATCH qemu-server] Use crm-command stop to allow shutdown with timeout and hard stop for HA
On 11/13/19 12:55 PM, Fabian Ebner wrote: > On 11/13/19 9:55 AM, Thomas Lamprecht wrote: >> On 11/12/19 11:03 AM, Fabian Ebner wrote: >>> The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a >>> value of 0 would trigger a hard stop for HA managed VMs. Like this the API >>> description stays valid for all cases. >>> >>> Signed-off-by: Fabian Ebner >>> --- >>> >>> In vm_shutdown we'd like to pass along the timeout parameter to the HA >>> stack, >>> but the value 0 triggers a special behavior there, namely a hard stop. >>> So either the description needs to be changed to mention this behavior or >>> we just don't allow a timeout of 0 in vm_shutdown. Since a shutdown with >>> timeout 0 doesn't really make sense anyways, I opted for the latter. >>> It's the same situation for containers. >>> >> >> timeout == 0 just means instant stop, I did not checked this, but as >> both CTs and VMs allow to pass 0 it really smells like it was done by >> design.. ^^ >> >> Also, limiting API value ranges could be seen as API breakage, and that >> should be avoided if possible.. >> >> What was the behaviour of passing this for a non-HA VM/CT? >> > > If I interpreted the code correctly: > > * For 'qm shutdown --timeout=0': > ** For non-HA managed VMs it leads to (depending on whether guest agent is > enabled) a qmp command 'guest_shutdown' or 'system_powerdown'. > Then (since timeout is 0 this happens immediately) depending on --force we > either kill with SIGTERM or die with "got timeout". > ** For HA managed VMs 'qm shutdown --timeout=0' turns into a vm_stop issuing > a qmp command 'quit' and we actually wait 60 seconds for that command. > > > * For 'pct shutdown --timeout=0': > ** For non-HA managed containers we always give an extra 5 seconds of > timeout, so 5 seconds here. > ** For HA managed containers it will result in an immediate kill. > > I mean it probably doesn't really matter, there was a discrepancy between > what happens for HA managed and non-HA managed VM/CT already. The discrepancy between HA and non-HA was not was I meant, just the non-HA part was relevant to see how the API change of disallowing 0 as timeout could affect peoples use-cases. > I can send a v2 without the new minima if you like. Yes, please. And the schema change should have been in a second patch anyway - it was independent of the HA crm-command adaption, and without that mixed in I could have already applied that one, just FYI :) Thanks! ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server] Use crm-command stop to allow shutdown with timeout and hard stop for HA
On 11/13/19 9:55 AM, Thomas Lamprecht wrote: On 11/12/19 11:03 AM, Fabian Ebner wrote: The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a value of 0 would trigger a hard stop for HA managed VMs. Like this the API description stays valid for all cases. Signed-off-by: Fabian Ebner --- In vm_shutdown we'd like to pass along the timeout parameter to the HA stack, but the value 0 triggers a special behavior there, namely a hard stop. So either the description needs to be changed to mention this behavior or we just don't allow a timeout of 0 in vm_shutdown. Since a shutdown with timeout 0 doesn't really make sense anyways, I opted for the latter. It's the same situation for containers. timeout == 0 just means instant stop, I did not checked this, but as both CTs and VMs allow to pass 0 it really smells like it was done by design.. ^^ Also, limiting API value ranges could be seen as API breakage, and that should be avoided if possible.. What was the behaviour of passing this for a non-HA VM/CT? If I interpreted the code correctly: * For 'qm shutdown --timeout=0': ** For non-HA managed VMs it leads to (depending on whether guest agent is enabled) a qmp command 'guest_shutdown' or 'system_powerdown'. Then (since timeout is 0 this happens immediately) depending on --force we either kill with SIGTERM or die with "got timeout". ** For HA managed VMs 'qm shutdown --timeout=0' turns into a vm_stop issuing a qmp command 'quit' and we actually wait 60 seconds for that command. * For 'pct shutdown --timeout=0': ** For non-HA managed containers we always give an extra 5 seconds of timeout, so 5 seconds here. ** For HA managed containers it will result in an immediate kill. I mean it probably doesn't really matter, there was a discrepancy between what happens for HA managed and non-HA managed VM/CT already. I can send a v2 without the new minima if you like. PVE/API2/Qemu.pm | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index c31dd1d..16a7a04 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -2111,7 +2111,7 @@ __PACKAGE__->register_method({ print "Requesting HA stop for VM $vmid\n"; - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", '0']; PVE::Tools::run_command($cmd); return; }; @@ -2204,7 +2204,7 @@ __PACKAGE__->register_method({ timeout => { description => "Wait maximal timeout seconds.", type => 'integer', - minimum => 0, + minimum => 1, optional => 1, }, forceStop => { @@ -2267,12 +2267,13 @@ __PACKAGE__->register_method({ if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') { + my $timeout = $param->{timeout} // 60; my $hacmd = sub { my $upid = shift; print "Requesting HA stop for VM $vmid\n"; - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", "$timeout"]; PVE::Tools::run_command($cmd); return; }; ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server] Use crm-command stop to allow shutdown with timeout and hard stop for HA
On 11/12/19 11:03 AM, Fabian Ebner wrote: > The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a > value of 0 would trigger a hard stop for HA managed VMs. Like this the API > description stays valid for all cases. > > Signed-off-by: Fabian Ebner > --- > > In vm_shutdown we'd like to pass along the timeout parameter to the HA stack, > but the value 0 triggers a special behavior there, namely a hard stop. > So either the description needs to be changed to mention this behavior or > we just don't allow a timeout of 0 in vm_shutdown. Since a shutdown with > timeout 0 doesn't really make sense anyways, I opted for the latter. > It's the same situation for containers. > timeout == 0 just means instant stop, I did not checked this, but as both CTs and VMs allow to pass 0 it really smells like it was done by design.. ^^ Also, limiting API value ranges could be seen as API breakage, and that should be avoided if possible.. What was the behaviour of passing this for a non-HA VM/CT? > PVE/API2/Qemu.pm | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index c31dd1d..16a7a04 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -2111,7 +2111,7 @@ __PACKAGE__->register_method({ > > print "Requesting HA stop for VM $vmid\n"; > > - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', > 'stopped']; > + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", > '0']; > PVE::Tools::run_command($cmd); > return; > }; > @@ -2204,7 +2204,7 @@ __PACKAGE__->register_method({ > timeout => { > description => "Wait maximal timeout seconds.", > type => 'integer', > - minimum => 0, > + minimum => 1, > optional => 1, > }, > forceStop => { > @@ -2267,12 +2267,13 @@ __PACKAGE__->register_method({ > > if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne > 'ha') { > > + my $timeout = $param->{timeout} // 60; > my $hacmd = sub { > my $upid = shift; > > print "Requesting HA stop for VM $vmid\n"; > > - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', > 'stopped']; > + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", > "$timeout"]; > PVE::Tools::run_command($cmd); > return; > }; > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server] Use crm-command stop to allow shutdown with timeout and hard stop for HA
The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a value of 0 would trigger a hard stop for HA managed VMs. Like this the API description stays valid for all cases. Signed-off-by: Fabian Ebner --- In vm_shutdown we'd like to pass along the timeout parameter to the HA stack, but the value 0 triggers a special behavior there, namely a hard stop. So either the description needs to be changed to mention this behavior or we just don't allow a timeout of 0 in vm_shutdown. Since a shutdown with timeout 0 doesn't really make sense anyways, I opted for the latter. It's the same situation for containers. PVE/API2/Qemu.pm | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index c31dd1d..16a7a04 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -2111,7 +2111,7 @@ __PACKAGE__->register_method({ print "Requesting HA stop for VM $vmid\n"; - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", '0']; PVE::Tools::run_command($cmd); return; }; @@ -2204,7 +2204,7 @@ __PACKAGE__->register_method({ timeout => { description => "Wait maximal timeout seconds.", type => 'integer', - minimum => 0, + minimum => 1, optional => 1, }, forceStop => { @@ -2267,12 +2267,13 @@ __PACKAGE__->register_method({ if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') { + my $timeout = $param->{timeout} // 60; my $hacmd = sub { my $upid = shift; print "Requesting HA stop for VM $vmid\n"; - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", "$timeout"]; PVE::Tools::run_command($cmd); return; }; -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel