nm: Refactor NmActiveConnection and NmConnection search code #2903
Conversation
|
Skipping CI for Draft Pull Request. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
52fd5ae to
e3faed6
Compare
| // Using &str for UUID or name will complex the code due to ownership, | ||
| // Considering this duplication on UUID and name only created once at | ||
| // `Self::new()` stage, it is acceptable cost. | ||
| // | ||
| // Using NmIfaceType instead of &NmIfaceType in order to matching veth | ||
| // interface against ethernet NmConnection, we need to duplicate NmConnection | ||
| // both to Veth and Ethernet key. | ||
| // | ||
| // Using String instead of &str for interface name indexing because VLAN | ||
| // connection can have auto-generated `connection.interface-name` when it is | ||
| // empty. |
There was a problem hiding this comment.
I would remove this comment, too low level detail for such a long comment.
There was a problem hiding this comment.
I wrote it in case you ask me why store cloned String instead of &str.
| // UUID to NmActiveConnection | ||
| nm_acs: HashMap<String, Rc<NmActiveConnection>>, | ||
| // (name, nm_iface_type) to Rc<NmActiveConnection> | ||
| // Veth will also stored into `NmIfaceType::Ethernet` | ||
| nm_acs_index: HashMap<(String, NmIfaceType), Rc<NmActiveConnection>>, | ||
|
|
||
| // (name, nm_iface_type) to Rc<NmConnection> | ||
| // Veth will also stored into `NmIfaceType::Ethernet` | ||
| nm_applied_conns_index: HashMap<(String, NmIfaceType), Rc<NmConnection>>, | ||
|
|
||
| // (name, nm_iface_type) to Rc<NmConnection> | ||
| // Veth will also stored into `NmIfaceType::Ethernet` | ||
| nm_saved_conns_index: HashMap<(String, NmIfaceType), Vec<Rc<NmConnection>>>, | ||
|
|
||
| // (mac_upper_case, nm_iface_type) to Vec<Rc<NmConnection>> for mac | ||
| // identifier Veth will also stored into `NmIfaceType::Ethernet` | ||
| nm_saved_conns_mac: HashMap<(String, NmIfaceType), Vec<Rc<NmConnection>>>, |
There was a problem hiding this comment.
Instead of these comments, better to explain the content in the field name itself. Example: active_conns_by_uuid instead of nm_acs, applied_cons_by_iface instead of nm_applied_conns_index, etc.
| // For all applied NmConnection, it should have existing | ||
| // network interface defined, hence we ignore unexpected | ||
| // NmConnection | ||
| log::debug!( | ||
| "Ignoring applied NmConnection which cannot resolve its \ | ||
| network interface {nm_conn:?}" | ||
| ); |
There was a problem hiding this comment.
If it's something that must not happen, better to return an error or panic via assertion.
There was a problem hiding this comment.
User might have existing NmConnection pointing to deleted NIC, it is valid environment, we should not raise any error or warning on this. Treat is debug log can be collected when failure happens.
| .and_modify(|nm_conns| nm_conns.push(nm_conn.clone())) | ||
| .or_insert(vec![nm_conn.clone()]); | ||
| } | ||
| if nm_conn.iface_name().unwrap_or_default().is_empty() { |
There was a problem hiding this comment.
Do we want to store by MAC only if interface name is empty? Doesn't make sense to store it anyway?
There was a problem hiding this comment.
When user have disconnected NIC, nmstate should support deleting its mac-identifier NmConnection.
| self.nm_saved_conns_index | ||
| .entry((name.to_string(), NmIfaceType::Ethernet)) | ||
| .and_modify(|nm_conns| nm_conns.push(nm_conn.clone())) | ||
| .or_insert(vec![nm_conn.clone()]); |
There was a problem hiding this comment.
nit: use or_insert_with to avoid eagerly evaluation of the argument. With or_insert you will always allocate a Vec and clone the Rc.
Alternatively, I think that .entry(...).or_insert_with(|| Vec::new()).push(nm_conn.clone()) would work too.
Same for the other 3 times in the function.
rust/src/lib/nm/profile_search.rs
Outdated
| self.nm_acs.contains_key(uuid) | ||
| } | ||
|
|
||
| pub(crate) fn get_applied_by_name_type( |
There was a problem hiding this comment.
Suggestion: get_applied_by_iface
There was a problem hiding this comment.
We already have get_applied() which takes BaseInterface, so this by_iface might be not fit here.
rust/src/lib/nm/profile_search.rs
Outdated
| if let Some(mac) = nm_conn | ||
| .wired | ||
| .as_ref() | ||
| .and_then(|s| s.mac_address.as_deref()) | ||
| { | ||
| if let Some(name) = cur_state.interfaces.iter().find_map(|iface| { | ||
| if iface.base_iface().mac_address.as_deref() == Some(mac) { | ||
| Some(iface.name().to_string()) | ||
| } else { | ||
| None | ||
| } | ||
| }) { | ||
| return Some(name); | ||
| } | ||
| } | ||
| None |
There was a problem hiding this comment.
For VLAN, MACVLAN and MACSEC, ethernet.mac-address is the MAC of the parent, not of the interface itself.
rust/src/lib/nm/profile_search.rs
Outdated
| cur_state: NetworkState, | ||
| } | ||
|
|
||
| impl ProfileSearch { |
There was a problem hiding this comment.
Suggestion: rename to NmConnectionMatcher.
|
Please try to split in smaller logical commits for the final review. |
54e4ad2 to
cdf8867
Compare
I will split them into small parts once CI are all happy. Still coding. |
Previously, the `nm_dbus` was designed to be standalone crate without dependency on nmstate, so `iface_type_to_nm()` is used to convert from `InterfaceType` to `NmIfaceType`. But we don't have plan to release `nm_dbus` crate anymore, so we should use From trait instead of function for conversion between `InterfaceType` and `NmIfaceType`. Signed-off-by: Gris Ge <fge@redhat.com>
Introducing `NmSettingConnection.autoconnect_priority`, it holds `i32` for priority when multiple NmConnection is capable for activating the same NmDevice, the bigger number is better. This property will be use for choosing the correct NmConnection to modify when `nmstatectl apply` with multiple connections in NM. Signed-off-by: Gris Ge <fge@redhat.com>
Introducing `NmSettingConnection.timestamp` holding the last successful activation time since EPOCH in seconds. This property will be used for nmstate to choose the correct NmConnection to modify when multiple NmConnection founds. Signed-off-by: Gris Ge <fge@redhat.com>
49f63cb to
eedef40
Compare
Creating struct `NmConnectionMatcher` to unify the effort on searching NmConnection for querying and modification. The `NmConnectionMatcher` will create indexes at initialization to speed up follow up queries. Signed-off-by: Gris Ge <fge@redhat.com>
Use `NmConnectionMatcher` for matching NmDevice with NmConnection during `nmstatectl show`. Signed-off-by: Gris Ge <fge@redhat.com>
Use `NmConnectionMatcher` to find the correct NmConnection to modify/delete. Resolves: https://issues.redhat.com/browse/RHEL-92916 Resolves: https://issues.redhat.com/browse/RHEL-86240 Signed-off-by: Gris Ge <fge@redhat.com>
Support handling HSR interface type when generating `BaseInterface` from `NmDevice`. This was not a problem because old patch ws using `NmConnection` to build up `BaseInterface`. When we refactor the NmConnection search code, we unified the effort by using NmDevice only for querying. Existing HSR test cases are enough for testing this patch. Signed-off-by: Gris Ge <fge@redhat.com>
With a dummy NM connection created by deactivated, applying desire state
like:
```
interfaces:
- name: dummy1
state: absent
```
will not delete the NM connection.
This is because `NmConnectionMatcher::get_saved()` is searching for
NmConnection with `NmIfaceType::Unknown`.
To fix that issue, when `get_saved()` got unknown interface, all
`NmConnection` matching specified name will be returned.
The integration test case `test_state_absent_can_remove_down_profiles`
can reproduce the issue and confirm fix.
Signed-off-by: Gris Ge <fge@redhat.com>
When modifying existing NmConnection with desired state: ``` --- interfaces: - name: port1 type: ethernet mac-address: 00:23:45:67:89:1a identifier: mac-address ``` The `NmConnectionMatcher::get_prefered_saved()` will return empty None because name in this case is `port1` instead of the actual NIC name. Fixed by rename existing `NmConnectionMatcher::get_prefered_saved()` to `get_prefered_saved_by_name_type()` and the new `NmConnectionMatcher::get_prefered_saved()` is accepting `BaseInterface`. When got `InterfaceIdentifier::MacAddress`, we search the MAC address and interface type instead of its profile name. Signed-off-by: Gris Ge <fge@redhat.com>
Previously we are depending `NmApi::connection_reapply()` to find the correct NmDevice to reapply. But `test_reapply_bond_port_ref_by_mac` test case sometimes noticed it choose the wrong NmDevice which lead to reapply failure. The root cause is bond ports are holding the same MAC address after attached to a bond. If we search NmDevice by interface type and MAC, we might get the incorrect one. To fix this issue: * Expanded `NmActiveConnection` to hold NmDevice object path. * Changed `activate_nm_profiles()` will use `NmConnectionMatcher` which holds pre-apply NmActiveConnection. * Changed `NmApi::connection_reapply()` to take object path of `NmDevice` explicitly instead of guessing. Existing test case `test_reapply_bond_port_ref_by_mac` is enough to test this patch. Signed-off-by: Gris Ge <fge@redhat.com>
Given a interface is activated by multiconnect NmConnection, when applying state like ``` interfaces: - name: eth1 state: up ``` , nmstate will modify the multiconnect interface which is not expected. The fix is do not return multiconnect NmConnection when `iface.is_up_exist_config()`. This is work well before `NmConnectionMatcher` is because: * Old code does not prefer activated `NmConnection` as saved profile but only searching name and interface type * No `NmConnection` been found for `iface.is_up_exist_config()`. * Nmstate using current state to create new `NmConnection`. Existing test case `test_preserve_ip_settings_from_multiconnect` is enough to test this case. Signed-off-by: Gris Ge <fge@redhat.com>
|
PR is ready for review. |
|
@cathay4t: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Introducing
NmConnectionMatcherto unify the effort on find the matchingNmActiveConnectionandNmConnection.Resolves: https://issues.redhat.com/browse/RHEL-92916
Resolves: https://issues.redhat.com/browse/RHEL-86240