Add bonding support #885

teclator · 2023-11-22T21:05:47Z

This PR adds support for configuring a Bond connection using Agama auto-installation.

The model

Initially, we thought each connection struct would take ownership of its ports (basically, other connections). However, after some discussion with the networking team, we agreed to keep a "reference" in the opposite direction (from the port to the controller). At this point, each connection that belongs to a bond keeps its parent UUID.

Configuring a port

When using the auto-installation you can define the port with something like this:

      {
        id: 'bond0',
        bond: {
            ports: ['eth0', 'eth1'],
            mode: 'active-backup',
            options: "primary=eth1"

        }
      },
      {
        id: 'Wired Adapter 1',
        interface: 'eth1'
      }

In NetworkManager, it is mandatory to specify an interface name. However, Agama will use the id if you do not define the interface. About the ports, it is not required to define them (in the example above, eth0 is missing). Agama will create a connection automatically. And, again, you can use the id or the interface to link both connections.

Improving the D-Bus interface

As the list of ports is not part of the BondConnection struct, it is not straightforward to expose that information in the D-Bus interface. After all, the code that exposes each connection over D-Bus only had access to the information of its specific connection (actually, it owns a clone of the connection).

For that reason, starting with the Bond interface, we implemented a better mechanism (based on message passing) to interact with the data model. We will open a separate PR to adapt the rest of interfaces (with more information about the problems it solves).

What's next?

Add vlan model #918
Changing the network model #924
Fix a ton of issues detected by Clippy.

References

@cfconrad started to work on bonding support on Add bond model #783.

Testing

Tested manually (see HackWeek Project: Learn to develop some Rust terminal UI for Agama teclator/agama#1).
Added some unit and integration tests.

coveralls · 2023-11-22T21:09:53Z

coverage: 74.655% (-0.4%) from 75.051%
when pulling 9edb940 on add_bonding_support
into c374ec9 on master.

imobachgs

Thanks! It is a lot of work.

I have not reviewed the code 100%, but it goes in the right direction. I have some suggestions (do not fear combinators 😉) but we can discuss them in a pair programming session.

rust/agama-dbus-server/src/network/model.rs

rust/agama-dbus-server/src/network/nm/client.rs

imobachgs · 2023-11-23T19:57:02Z

rust/agama-dbus-server/src/network/system.rs

+            Action::UpdateControllerConnection(conn, settings, tx) => {
+                let id = &conn.clone();
+                let id = id.id();
+                self.state.update_controller_connection(conn, settings)?;


This method could return a reference to the updated connection.

rust/agama-dbus-server/src/network/system.rs

imobachgs · 2023-11-24T09:36:01Z

rust/agama-dbus-server/src/network/model.rs

+    /// Additionally, it registers the connection to be removed when the changes are applied.
+    pub fn update_controller_connection(
+        &mut self,
+        mut conn: Connection,


missing docs

rust/agama-dbus-server/src/network/model.rs

cfconrad · 2023-11-28T13:17:20Z

rust/agama-dbus-server/src/network/model.rs

+        let opts = &self
+            .0
+            .iter()
+            .map(|(key, value)| format!("{key}={value}"))
+            .collect::<Vec<_>>();
+
+        write!(f, "{}", opts.join(" "))
+    }


Not enough rusty?

Suggested change

let opts = &self

.0

.iter()

.map(|(key, value)| format!("{key}={value}"))

.collect::<Vec<_>>();

write!(f, "{}", opts.join(" "))

}

let mut spacer = "";

for (key, value) in &self.0 {

write!(f, "{spacer}{key}={value}")?;

spacer = " ";

}

Ok(())

Well, I would say that it is more a Ruby-like approach :-) Both of them look expressive enough.

I thought the later would have less implicit memory effort, as it writes directly to the stream. But there is not critical path and I don't have hard feelings here :-)

cfconrad · 2023-11-28T15:57:55Z

rust/agama-dbus-server/src/network/model.rs

+                            ConnectionBuilder::new(port)
+                                .with_type(DeviceType::Ethernet)
+                                .with_interface(port)
+                                .with_controller(controller.id())
+                                .build()


I'm struggle with this, when I think of a scenario which have a other DeviceType then Ethernet, e.g.:

wlan0 -m-+ |-> bond0 eth2 -m-+

How do you think we are going to implement this?

We should obtain the device type from the system, so this is actually an issue that we might fix

what is, if it's also a virtual interface like:

vlan9 -m-+ |-> bond0 eth2 -m-+

And we just about to create them?

cfconrad · 2023-11-28T16:11:53Z

