YAML consolidation: `netplan set` (FR-702) by schopin-pro · Pull Request #254 · canonical/netplan

schopin-pro · 2022-01-11T11:42:25Z

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 · 2022-01-11T11:42:59Z

This builds up on #252 hence the draft status.

codecov-commenter · 2022-01-11T11:50:48Z

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.

src/netplan.c

schopin-pro · 2022-01-26T16:37:00Z

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.

src/types.h

src/parse.c

tests/test_cli_get_set.py

schopin-pro · 2022-02-01T17:43:16Z

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.

tests/test_libnetplan.py

src/parse.c

netplan/cli/commands/set.py

src/parse.c

src/types.c

tests/test_cli_get_set.py

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 · 2022-04-28T13:35:15Z

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 · 2022-04-29T14:07:18Z

IMHO we should completely exclude global_state from the ABI check

ACK. Should be resolved by a4a8ac8

src/netplan.c

This hidden option makes things much easier to debug. Inserting unconditional breakpoint() statements in the code is somewhat error-prone as shelling out to new `netplan` process is fairly common throughout the code base, and there are also some systemd services that spawn netplan instances.

This will be useful for `netplan set` to remove emptied-out config files. V2: Fix white space

Any data that is parsed from such a file is considered as having no source file. Existing netdefs retain their original filename, and new ones have a NULL one, which will be exploited later on. V2: Remove erroneous copy-pasted comment

This support is done by pre-processing the input files to identify the null fields, and when we find a key that matches a null field we simply skip to the next key. This necessitates to keep track of the complete key throughout the parser, hence the relatively big size of the patch. V2: * Rename prefix arguments to key_prefix * Use \t rather than . as prefix segment separator

This new function does a targetted update of a given config file, exporting all relevant netdefs (including those without defined config file, e.g. those that have been created via a patch) V2: * Use the tempfile+rename trick to avoid data loss on error * Style fixes V3: * Add a test for removal of unremovable files

This new function basically refreshes all the configuration files that have been used to create the current state. This is where the global source file tracking comes into play, as it allows us to delete config files that didn't result in any actual settings in the final state, i.e. because their defs were removed in a subsequent patch. This function will be used in the netplan set rewrite. V2: * Do some default_filename early sanity checks * Error out if removal of obsolete files fails.

A "set expression" here consists of a path formed of TAB-separated keys, indicating where in the YAML doc we want to make our changes, and a valid YAML expression that will be the payload to insert at that place. The result is a well-formed YAML document.

snapd uses a JSON serializer to format their payloads, which makes sense. Also, the previous version wasn't technically valid YAML anyway.

This removes the dependency on PyYAML from this subcommand. V2: Remove superfluous test data V3: Fix code comment left over from previous version on string escaping

The last use case of this function was in the previous implementation of `netplan set`, and it isn't considered part of our public API.

schopin-pro · 2022-05-03T12:13:56Z

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

slyon · 2022-05-03T13:49:07Z

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.

…27584) The "unlink" part at the bottom of netplan.c:netplan_state_update_yaml_hierarchy seems to ignore any global values (such as "renderer") and operates on netdefs only. The issue is happens due to a combination of the following PRs: canonical#254 canonical#299 Which got SRUed into Jammy via 0.105-0ubuntu2~22.04.2 Here's a minimal reproducer: ``` netplan set network.renderer=NetworkManager --origin-hint=00-no-netdefs-just-globals netplan set network.ethernets.eth99.dhcp4=true --origin-hint=90-some-netdefs ls -l /etc/netplan/ netplan set network.ethernets.eth99=NULL cat /etc/netplan/00-no-netdefs-just-globals.yaml ``` FR-4793

…27584) The "unlink" part at the bottom of netplan.c:netplan_state_update_yaml_hierarchy seems to ignore any global values (such as "renderer") and operates on files containing netdefs only. The issue is happens due to a combination of the following PRs: canonical#254 canonical#299 Which got SRUed into Jammy via 0.105-0ubuntu2~22.04.2 Here's a minimal reproducer: ``` netplan set network.renderer=NetworkManager --origin-hint=00-no-netdefs-just-globals netplan set network.ethernets.eth99.dhcp4=true --origin-hint=90-some-netdefs ls -l /etc/netplan/ netplan set network.ethernets.eth99=NULL cat /etc/netplan/00-no-netdefs-just-globals.yaml ``` FR-4793

…27584) The "unlink" part at the bottom of netplan.c:netplan_state_update_yaml_hierarchy seems to ignore any global values (such as "renderer") and operates on files containing netdefs only. The issue is happens due to a combination of the following PRs: #254 #299 Which got SRUed into Jammy via 0.105-0ubuntu2~22.04.2 Here's a minimal reproducer: ``` netplan set network.renderer=NetworkManager --origin-hint=00-no-netdefs-just-globals netplan set network.ethernets.eth99.dhcp4=true --origin-hint=90-some-netdefs ls -l /etc/netplan/ netplan set network.ethernets.eth99=NULL cat /etc/netplan/00-no-netdefs-just-globals.yaml ``` FR-4793

