Netplan state diff #386

daniloegea · 2023-07-28T17:10:29Z

Description

This is the first iteration of the diff feature. At the moment, only a few number of resources are analyzed:

IP Addresses
DNS nameserver and search domains
Routes
MAC addresses
Missing interfaces

A number of heuristics were used to eliminate configuration we don't want to compare. These are basically settings that are very likely assigned automatically, such as IPs and routes assigned via DHCP. The reason for that is that this information will not be found in Netplan.
For example: if the interface has dhcp4 enabled and an empty list of addresses in Netplan, we don't report to the user that the IP that is assigned, via DHCP, to that interface in the system is not present in Netplan. We do report though the case where the interface has no IPs but DHCP is enabled in Netplan and when extra IPs are found.

Experimental user interface

It still doesn't include a user interface for the data presentation, but I have an experimental one available in a separate branch.

I added a new experimental command, netplan diff, to enable people to look at one of the suggestions for the user interface. I also added a --diff to netplan status that makes the diff be shown along with the original information. None of this is definitive and can be changed/removed as I get peoples opinions.

I packaged it in a PPA. It's available here: https://launchpad.net/~danilogondolfo/+archive/ubuntu/netplan-diff/

Please, install the PPA and try running netplan status --all --diff and netplan diff and let me know your opinion :)

netplan diff accepts the parameter -s which you can use to change the table style. There are 6 styles available, from 0 to 5.

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 · 2023-07-31T10:07:57Z

Thanks for providing the PPA test builds! The netplan diff styles 0, 1 & 4 look very similar to me (just differing in colors a bit), while 2 & 3 look a bit misaligned on my screen. It seems to take lots of horizontal screen space (especially the "Routes" section), which makes it hard to use on portrait mode screens (my setup) or probably serial consoles, too.

I personally like the inline mode of netplan status --diff better, while it's still a bit plain. I needed to take several looks to spot the actual differences. I wonder if we could have a git diff inline-style output of netplan status (or maybe even of netplan get)? IMO this would make reading the output easier, as it relates to an existing output/layout, instead of introducing a new one (see mockup below). WDYT?

Could you maybe prepare screenshots of the different modes presented in this PR, so we could bring it up for discussion during the next Netplan sync?

slyon

Thank you very much for working on this and going the extra mile of making all the required data available via libnetplan's API!

This is not yet a full review, as I'm running out of time here, but I wanted to leave some initial comments.

This PR will get in conflict with #385 (due to the export of the iterators via Python's ctypes, vs cffi). I wonder if we should already start rebasing it on top of that other PR?

src/util.c

netplan_cli/libnetplan.py

netplan_cli/cli/state.py

slyon · 2023-08-10T14:00:46Z

netplan_cli/cli/state.py

+            route = route + f' table {self.table}'
+        return route
+
+    def __eq__(self, route):


note: This will be interesting, comparing routes is tricky. I'll need to have a closer look into that.

python-cffi/netplan/netdef.py

slyon

Thanks for rebasing on top of the CFFI bindings! This is still a big PR and we might not be able to get it fully landed for inclusion in v0.107 today.

python-cffi/netplan/netdef.py

netplan_cli/cli/state.py

daniloegea · 2023-10-27T17:15:56Z

Hi Lukas, thanks for reviewing this PR. I tried to address the issues you've found and answer your questions. I think it's looking a bit better now.

There are some fields that exist in Netplan but not in the system and vice-versa, such as on-link and protocol. Using the default dataclass methods makes it harder to compare objects.

slyon

Thank you for addressing my comments!
I have one tiny nitpick inline, which I'd like to see fixed. But after that, I think this should be ready for merging.

netplan_cli/cli/state_diff.py

The state_diff submodule is responsible for comparing system's and netplan's states and calculating the differences. The new class NetplanDiffState must be instantiated with both states. The method get_full_state() will return both system's and netplan's states without calculating the differences. The diff calculation will be added in the next commits.

The get_diff() will calculate the differences between system's and Netplan's states and return a report to the caller. This commit adds support for checking what interfaces are present only in one of the states.

get_diff() will compare and report differences in the addresses present in the system and Netplan. Addresses detected as dynamically assigned will not be reported as missing as they will not be defined in Netplan.

A number of heuristics are implemented to eliminate routes we don't want to compare, such as routes installed dynamically, as they will not be found in Netplan.

It will be used to serialize the state and diff.

For netplan interfaces add the name and type. For system interfaces add the name, type and index.

Also add a few integrations tests that use tools/diff.py to test the diff against a running system configured with Netplan.

daniloegea force-pushed the netplan_diff_round2 branch 10 times, most recently from 60ce1b7 to 9b7954b Compare August 5, 2023 21:16

