YAML consolidation: netplan set (FR-702)

[schopin-pro]

Description

As part of the larger PyYAML cleanup effort, this patch set rewrites the netplan set to use libnetplan's own YAML parser instead of PyYAML. This meant enriching the parser to deal with null fields, that need to be tracked out-of-band, as well as creating new APIs to create YAML files out of a Netplan state, with different nuances to match the previous behavior.

Checklist

• Runs make check successfully.
• Retains 100% code coverage (make check-coverage).

[schopin-pro]

This builds up on #252 hence the draft status.

[codecov-commenter]

Codecov Report

Merging #254 (a4a8ac8) into main (a4a8ac8) will not change coverage.
The diff coverage is n/a.

❗ Current head a4a8ac8 differs from pull request most recent head f4bbd76. Consider uploading reports for the commit f4bbd76 to get more accurate results

@@           Coverage Diff           @@
##             main     #254   +/-   ##
=======================================
  Coverage   99.07%   99.07%           
=======================================
  Files          61       61           
  Lines       10862    10862           
=======================================
  Hits        10762    10762           
  Misses        100      100           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4a8ac8...f4bbd76. Read the comment docs.

[schopin-pro]

Ran the autopkgtests manually:

autopkgtest [17:20:13]: @@@@@@@@@@@@@@@@@@@@ summary
ovs                  PASS
ethernets            PASS
bridges              PASS
bonds                PASS
routing              PASS
vlans                PASS
wifi                 PASS
tunnels              PASS
scenarios            PASS
regressions          PASS
autostart            PASS
cloud-init           PASS

[slyon]

This is another big PR, but the change makes the implementation so much cleaner and more explicit. I really like your overall approach!

I left several comments inline about some details that we should address in a 2nd version of PR.

[schopin-pro]

For the sake of being able to test stuff, I included the commit of #257 since without it the null-handling breaks stuff.

[slyon]

Thank you very much, we're making good progress with this big PR and are getting much closer to a unified YAML parser. kudos!

I've left some smaller inline comments, nothing critical.

Another thing to mention is that make check-coverage currently fails, due to netplan/cli/sriov.py:363, which can probably just be marked as "nocover", as it's just about (non-existing) hardware specific quirks.

[slyon]

Thank you! All my remarks have been addressed. This is ready to be merged IMO.

Abigail is detecting some ABI changes, but they are all compatible (added functions & appended sources to NetplanState). Feel free to update the ABI XML to something like this (after a rebase, to have abi-compat/focal_0.104.xml available): https://paste.ubuntu.com/p/BK8HGnzPrG/ and merge this PR afterwards.

abidiff abi-compat/focal_0.104.xml libnetplan.so.0.0 --headers-dir2 include/ --header-file2 src/abi.h
Functions changes summary: 0 Removed, 0 Changed (26 filtered out), 5 Added functions
Variables changes summary: 0 Removed, 1 Changed, 0 Added variable

5 Added functions:

  [A] 'function gboolean netplan_parser_load_nullable_fields(NetplanParser*, int, GError**)'    {netplan_parser_load_nullable_fields}
  [A] 'function gboolean netplan_parser_load_yaml_from_fd(NetplanParser*, int, GError**)'    {netplan_parser_load_yaml_from_fd}
  [A] 'function gboolean netplan_state_update_yaml_hierarchy(const NetplanState*, const char*, const char*, GError**)'    {netplan_state_update_yaml_hierarchy}
  [A] 'function gboolean netplan_state_write_yaml_file(const NetplanState*, const char*, const char*, GError**)'    {netplan_state_write_yaml_file}
  [A] 'function gboolean netplan_util_create_yaml_patch(const char*, const char*, int, GError**)'    {netplan_util_create_yaml_patch}

1 Changed variable:

  [C] 'NetplanState global_state' was changed at abi_compat.c:64:1:
    size of symbol changed from 168 to 176

[schopin-pro]

IMHO we should completely exclude `global_state` from the ABI check, as it's something that's never been part of the API and is just an intermediate label to construct the `netdefs` and `ordered` public objects.

[slyon]

IMHO we should completely exclude global_state from the ABI check

ACK. Should be resolved by a4a8ac8

[schopin-pro]

Regarding editing the ABI doc, shouldn't we hold until release? That way we can still tweak symbols introduced in previous PRs

[slyon]

Regarding editing the ABI doc, shouldn't we hold until release? That way we can still tweak symbols introduced in previous PRs

Yes, makes sense, especially as it does not break ABI in its current form! Feel free to merge this PR.