[pve-devel] [PATCH storage/docs 0/2] fix #5779: storage: rbd: allow setting custom KRBD map option(s)

[pve-devel] [PATCH storage/docs 0/2] fix #5779: storage: rbd: allow setting custom KRBD map option(s)

[Friedrich Weber]

Add an optional `krbd-map-options` property to the RBD storage that can be
used to pass custom map options to KRBD. Currently, only the `rxbounce` option
is supported. Setting this option may be necessary in setups with Windows VMs,
as reported in the forum [1]. For now, the option is not exposed in the GUI.

See patch #1 for more details. Patch #2 adds a section to the docs.

storage:

Friedrich Weber (1):
  fix #5779: rbd: allow to pass custom krbd map options

 src/PVE/Storage/Plugin.pm    | 10 ++++++++++
 src/PVE/Storage/RBDPlugin.pm | 14 +++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)


docs:

Friedrich Weber (1):
  storage: rbd: document KRBD map options property

 pve-storage-rbd.adoc | 10 ++++++++++
 1 file changed, 10 insertions(+)


Summary over all repositories:
  3 files changed, 33 insertions(+), 1 deletions(-)

-- 
Generated by git-murpp 0.7.3


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

[pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Friedrich Weber]

When KRBD is enabled for an RBD storage, the storage plugin calls out
to `rbd map` to map an RBD image as a block device on the host.
Sometimes it might be necessary to pass custom options to `rbd map`.
For instance, in some setups with Windows VMs, KRBD logs `bad
crc/signature` and VMs performance is degraded unless the `rxbounce`
option is enabled, as reported in the forum [1].

To allow users to specify custom options for KRBD, introduce a
corresponding `krbd-map-options` property to the RBD plugin. The
property is designed to only accept a supported set of map options.
For now, this is only the `rxbounce` map option, but the supported set
can be extended in the future.

The reasoning for constraining the supported set of map options
instead of allowing to pass a free-form option string is as follows:
If `rxbounce` turns out to be a sensible default, accepting a
free-form option string now will make it hard to switch over the
default to `rxbounce` while still allowing users to disable `rxbounce`
if needed. This would require scanning the free-form string for a
`norxbounce` or similar, which is cumbersome.

If users need to set a map option that `krbd-map-options` does not
support (yet), they can alternatively set the RBD config option
`rbd_default_map_options` [2].

[1] https://forum.proxmox.com/threads/155741/
[2] https://github.com/ceph/ceph/blob/b2a4bd840/src/common/options/rbd.yaml.in#L507

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 src/PVE/Storage/Plugin.pm    | 10 ++++++++++
 src/PVE/Storage/RBDPlugin.pm | 14 +++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 8cc693c..02be257 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -394,6 +394,16 @@ sub verify_dir_override {
     die "invalid override '$value'\n";
 }
 
+PVE::JSONSchema::register_format('pve-storage-krbd-map-option', \&verify_krbd_map_option);
+sub verify_krbd_map_option {
+    my ($option, $noerr) = @_;
+
+    return $option if $option eq 'rxbounce';
+
+    return undef if $noerr;
+    die "invalid krbd map option '$option'\n";
+}
+
 sub private {
     return $defaultData;
 }
diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index f45ad3f..8110cff 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -394,6 +394,12 @@ sub properties {
 	    description => "Client keyring contents (for external clusters).",
 	    type => 'string',
 	},
+	'krbd-map-options' => {
+	    description => "Additional map options (only for krbd). Supported:"
+		." rxbounce.",
+	    type => 'string',
+	    format => 'pve-storage-krbd-map-option-list',
+	},
     };
 }
 
@@ -410,6 +416,7 @@ sub options {
 	krbd => { optional => 1 },
 	keyring => { optional => 1 },
 	bwlimit => { optional => 1 },
+	'krbd-map-options' => { optional => 1 },
     };
 }
 
@@ -726,7 +733,12 @@ sub map_volume {
     # features can only be enabled/disabled for image, not for snapshot!
     $krbd_feature_update->($scfg, $storeid, $img_name);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name);
+    my @additional_options = ();
+    if ($scfg->{'krbd-map-options'}) {
+	@additional_options = ('--options', $scfg->{'krbd-map-options'});
+    }
+
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name, @additional_options);
     run_rbd_command($cmd, errmsg => "can't map rbd volume $name");
 
     return $kerneldev;
