Ignore bad NetDefs and files via parser flags

[daniloegea]

Description

This is an implementation of what I called "parser flags". The idea is to enable the user to change some parsing decisions.

The main application at the moment is to support ignoring parsing errors. Currently Netplan will not generate any configuration if a little mistake is made in one of its YAML files. It can be really bad if you're operating a remote system and end up rebooting the system with a syntax issue in one of your files. Your system will basically boot without any network configuration.

The IGNORE_ERRORS flag will allow the Netplan generator to ignore bad files and bad netdefs so it will still generate some configuration.

TESTS

Integration tests will be executed twice now, once without the ignore_errors flags and once with it enabled. It's intended to increase the confidence that the parser behaves as expected when the flag is enabled and the configuration is good.

I prepared a PPA for Ubuntu Noble with this patch: https://launchpad.net/~danilogondolfo/+archive/ubuntu/netplan-parser-flags

Suggestions of tests:

It can be easily tested in a LXD VM. You can add some broken configuration and reboot the VM or call /usr/libexec/netplan/generate -i to see the parsing process when it's ignoring errors

1. Break the default 50-cloud-init.yaml and reboot the VM. For example, the config below should still bring enp5s0 up and with DHCP working.

network:
  version: 2
  ethernets:
    enp5s0:
      dhcp4: true
      dhcp: true

2. Add a second file that is not a valid YAML
3. Add a third broken file with some complex configuration and dependencies between netdefs. For example, with the configuration below eth2 should still have a backend configuration file and it should contain Bond=bond0.

network:
  ethernets:
    eth0: {}
    eth1: {}
    eth2:
      a: b

  bonds:
    bond0:
      interfaces:
        - eth0
        - eth1
        - eth2

4. Add a fourth broken file with missing dependencies. For example, with the config below you still should have a br0 interface created:

network:
  bridges:
    br0:
      addresses:
        - 10.20.30.40/24

      interfaces:
        - aaaa
        - bbbb
        - dddd

Checklist

• Runs make check successfully.
• Retains 100% code coverage (make check-coverage).
• New/changed keys in YAML format are documented.
• (Optional) Adds example YAML for new feature.
• (Optional) Closes an open bug in Launchpad.

[schopin-pro]

Well, that was a fun lunch break :)

[slyon]

I like the overall approach of this. It is a slight change in behavior, but only for the better, as a non-configured network (due to parsing errors) is always a bad thing. It could lead to unexpected network configuration if some files are ignored but other aren't, but IMO that is still better than no network configuration at all.

[daniloegea]

Thanks for your comments, lads. I basically adopted all of them.

I decided to not drop bad netdefs anymore. I think it's actually better to keep them and generate configuration for what was parsed until the point it failed. There is a chance the interface will still be brought up and work if the error happened after settings IPs or DHCP for example.

To make sure (or at least increase the confidence) that these changes will not cause a change in behavior when the flag is enabled and all the configuration is good, I changed our integration/run.py script to run each test twice, with and without the flag.

The new "Configuration fuzzing" CI action is catching a memory leak! As far as I can tell, it's happening inside glib and there is a comment in the code that handles datalists saying it could happen, see FR-5666 for more details. So, this leak is not introduces by these changes.

I prepared a PPA and added to suggestions of tests in the PR comments.

[slyon]

Thanks a lot for working through all the remarks. This should be relatively safe now.

The doubling of testing-time is a though sell... OTOH more coverage is better. But at least it should be easy to disable (e.g. through ENV variable), so we can skip the double-testing for local tests.

I left a bunch of additional inline comments that need additional consideration.

[daniloegea]

Thanks Lukas, I tried to address all of your comments.

I added an error counter to the parser state instead of a boolean, that might be more useful I think.
I also added a new parser_error to the enum and exposed it to the python code.
I will squash the two extra commits that contain these changes later.

The first commit was split into the definition and implementation of flags as suggested by @schopin-pro in a conversation we had.

The config_fuzzer was changed to run the generator with the --ignore-errors flag for one YAML at a time to avoid that annoying glib memory leak. It's not the ideal test though... I need to investigate how the leak can be identified and ignored.

[slyon]

Thanks for addressing the remarks! lgtm.

I like the error handling to be integrated with existing error handling logic and the .error_count field.