nm: Fix error when modify port list of down interface

rust/src/lib/nm/connection.rs

@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
+use std::collections::HashSet;
+
 use super::{
     NmConnectionMatcher,
     device::create_index_for_nm_devs,
@@ -16,6 +18,7 @@ pub(crate) struct PreparedNmConnections {
     pub(crate) to_store: Vec<NmConnection>,
     pub(crate) to_activate: Vec<NmConnection>,
     pub(crate) to_deactivate: Vec<NmDevice>,
+    pub(crate) to_delete: Vec<String>,
 }
 
 pub(crate) fn prepare_nm_conns(
@@ -137,11 +140,24 @@ pub(crate) fn prepare_nm_conns(
 
     fix_ip_dhcp_timeout(&mut nm_conns_to_update);
 
-    Ok(PreparedNmConnections {
+    let mut ret = PreparedNmConnections {
         to_store: nm_conns_to_update,
         to_activate: nm_conns_to_activate,
         to_deactivate: nm_devs_to_deactivate,
-    })
+        to_delete: Vec::new(),
+    };
+
+    // When port list is desired on controller interface, we need to update
+    // saved NmConnection to prevent undesired port attached by
+    // `auto-connect-ports: true`. Hence we use this Vec to track
+    // port list desired ifaces.
+    detach_saved_nm_conn_from_controller(
+        &mut ret,
+        &merged_state.interfaces,
+        conn_matcher,
+    );
+
+    Ok(ret)
 }
 
 // When a new virtual interface is desired, if its controller is also newly
@@ -212,3 +228,147 @@ fn can_skip_activation(
     }
     false
 }
+
+// If a controller has port list desired, detach saved inactive NmConnection
+// from this controller
+fn detach_saved_nm_conn_from_controller(
+    prepared_conns: &mut PreparedNmConnections,
+    merged_ifaces: &MergedInterfaces,
+    conn_matcher: &NmConnectionMatcher,
+) {
+    // Only need to check inactive NmConnection because active NmConnection
+    // is already processed by `MergedInterfaces::handle_changed_ports()`.
+    let saved_inactive_port_nm_conn: Vec<&NmConnection> = conn_matcher
+        .saved_iter()
+        .filter(|nm_conn| {
+            if let Some(uuid) = nm_conn.uuid() {
+                !conn_matcher.is_uuid_activated(uuid)
+                    && !is_prepared_to_store(prepared_conns, uuid)
+                    && nm_conn.controller().is_some()
+            } else {
+                false
+            }
+        })
+        .collect();
+
+    if saved_inactive_port_nm_conn.is_empty() {
+        return;
+    }
+
+    let mut changed_ctrl_names: HashSet<(&str, NmIfaceType)> = HashSet::new();
+    let mut changed_ctrl_uuids: HashSet<(&str, NmIfaceType)> = HashSet::new();
+
+    for merged_iface in merged_ifaces.iter().filter(|merged_iface| {
+        is_ctrl_iface_desired_port_changes(merged_iface)
+            && merged_iface.for_apply.as_ref().map(|i| i.is_up()) == Some(true)
+    }) {
+        let ctrl_iface_name = merged_iface.merged.name();
+        let ctrl_iface_type = &merged_iface.merged.base_iface().iface_type;
+        let ctrl_nm_iface_type = NmIfaceType::from(ctrl_iface_type);
+        if let Some(ctrl_nm_conn) = prepared_conns
+            .to_activate
+            .as_slice()
+            .iter()
+            .find(|nm_conn| {
+                nm_conn.iface_name() == Some(ctrl_iface_name)
+                    && nm_conn.iface_type() == Some(&ctrl_nm_iface_type)
+            })
+            && let Some(uuid) = ctrl_nm_conn.uuid()
+        {
+            changed_ctrl_names.insert((ctrl_iface_name, ctrl_nm_iface_type));
+            changed_ctrl_uuids.insert((uuid, ctrl_nm_iface_type));
+        }
+    }
+
+    let mut orphan_ovs_port_uuids: HashSet<&str> = HashSet::new();
+    let mut orphan_ovs_port_names: HashSet<&str> = HashSet::new();
+
+    for nm_conn in saved_inactive_port_nm_conn.as_slice() {
+        if let Some(ctrl_name) = nm_conn.controller()
+            && let Some(nm_ctrl_type) = nm_conn.controller_type()
+            && ((is_uuid(ctrl_name)
+                && changed_ctrl_uuids.contains(&(ctrl_name, *nm_ctrl_type)))
+                || changed_ctrl_names.contains(&(ctrl_name, *nm_ctrl_type)))
+        {
+            if nm_conn.iface_type() == Some(&NmIfaceType::OvsPort) {
+                if let Some(uuid) = nm_conn.uuid() {
+                    log::debug!("Deleting orphan OVS port {uuid}");
+                    log::info!(
+                        "Deleting connection {uuid}: {}/ovs-port",
+                        nm_conn.iface_name().unwrap_or_default(),
+                    );
+                    prepared_conns.to_delete.push(uuid.to_string());
+                    orphan_ovs_port_uuids.insert(uuid);
+                    if let Some(ovs_port_iface_name) = nm_conn.iface_name() {
+                        orphan_ovs_port_names.insert(ovs_port_iface_name);
+                    }
+                }
+            } else {
+                let mut changed_nm_conn = (*nm_conn).clone();
+                if let Some(nm_conn_set) = changed_nm_conn.connection.as_mut() {
+                    log::debug!(
+                        "Detaching inactive controller port {}: {}/{}",
+                        nm_conn.uuid().unwrap_or_default(),
+                        nm_conn.iface_name().unwrap_or_default(),
+                        nm_conn.iface_type().unwrap_or(&NmIfaceType::Unknown)
+                    );
+                    nm_conn_set.controller = None;
+                    nm_conn_set.controller_type = None;
+                    changed_nm_conn.bond_port = None;
+                    changed_nm_conn.bridge_port = None;
+                    prepared_conns.to_store.push(changed_nm_conn);
+                }
+            }
+        }
+    }
+
+    // Detach OVS system and internal interface which refer to orphan OVS ports
+    for nm_conn in saved_inactive_port_nm_conn {
+        if nm_conn.controller_type() == Some(&NmIfaceType::OvsPort)
+            && let Some(ctrl_name) = nm_conn.controller()
+            && (is_uuid(ctrl_name)
+                && orphan_ovs_port_uuids.contains(&ctrl_name)
+                || orphan_ovs_port_names.contains(&ctrl_name))
+        {
+            // OVS internal interface cannot live without its parent
+            if nm_conn.iface_type() == Some(&NmIfaceType::OvsIface) {
+                if let Some(uuid) = nm_conn.uuid() {
+                    prepared_conns.to_delete.push(uuid.to_string());
+                }
+            } else {
+                let mut changed_nm_conn = nm_conn.clone();
+                if let Some(nm_conn_set) = changed_nm_conn.connection.as_mut() {
+                    log::debug!(
+                        "Detaching inactive OVS interface {}: {}/{}",
+                        nm_conn.uuid().unwrap_or_default(),
+                        nm_conn.iface_name().unwrap_or_default(),
+                        nm_conn.iface_type().unwrap_or(&NmIfaceType::Unknown)
+                    );
+                    nm_conn_set.controller = None;
+                    nm_conn_set.controller_type = None;
+                    changed_nm_conn.ovs_iface = None;
+                    prepared_conns.to_store.push(changed_nm_conn);
+                }
+            }
+        }
+    }
+}
+
+fn is_prepared_to_store(
+    prepared_conns: &PreparedNmConnections,
+    uuid: &str,
+) -> bool {
+    prepared_conns
+        .to_store
+        .as_slice()
+        .iter()
+        .any(|nm_conn| nm_conn.uuid() == Some(uuid))
+}
+
+fn is_ctrl_iface_desired_port_changes(merged_iface: &MergedInterface) -> bool {
+    merged_iface.for_apply.as_ref().map(|i| i.ports().is_some()) == Some(true)
+}
+
+pub(crate) fn is_uuid(value: &str) -> bool {
+    uuid::Uuid::parse_str(value).is_ok()
+}

rust/src/lib/nm/query_apply/apply.rs

@@ -4,18 +4,18 @@ use std::collections::HashSet;
 
 use super::super::{
     NmConnectionMatcher,
-    connection::{PreparedNmConnections, prepare_nm_conns},
+    connection::{PreparedNmConnections, is_uuid, prepare_nm_conns},
     device::create_index_for_nm_devs,
     error::nm_error_to_nmstate,
     nm_dbus::{NmApi, NmConnection, NmIfaceType, NmVersion, NmVersionInfo},
     query_apply::{
-        activate_nm_connections, connection::is_uuid,
-        deactivate_nm_connections, deactivate_nm_devices,
-        delete_exist_connections, delete_orphan_ovs_ports,
-        dispatch::apply_dispatch_script, dns::store_dns_config, is_hsr_changed,
-        is_ip_tunnel_changed, is_ipvlan_changed, is_mptcp_flags_changed,
-        is_route_removed, is_veth_peer_changed, is_vlan_changed,
-        is_vrf_table_id_changed, is_vxlan_changed, save_nm_connections,
+        activate_nm_connections, deactivate_nm_connections,
+        deactivate_nm_devices, delete_exist_connections,
+        delete_orphan_ovs_ports, dispatch::apply_dispatch_script,
+        dns::store_dns_config, is_hsr_changed, is_ip_tunnel_changed,
+        is_ipvlan_changed, is_mptcp_flags_changed, is_route_removed,
+        is_veth_peer_changed, is_vlan_changed, is_vrf_table_id_changed,
+        is_vxlan_changed, save_nm_connections,
     },
     route::store_route_config,
     route_rule::store_route_rule_config,
@@ -106,6 +106,7 @@ pub(crate) async fn nm_apply(
         to_store: nm_conns_to_store,
         to_activate: nm_conns_to_activate,
         to_deactivate: nm_devs_to_deactivate,
+        to_delete: nm_conns_to_delete,
     } = prepare_nm_conns(
         &merged_state,
         &conn_matcher,
@@ -115,6 +116,13 @@ pub(crate) async fn nm_apply(
         ipv4_forward_support,
     )?;
 
+    for uuid in &nm_conns_to_delete {
+        nm_api
+            .connection_delete(uuid)
+            .await
+            .map_err(nm_error_to_nmstate)?;
+    }
+
     let nm_conns_to_deactivate_first = gen_nm_conn_need_to_deactivate_first(
         &merged_state.interfaces,
         nm_conns_to_activate.as_slice(),

rust/src/lib/nm/query_apply/connection.rs

@@ -312,7 +312,3 @@ async fn reapply_or_activate(
 
     Ok(())
 }
-
-pub(crate) fn is_uuid(value: &str) -> bool {
-    uuid::Uuid::parse_str(value).is_ok()
-}

rust/src/lib/nm/query_apply/ovs.rs

@@ -5,10 +5,11 @@ use std::collections::HashSet;
 use super::{
     super::{
         NmConnectionMatcher,
+        connection::is_uuid,
         nm_dbus::{NmApi, NmConnection, NmIfaceType},
         show::fill_iface_by_nm_conn_data,
     },
-    connection::{delete_connections, is_uuid},
+    connection::delete_connections,
 };
 use crate::{
     InterfaceType, MergedInterface, MergedInterfaces, NetworkState,

tests/integration/bond_test.py

@@ -29,6 +29,7 @@
 from .testlib.bridgelib import add_port_to_bridge
 from .testlib.bridgelib import create_bridge_subtree_state
 from .testlib.bridgelib import linux_bridge
+from .testlib.dummy import dummy_interface
 from .testlib.env import is_k8s
 from .testlib.ifacelib import get_mac_address
 from .testlib.ifacelib import ifaces_init
@@ -1775,3 +1776,49 @@ def test_attach_new_vlan_of_new_bond_to_exist_bridge(empty_br0, cleanup_vlan):
         )
         libnmstate.apply(state)
         assertlib.assert_state_match(state)
+
+
+@pytest.fixture
+def bond99_down_with_2_ports_in_config(bond99_with_2_port):
+    libnmstate.apply(
+        yaml.load(
+            """
+            interfaces:
+              - name: bond99
+                type: bond
+                state: down
+            """,
+            Loader=yaml.SafeLoader,
+        )
+    )
+    yield
+
+
+@pytest.fixture
+def dummy1_up():
+    with dummy_interface("dummy1"):
+        yield
+
+
+def test_changed_port_list_of_down_bond(
+    bond99_down_with_2_ports_in_config, dummy1_up
+):
+    libnmstate.apply(
+        yaml.load(
+            """
+            interfaces:
+            - name: bond99
+              type: bond
+              state: up
+              link-aggregation:
+                mode: balance-rr
+                port:
+                  - dummy1
+            """,
+            Loader=yaml.SafeLoader,
+        )
+    )
+
+    bond_iface = statelib.show_only(("bond99",))[Interface.KEY][0]
+
+    assert bond_iface[Bond.CONFIG_SUBTREE][Bond.PORT] == ["dummy1"]

tests/integration/linux_bridge_test.py

@@ -32,14 +32,14 @@
 from .testlib.bridgelib import generate_vlan_id_range_config
 from .testlib.bridgelib import linux_bridge
 from .testlib.cmdlib import exec_cmd
+from .testlib.dummy import dummy_interface
 from .testlib.ifacelib import get_mac_address
 from .testlib.iproutelib import ip_monitor_assert_stable_link_up
 from .testlib.retry import retry_till_true_or_timeout
 from .testlib.statelib import show_only
 from .testlib.vlan import vlan_interface
 from .testlib.yaml import load_yaml
 
-
 TEST_BRIDGE0 = "linux-br0"
 TEST_TAP0 = "test-tap0"
 TEST_BOND0 = "test-bond0"
@@ -1382,3 +1382,47 @@ def test_change_mtu_with_stable_link_up(bridge0_with_port0):
     )
 
     assertlib.assert_state(desired_state)
+
+
+@pytest.fixture
+def down_bridge0(bridge0_with_port0):
+    libnmstate.apply(
+        load_yaml(
+            f"""---
+        interfaces:
+        - name: {TEST_BRIDGE0}
+          type: linux-bridge
+          state: down"""
+        )
+    )
+    yield
+
+
+@pytest.fixture
+def dummy1_up():
+    with dummy_interface("dummy1"):
+        yield
+
+
+def test_changed_port_list_of_down_linux_bridge(down_bridge0, dummy1_up):
+    libnmstate.apply(
+        load_yaml(
+            f"""---
+      interfaces:
+        - name: {TEST_BRIDGE0}
+          type: linux-bridge
+          state: up
+          bridge:
+            options:
+              stp:
+                enabled: false
+            port:
+              - name: dummy1"""
+        )
+    )
+
+    current_state = show_only([TEST_BRIDGE0])
+    br_iface = current_state[Interface.KEY][0]
+    br_ports = br_iface[LinuxBridge.CONFIG_SUBTREE][LinuxBridge.PORT_SUBTREE]
+    assert len(br_ports) == 1
+    assert br_ports[0][LinuxBridge.Port.NAME] == "dummy1"