New submodule for state manipulation#379
Conversation
|
+1 I very much like the approach. Having all state manipulation in a common (private) Python module should help to ease maintenance, going forward. I also like the fact that you're refactoring before implementing new functionality! Thanks. |
daniloegea
force-pushed
the
netplan_diff_round1
branch
from
0a89a85 to
ff55faa
Compare
|
The DebCI failure seems unrelated. |
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thank you very much! As stated above, I like the refactoring a lot. Having all data handling inside a central module is very useful. Then, using very simple code inside the views (get.py / status.py) to access this data (read only) and display it in the relevant format.
I've put a bunch of inline comments, two of which are especially important:
class NetdefInterfacevs configmanager.py{Netplan,System}ConfigState[Data]class harmonization
daniloegea
force-pushed
the
netplan_diff_round1
branch
3 times, most recently
from
a52fc7e to
42487af
Compare
|
Thanks, Lukas. I tried to address all your comments. I got rid of the *ConfigStateData and NetdefInterface classes. I also add an interface to iterate over the IP addresses present in a netdef. The idea is to add all the interfaces that will be used by netplan diff. With this there is no need for a new class to store this information. |
|
Not sure why tests are failing on Fedora... it works locally... |
There was a problem hiding this comment.
Thank you very much, Danilo! This is looking very clean now.
I put some (non-blocking) nitpicks as inline comments. But as this is all still internal code (not exported through any API or binding), we can still change the names or implementation details in the future, if you want to merge it as-is.
We should investigate the RHEL/Rocky and Fedora tests failures, though. They seem to pass in the main branch, so are probably related to the changes in this PR.
Edit: The RPM failures are potentially related to RPM's make check call not using the proper libnetplan.so (maybe we need some LD_LIBRARY_PATH quirks, so it can pick up the new iterator)
state.py is responsible for collecting and storing the network configuration. It will also be responsible for detecting differences between the system's and netplan configuration in the future. The main motivation for this new submodule is the centralization of state management. The code used by "netplan status" to read and aggregate the system's network configuration and the code used by "netplan get" to read the Netplan's configuration was moved to this module.
Make use of the new module to read the configuration used by "netplan get" and "netplan status".
daniloegea
force-pushed
the
netplan_diff_round1
branch
2 times, most recently
from
cbdb64d to
31ded93
Compare
|
I changed the iterator implementation to return all the IPs from |
It enables us to retrieve the list of IPs, (V4 and V6) from a given netdef via Python bindings. The same iterator will consume the ip4_addresses, ip6_addresses and address_options (where IPs with options are stored). Each item returned by the _next() function is a NetplanAddressOptions*, even for addresses without options. Each object is released when the next one is requested.
daniloegea
force-pushed
the
netplan_diff_round1
branch
from
31ded93 to
338782b
Compare
|
I changed the iterator implementation to free objects as new ones are produced. Now the caller doesn't need to do that, destroying the iterator should be enough to release all the memory. |
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thank you, lgtm!
praise: I like how you implemented the AddressIterator to automatically free the memory it doesn't need anymore, making it transparent to the (Python) caller.
Two tiny suggestions inline, but feel free to merge as-is if you feel like.
2 participants
Description
It moves the code that collects and aggregates system's and netplan's network configuration to a submodule called
state. It's intended to be used by netplan commands that requires configuration manipulation such asnetplan statusandnetplan get.The motivation for that is to add the logic for
netplan status --diff(ornetplan diff) in the same submodule in the near future. So the manipulation of the state will be all in the same place.It's organized in 2 classes:
SystemConfigState: responsible for collecting and storing the current system networking configuration. It's currently used bynetplan status. The code is essentially the same fromnetplan status.NetplanConfigState: responsible for collecting and storing the current Netplan configuration. It's currently used bynetplan get.It also adds an iterator that returns the IP addresses present in a netdef. The idea is to add more getters for other properties that will be used by netplan diff.
Checklist
make checksuccessfully.make check-coverage).