rust/agama-dbus-server/src/network/nm/client.rs

+
+        let path = if let Ok(proxy) = self.get_connection_proxy(conn.uuid()).await {
+            let original = proxy.get_settings().await?;
+            let merged = merge_dbus_connections(&original, &dbus_conn);
+            proxy.update(merged).await?;
+            OwnedObjectPath::from(proxy.path().to_owned())
+        } else {
+            let proxy = SettingsProxy::new(&self.connection).await?;
+            proxy.add_connection(dbus_conn).await?
+        };
+
+        if let Connection::Bond(bond) = conn {
+            let _ = bond.bond.ports.iter().map(|port| {
+                self.add_or_update_port_connection(
+                    port,
+                    bond.base.interface.to_string(),
+                    "bond".to_string(),
+                )
+            });
+        }


Shouldn't this be the same as as simply calling add_or_update_connection() ?

I see, there are small differences, e.g. not calling cleanup_dbus_connection(), but why?

cfconrad · 2023-11-28T16:25:23Z

rust/agama-dbus-server/src/network/nm/client.rs

            }
        }
+


Maybe I miss something, but for me it looks like we are putting all connections which do not have a controller (none port connections) into a vector which is in that hashmap controllers... the other connections into connections. But we forget to link them. Which I think is mandatory, or we would loose the ports config...

cfconrad · 2023-11-28T16:46:16Z

rust/agama-lib/share/profile.schema.json

+                  "ports": {
+                    "type": "array",
+                    "items": {
+                      "description": "A list of the ports to be bonded",


Maybe we need to be more precise here:

Is it a connection name or interface name

What happen if it is a connection name, but no connection with this name exists (defaults)

cfconrad

I'm not convinced to store the ports inside the controller device.
Of course it is nice in the first place, when we have a simple configuration where we just wanna add some ports to a bond. But if theses ports it self are something different (wireless, vlan, bridge) they need a connection definition anyway.

If we do not have a connection of a port and we are going to auto create one, I think we should do it next to the user input. And internally just work with a list of connections, which have only one link to a lower/parent device (like VLAN has) and one link to a upper/controller (like a bridge/bond ports have).

cfconrad · 2023-11-30T09:49:23Z

rust/agama-lib/src/network/types.rs

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy)]
+pub enum BondMode {
+    #[serde(rename = "balance-rr")]


Just a hint. As I'm not sure what makes the more sense/ is better to read!

In our xml parsing part we used serde_with and strum_macros crates to achieve similar things.
https://github.com/jcronenberg/agama/blob/523ebe9abf4055c67952d2d50e6f1fbe734044b5/rust/agama-migrate-wicked/src/interface.rs#L167

We have been considering introducing strum_macros for some time, but we were unsure about introducing another dependency. But, to be honest, we already have a good share of them :-). So perhaps we follow the same path as you. Thanks!

* Just for consistency reasons.

jcronenberg

Just had a quick look over mostly just the docs. Sorry for being nitpicky with the grammar and documentation 🙂

rust/agama-dbus-server/src/network/action.rs

jcronenberg · 2023-12-14T12:09:28Z

rust/agama-dbus-server/src/network/model.rs

