Bug 2100126

(In reply to Tony Asleson from comment #0)
> Description of problem:
> 
> The lvm JSON output does not adhere to the standard.  This prevents standard
> JSON parsers to parse the output as expected.  The JSON specification:
> 
> https://www.json.org/
> 
> The following items are known at this time.
> 
> 1. Numeric values are double quoted and shouldn't be
> 2. lvm tags could be represented as an array of strings
>    instead of a comma separated string

OK, I think we could fix this quite easily, will try to patch (...but I need to recall that code a bit, I haven't seen that for a while). Though, we'd be changing the existing output someone may use already, but we can consider that as a "fix".

> 3. Empty numeric values would need to have a value,
>    not be an empty string.
> 

Checking with the JSON specs, can we just use 'null' for this?

> Additional info:
> 
> I'll work on a golang wrapper around lvm with all the known lvm
> fields to see what other things could be changed to make the
> JSON output more standards compliant.

OK, let me know... Thanks!

(In reply to Peter Rajnoha from comment #1)
> (In reply to Tony Asleson from comment #0)
> > Description of problem:
> > 
> > The lvm JSON output does not adhere to the standard.  This prevents standard
> > JSON parsers to parse the output as expected.  The JSON specification:
> > 
> > https://www.json.org/
> > 
> > The following items are known at this time.
> > 
> > 1. Numeric values are double quoted and shouldn't be
> > 2. lvm tags could be represented as an array of strings
> >    instead of a comma separated string
> 
> OK, I think we could fix this quite easily, will try to patch (...but I need
> to recall that code a bit, I haven't seen that for a while). Though, we'd be
> changing the existing output someone may use already, but we can consider
> that as a "fix".

I think it would be better to have an option to turn on this fix as this could
break existing users.


> > 3. Empty numeric values would need to have a value,
> >    not be an empty string.
> > 
> 
> Checking with the JSON specs, can we just use 'null' for this?

I don't think so, but let me investigate some more.

> 
> > Additional info:
> > 
> > I'll work on a golang wrapper around lvm with all the known lvm
> > fields to see what other things could be changed to make the
> > JSON output more standards compliant.
> 
> OK, let me know... Thanks!

I'll post a link to the code when I get something put together, thanks.

(In reply to Tony Asleson from comment #2)
> I think it would be better to have an option to turn on this fix as this
> could
> break existing users.
> 

OK, I can add a config for that...

> 
> > > 3. Empty numeric values would need to have a value,
> > >    not be an empty string.
> > > 
> > 
> > Checking with the JSON specs, can we just use 'null' for this?
> 
> I don't think so, but let me investigate some more.
> 

Looks that 'null' can be used for any type of value in JSON. So if it's possible, I'd rather use that 'null' than any "zero value", because for certain specific fields, the value is really not defined at all and giving a zero value would be a bit misleading (e.g. "lv_data_percent" which is related only to thin and cache). We don't output these undefined values in non-json output too - they're numeric fields mostly (== we don't give a zero value for such field, just blank field). For string values, we use "" for undefined.

I have the changes here in this branch so far (still need to add the config option to enable/disable the json arrays for array values): https://sourceware.org/git/?p=lvm2.git;a=shortlog;h=refs/heads/dev-prajnoha-report-strlist-tuning

> (In reply to Tony Asleson from comment #2)
> > I think it would be better to have an option to turn on this fix as this
> > could
> > break existing users.

Looked into this a bit and there's one practical annoyance here. Thing is, the formatting, including JSON, is implemented right inside libdevmapper. The settings we can configure for report are:

  - a separator to use (this is directly a parameter for dm_report_init/dm_report_init_with_selection)

  - global report flags: 

    #define DM_REPORT_OUTPUT_MASK                   0x000000FF                                                                              
    #define DM_REPORT_OUTPUT_ALIGNED                0x00000001                                                                              
    #define DM_REPORT_OUTPUT_BUFFERED               0x00000002                                                                              
    #define DM_REPORT_OUTPUT_HEADINGS               0x00000004                                                                                                                                                                                                                                                                                                                                     
    #define DM_REPORT_OUTPUT_FIELD_NAME_PREFIX      0x00000008                                                                              
    #define DM_REPORT_OUTPUT_FIELD_UNQUOTED         0x00000010                                                                              
    #define DM_REPORT_OUTPUT_COLUMNS_AS_ROWS        0x00000020                                                                              
    #define DM_REPORT_OUTPUT_MULTIPLE_TIMES         0x00000040   

So, right now, the only way I could set how JSON does formatting is by introducing a new flag like "DM_REPORT_OUTPUT_JSON_NUMERIC_FIELDS_WITHOUT_QUOTES", but that seems inadequate because those flags are used to change global settings, not granular ones such as how a concrete format (JSON) is going to behave in a very granular and concrete situation ("putting quotes around numeric values").

This possibly directs us to introducing a new API function to control such granular report settings, but I'm hesitating whether it's worth it - I'm not sure if there will ever be any more settings to configure. Maybe a few could appear in the future, but looks like an overkill right now.

Our options to deal with this are:

  1) just define DM_REPORT_OUTPUT_JSON_NUMERIC_FIELD_WITHOUT_QUOTES as global flag, not matter if it looks weird and inadequate and only applies to a very concrete thing
  2) add a new API function to control more granular settings for report (this looks as a huge overkill if we're not going to use it for more settings)
  3) add environment variable that libdm/report would see and change behaviour based on that (looks hacky and also inadequate)
  4) or we will simply hardcode the change in output and any possible current users of JSON would need to adapt in newer versions of libdm/lvm2 (looks bad in the eyes of users as we'll change the format)

What do you think?

Playing around with using 'null' for values does work.  However, for languages
like golang when a numeric value is 'null' it defaults to its default value
which is typically 0 for numeric values.  Having fields only apply in certain contexts
is problematic.  So even in those cases, a consumer will likely require hand written
code to handle.


>   1) just define DM_REPORT_OUTPUT_JSON_NUMERIC_FIELD_WITHOUT_QUOTES as
> global flag, not matter if it looks weird and inadequate and only applies to
> a very concrete thing
>   2) add a new API function to control more granular settings for report
> (this looks as a huge overkill if we're not going to use it for more
> settings)
>   3) add environment variable that libdm/report would see and change
> behaviour based on that (looks hacky and also inadequate)
>   4) or we will simply hardcode the change in output and any possible
> current users of JSON would need to adapt in newer versions of libdm/lvm2
> (looks bad in the eyes of users as we'll change the format)
> 
> What do you think?

#4 is not an option IMHO.

#1 However what if the name is something like
DM_REPORT_JSON_STD_COMPLIANT so we can address everything we find?

(In reply to Tony Asleson from comment #5)
> Playing around with using 'null' for values does work.  However, for
> languages
> like golang when a numeric value is 'null' it defaults to its default value
> which is typically 0 for numeric values.  Having fields only apply in
> certain contexts
> is problematic.  So even in those cases, a consumer will likely require hand
> written
> code to handle.

Yes, I understand, it's not ideal. But the fact is that LVs are heterogeneous and not all fields apply to all types. At the same time, we have the tabular output so certain cells in the output are really 'null' naturally. I could output some zero value instead, but I'm afraid that would be much more misleading than the 'null'. So I'd rather use the 'null' (if the format allows us to do that) even though it requires the possible reader to do an extra check.

Also, the output in JSON would deviate from output in standard format in case we used 'zero value' instead of 'null value'.

> #1 However what if the name is something like
> DM_REPORT_JSON_STD_COMPLIANT so we can address everything we find?

OK, I think we could do that as compromise... if it doesn't matter that it would also cover all sorts of fixes we find on the way in the future.

(In reply to Tony Asleson from comment #5)
> #1 However what if the name is something like
> DM_REPORT_JSON_STD_COMPLIANT so we can address everything we find?

I've just chatted with Zdenek about all this and he has a good suggestion - we might as well define a new "format" specifier, say "--reportformat json_std_compliant". That would probably be the easiest way - we'd still keep the original "json" for existing users who expect the output as it is today, as well as the new "json_std_compliant" format.

(In reply to Tony Asleson from comment #5)
> Playing around with using 'null' for values does work.  However, for
> languages
> like golang when a numeric value is 'null' it defaults to its default value
> which is typically 0 for numeric values.  Having fields only apply in
> certain contexts
> is problematic.  So even in those cases, a consumer will likely require hand
> written
> code to handle.

The issue with reporting some zero value instead of 'null' is that then we lose possibility of detecting the difference between "undefined value" and "zero value". If using the 'null', is this just an issue with a concrete golang library for parsing JSON or can you choose a different one where the 'null' is handled better? I don't know what's the output from that golang parser - are they some structures that are filled in with values after parsing JSON?

If the 'null' is an issue, we could create a format called "--reportformat json_without_nulls", if we were to follow the logic with introducting a new format specifier.

(In reply to Peter Rajnoha from comment #7)
> (In reply to Tony Asleson from comment #5)
> > #1 However what if the name is something like
> > DM_REPORT_JSON_STD_COMPLIANT so we can address everything we find?
> 
> I've just chatted with Zdenek about all this and he has a good suggestion -
> we might as well define a new "format" specifier, say "--reportformat
> json_std_compliant". That would probably be the easiest way - we'd still
> keep the original "json" for existing users who expect the output as it is
> today, as well as the new "json_std_compliant" format.

This seems like the best way

> (In reply to Tony Asleson from comment #5)
> > Playing around with using 'null' for values does work.  However, for
> > languages
> > like golang when a numeric value is 'null' it defaults to its default value
> > which is typically 0 for numeric values.  Having fields only apply in
> > certain contexts
> > is problematic.  So even in those cases, a consumer will likely require hand
> > written
> > code to handle.
> 
> The issue with reporting some zero value instead of 'null' is that then we
> lose possibility of detecting the difference between "undefined value" and
> "zero value". If using the 'null', is this just an issue with a concrete
> golang library for parsing JSON or can you choose a different one where the
> 'null' is handled better? I don't know what's the output from that golang
> parser - are they some structures that are filled in with values after
> parsing JSON?

Golang has the ability to serialize JSON to a structure or structure to JSON.
Having values that aren't applicable should likely use 'null', which in the
case of golang will be treated as the default value.  I don't see a good way
to not require custom serialization/de-serialization code for golang, so
lets stick to what makes the most sense for values which aren't applicable,
which seems like a 'null'.

Updated patchset with new '--reportformat json_std' cmd line option and 'report/output_format = "json_std"' config file option: https://sourceware.org/git/?p=lvm2.git;a=shortlog;h=refs/heads/dev-prajnoha-json_std

Example output in all 3 formats ("basic" - no change, "json" - no change, "json_std" - new one):

❯  lvs -o name,vg_name,data_percent,lv_tags,lv_kernel_major
  LV    VG     Data%  LV Tags  KMaj
  root  fedora                  253
  swap  fedora                  253
  lvol1 vg     0.00   a,bc,def  253
  lvol2 vg                       -1
  pool  vg     0.00             253


❯  lvs -o name,vg_name,data_percent,lv_tags,lv_kernel_major --reportformat json
  {
      "report": [
          {
              "lv": [
                  {"lv_name":"root", "vg_name":"fedora", "data_percent":"", "lv_tags":"", "lv_kernel_major":"253"},
                  {"lv_name":"swap", "vg_name":"fedora", "data_percent":"", "lv_tags":"", "lv_kernel_major":"253"},
                  {"lv_name":"lvol1", "vg_name":"vg", "data_percent":"0.00", "lv_tags":"a,bc,def", "lv_kernel_major":"253"},
                  {"lv_name":"lvol2", "vg_name":"vg", "data_percent":"", "lv_tags":"", "lv_kernel_major":"-1"},
                  {"lv_name":"pool", "vg_name":"vg", "data_percent":"0.00", "lv_tags":"", "lv_kernel_major":"253"}
              ]
          }
      ]
  }

❯  lvs -o name,vg_name,data_percent,lv_tags,lv_kernel_major --reportformat json_std
  {
      "report": [
          {
              "lv": [
                  {"lv_name":"root", "vg_name":"fedora", "data_percent":null, "lv_tags":[], "lv_kernel_major":253},
                  {"lv_name":"swap", "vg_name":"fedora", "data_percent":null, "lv_tags":[], "lv_kernel_major":253},
                  {"lv_name":"lvol1", "vg_name":"vg", "data_percent":0.00, "lv_tags":["a","bc","def"], "lv_kernel_major":253},
                  {"lv_name":"lvol2", "vg_name":"vg", "data_percent":null, "lv_tags":[], "lv_kernel_major":-1},
                  {"lv_name":"pool", "vg_name":"vg", "data_percent":0.00, "lv_tags":[], "lv_kernel_major":253}
              ]
          }
      ]
  }

Upstream commits 02f015990b2d077f304e19c9ff11728d4480bee8..12ffa753f64eeea946e5331e64ed9d561de98c6b

+ additional upstream commit 1ab6d0d8d8d043a99cb44531de563c7aeb1f0f75

+ upstream commit 800436d2affd4142b9d4b405112c4c30f1d31b5b

Marking Verified:Tested in the latest rpms.

kernel-5.14.0-176.el9    BUILT: Wed Oct 12 03:57:18 AM CDT 2022
lvm2-2.03.17-1.el9    BUILT: Thu Nov 10 10:02:16 AM CST 2022
lvm2-libs-2.03.17-1.el9    BUILT: Thu Nov 10 10:02:16 AM CST 2022


[root@hayes-03 ~]# lvs -o name,vg_name,data_percent,lv_tags,lv_kernel_major --reportformat json
  {
      "report": [
          {
              "lv": [
                  {"lv_name":"vdo_lv", "vg_name":"vdo_sanity", "data_percent":"0.00", "lv_tags":"", "lv_kernel_major":"253"},
                  {"lv_name":"vpool0", "vg_name":"vdo_sanity", "data_percent":"12.06", "lv_tags":"", "lv_kernel_major":"-1"}
              ]
          }
      ]
  }

[root@hayes-03 ~]# lvs -o name,vg_name,data_percent,lv_tags,lv_kernel_major --reportformat json_std
  {
      "report": [
          {
              "lv": [
                  {"lv_name":"vdo_lv", "vg_name":"vdo_sanity", "data_percent":0.00, "lv_tags":[], "lv_kernel_major":253},
                  {"lv_name":"vpool0", "vg_name":"vdo_sanity", "data_percent":12.06, "lv_tags":[], "lv_kernel_major":-1}
              ]
          }
      ]
  }

Marking VERIFIED in the latest rpms as well.

kernel-5.14.0-212.el9    BUILT: Tue Dec 13 03:37:26 PM CST 2022
lvm2-2.03.17-3.el9    BUILT: Wed Dec  7 10:41:40 AM CST 2022
lvm2-libs-2.03.17-3.el9    BUILT: Wed Dec  7 10:41:40 AM CST 2022


[root@hayes-03 ~]#  lvs -o name,vg_name,data_percent,lv_tags,lv_kernel_major --reportformat json
  {
      "report": [
          {
              "lv": [
                  {"lv_name":"pool", "vg_name":"vdo_sanity", "data_percent":"0.00", "lv_tags":"", "lv_kernel_major":"253"},
                  {"lv_name":"vdo_lv", "vg_name":"vdo_sanity", "data_percent":"0.00", "lv_tags":"", "lv_kernel_major":"253"},
                  {"lv_name":"vdo_pool", "vg_name":"vdo_sanity", "data_percent":"", "lv_tags":"", "lv_kernel_major":"253"}
              ]
          }
      ]
  }
[root@hayes-03 ~]# lvs -o name,vg_name,data_percent,lv_tags,lv_kernel_major --reportformat json_std
  {
      "report": [
          {
              "lv": [
                  {"lv_name":"pool", "vg_name":"vdo_sanity", "data_percent":0.00, "lv_tags":[], "lv_kernel_major":253},
                  {"lv_name":"vdo_lv", "vg_name":"vdo_sanity", "data_percent":0.00, "lv_tags":[], "lv_kernel_major":253},
                  {"lv_name":"vdo_pool", "vg_name":"vdo_sanity", "data_percent":null, "lv_tags":[], "lv_kernel_major":253}
              ]
          }
      ]
  }

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (lvm2 bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2023:2544