-- 
2.39.5



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

[pve-devel] [PATCH docs 2/2] storage: rbd: document KRBD map options property

[Friedrich Weber]

Describe the new `krbd-map-options` property, and mention under which
circumstances the `rxbounce` option may be necessary.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 pve-storage-rbd.adoc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/pve-storage-rbd.adoc b/pve-storage-rbd.adoc
index 5fe558a..71fbc02 100644
--- a/pve-storage-rbd.adoc
+++ b/pve-storage-rbd.adoc
@@ -56,6 +56,16 @@ Enforce access to rados block devices through the krbd kernel module. Optional.
 
 NOTE: Containers will use `krbd` independent of the option value.
 
+krbd-map-options::
+
+Comma-separated list of additional map options for KRBD, see the Ceph docs.
+footnote:[Kernel RBD (KRBD) options
+{cephdocs-url}/man/8/rbd/#kernel-rbd-krbd-options] Optional.
++
+Currently, only the `rxbounce` option is supported. In case the pool hosts
+Windows VM disks, setting `rxbounce` can be necessary to avoid performance
+degradation and `bad crc/signature` warnings in the host journal.
+
 .Configuration Example for a external Ceph cluster (`/etc/pve/storage.cfg`)
 ----
 rbd: ceph-external
-- 
2.39.5



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

Re: [pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Aaron Lauterer]

Does what it claims to do, setting the parameter `rxbounce` when mapping 
the RBD disk.

Therefore:

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>

But one question inline...

On  2024-10-25  13:13, Friedrich Weber wrote:
> When KRBD is enabled for an RBD storage, the storage plugin calls out
> to `rbd map` to map an RBD image as a block device on the host.
> Sometimes it might be necessary to pass custom options to `rbd map`.
> For instance, in some setups with Windows VMs, KRBD logs `bad
> crc/signature` and VMs performance is degraded unless the `rxbounce`
> option is enabled, as reported in the forum [1].
> 
> To allow users to specify custom options for KRBD, introduce a
> corresponding `krbd-map-options` property to the RBD plugin. The
> property is designed to only accept a supported set of map options.
> For now, this is only the `rxbounce` map option, but the supported set
> can be extended in the future.
> 
> The reasoning for constraining the supported set of map options
> instead of allowing to pass a free-form option string is as follows:
> If `rxbounce` turns out to be a sensible default, accepting a
> free-form option string now will make it hard to switch over the
> default to `rxbounce` while still allowing users to disable `rxbounce`
> if needed. This would require scanning the free-form string for a
> `norxbounce` or similar, which is cumbersome.
> 
> If users need to set a map option that `krbd-map-options` does not
> support (yet), they can alternatively set the RBD config option
> `rbd_default_map_options` [2].
> 
> [1] https://forum.proxmox.com/threads/155741/
> [2] https://github.com/ceph/ceph/blob/b2a4bd840/src/common/options/rbd.yaml.in#L507
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>   src/PVE/Storage/Plugin.pm    | 10 ++++++++++
>   src/PVE/Storage/RBDPlugin.pm | 14 +++++++++++++-
>   2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 8cc693c..02be257 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -394,6 +394,16 @@ sub verify_dir_override {
>       die "invalid override '$value'\n";
>   }
>   
> +PVE::JSONSchema::register_format('pve-storage-krbd-map-option', \&verify_krbd_map_option);
> +sub verify_krbd_map_option {
> +    my ($option, $noerr) = @_;
> +
> +    return $option if $option eq 'rxbounce';
> +
> +    return undef if $noerr;
> +    die "invalid krbd map option '$option'\n";
> +}
> +

Why do you place this RBD specific option in the general `Plugin.pm` and 
not into the `RBDPlugin.pm`?

A quick test of mine where I places this in the `RBDPlugin.pm` before 
the properties sub seemed to have worked fine.

>   sub private {
>       return $defaultData;
>   }
> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
> index f45ad3f..8110cff 100644
> --- a/src/PVE/Storage/RBDPlugin.pm
> +++ b/src/PVE/Storage/RBDPlugin.pm
> @@ -394,6 +394,12 @@ sub properties {
>   	    description => "Client keyring contents (for external clusters).",
>   	    type => 'string',
>   	},
> +	'krbd-map-options' => {
> +	    description => "Additional map options (only for krbd). Supported:"
> +		." rxbounce.",
> +	    type => 'string',
> +	    format => 'pve-storage-krbd-map-option-list',
> +	},
>       };
>   }
>   
> @@ -410,6 +416,7 @@ sub options {
>   	krbd => { optional => 1 },
>   	keyring => { optional => 1 },
>   	bwlimit => { optional => 1 },
> +	'krbd-map-options' => { optional => 1 },
>       };
>   }
>   
> @@ -726,7 +733,12 @@ sub map_volume {
>       # features can only be enabled/disabled for image, not for snapshot!
>       $krbd_feature_update->($scfg, $storeid, $img_name);
>   
> -    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name);
> +    my @additional_options = ();
> +    if ($scfg->{'krbd-map-options'}) {
> +	@additional_options = ('--options', $scfg->{'krbd-map-options'});
> +    }
> +
> +    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name, @additional_options);
>       run_rbd_command($cmd, errmsg => "can't map rbd volume $name");
>   
>       return $kerneldev;



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

