[pve-devel] [PATCH manager/qemu-server/pve-container v2] Add checkbox for automatic restart of CT/VM after rollback

[pve-devel] [PATCH manager/qemu-server/pve-container v2] Add checkbox for automatic restart of CT/VM after rollback

[Stefan Hanreich]

Update to my first patch, that added a warning to the rollback dialog. As
discussed in Bugzilla, I added a checkbox to toggle an automatic restart to the
rollback window instead of displaying a warning. The restart functionality
itself is implemented in the respective backends.

One difference to the existing dialog window is, that the new one doesn't use
MessageBox anymore as its base component. This is because, it is quite tricky
to add custom elements to the MessageBox and get their values. This leads to
the dialog looking slightly different, since there is no question mark icon
anymore.

pve-manager:

Stefan Hanreich (1):
  fix #4228: ui: Add checkbox to rollback dialog for automatically
    restarting VM/CT

 www/manager6/tree/SnapshotTree.js | 62 +++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 7 deletions(-)


qemu-server:

Stefan Hanreich (1):
  fix #4228: add start parameter to rollback endpoint for automatic
    restarting of VM

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


pve-container:

Stefan Hanreich (1):
  fix #4228: add start parameter to rollback endpoint for automatic
    restarting of CT

 src/PVE/API2/LXC/Snapshot.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH pve-container v2 1/1] fix #4228: add start parameter to rollback endpoint for automatic restarting of CT

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/API2/LXC/Snapshot.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/API2/LXC/Snapshot.pm b/src/PVE/API2/LXC/Snapshot.pm
index 4be16ad..62adaee 100644
--- a/src/PVE/API2/LXC/Snapshot.pm
+++ b/src/PVE/API2/LXC/Snapshot.pm
@@ -272,6 +272,11 @@ __PACKAGE__->register_method({
 	    node => get_standard_option('pve-node'),
 	    vmid => get_standard_option('pve-vmid'),
 	    snapname => get_standard_option('pve-snapshot-name'),
+	    start => {
+		optional => 1,
+		type => 'string',
+		description => "whether the container should get restarted afterwards",
+	    },
 	},
     },
     returns => {
@@ -291,9 +296,16 @@ __PACKAGE__->register_method({
 
 	my $snapname = extract_param($param, 'snapname');
 
+	my $start = extract_param($param, 'start');
+
 	my $realcmd = sub {
 	    PVE::Cluster::log_msg('info', $authuser, "rollback snapshot LXC $vmid: $snapname");
 	    PVE::LXC::Config->snapshot_rollback($vmid, $snapname);
+
+	    if ($start) {
+		PVE::Cluster::log_msg('info', $authuser, "start CT $vmid");
+		PVE::API2::LXC::Status->vm_start({ vmid => $vmid, node => $node })
+	    }
 	};
 
 	my $worker = sub {
-- 
2.30.2

Re: [pve-devel] [PATCH pve-container v2 1/1] fix #4228: add start parameter to rollback endpoint for automatic restarting of CT

[Thomas Lamprecht]

Am 12/09/2022 um 14:00 schrieb Stefan Hanreich:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  src/PVE/API2/LXC/Snapshot.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/PVE/API2/LXC/Snapshot.pm b/src/PVE/API2/LXC/Snapshot.pm
> index 4be16ad..62adaee 100644
> --- a/src/PVE/API2/LXC/Snapshot.pm
> +++ b/src/PVE/API2/LXC/Snapshot.pm
> @@ -272,6 +272,11 @@ __PACKAGE__->register_method({
>  	    node => get_standard_option('pve-node'),
>  	    vmid => get_standard_option('pve-vmid'),
>  	    snapname => get_standard_option('pve-snapshot-name'),
> +	    start => {
> +		optional => 1,
> +		type => 'string',

why is this a string and not a 'boolean'? We normally also document the default
in the schema through, well, the default => X key.

> +		description => "whether the container should get restarted afterwards",

Please try to use somewhat full sentences, that can stand alone and avoids question
like "afterward what?", e.g.:

"Whether the container should get started after rolling back successfully"

> +	    },
>  	},
>      },
>      returns => {
> @@ -291,9 +296,16 @@ __PACKAGE__->register_method({
>  
>  	my $snapname = extract_param($param, 'snapname');
>  
> +	my $start = extract_param($param, 'start');

do you need to extract (i.e., delete from $param hash) the parameter?
Otherwise just use $param->{start} directly below.

> +
>  	my $realcmd = sub {
>  	    PVE::Cluster::log_msg('info', $authuser, "rollback snapshot LXC $vmid: $snapname");
>  	    PVE::LXC::Config->snapshot_rollback($vmid, $snapname);
> +
> +	    if ($start) {
> +		PVE::Cluster::log_msg('info', $authuser, "start CT $vmid");

why produce a cluster log here? The vm_start already does a syslog and if
we'd like to get a cluster log too (or instead of that) it would be an independent
patch and done in PVE::API2::LXC::Status's vm_start code (not that I see much need
for that)

> +		PVE::API2::LXC::Status->vm_start({ vmid => $vmid, node => $node })
> +	    }
>  	};
>  
>  	my $worker = sub {

[pve-devel] [PATCH qemu-server v2 1/1] fix #4228: add start parameter to rollback endpoint for automatic restarting of VM

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 PVE/API2/Qemu.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index d9ef201..8bdda11 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4745,6 +4745,11 @@ __PACKAGE__->register_method({
 	    node => get_standard_option('pve-node'),
 	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
 	    snapname => get_standard_option('pve-snapshot-name'),
+	    start => {
+		optional => 1,
+		type => 'string',
+		description => "whether the container should get restarted afterwards",
+	    },
 	},
     },
     returns => {
@@ -4764,9 +4769,16 @@ __PACKAGE__->register_method({
 
 	my $snapname = extract_param($param, 'snapname');
 
+	my $start = extract_param($param, 'start');
+
 	my $realcmd = sub {
 	    PVE::Cluster::log_msg('info', $authuser, "rollback snapshot VM $vmid: $snapname");
 	    PVE::QemuConfig->snapshot_rollback($vmid, $snapname);
+
+	    if ($start) {
+		PVE::Cluster::log_msg('info', $authuser, "start VM $vmid");
+		PVE::API2::Qemu->vm_start({ vmid => $vmid, node => $node });
+	    }
 	};
 
 	my $worker = sub {
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server v2 1/1] fix #4228: add start parameter to rollback endpoint for automatic restarting of VM

[Thomas Lamprecht]

Am 12/09/2022 um 14:00 schrieb Stefan Hanreich:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index d9ef201..8bdda11 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4745,6 +4745,11 @@ __PACKAGE__->register_method({
>  	    node => get_standard_option('pve-node'),
>  	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
>  	    snapname => get_standard_option('pve-snapshot-name'),
> +	    start => {
> +		optional => 1,
> +		type => 'string',
> +		description => "whether the container should get restarted afterwards",

same as for cotainer: fix type, add default improve description, but please
also with s/container/VM/ here ;-)

> +	    },
>  	},
>      },
>      returns => {
> @@ -4764,9 +4769,16 @@ __PACKAGE__->register_method({
>  
>  	my $snapname = extract_param($param, 'snapname');
>  
> +	my $start = extract_param($param, 'start');

same as pve-container w.r.t. param extraction need

> +
>  	my $realcmd = sub {
>  	    PVE::Cluster::log_msg('info', $authuser, "rollback snapshot VM $vmid: $snapname");
>  	    PVE::QemuConfig->snapshot_rollback($vmid, $snapname);
> +
> +	    if ($start) {
> +		PVE::Cluster::log_msg('info', $authuser, "start VM $vmid");

same as my comment for the pve-container w.r.t adding a cluster log here:
iff, separate patch and done centrally, but we can omit that completely
for now too,

> +		PVE::API2::Qemu->vm_start({ vmid => $vmid, node => $node });
> +	    }
>  	};
>  
>  	my $worker = sub {

[pve-devel] [PATCH manager v2 1/1] fix #4228: ui: Add checkbox to rollback dialog for automatically restarting VM/CT

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
I've added a custom Window containing the dialog, since using MessageBox
for this seemed a bit tricky. Maybe I could move the definition of the
components outside the handler and only instantiate them there? I
thought it was fine for now, since the Window is specific to the
SnapshotTree anyway.

 www/manager6/tree/SnapshotTree.js | 62 +++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/www/manager6/tree/SnapshotTree.js b/www/manager6/tree/SnapshotTree.js
index 97268072..a6f25f19 100644
--- a/www/manager6/tree/SnapshotTree.js
+++ b/www/manager6/tree/SnapshotTree.js
@@ -61,7 +61,9 @@ Ext.define('PVE.guest.SnapshotTree', {
 	    me.mon(win, 'destroy', me.reload, me);
 	},
 
-	snapshotAction: function(action, method) {
+	snapshotAction: function(action, method, params) {
+	    params = params || {};
+
 	    let me = this;
 	    let view = me.getView();
 	    let vm = me.getViewModel();
@@ -75,6 +77,7 @@ Ext.define('PVE.guest.SnapshotTree', {
 	    Proxmox.Utils.API2Request({
 		url: `/nodes/${nodename}/${type}/${vmid}/snapshot/${snapname}/${action}`,
 		method: method,
+		params: params,
 		waitMsgTarget: view,
 		callback: function() {
 		    me.reload();
@@ -89,9 +92,8 @@ Ext.define('PVE.guest.SnapshotTree', {
 		},
 	    });
 	},
-
-	rollback: function() {
-	    this.snapshotAction('rollback', 'POST');
+	rollback: function(params) {
+	    this.snapshotAction('rollback', 'POST', params);
 	},
 	remove: function() {
 	    this.snapshotAction('', 'DELETE');
@@ -255,14 +257,60 @@ Ext.define('PVE.guest.SnapshotTree', {
 	    bind: {
 		disabled: '{!canRollback}',
 	    },
-	    confirmMsg: function() {
+	    handler: function() {
 		let view = this.up('treepanel');
+
 		let rec = view.getSelection()[0];
 		let vmid = view.getViewModel().get('vmid');
-		return Proxmox.Utils.format_task_description('qmrollback', vmid) +
+		let label = Proxmox.Utils.format_task_description('qmrollback', vmid) +
 		    ` '${rec.data.name}'? ${gettext("Current state will be lost.")}`;
+
+		let formPanel = Ext.create({
+		    xtype: 'inputpanel',
+		    bodyPadding: 10,
+		    border: false,
+		    items: [
+			{
+			    xtype: 'label',
+			    html: label,
+			},
+			{
+			    xtype: 'proxmoxcheckbox',
+			    name: 'start',
+			    margin: '5 0 0 0',
+			    boxLabel: gettext('Start guest after rollback'),
+			},
+		    ],
+		});
+
+		let win = Ext.create("Ext.window.Window", {
+		    width: 400,
+		    layout: 'fit',
+		    title: gettext('Confirm'),
+		    closable: true,
+		    resizable: false,
+		    modal: true,
+		    buttons: [
+			{
+			    text: 'Yes',
+			    handler: function() {
+				view.controller.rollback(formPanel.getValues());
+				win.close();
+			    },
+			},
+			{
+			    text: 'No',
+			    handler: function() {
+				win.close();
+			    },
+			},
+		    ],
+		    buttonAlign: 'center',
+		    items: [formPanel],
+		});
+
+		win.show();
 	    },
-	    handler: 'rollback',
 	},
 	'-',
 	{
-- 
2.30.2

Re: [pve-devel] [PATCH manager v2 1/1] fix #4228: ui: Add checkbox to rollback dialog for automatically restarting VM/CT

[Thomas Lamprecht]

Am 12/09/2022 um 14:00 schrieb Stefan Hanreich:

some (short) commit message body can often be nice to have, especially if not
totally trivial (like a typo fix) and it definitively doesn't have to be a full
essay, but I yet have to see the patch here on the list where I'd feel like I
got a too lengthy commit message ;-)

> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> I've added a custom Window containing the dialog, since using MessageBox
> for this seemed a bit tricky. Maybe I could move the definition of the
> components outside the handler and only instantiate them there? I
> thought it was fine for now, since the Window is specific to the
> SnapshotTree anyway.

see below.

> 
>  www/manager6/tree/SnapshotTree.js | 62 +++++++++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/www/manager6/tree/SnapshotTree.js b/www/manager6/tree/SnapshotTree.js
> index 97268072..a6f25f19 100644
> --- a/www/manager6/tree/SnapshotTree.js
> +++ b/www/manager6/tree/SnapshotTree.js
> @@ -61,7 +61,9 @@ Ext.define('PVE.guest.SnapshotTree', {
>  	    me.mon(win, 'destroy', me.reload, me);
>  	},
>  
> -	snapshotAction: function(action, method) {
> +	snapshotAction: function(action, method, params) {
> +	    params = params || {};
> +
>  	    let me = this;
>  	    let view = me.getView();
>  	    let vm = me.getViewModel();
> @@ -75,6 +77,7 @@ Ext.define('PVE.guest.SnapshotTree', {
>  	    Proxmox.Utils.API2Request({
>  		url: `/nodes/${nodename}/${type}/${vmid}/snapshot/${snapname}/${action}`,
>  		method: method,
> +		params: params,
>  		waitMsgTarget: view,
>  		callback: function() {
>  		    me.reload();
> @@ -89,9 +92,8 @@ Ext.define('PVE.guest.SnapshotTree', {
>  		},
>  	    });
>  	},
> -
> -	rollback: function() {
> -	    this.snapshotAction('rollback', 'POST');
> +	rollback: function(params) {
> +	    this.snapshotAction('rollback', 'POST', params);
>  	},
>  	remove: function() {
>  	    this.snapshotAction('', 'DELETE');
> @@ -255,14 +257,60 @@ Ext.define('PVE.guest.SnapshotTree', {
>  	    bind: {
>  		disabled: '{!canRollback}',
>  	    },
> -	    confirmMsg: function() {
> +	    handler: function() {
>  		let view = this.up('treepanel');
> +
>  		let rec = view.getSelection()[0];
>  		let vmid = view.getViewModel().get('vmid');
> -		return Proxmox.Utils.format_task_description('qmrollback', vmid) +
> +		let label = Proxmox.Utils.format_task_description('qmrollback', vmid) +
>  		    ` '${rec.data.name}'? ${gettext("Current state will be lost.")}`;
> +

I'd rather go for using proxmoxWindowEdit here instead, gives you form, basic desired
window handling and API call for free, as the snapshotAction then could be actually
dropped by inlining the relevant code in "remove", which would be the only call side
left then, could maybe get eliminated completely by using a proxmoxStdRemoveButton
for that in a separate patch, but mostly mentioning for completeness sake, wouldn't
block this one here.

> +		let formPanel = Ext.create({
> +		    xtype: 'inputpanel',
> +		    bodyPadding: 10,
> +		    border: false,
> +		    items: [
> +			{
> +			    xtype: 'label',
> +			    html: label,
> +			},
> +			{
> +			    xtype: 'proxmoxcheckbox',
> +			    name: 'start',
> +			    margin: '5 0 0 0',
> +			    boxLabel: gettext('Start guest after rollback'),
> +			},
> +		    ],
> +		});
> +
> +		let win = Ext.create("Ext.window.Window", {
> +		    width: 400,
> +		    layout: 'fit',
> +		    title: gettext('Confirm'),
> +		    closable: true,
> +		    resizable: false,
> +		    modal: true,
> +		    buttons: [
> +			{
> +			    text: 'Yes',
> +			    handler: function() {
> +				view.controller.rollback(formPanel.getValues());
> +				win.close();
> +			    },
> +			},
> +			{
> +			    text: 'No',
> +			    handler: function() {
> +				win.close();
> +			    },
> +			},
> +		    ],
> +		    buttonAlign: 'center',
> +		    items: [formPanel],
> +		});
> +
> +		win.show();

use the `autoShow: true,` configuration for new code instead

>  	    },
> -	    handler: 'rollback',
>  	},
>  	'-',
>  	{