[pve-devel] [PATCH qemu-server/manager/docs 0/3] fix #4140: Add option to disable `fs-freeze`/`-thaw` on backup

[pve-devel] [PATCH qemu-server/manager/docs 0/3] fix #4140: Add option to disable `fs-freeze`/`-thaw` on backup

[Christoph Heiss]

TL;DR: on Windows guests, the guest agent calls into the VSS (Volume
Shadow Copy) subsystem, which has potential to mess up applications
which hook into this themselves like SQL server, as described in the bug
report [0].

Building upon the proposal in that report, add an (default-enabled)
option to the QGA configuration to disable the behavior if really
needed. Due to its potential to wreak havor, show the user a warning
when disabled.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4140

Christoph Heiss (1):
      vzdump: Add VM QGA option to skip `fs-freeze`/`fs-thaw` on backup

 PVE/QemuServer.pm        | 8 +++++++-
 PVE/VZDump/QemuServer.pm | 5 +++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Christoph Heiss (1):
      ui: qga: Add option to turn off fs-freeze/thaw for backup

 www/manager6/Utils.js                     |  2 ++
 www/manager6/form/AgentFeatureSelector.js | 30 +++++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

Christoph Heiss (1):
      qm: Add section explaining fs-freeze/thaw QGA option

 qm.adoc | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

--
2.39.1