…27584) Bug: https://bugs.launchpad.net/netplan/+bug/2027584 Origin: canonical/netplan@16bad06 The "unlink" part at the bottom of netplan.c:netplan_state_update_yaml_hierarchy seems to ignore any global values (such as "renderer") and operates on files containing netdefs only. The issue is happens due to a combination of the following PRs: canonical/netplan#254 canonical/netplan#299 Which got SRUed into Jammy via 0.105-0ubuntu2~22.04.2 Here's a minimal reproducer: ``` netplan set network.renderer=NetworkManager --origin-hint=00-no-netdefs-just-globals netplan set network.ethernets.eth99.dhcp4=true --origin-hint=90-some-netdefs ls -l /etc/netplan/ netplan set network.ethernets.eth99=NULL cat /etc/netplan/00-no-netdefs-just-globals.yaml ``` FR-4793 Gbp-Pq: Name 0020-netplan.c-Don-t-drop-files-with-just-global-values-o.patch

schopin-pro force-pushed the yaml-consolidation-set branch from 787d0bb to 2093f00 Compare January 11, 2022 11:47

schopin-pro mentioned this pull request Jan 11, 2022

YAML consolidation: final patch set for configmanager.py (FR-702) #255

Merged

2 tasks

schopin-pro force-pushed the yaml-consolidation-set branch 2 times, most recently from 99d5d60 to 9592983 Compare January 13, 2022 15:16

schopin-pro commented Jan 25, 2022

View reviewed changes

src/netplan.c Show resolved Hide resolved

schopin-pro force-pushed the yaml-consolidation-set branch from 9592983 to d6c72b7 Compare January 26, 2022 15:34

schopin-pro marked this pull request as ready for review January 26, 2022 15:35

schopin-pro requested a review from slyon January 26, 2022 15:35

slyon reviewed Feb 1, 2022

View reviewed changes

schopin-pro force-pushed the yaml-consolidation-set branch from d6c72b7 to 643d714 Compare February 1, 2022 17:42

schopin-pro force-pushed the yaml-consolidation-set branch 2 times, most recently from c181a6f to 5ee4321 Compare February 3, 2022 17:50

schopin-pro mentioned this pull request Feb 8, 2022

Enable eswitch mode setting on SmartNICs #253

Merged

5 tasks

schopin-pro force-pushed the yaml-consolidation-set branch from 5ee4321 to 9c45c8d Compare March 15, 2022 10:41

schopin-pro force-pushed the yaml-consolidation-set branch from 9c45c8d to b3775b3 Compare April 13, 2022 09:24

slyon reviewed Apr 22, 2022

View reviewed changes

schopin-pro force-pushed the yaml-consolidation-set branch 3 times, most recently from ed51f04 to eaa64fd Compare April 26, 2022 11:21

slyon approved these changes Apr 27, 2022

View reviewed changes

slyon added the ABI-compatible ABI changed in a compatible way label Apr 27, 2022

slyon reviewed Apr 29, 2022

View reviewed changes

src/netplan.c Show resolved Hide resolved

slyon reviewed Apr 29, 2022

View reviewed changes

src/netplan.c Outdated Show resolved Hide resolved

schopin-pro added 5 commits May 2, 2022 11:19

lib: centralize the IP rule reset into a standalone function

b18981b

lib: keep track of all source files for a given state

a6defa3

This will be useful for `netplan set` to remove emptied-out config files. V2: Fix white space

schopin-pro force-pushed the yaml-consolidation-set branch from eaa64fd to 7f0fb1c Compare May 2, 2022 11:14

schopin-pro added 5 commits May 2, 2022 13:17

tests: set: use JSON as a format for complex payloads

abf21fa

snapd uses a JSON serializer to format their payloads, which makes sense. Also, the previous version wasn't technically valid YAML anyway.

netplan: set: rewrite to use the internal YAML generator

eacd649

This removes the dependency on PyYAML from this subcommand. V2: Remove superfluous test data V3: Fix code comment left over from previous version on string escaping

netplan: libnetplan: remove now-unused netplan_parse function

f4bbd76

The last use case of this function was in the previous implementation of `netplan set`, and it isn't considered part of our public API.

schopin-pro force-pushed the yaml-consolidation-set branch from 7f0fb1c to f4bbd76 Compare May 2, 2022 11:17

slyon merged commit fa245b8 into canonical:main May 3, 2022

schopin-pro deleted the yaml-consolidation-set branch May 5, 2022 10:05

schopin-pro restored the yaml-consolidation-set branch May 5, 2022 10:05

schopin-pro deleted the yaml-consolidation-set branch May 5, 2022 10:06

schopin-pro restored the yaml-consolidation-set branch May 5, 2022 10:06

slyon mentioned this pull request Jul 19, 2023

netplan.c: Don't drop files with just global values on 'set' (LP: #2027584) #382

Merged

5 tasks

Conversation

schopin-pro commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

schopin-pro commented Jan 11, 2022

Uh oh!

codecov-commenter commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

schopin-pro commented Jan 26, 2022

Uh oh!

slyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schopin-pro commented Feb 1, 2022

Uh oh!

slyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slyon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schopin-pro commented Apr 28, 2022 via email

Uh oh!

slyon commented Apr 29, 2022

Uh oh!

Uh oh!

Uh oh!

schopin-pro commented May 3, 2022

Uh oh!

slyon commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

schopin-pro commented Jan 11, 2022 •

edited

Loading

codecov-commenter commented Jan 11, 2022 •

edited

Loading

slyon left a comment •

edited

Loading

slyon commented May 3, 2022 •

edited

Loading