[pve-devel] [PATCH common] properly encode YAML via YAML::XS

[pve-devel] [PATCH common] properly encode YAML via YAML::XS

[Fabian Grünbichler]

otherwise we get strange errors when formatting data that was originally
JSON, and can thus contain JSON::true/JSON::false.

one example is the QMP query-blockstats command, which gets called (and
the resulting values returned) by /nodes/NODE/qemu/VMID/status/current

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    alternatives include:
    - dropping --output-format yaml altogether
    - manually recursively mapping JSON::true/false to some sensible value before dumping
    - outputting JSON instead of YAML, since the former is a subset of the latter (thanks Dominik ;))
    
    https://forum.proxmox.com/threads/yaml-issues-with-pvesh.76017/

 debian/control          | 1 +
 src/PVE/CLIFormatter.pm | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/debian/control b/debian/control
index 4aa95ed..d7508f5 100644
--- a/debian/control
+++ b/debian/control
@@ -34,6 +34,7 @@ Depends: libclone-perl,
          libtimedate-perl,
          liburi-perl,
          libwww-perl,
+         libyaml-libyaml-perl,
          ${misc:Depends},
          ${perl:Depends},
 Breaks: ifupdown2 (<< 2.0.1-1+pve5),
diff --git a/src/PVE/CLIFormatter.pm b/src/PVE/CLIFormatter.pm
index 4f18fa9..ccecfc3 100644
--- a/src/PVE/CLIFormatter.pm
+++ b/src/PVE/CLIFormatter.pm
@@ -5,7 +5,8 @@ use warnings;
 
 use I18N::Langinfo;
 use POSIX qw(strftime);
-use CPAN::Meta::YAML; # comes with perl-modules
+use YAML::XS; # supports Dumping JSON::PP::Boolean
+$YAML::XS::Boolean = "JSON::PP";
 
 use PVE::JSONSchema;
 use PVE::PTY;
@@ -87,7 +88,7 @@ PVE::JSONSchema::register_renderer('bytes', \&render_bytes);
 sub render_yaml {
     my ($value) = @_;
 
-    my $data = CPAN::Meta::YAML::Dump($value);
+    my $data = YAML::XS::Dump($value);
     $data =~ s/^---[\n\s]//; # remove yaml marker
 
     return $data;
@@ -440,7 +441,7 @@ sub print_api_result {
     }
 
     if ($format eq 'yaml') {
-	print encode('UTF-8', CPAN::Meta::YAML::Dump($data));
+	print encode('UTF-8', YAML::XS::Dump($data));
     } elsif ($format eq 'json') {
 	# Note: we always use utf8 encoding for json format
 	print to_json($data, {utf8 => 1, allow_nonref => 1, canonical => 1 }) . "\n";
-- 
2.20.1

Re: [pve-devel] [PATCH common] properly encode YAML via YAML::XS

[Thomas Lamprecht]

On 9/17/20 1:16 PM, Fabian Grünbichler wrote:
> otherwise we get strange errors when formatting data that was originally
> JSON, and can thus contain JSON::true/JSON::false.
> 
> one example is the QMP query-blockstats command, which gets called (and
> the resulting values returned) by /nodes/NODE/qemu/VMID/status/current
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     alternatives include:
>     - dropping --output-format yaml altogether

FWIW: I like yaml, but we do not have it yet in PBS.
It's at least not an option for the 6.x release.

>     - manually recursively mapping JSON::true/false to some sensible value before dumping

hmm, could do, not ideal, mostly because we then recurse everything
and the outputter does another time - feels not good.

>     - outputting JSON instead of YAML, since the former is a subset of the latter (thanks Dominik ;))

NAK, while technical true I see no point in doing that. yaml is used
over JSON for being more concise and having less syntax noise getting
in ones way when reading it.

Was this issued raised on the currently used module's upstream?
Maybe we/they could fix it there too, helping more than just our use
case.


That said, I have no real objection against using this XS binding of
libyaml-0-2.
btw. we get that already installed on ceph setups through the dependency
chain: ceph-mgr -> python3-yaml -> libyaml-0-2

Re: [pve-devel] [PATCH common] properly encode YAML via YAML::XS

[Fabian Grünbichler]

On September 17, 2020 5:06 pm, Thomas Lamprecht wrote:
> On 9/17/20 1:16 PM, Fabian Grünbichler wrote:
>> otherwise we get strange errors when formatting data that was originally
>> JSON, and can thus contain JSON::true/JSON::false.
>> 
>> one example is the QMP query-blockstats command, which gets called (and
>> the resulting values returned) by /nodes/NODE/qemu/VMID/status/current
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> 
>> Notes:
>>     alternatives include:
>>     - dropping --output-format yaml altogether
> 
> FWIW: I like yaml, but we do not have it yet in PBS.
> It's at least not an option for the 6.x release.
> 
>>     - manually recursively mapping JSON::true/false to some sensible value before dumping
> 
> hmm, could do, not ideal, mostly because we then recurse everything
> and the outputter does another time - feels not good.
> 
>>     - outputting JSON instead of YAML, since the former is a subset of the latter (thanks Dominik ;))
> 
> NAK, while technical true I see no point in doing that. yaml is used
> over JSON for being more concise and having less syntax noise getting
> in ones way when reading it.
> 
> Was this issued raised on the currently used module's upstream?
> Maybe we/they could fix it there too, helping more than just our use
> case.

