Conversation
daniloegea
force-pushed
the
netplan_status_diff_fixes
branch
2 times, most recently
from
0a3daeb to
89ef0da
Compare
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
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,
}
]
89ef0da to
2878398
Compare
|
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 |
There was a problem hiding this comment.
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_strcan returnNoneor the empty string- Should we drop the
MISSINGstate, to unclutter the output? - Should we display unknown interfaces types as
otherinstead ofunknownto stay in line withstatus.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.
It will return the value of netdef->vrf_link. It also implements the Python binding and add it to the netdef.links property.
Analyze if interfaces are correctly attached to their parents, such as if an ethernet is really part of a bridge/bond/vrf, and if a bridge/bond/vrf has all the members it should have.
If an interface was removed from the Netplan state and the configuration wasn't applied yet, the interfaces will have a netdef_id. Check if the interface doesn't have a netplan_state and skip it so we will not show a diff for interfaces that were deleted from Netplan.
When the loopback interface is managed with Netplan, filter out the following route from the list: local 127.0.0.0/8 dev lo table local proto kernel scope host src 127.0.0.1
…ables It will be used to convert route table numbers to known names.
To avoid "Unknown device type: ..." warning logs.
That is to stay consistent with what we have in cli/commands/status.py:_display_interface_header()
netplan status --diff will use the state_diff module to output inconsistencies in the network configuration. status: split display in multiple methods status: refactoring: fix typo and drop unused code status: refactor _display_routes Drop unused code and make use of the new route table name lookup. tests: add a test file for netplan status --diff tests: add some shallow cli tests All the output tests are in test_status_diff. Adding some very shallow cli tests for coverage sake. status/diff: use the unspecified address to for missing DHCP addresses Instead of "Missing DHCP address" it will show "0.0.0.0/0 (dhcp)" for IPv4 and "::/0 (dhcp)" for IPv6. status/diff: make it work with a single targeted interface netplan status --diff <interface> will only display the configuration for a single interface. status/diff: mute field names as well when there is no diff status/diff: make json/yaml output work with a single targeted interface netplan status --diff -f json <interface> will only display the configuration for a single interface. status: refactoring Move common code to a single place and drop duplication. Don't display a linebreak in the end of the output. Some minor code style changes. cli:status: _get_missing_property_str to return '' in failure case cli:status: avoid showing a MISSING state
daniloegea
force-pushed
the
netplan_status_diff_fixes
branch
from
a32825f to
7556768
Compare
|
Hi Lukas, thanks for the review. I incorporated all of your commits, except for the change in I squashed all the commits related to the status command in a single commit but kept the history in the commit log. |
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thanks a lot. That works for me!
2 participants
Description
This PR introduces
netplan status --diff. It will use thestate_diffmodule to output the inconsistencies in the network configuration between the system and the netplan configuration.The diff will be displayed the same way
netplan statusalready displays information. Even though it's a nice approach, plugging thediffdisplay code in the existingstatuscode 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
make checksuccessfully.make check-coverage).