@@ -43,27 +45,50 @@ impl NetworkState {

    /// Get connection by UUID
    ///
-    /// * `uuid`: connection UUID
+    /// * `id`: connection UUID


Suggested change

/// * `id`: connection UUID

/// * `uuid`: connection UUID

rust/agama-dbus-server/src/network/model.rs

rust/agama-lib/src/network/client.rs

rust/agama-lib/src/network/proxies.rs

rust/agama-dbus-server/src/network/model.rs

Co-authored-by: Jorik Cronenberg <jcronenberg@suse.de>

cfconrad · 2023-12-14T14:31:52Z

rust/agama-dbus-server/src/network/nm/dbus.rs

-        for (k, v) in wireless_dbus {
-            result.insert(k, v);
+    match &conn {
+        Connection::Ethernet(_) | Connection::Loopback(_) => {


I think is_ethernet() was the hook to identify connections which need to set the customized macaddr via 802-3-ethernet.assigned-mac-address. AFAIK Dummy was one of them and Bond should be one.

If we do it this way. We would need to set mac in the corresponding match block of Connection::Bond and Connection::Dummy.
Or we extract the writing of 802-3-ethernet.assigned-mac-address out of this match block.

cfconrad

Looks good to me, thx for the effort!

imobachgs

It works in our tests 🎆 (creating and updating the bond and its ports).

## Problem Agama's network model is heavily inspired by [nmstate](https://github.com/nmstate/nmstate/blob/c9fd1e80d3c87e1f844edcd86e9d36ae499ce717/rust/src/lib/ifaces/ethernet.rs#L64). Connections are represented [by an enum](https://github.com/openSUSE/agama/blob/2bc452b39e4460122ab52c5a72ee0d1dfe154cec/rust/agama-dbus-server/src/network/model.rs#L220) and each variant contains a struct which depends on the type (e.g., the [WirelessConnection](https://github.com/openSUSE/agama/blob/2bc452b39e4460122ab52c5a72ee0d1dfe154cec/rust/agama-dbus-server/src/network/model.rs#L542) struct). Those structs have a `base` field that points to a [BaseConnection struct](https://github.com/openSUSE/agama/blob/master/rust/agama-dbus-server/src/network/model.rs#L328) which holds the common fields for all connection types. And that's perfectly fine. However, we decided to introduce some methods to hide the fact that the common information lives in a separate `struct`. We could say it is for encapsulation reasons, but I don't know whether it is a good idea. Even we created some *getters* and *setters* for those fields, adding quite some [boilerplate](https://github.com/openSUSE/agama/blob/master/rust/agama-dbus-server/src/network/model.rs#L264-L299) [code](https://github.com/openSUSE/agama/blob/master/rust/agama-dbus-server/src/network/model.rs#L318-L324) Additionally, until we introduced the `ConnectionBuilder` (in #885), creating a new connection with some custom data (like an id and a UUID) was not straightforward because you needed to make the `BaseConnection` first (see [this example](https://github.com/openSUSE/agama/blob/2bc452b39e4460122ab52c5a72ee0d1dfe154cec/rust/agama-dbus-server/src/network/model.rs#L136-L141). ## Proposal Although I am not 100% sure whether it is a better idea, I am exploring the possibility of using a simpler layout to represent a connection. In this case, we would use a single `struct` containing the common fields plus a `config` field for specific stuff. That field contains an `enum` which represents each kind of connection. ```rust /// Represents an availble network connection #[derive(Debug, Default, Clone)] pub struct Connection { pub id: String, pub uuid: Uuid, pub mac_address: MacAddress, pub ip_config: IpConfig, pub status: Status, pub interface: String, pub match_config: MatchConfig, pub config: ConnectionConfig, } #[derive(Default, Debug, PartialEq, Clone)] pub enum ConnectionConfig { #[default] Ethernet, Wireless(WirelessConfig), Loopback, Dummy, } ``` And not getters/setters as they are not needed at all. ## Testing - Adapted the unit tested.

## Problem Until now, the communication between our network model and its D-Bus interface worked only in one direction (from D-Bus to the model). And that was pretty limiting. ## Solution In #885 we introduce the usage of `oneshot::channel()` to enable bi-directional communication between the model and the D-Bus interface. It worked rather well, so it is time to extend this approach. Some use cases that are covered: * Perform a complex validation in the model (considering the whole model) and return the result to the D-Bus interface. * Return the D-Bus path of a new connection (implemented in this PR). But there is another interesting implication: the model is the single source of truth (no more cloned data in the interface). In the future, we are expected to listen to NetworkManager and update some parts of our model accordingly. Not having to replicate that information to D-Bus (except when new things add or disappear) makes things easier. Note: `org.opensuse.Agama1.Network.Device` and `org.opensuse.Agama1.Network.Devices` are excluded by now. ## Testing - *Added a new unit test* - *Tested manually* ## To do - [x] Reduce the boilerplate associated to the usage of channels.

imobachgs reviewed Nov 23, 2023

View reviewed changes

imobachgs reviewed Nov 24, 2023

View reviewed changes

rust/agama-dbus-server/src/network/system.rs Outdated Show resolved Hide resolved

imobachgs reviewed Nov 24, 2023

View reviewed changes

teclator force-pushed the add_bonding_support branch from e8c9872 to aeed3ab Compare November 24, 2023 10:51

cfconrad reviewed Nov 28, 2023

View reviewed changes

rust/agama-dbus-server/src/network/model.rs Outdated Show resolved Hide resolved

cfconrad reviewed Nov 28, 2023

View reviewed changes

cfconrad reviewed Nov 30, 2023

View reviewed changes

teclator force-pushed the add_bonding_support branch from 1e3466d to 2052bf6 Compare December 1, 2023 14:31

cfconrad and others added 16 commits December 5, 2023 10:25

Add Bonding model

256c076

nm-dbus: use WIRELESS_KEY instead of slice

2f4e8b9

nm-dbus: initial bonding to dbus impl

5053b87

Added bonding configuration support

a241308

Mutable connection

48480fa

Bond schema and settings

9d0369c

Bonding DBUS API fixes

9a4f521

Fix device type DBUS method

00c4177

Add controller and interface name

802dbb6

Cleanup empty fields

25df7cf

Wait for ports to be added

8284e57

Cleanup BondConfig code

4c0e1e8

Some fixes and adding a bond connection unit test

0ccfff5

Handle potential problems parsing bonding options

729f5bd

controller_from_dbus does not consume the argument

7aea974

* Just for consistency reasons.

Some small fixed based on CR

864376f

imobachgs added 8 commits December 13, 2023 10:49

Fix test_set_bonding_ports test

7115509

Fix writing of network connections

d8e0ad3

Always write the interface-name in bonds

483f771

Improve error reporting when adding a bond

fa01ee0

Fix network example

e7e2d63

Minor refactoring

c5f4b07

Fallback to connection IDs when exposing the ports

f17fb52

Code and documentation clean-up

447f77f

imobachgs marked this pull request as ready for review December 13, 2023 22:37

imobachgs added 2 commits December 13, 2023 22:39

Merge branch 'master' into add_bonding_support

ef2b7dd

Update the agama-cli.changes file

f118c42

jcronenberg reviewed Dec 14, 2023

View reviewed changes

Documentation and grammar fixes

1a9ffd5

Co-authored-by: Jorik Cronenberg <jcronenberg@suse.de>

cfconrad reviewed Dec 14, 2023

View reviewed changes

imobachgs and others added 4 commits December 14, 2023 14:43

Merge branch 'master' into add_bonding_support

c12d79d

Include the ethernet section in dummy and bond connections too

cd47d9d

Fix client set interface when not present

f335cf0

Add support to read the bond ports from NetworkManager

5f1fa3d

imobachgs requested a review from cfconrad December 15, 2023 06:35

cfconrad approved these changes Dec 15, 2023

View reviewed changes

imobachgs and others added 3 commits December 15, 2023 10:51

Remove the Bond D-Bus interface when cleaning the type

c73a8ef

Fixes for bonding support

d7fdfd3

Publish the connection uuid and controller

9edb940

imobachgs approved these changes Dec 15, 2023

View reviewed changes

teclator merged commit 627f657 into master Dec 15, 2023
2 checks passed

teclator deleted the add_bonding_support branch December 15, 2023 11:30

imobachgs mentioned this pull request Dec 26, 2023

Improve D-Bus network interface #961

Merged

1 task

imobachgs mentioned this pull request Feb 12, 2024

Update to release 7 #947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bonding support #885

Add bonding support #885

teclator commented Nov 22, 2023 •

edited by imobachgs

Loading

coveralls commented Nov 22, 2023 •

edited

Loading

imobachgs left a comment

imobachgs Nov 23, 2023

imobachgs Nov 24, 2023

cfconrad Nov 28, 2023 •

edited

Loading

imobachgs Dec 13, 2023

cfconrad Dec 14, 2023

cfconrad Nov 28, 2023 •

edited

Loading

teclator Nov 30, 2023 •

edited

Loading

cfconrad Nov 30, 2023

cfconrad Nov 28, 2023 •

edited

Loading

cfconrad Nov 28, 2023 •

edited

Loading

cfconrad Nov 28, 2023

cfconrad left a comment •

edited

Loading

cfconrad Nov 30, 2023

imobachgs Nov 30, 2023

jcronenberg left a comment

jcronenberg Dec 14, 2023

cfconrad Dec 14, 2023

cfconrad left a comment

imobachgs left a comment

Add bonding support #885

Add bonding support #885

Conversation

teclator commented Nov 22, 2023 • edited by imobachgs Loading

The model

Configuring a port

Improving the D-Bus interface

What's next?

References

Testing

coveralls commented Nov 22, 2023 • edited Loading

imobachgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfconrad Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfconrad Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

teclator Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfconrad Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

cfconrad Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfconrad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcronenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfconrad left a comment

Choose a reason for hiding this comment

imobachgs left a comment

Choose a reason for hiding this comment

teclator commented Nov 22, 2023 •

edited by imobachgs

Loading

coveralls commented Nov 22, 2023 •

edited

Loading

cfconrad Nov 28, 2023 •

edited

Loading

cfconrad Nov 28, 2023 •

edited

Loading

teclator Nov 30, 2023 •

edited

Loading

cfconrad Nov 28, 2023 •

edited

Loading

cfconrad Nov 28, 2023 •

edited

Loading

cfconrad left a comment •

edited

Loading