cli:set: update only specific origin-hint if given (LP: #1997467)

[slyon]

Description

This PR solves multiple issues in netplan set. First, deletion of origin-hint files on update (i.e. if the netdef is already contained inside the origin-hint file). This happens when repeatedly calling netplan set on the same origin-hint and netdef, e.g.:

    Starting with an empty Netplan config in /etc/netplan:
    $ netplan set --origin-hint test "bridges.br54.dhcp4=false"
    => will create test.yaml, containing the br54 definition
    $ netplan set --origin-hint test "bridges.br54.dhcp4=true"
    => updating the same br54 netdef (re-using 'netplan set') will delete test.yaml

This is fixed by the first commit:

• 8931bf3 generate:util: fix double-slash root filepath

Next, when parsing the full YAML hierarchy, stanza updates might be siphoned away, as the same interface might have been defined in another file (e.g. 0-snapd-defaults.yaml) and therefore any updates are redirected to that previous file, or ignored if an origin-hint is passed, which does not match the previous filename (e.g. origin-hint: 90-snapd-config.yaml). Still we always need to parse the full YAML hierarchy in order to validate our data. For example, somebody might call netplan set --origin-hint=test "ethernets.br54.interfaces=[eth0]", which will end up having the new br54 data in hint.yaml but still relies on a definition of eth0 in some other YAML file. If somebody calls netplan set --origin-hint=test "ethernets.eth0.dhcp4=true" OTOH, we want to ignore any previous definition of eth0 and write our update into hint.yaml (assuming it will be a higher order file, compared to the existing YAML hierarchy).
We implemented a load_nullable_overrides() function, to ignore global values or netdefs that are supposed to be written into an origin-hint file.

Last, we want global values (like "renderer") to be conserved per file, instead of written to any new file. When writing a new file (e.g. origin-hint.yaml) we want any renderer value inside the YAML hierarchy to stay unchanged. Also, we only want to add a renderer stanza to the new file if that was given via a "netplan set ..." YAML patch (or an already existing origin-hint file already contains this stanza). So we need to track the global values on a per YAML file base and confirm the filename/path before writing those.

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. LP#1997467

[schopin-pro]

See the only inline comment for details as to why this fix doesn't work :-(

[schopin-pro]

issue: this won't work in the general case

The only reason this works now is because you have standalone definitions. If your new definition refers to an interface defined in another file, netplan set will fail.

A quickfix would be to add a field to netdefs (in the private_data field) to mark them as "touched" when parsing a file without name (npp->current.file_path == NULL), and dump them out in write_yaml_file (or add another variant of this function with these semantics).

The problem with this quickfix is that you'll get the entire netdef dumped, not just the patched parts. A more involved fix would be to change the dirtiness tracker to store origin file rather than a simple boolean. This would allow for fine-grained control over which field we write out, but the implementation could get tricky (or very memory-hungry)

[slyon]

Thank you very much for your review and hints. I need to allocate some extra time to dive deeper into this and fix it properly!

[slyon]

I think the problem described in LP#1997467 should be fixed now. In addition to fixing/improving the path handling (comparison/matching of "invalid" strings, starting with //) and the get/set handling of the global renderer field, depending on which file it was parsed from (this hadn't been tracked at all, up to now).

This PR still needs some refinement/cleanup/comments/proper commit messages and maybe handling of additional edge cases, like the global version or openvswitch fields. But maybe @schopin-pro and/or @daniloegea might already have a brief look at it, considering it a RFC.

[schopin-pro]

I'm not sure I fully grasp the new approach, but I have a first question in mind:

What happens if I have this file:

# read-only file
network:
  ethernets:
    eth0:
      dhcp4: true

and I do netplan set --origin-hint=writable.yaml ethernets.eth0=null ?

[slyon]

I'm not sure I fully grasp the new approach, but I have a first question in mind:

What happens if I have this file:

# read-only file
network:
  ethernets:
    eth0:
      dhcp4: true

and I do netplan set --origin-hint=writable.yaml ethernets.eth0=null ?

I think this example has a flaw.. we're generally expecting read/write permissions on the config files (at least for the owner/root), as of course we cannot update or modify read-only files and would error out.

Assuming that the "read-only file" pasted above is not the same as writable.yaml, this wouldn't matter in this case, though. If the --origin-hint=writable.yaml parameter is given, it means that Netplan is supposed to only work/write on that specific origin-hint file, considering the rest of the YAML hierarchy as read-only, just for context/reference of dependent interface definitions.

Basically, this netplan set commands means: "Delete eth0 from writable.yaml".
If writable.yaml exists (and contains an "eth0" definition) this definition would be deleted.
If writable.yaml doesn't exist, it would not exist after executing the command.
The read-only_file.yaml would stay untouched at all times.

[schopin-pro]

You're almost there, but there are still holes in your solution.

There's a way to paper over said holes by parsing twice the data, once for validation and once for rendering, but that's only a short-term kludge. The underlying issue is the baked-in assumption that netdef is defined only in one place, whereas even individual pieces of data can have several different origins. Fixing the root cause would take much more efforts though, so let's fix the issue at hand first :)

[schopin-pro]

LGTM to me, I just have the usual couple of stray thoughts and remarks but nothing blocking, feel free to ignore them and merge :)

[slyon]

Thank you for all the input. CI is passing and I verified the reproducers from LP#1997467 are passing (using PPA build: https://launchpad.net/~slyon/+archive/ubuntu/netplan-set-lp1997467)

Merging.