[pve-devel] [PATCH qemu-server 1/3] vzdump: Add VM QGA option to skip `fs-freeze`/`fs-thaw` on backup

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 PVE/QemuServer.pm        | 8 +++++++-
 PVE/VZDump/QemuServer.pm | 5 +++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e4d1a70..e409db1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -160,7 +160,13 @@ my $agent_fmt = {
 	description => "Run fstrim after moving a disk or migrating the VM.",
 	type => 'boolean',
 	optional => 1,
-	default => 0
+	default => 0,
+    },
+    fsfreeze_thaw => {
+	description => "Freeze/thaw filesystems on backup for consistency.",
+	type => 'boolean',
+	optional => 1,
+	default => 1,
     },
     type => {
 	description => "Select the agent type",
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 0eb5ec6..5ff46f7 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -862,6 +862,11 @@ sub qga_fs_freeze {
 	return;
     }

+    if (!PVE::QemuServer::get_qga_key($self->{vmlist}->{$vmid}, 'fsfreeze_thaw')) {
+	$self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
+	return;
+    }
+
     $self->loginfo("issuing guest-agent 'fs-freeze' command");
     eval { mon_cmd($vmid, "guest-fsfreeze-freeze") };
     $self->logerr($@) if $@;
--
2.39.1

Re: [pve-devel] [PATCH qemu-server 1/3] vzdump: Add VM QGA option to skip `fs-freeze`/`fs-thaw` on backup

[Thomas Lamprecht]

Am 01/02/2023 um 13:59 schrieb Christoph Heiss:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  PVE/QemuServer.pm        | 8 +++++++-
>  PVE/VZDump/QemuServer.pm | 5 +++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index e4d1a70..e409db1 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -160,7 +160,13 @@ my $agent_fmt = {
>  	description => "Run fstrim after moving a disk or migrating the VM.",
>  	type => 'boolean',
>  	optional => 1,
> -	default => 0
> +	default => 0,
> +    },
> +    fsfreeze_thaw => {

Please rename this to:

'freeze-fs-on-backup'


> +	description => "Freeze/thaw filesystems on backup for consistency.",

detail/nit: s/filesystems/guest filesystem/

> +	type => 'boolean',
> +	optional => 1,
> +	default => 1,
>      },
>      type => {
>  	description => "Select the agent type",
> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
> index 0eb5ec6..5ff46f7 100644
> --- a/PVE/VZDump/QemuServer.pm
> +++ b/PVE/VZDump/QemuServer.pm
> @@ -862,6 +862,11 @@ sub qga_fs_freeze {
>  	return;
>      }
> 
> +    if (!PVE::QemuServer::get_qga_key($self->{vmlist}->{$vmid}, 'fsfreeze_thaw')) {

this is wrong as get_qga_key which uses parse_guest_agent which uses parse_property_string
doesn't sets the default from the schema on parse, so you'd also skip if it's not defined
(please negative test changes too)

Rather do something like:

my $freeze = PVE::QemuServer::get_qga_key($self->{vmlist}->{$vmid}, 'freeze-fs-on-backup') // 1;
if (!$freeze) {
    # ...

> +	$self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
> +	return;
> +    }
> +
>      $self->loginfo("issuing guest-agent 'fs-freeze' command");
>      eval { mon_cmd($vmid, "guest-fsfreeze-freeze") };
>      $self->logerr($@) if $@;

Re: [pve-devel] [PATCH qemu-server 1/3] vzdump: Add VM QGA option to skip `fs-freeze`/`fs-thaw` on backup

[Christoph Heiss]

Thanks for the review!

On Thu, Feb 23, 2023 at 11:55:55AM +0100, Thomas Lamprecht wrote:
> Am 01/02/2023 um 13:59 schrieb Christoph Heiss:
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >  PVE/QemuServer.pm        | 8 +++++++-
> >  PVE/VZDump/QemuServer.pm | 5 +++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index e4d1a70..e409db1 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -160,7 +160,13 @@ my $agent_fmt = {
> >  	description => "Run fstrim after moving a disk or migrating the VM.",
> >  	type => 'boolean',
> >  	optional => 1,
> > -	default => 0
> > +	default => 0,
> > +    },
> > +    fsfreeze_thaw => {
>
> Please rename this to:
>
> 'freeze-fs-on-backup'
Will do, I wasn't really all that happy with the option name anyway.

>
>
> > +	description => "Freeze/thaw filesystems on backup for consistency.",
>
> detail/nit: s/filesystems/guest filesystem/
Ack.

>
> > +	type => 'boolean',
> > +	optional => 1,
> > +	default => 1,
> >      },
> >      type => {
> >  	description => "Select the agent type",
> > diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
> > index 0eb5ec6..5ff46f7 100644
> > --- a/PVE/VZDump/QemuServer.pm
> > +++ b/PVE/VZDump/QemuServer.pm
> > @@ -862,6 +862,11 @@ sub qga_fs_freeze {
> >  	return;
> >      }
> >
> > +    if (!PVE::QemuServer::get_qga_key($self->{vmlist}->{$vmid}, 'fsfreeze_thaw')) {
>
> this is wrong as get_qga_key which uses parse_guest_agent which uses parse_property_string
> doesn't sets the default from the schema on parse, so you'd also skip if it's not defined
> (please negative test changes too)
Good to know! I don't remember if I tested it with an unchanged/fresh
VM, but I guess not (and it was also some time ago). I'll change it as
suggested below and do more thorough testing of such cases too for the
next spin.

>
> Rather do something like:
>
> my $freeze = PVE::QemuServer::get_qga_key($self->{vmlist}->{$vmid}, 'freeze-fs-on-backup') // 1;
> if (!$freeze) {
>     # ...
>
> > +	$self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
> > +	return;
> > +    }
> > +
> >      $self->loginfo("issuing guest-agent 'fs-freeze' command");
> >      eval { mon_cmd($vmid, "guest-fsfreeze-freeze") };
> >      $self->logerr($@) if $@;
>
>

[pve-devel] [PATCH manager 2/3] ui: qga: Add option to turn off fs-freeze/thaw for backup

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 www/manager6/Utils.js                     |  2 ++
 www/manager6/form/AgentFeatureSelector.js | 30 ++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index d8c0bf5a..cb99d7d6 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -475,6 +475,8 @@ Ext.define('PVE.Utils', {
 		    virtio: "VirtIO",
 		};
 		displayText = map[value] || Proxmox.Utils.unknownText;
+	    } else if (key === 'fsfreeze_thaw' && PVE.Parser.parseBoolean(value)) {
+		continue;
 	    } else if (PVE.Parser.parseBoolean(value)) {
 		displayText = Proxmox.Utils.enabledText;
 	    }
diff --git a/www/manager6/form/AgentFeatureSelector.js b/www/manager6/form/AgentFeatureSelector.js
index 0dcc6ecb..eaf3793e 100644
--- a/www/manager6/form/AgentFeatureSelector.js
+++ b/www/manager6/form/AgentFeatureSelector.js
@@ -21,6 +21,26 @@ Ext.define('PVE.form.AgentFeatureSelector', {
 	    },
 	    disabled: true,
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    boxLabel: gettext('Freeze/thaw filesystems on backup for consistency'),
+	    name: 'fsfreeze_thaw',
+	    reference: 'fsfreeze_thaw',
+	    bind: {
+		disabled: '{!enabled.checked}',
+	    },
+	    disabled: true,
+	    uncheckedValue: '0',
+	    defaultValue: '1',
+	},
+	{
+	    xtype: 'displayfield',
+	    userCls: 'pmx-hint',
+	    value: gettext('Filesystem freeze/thaw disabled. This can lead to inconsistent disk backups.'),
+	    bind: {
+		hidden: '{fsfreeze_thaw.checked}',
+	    },
+	},
 	{
 	    xtype: 'displayfield',
 	    userCls: 'pmx-hint',
@@ -47,12 +67,20 @@ Ext.define('PVE.form.AgentFeatureSelector', {
     ],

     onGetValues: function(values) {
-	var agentstr = PVE.Parser.printPropertyString(values, 'enabled');
+	if (PVE.Parser.parseBoolean(values.fsfreeze_thaw)) {
+	    delete values.fsfreeze_thaw;
+	}
+
+	let agentstr = PVE.Parser.printPropertyString(values, 'enabled');
 	return { agent: agentstr };
     },

     setValues: function(values) {
 	let res = PVE.Parser.parsePropertyString(values.agent, 'enabled');
+	if (!Ext.isDefined(res.fsfreeze_thaw)) {
+	    res.fsfreeze_thaw = 1;
+	}
+
 	this.callParent([res]);
     },
 });
--
2.39.1

[pve-devel] [PATCH docs 3/3] qm: Add section explaining fs-freeze/thaw QGA option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 qm.adoc | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index 8a49283..f029ceb 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -1061,6 +1061,26 @@ operations that have the potential to write out zeros to the storage:
 
 On a thin provisioned storage, this can help to free up unused space.
 
+Filesystem freeze/thaw
+^^^^^^^^^^^^^^^^^^^^^^
+
+By default, VM filesystems are synced via the 'fs-freeze' command when a backup
+is performed, to provide consistency. On Windows guests, this calls into the VSS
+(volume shadow copy) subsystem.
+
+There might be cases where this needs to be disabled, e.g. when running
+applications which hook into the Windows VSS subsystem themselves. This has
+been observed to be necessary when running SQL Server, which otherwise might
+break its own differential backup mode.
+
+This behavior can be turned off using the 'Freeze/thaw filesystems on backup
+for consistency' option. With this option disabled, {pve} will skip the
+'fs-freeze' and 'fs-thaw' commands when running backup jobs.
+
+NOTE: Disabling this option can potentially lead to backups with inconsistent
+filesystems and should therefore only be changed if you know what you are
+doing.
+
 Troubleshooting
 ^^^^^^^^^^^^^^^
 
-- 
2.39.1

Re: [pve-devel] [PATCH docs 3/3] qm: Add section explaining fs-freeze/thaw QGA option

[Thomas Lamprecht]

Am 01/02/2023 um 13:59 schrieb Christoph Heiss:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  qm.adoc | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/qm.adoc b/qm.adoc
> index 8a49283..f029ceb 100644
> --- a/qm.adoc
> +++ b/qm.adoc
> @@ -1061,6 +1061,26 @@ operations that have the potential to write out zeros to the storage:
>  
>  On a thin provisioned storage, this can help to free up unused space.
>  
> +Filesystem freeze/thaw
> +^^^^^^^^^^^^^^^^^^^^^^

Would title case this and add "on Backup", i.e.: Filesystem Freeze & Thaw on Backup

> +
> +By default, VM filesystems are synced via the 'fs-freeze' command when a backup

I know we're already in the section, but it might help to remind user 
s/command/QEMU Guest Agent Command/

> +is performed, to provide consistency. On Windows guests, this calls into the VSS
> +(volume shadow copy) subsystem.

I'd either move the last sentence to the next paragraph or also mention how it's
done in Linux, as otherwise this IMO reads as it has been put slightly out of place.

> +
> +There might be cases where this needs to be disabled, e.g. when running

as per our technical writing style guide (see intranet wiki) s/e.g./for example/

> +applications which hook into the Windows VSS subsystem themselves. This has
> +been observed to be necessary when running SQL Server, which otherwise might
> +break its own differential backup mode.

I'd reword this a bit, maybe something a long the lines of:

"Some applications handle consistent backups themselves by hooking into the
Windows VSS layer, a fs-freeze might then interfere with that. For example
calling fs-freeze whit some SQL Servers triggers VSS to call the SQL Writer
VSS module in a mode that breaks the SQL backup chain for differential backups.

For such setups you can configure {pve} to not issue a freeze and thaw cycle
on backup by setting the `freeze-fs-on-backup` qga option to `0`, which can
be also done via the web UI."

Below might be obsolete if (something like) above is adopted, so just commenting
two minor nits for completeness sake:

> +
> +This behavior can be turned off using the 'Freeze/thaw filesystems on backup
> +for consistency' option. With this option disabled, {pve} will skip the

s/skip/skip issuing/

> +'fs-freeze' and 'fs-thaw' commands when running backup jobs.

s/commands/QGA commands/


> +
> +NOTE: Disabling this option can potentially lead to backups with inconsistent
> +filesystems and should therefore only be changed if you know what you are

s/changed/disabled/

> +doing.
> +
>  Troubleshooting
>  ^^^^^^^^^^^^^^^
>  

Re: [pve-devel] [PATCH docs 3/3] qm: Add section explaining fs-freeze/thaw QGA option

[Christoph Heiss]

Thanks again, I'll incorporate your suggestions below as appriopriate
for the next spin!

On Thu, Feb 23, 2023 at 12:34:22PM +0100, Thomas Lamprecht wrote:
> Am 01/02/2023 um 13:59 schrieb Christoph Heiss:
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >  qm.adoc | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/qm.adoc b/qm.adoc
> > index 8a49283..f029ceb 100644
> > --- a/qm.adoc
> > +++ b/qm.adoc
> > @@ -1061,6 +1061,26 @@ operations that have the potential to write out zeros to the storage:
> >
> >  On a thin provisioned storage, this can help to free up unused space.
> >
> > +Filesystem freeze/thaw
> > +^^^^^^^^^^^^^^^^^^^^^^
>
> Would title case this and add "on Backup", i.e.: Filesystem Freeze & Thaw on Backup
>
> > +
> > +By default, VM filesystems are synced via the 'fs-freeze' command when a backup
>
> I know we're already in the section, but it might help to remind user
> s/command/QEMU Guest Agent Command/
>
> > +is performed, to provide consistency. On Windows guests, this calls into the VSS
> > +(volume shadow copy) subsystem.
>
> I'd either move the last sentence to the next paragraph or also mention how it's
> done in Linux, as otherwise this IMO reads as it has been put slightly out of place.
>
> > +
> > +There might be cases where this needs to be disabled, e.g. when running
>
> as per our technical writing style guide (see intranet wiki) s/e.g./for example/
>
> > +applications which hook into the Windows VSS subsystem themselves. This has
> > +been observed to be necessary when running SQL Server, which otherwise might
> > +break its own differential backup mode.
>
> I'd reword this a bit, maybe something a long the lines of:
>
> "Some applications handle consistent backups themselves by hooking into the
> Windows VSS layer, a fs-freeze might then interfere with that. For example
> calling fs-freeze whit some SQL Servers triggers VSS to call the SQL Writer
> VSS module in a mode that breaks the SQL backup chain for differential backups.
>
> For such setups you can configure {pve} to not issue a freeze and thaw cycle
> on backup by setting the `freeze-fs-on-backup` qga option to `0`, which can
> be also done via the web UI."
>
> Below might be obsolete if (something like) above is adopted, so just commenting
> two minor nits for completeness sake:
>
> > +
> > +This behavior can be turned off using the 'Freeze/thaw filesystems on backup
> > +for consistency' option. With this option disabled, {pve} will skip the
>
> s/skip/skip issuing/
>
> > +'fs-freeze' and 'fs-thaw' commands when running backup jobs.
>
> s/commands/QGA commands/
>
>
> > +
> > +NOTE: Disabling this option can potentially lead to backups with inconsistent
> > +filesystems and should therefore only be changed if you know what you are
>
> s/changed/disabled/
>
> > +doing.
> > +
> >  Troubleshooting
> >  ^^^^^^^^^^^^^^^
> >
>