Netplan status --diff

[daniloegea]

Description

This PR introduces netplan status --diff. It will use the state_diff module to output the inconsistencies in the network configuration between the system and the netplan configuration.

The diff will be displayed the same way netplan status already displays information. Even though it's a nice approach, plugging the diff display code in the existing status code made things a bit more complicated. I have refactored it (only after have plugged the code, sorry for that) and split the code responsible for each block of information in their own methods.

It also adds the information about bonds, bridges and VRFs introduced by #420.

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.

[slyon]

Thank you very much Danilo, I very much like the looks of this!
And wow! Crafting all the data for test_status_diff.py must have been lots of busywork, but it's worth the effort. Thanks for going the extra mile!

Should this PR become a squash merge (due to the refactoring happening only after the previous commits)?

I left some non-critical inline-comments, which I'd like to see discussed before merging it.
Also, I think we should update the manpage/docs around status --diff[-only], but this could (should?) happen in a separate PR, so we can more easily get Robert's feedback, too.

A final (non-blocking) thought: We should consider using ip addr information to decide if an IP address got assigned through DHCP, looking at the dynamic flag. As DHCPv6 IP addresses are mostly not correctly detected on my system. Probably also in a separate PR, as this one is already pretty big.

$ ip -d -j addr show dev lan0 | jq
[...]
    "addr_info": [
      {
        "family": "inet",
        "local": "192.168.178.141",
        "prefixlen": 24,
        [...]
        "scope": "global",
        "dynamic": true,
      },
      {
        "family": "inet6",
        "local": "2001:16b8:bdba:b100:fae4:3bff:fe2d:3bb7",
        "prefixlen": 64,
        [...]
        "scope": "global",
        "dynamic": true,
      }
    ]

[daniloegea]

Thank you, Lukas. I think it's ready for another round of code review. I believe I addressed most of your comments, I think the main ones being the linebreak at the end of the output, the code duplication and the json/yaml output.

Regarding squashing all the commits, I don't know. While there is a little mess in the middle, there are multiple commits that are well separated. Maybe all the ones names as status and status/diff (and the tests I guess) could be squashed together, but not the rest.

[slyon]

Thank you for the fixes! This is looking really good. I like using the new resulting sub-command a lot!

Could you please rebase and maybe clean up the git history a bit, before we merge it?

Besides that I left have some tiny comments

• "Unknown device type ..." warnings on my system (probably unrelated, but easy to fix)
• _get_missing_property_str can return None or the empty string
• Should we drop the MISSING state, to unclutter the output?
• Should we display unknown interfaces types as other instead of unknown to stay in line with status.py?

I'll push some commits on top of your branch to fix the mentioned issues. Feel free to integrate them into your rebase or revert/drop them with a comment.

[daniloegea]

Hi Lukas, thanks for the review. I incorporated all of your commits, except for the change in _get_missing_property_str. My thought is that returning an empty string is consistent with the return type hint. In fact, we have this situation in many places in the frontend, but that's for another PR...

I squashed all the commits related to the status command in a single commit but kept the history in the commit log.

[slyon]

Thanks a lot. That works for me!