not raised, but given the docs/description I'd say chances are rather 
slim:

    This module implements a subset of the YAML specification for use in
    reading and writing CPAN metadata files like META.yml and MYMETA.yml. It
    should not be used for any other general YAML parsing or generation
    task.

it's based on/derived from YAML::Tiny, which states:

    It only supports a very basic subset of the full YAML specification.

    Usage is targeted at files like Perl's META.yml, for which a small and
    easily-embeddable module is extremely attractive.

    Features will only be added if they are human readable, and can be
    written in a few lines of code. Please don't be offended if your request
    is refused. Someone has to draw the line, and for YAML::Tiny that
    someone is me.

> 
> 
> That said, I have no real objection against using this XS binding of
> libyaml-0-2.
> btw. we get that already installed on ceph setups through the dependency
> chain: ceph-mgr -> python3-yaml -> libyaml-0-2

install size is also very small (xs+lib are ~200kb), memory overhead 
probably quite a bit more? we could load it only in the code-path where 
we render yaml ;)

Re: [pve-devel] [PATCH common] properly encode YAML via YAML::XS

[Thomas Lamprecht]

On 9/18/20 9:13 AM, Fabian Grünbichler wrote:
> On September 17, 2020 5:06 pm, Thomas Lamprecht wrote:
>> Was this issued raised on the currently used module's upstream?
>> Maybe we/they could fix it there too, helping more than just our use
>> case.
> 
> not raised, but given the docs/description I'd say chances are rather 
> slim:
> 
>     This module implements a subset of the YAML specification for use in
>     reading and writing CPAN metadata files like META.yml and MYMETA.yml. It
>     should not be used for any other general YAML parsing or generation
>     task.
> 
> it's based on/derived from YAML::Tiny, which states:
> 
>     It only supports a very basic subset of the full YAML specification.
> 
>     Usage is targeted at files like Perl's META.yml, for which a small and
>     easily-embeddable module is extremely attractive.
> 
>     Features will only be added if they are human readable, and can be
>     written in a few lines of code. Please don't be offended if your request
>     is refused. Someone has to draw the line, and for YAML::Tiny that
>     someone is me.


ah ok, thanks for pointing that out.


>>
>>
>> That said, I have no real objection against using this XS binding of
>> libyaml-0-2.
>> btw. we get that already installed on ceph setups through the dependency
>> chain: ceph-mgr -> python3-yaml -> libyaml-0-2
> 
> install size is also very small (xs+lib are ~200kb), memory overhead 
> probably quite a bit more? we could load it only in the code-path where 
> we render yaml ;)
> 

Did some simple measurements of RSS from freshly restarted daemon/proxy workers
without client requests yet (Daemon module bases on CLIHandler which includes
CLIFormatter). I repeated them about 10 times and recorded min/max values:

pvedaemon worker:
before: max: 125072   min: 124968
after:  max: 124500   min: 124316

pveproxy worker:
before: max: 129184   min: 128816
after:  max: 128536   min: 128420

This isn't probably to significant statistically, but it actually seems that using
libyaml XS bindings saves RSS over the perl-modules CPAN one.
This comes probably mostly from the fact that it uses XSLoader[0], which allows
dynamic on-the-fly loading of libraries, so it is already only loaded once really
used.

[0]: https://perldoc.perl.org/XSLoader.html

[pve-devel] applied: Re: [PATCH common] properly encode YAML via YAML::XS

[Thomas Lamprecht]

On 9/17/20 1:16 PM, Fabian Grünbichler wrote:
> otherwise we get strange errors when formatting data that was originally
> JSON, and can thus contain JSON::true/JSON::false.
> 
> one example is the QMP query-blockstats command, which gets called (and
> the resulting values returned) by /nodes/NODE/qemu/VMID/status/current
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     alternatives include:
>     - dropping --output-format yaml altogether
>     - manually recursively mapping JSON::true/false to some sensible value before dumping
>     - outputting JSON instead of YAML, since the former is a subset of the latter (thanks Dominik ;))
>     
>     https://forum.proxmox.com/threads/yaml-issues-with-pvesh.76017/
> 
>  debian/control          | 1 +
>  src/PVE/CLIFormatter.pm | 7 ++++---
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
>

applied, thanks!