daniloegea force-pushed the netplan_diff_round2 branch 8 times, most recently from 0c2cf1c to 7fe90f8 Compare August 9, 2023 10:37

daniloegea marked this pull request as ready for review August 9, 2023 11:06

daniloegea requested a review from slyon August 9, 2023 12:49

slyon reviewed Aug 10, 2023

View reviewed changes

slyon mentioned this pull request Aug 16, 2023

Expose macaddress and DNS configuration from the netdef #395

Merged

5 tasks

daniloegea force-pushed the netplan_diff_round2 branch from 7fe90f8 to c956af8 Compare August 17, 2023 11:17

daniloegea commented Aug 17, 2023

View reviewed changes

python-cffi/netplan/netdef.py Outdated Show resolved Hide resolved

daniloegea force-pushed the netplan_diff_round2 branch from c956af8 to 4c6d7a8 Compare August 17, 2023 11:32

slyon reviewed Aug 17, 2023

View reviewed changes

daniloegea mentioned this pull request Aug 17, 2023

Netplan state refactoring #399

Merged

5 tasks

daniloegea force-pushed the netplan_diff_round2 branch 2 times, most recently from e6c31fc to 7490e8f Compare August 17, 2023 18:19

daniloegea force-pushed the netplan_diff_round2 branch from 011f078 to 2c785e3 Compare October 27, 2023 17:12

daniloegea requested a review from slyon October 27, 2023 17:13

daniloegea force-pushed the netplan_diff_round2 branch from 2c785e3 to fd8f80a Compare October 31, 2023 12:09

daniloegea force-pushed the netplan_diff_round2 branch 8 times, most recently from 045b336 to a24d267 Compare November 16, 2023 10:47

netdef: simplify NetplanRoute __eq__ and __hash__

61316b5

There are some fields that exist in Netplan but not in the system and vice-versa, such as on-link and protocol. Using the default dataclass methods makes it harder to compare objects.

daniloegea force-pushed the netplan_diff_round2 branch from a24d267 to 669e1cb Compare November 27, 2023 09:38

slyon approved these changes Nov 29, 2023

View reviewed changes

netplan_cli/cli/state_diff.py Outdated Show resolved Hide resolved

daniloegea force-pushed the netplan_diff_round2 branch 2 times, most recently from 65da003 to 6418296 Compare November 29, 2023 12:10

daniloegea and others added 11 commits November 29, 2023 12:37

state_diff: add the get_diff() method

68e998e

The get_diff() will calculate the differences between system's and Netplan's states and return a report to the caller. This commit adds support for checking what interfaces are present only in one of the states.

state_diff: add support for IP addresses analysis

336905a

get_diff() will compare and report differences in the addresses present in the system and Netplan. Addresses detected as dynamically assigned will not be reported as missing as they will not be defined in Netplan.

state_diff: add support for nameservers analysis

7a9ea67

state_diff: add support for search domains analysis

5605817

state_diff: add support for MAC addresses analysis

ea58943

state_diff: add support for routes analysis

2969a3f

A number of heuristics are implemented to eliminate routes we don't want to compare, such as routes installed dynamically, as they will not be found in Netplan.

state_diff: add a JSON encoder for NetplanRoutes

65a1921

It will be used to serialize the state and diff.

state_diff: add more information to the missing_interfaces result

df64595

For netplan interfaces add the name and type. For system interfaces add the name, type and index.

state_diff/get_diff(): return the interfaces sorted by their indices

59d8a19

tools/diff.py: add a temporary tool to test netplan diff

5fddf54

Also add a few integrations tests that use tools/diff.py to test the diff against a running system configured with Netplan.

daniloegea force-pushed the netplan_diff_round2 branch from 6418296 to 5fddf54 Compare November 29, 2023 12:44

slyon merged commit a3b9cff into canonical:main Nov 29, 2023
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Netplan state diff #386

Netplan state diff #386

daniloegea commented Jul 28, 2023 •

edited by slyon

Loading

slyon commented Jul 31, 2023

slyon left a comment

slyon Aug 10, 2023

slyon left a comment

daniloegea commented Oct 27, 2023

slyon left a comment

Netplan state diff #386

Netplan state diff #386

Conversation

daniloegea commented Jul 28, 2023 • edited by slyon Loading

Description

Experimental user interface

Checklist

slyon commented Jul 31, 2023

slyon left a comment

Choose a reason for hiding this comment

slyon Aug 10, 2023

Choose a reason for hiding this comment

slyon left a comment

Choose a reason for hiding this comment

daniloegea commented Oct 27, 2023

slyon left a comment

Choose a reason for hiding this comment

daniloegea commented Jul 28, 2023 •

edited by slyon

Loading