Re: [pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Friedrich Weber]

On 29/10/2024 14:58, Aaron Lauterer wrote:
> Does what it claims to do, setting the parameter `rxbounce` when mapping
> the RBD disk.
> 
> Therefore:
> 
> Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>

Thanks for testing!

>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> index 8cc693c..02be257 100644
>> --- a/src/PVE/Storage/Plugin.pm
>> +++ b/src/PVE/Storage/Plugin.pm
>> @@ -394,6 +394,16 @@ sub verify_dir_override {
>>       die "invalid override '$value'\n";
>>   }
>>   +PVE::JSONSchema::register_format('pve-storage-krbd-map-option',
>> \&verify_krbd_map_option);
>> +sub verify_krbd_map_option {
>> +    my ($option, $noerr) = @_;
>> +
>> +    return $option if $option eq 'rxbounce';
>> +
>> +    return undef if $noerr;
>> +    die "invalid krbd map option '$option'\n";
>> +}
>> +
> 
> Why do you place this RBD specific option in the general `Plugin.pm` and
> not into the `RBDPlugin.pm`?
> 
> A quick test of mine where I places this in the `RBDPlugin.pm` before
> the properties sub seemed to have worked fine.
> 

Yeah, good question. I placed it there mostly because the other format
definitions are also there :) But I see the point that it could also be
moved to RBDPlugin.pm (as according to your test this appears to work too).


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

Re: [pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Thomas Lamprecht]

Am 25/10/2024 um 13:13 schrieb Friedrich Weber:
> When KRBD is enabled for an RBD storage, the storage plugin calls out
> to `rbd map` to map an RBD image as a block device on the host.
> Sometimes it might be necessary to pass custom options to `rbd map`.
> For instance, in some setups with Windows VMs, KRBD logs `bad
> crc/signature` and VMs performance is degraded unless the `rxbounce`
> option is enabled, as reported in the forum [1].
> 
> To allow users to specify custom options for KRBD, introduce a
> corresponding `krbd-map-options` property to the RBD plugin. The
> property is designed to only accept a supported set of map options.
> For now, this is only the `rxbounce` map option, but the supported set
> can be extended in the future.
> 
> The reasoning for constraining the supported set of map options
> instead of allowing to pass a free-form option string is as follows:
> If `rxbounce` turns out to be a sensible default, accepting a
> free-form option string now will make it hard to switch over the
> default to `rxbounce` while still allowing users to disable `rxbounce`
> if needed. This would require scanning the free-form string for a
> `norxbounce` or similar, which is cumbersome.

Reading the Ceph KRBD option docs [0] it seems a bit like it might
be valid to always enable this for OS type Windows? Which could safe
us an option here and avoid doing this storage wide.

[0]: https://docs.ceph.com/en/reef/man/8/rbd/#kernel-rbd-krbd-options

> If users need to set a map option that `krbd-map-options` does not
> support (yet), they can alternatively set the RBD config option
> `rbd_default_map_options` [2].

But that would work now already? So this is basically just to expose it
directly in the PVE (UI) stack?

One reason I'm not totally happy with such stuff is that storage wide is
quite a big scope; users might then tend to configure the same Ceph pool as
multiple PVE storages, something that can have bad side effects.
We basically had this issue for when the krbd flag was added first, then
it was an "always use krbd or never user krbd" flag, now it's rather an
"always use krbd or else use what works (librbd for VMs and krbd for CTs)"
flag, and a big reason was that otherwise one would need two pools or,
worse, exposing the same pool twice to PVE. This patch feels a bit like
going slightly back to that direction, albeit it's not 1:1 the same and
it might be fine, but I'd also like to have the alternatives evaluated a
bit more closely before going this route.



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

Re: [pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Fabian Grünbichler]

> Thomas Lamprecht <t.lamprecht@proxmox.com> hat am 30.10.2024 09:41 CET geschrieben:
> 
>  
> Am 25/10/2024 um 13:13 schrieb Friedrich Weber:
> > When KRBD is enabled for an RBD storage, the storage plugin calls out
> > to `rbd map` to map an RBD image as a block device on the host.
> > Sometimes it might be necessary to pass custom options to `rbd map`.
> > For instance, in some setups with Windows VMs, KRBD logs `bad
> > crc/signature` and VMs performance is degraded unless the `rxbounce`
> > option is enabled, as reported in the forum [1].
> > 
> > To allow users to specify custom options for KRBD, introduce a
> > corresponding `krbd-map-options` property to the RBD plugin. The
> > property is designed to only accept a supported set of map options.
> > For now, this is only the `rxbounce` map option, but the supported set
> > can be extended in the future.
> > 
> > The reasoning for constraining the supported set of map options
> > instead of allowing to pass a free-form option string is as follows:
> > If `rxbounce` turns out to be a sensible default, accepting a
> > free-form option string now will make it hard to switch over the
> > default to `rxbounce` while still allowing users to disable `rxbounce`
> > if needed. This would require scanning the free-form string for a
> > `norxbounce` or similar, which is cumbersome.
> 
> Reading the Ceph KRBD option docs [0] it seems a bit like it might
> be valid to always enable this for OS type Windows? Which could safe
> us an option here and avoid doing this storage wide.
> 
> [0]: https://docs.ceph.com/en/reef/man/8/rbd/#kernel-rbd-krbd-options
> 
> > If users need to set a map option that `krbd-map-options` does not
> > support (yet), they can alternatively set the RBD config option
> > `rbd_default_map_options` [2].
> 
> But that would work now already? So this is basically just to expose it
> directly in the PVE (UI) stack?
> 
> One reason I'm not totally happy with such stuff is that storage wide is
> quite a big scope; users might then tend to configure the same Ceph pool as
> multiple PVE storages, something that can have bad side effects.
> We basically had this issue for when the krbd flag was added first, then
> it was an "always use krbd or never user krbd" flag, now it's rather an
> "always use krbd or else use what works (librbd for VMs and krbd for CTs)"
> flag, and a big reason was that otherwise one would need two pools or,
> worse, exposing the same pool twice to PVE. This patch feels a bit like
> going slightly back to that direction, albeit it's not 1:1 the same and
> it might be fine, but I'd also like to have the alternatives evaluated a
> bit more closely before going this route.

that would require a way to pass this information through via PVE::Storage::activate_volumes, which currently doesn't exist.. and of course, in a way this would increase coupling of (in this case) qemu-server and pve-storage. maybe it would make sense to evaluate whether we have other use cases for such a mechanism, and decide based on that?

in any case, if the option stays in pve-storage like proposed in this series, it seems its format should be an enum-string(-list), instead of a manual verify sub?


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

Re: [pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Friedrich Weber]

On 30/10/2024 09:41, Thomas Lamprecht wrote:
> Am 25/10/2024 um 13:13 schrieb Friedrich Weber:
>> When KRBD is enabled for an RBD storage, the storage plugin calls out
>> to `rbd map` to map an RBD image as a block device on the host.
>> Sometimes it might be necessary to pass custom options to `rbd map`.
>> For instance, in some setups with Windows VMs, KRBD logs `bad
>> crc/signature` and VMs performance is degraded unless the `rxbounce`
>> option is enabled, as reported in the forum [1].
>>
>> To allow users to specify custom options for KRBD, introduce a
>> corresponding `krbd-map-options` property to the RBD plugin. The
>> property is designed to only accept a supported set of map options.
>> For now, this is only the `rxbounce` map option, but the supported set
>> can be extended in the future.
>>
>> The reasoning for constraining the supported set of map options
>> instead of allowing to pass a free-form option string is as follows:
>> If `rxbounce` turns out to be a sensible default, accepting a
>> free-form option string now will make it hard to switch over the
>> default to `rxbounce` while still allowing users to disable `rxbounce`
>> if needed. This would require scanning the free-form string for a
>> `norxbounce` or similar, which is cumbersome.
> 
> Reading the Ceph KRBD option docs [0] it seems a bit like it might
> be valid to always enable this for OS type Windows? Which could safe
> us an option here and avoid doing this storage wide.

I don't think the 'bad crc/signature' errors necessarily occur for each
and every Windows VM on KRBD. But then again, I just set up a Windows
Server 2022 VM on KRBD and got ~10 of those quite quickly, with
innocuous actions (opening the browser and the like). Also some users
recently reported [1] the need for rxbounce.

So yes, enabling rxbounce for all Windows VM disks might be a good
alternative, but as Fabian points out, technically this isn't really
possible at the moment, because activate_volume doesn't know about the
corresponding VM disk's ostype.

>> If users need to set a map option that `krbd-map-options` does not
>> support (yet), they can alternatively set the RBD config option
>> `rbd_default_map_options` [2].
> 
> But that would work now already? So this is basically just to expose it
> directly in the PVE (UI) stack?

In my tests, setting the `rbd_default_map_options` works for enabling
rxbounce. A forum user reported problems with that approach and I asked
for more details [2], but I haven't heard back yet.

> One reason I'm not totally happy with such stuff is that storage wide is
> quite a big scope; users might then tend to configure the same Ceph pool as
> multiple PVE storages, something that can have bad side effects.
> We basically had this issue for when the krbd flag was added first, then
> it was an "always use krbd or never user krbd" flag, now it's rather an
> "always use krbd or else use what works (librbd for VMs and krbd for CTs)"
> flag, and a big reason was that otherwise one would need two pools or,
> worse, exposing the same pool twice to PVE. This patch feels a bit like
> going slightly back to that direction, albeit it's not 1:1 the same and
> it might be fine, but I'd also like to have the alternatives evaluated a
> bit more closely before going this route.

Yeah, I see the point.

Of course, another alternative is enabling `rxbounce` unconditionally,
as initially requested in [1]. I'm a hesitant to do that because from
reading its description I'd expect it could have a performance impact --
it's probably small, if any, but this should probably be checked before
changing the default.

[1] https://forum.proxmox.com/threads/155741/
[2] https://forum.proxmox.com/threads/155741/post-715664


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

Re: [pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Friedrich Weber]

On 30/10/2024 17:49, Friedrich Weber wrote:
> [...]
> 
> Yeah, I see the point.
> 
> Of course, another alternative is enabling `rxbounce` unconditionally,
> as initially requested in [1]. I'm a hesitant to do that because from
> reading its description I'd expect it could have a performance impact --
> it's probably small, if any, but this should probably be checked before
> changing the default.
> 

I took another look at this: When rxbounce was first introduced, there
was a discussion whether krbd could enabled automatically switch to
"rxbounce mode" if needed [0]. I asked upstream whether this seems
realistic [1], and they responded it's very unlikely to happen.

So the cleanest solution from a user point of view might be if PVE
automatically passes rxbounce only when mapping disks of Windows VMs.
But as Fabian points out [2], this would require some way to pass this
information to the storage layer.

Of course always passing rxbounce is still an option. Upstream confirmed
there is a theoretical performance impact, but it might be negligible in
practice [0]. So if benchmarks show the impact is indeed negligible, we
could go for that route.

But even with benchmarks done carefully, there is a chance that they
fail to catch a performance impact on some types of workloads. So in
order to not disturb setups that currently work fine without rxbounce, I
have a slight preference for only passing rxbounce when needed, even if
that requires some architectural changes.

[0]
https://lore.kernel.org/all/894a36483c241e0cc5154e09e8dd078f57a606d5.camel@kernel.org/
[1]
https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/message/ZSXCXPTLMQPV27Y7I375OBR7CN56LDGH/
[2]
https://lore.proxmox.com/pve-devel/1234079298.5156.1730294987348@webmail.proxmox.com/


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