Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing the network model #924

Merged
merged 2 commits into from
Dec 18, 2023
Merged

Changing the network model #924

merged 2 commits into from
Dec 18, 2023

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Dec 11, 2023

Problem

Agama's network model is heavily inspired by nmstate. Connections are represented by an enum and each variant contains a struct which depends on the type (e.g., the WirelessConnection struct). Those structs have a base field that points to a BaseConnection struct 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 code

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.

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.

/// 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.

@coveralls
Copy link

coveralls commented Dec 11, 2023

Coverage Status

coverage: 74.724% (+0.008%) from 74.716%
when pulling 15087be on imobachgs:refactor-network-model
into 05a339b on openSUSE:master.

@@ -57,25 +59,25 @@ pub fn connection_to_dbus(conn: &Connection) -> NestedHash {
///
/// This functions tries to turn a OwnedHashMap coming from D-Bus into a Connection.
pub fn connection_from_dbus(conn: OwnedNestedHash) -> Option<Connection> {
let base = base_connection_from_dbus(&conn)?;
let mut connection = base_connection_from_dbus(&conn)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it is a problem at all. This function show the case that we now easily transform a Connection from one type into the other, by setting the config enum. Before it was like, collecting common settings + specific settings and then construct the Connection.

But currently I do not see a problem with it. For me this approach has benefits over the current.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have the same opinion and it does not look like a problem (at least by now). I think this approach is simpler. But I will work on this once the bonding support is finally merged.

@imobachgs imobachgs mentioned this pull request Dec 14, 2023
teclator added a commit that referenced this pull request Dec 15, 2023
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:

```rust
      {
        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](https://github.com/openSUSE/agama/blob/add_bonding_support/rust/agama-dbus-server/src/network/dbus/interfaces.rs#L312)
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?

* #918
* #924 
* Fix a ton of issues detected by Clippy.

## References

* @cfconrad started to work on bonding support on #783.

## Testing

* Tested manually (see teclator#1).
* Added some unit and integration tests.
@imobachgs imobachgs changed the title [RFC] Changing the network model Changing the network model Dec 15, 2023
* Connection is now a struct containing the common fields.
* The connection holds specific configuration in the "config" field.
* The ConnectionConfig is an enum which models specific configurations.
* Adapt the rest of the code to the new API, including the tests.
Copy link
Contributor

@teclator teclator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@imobachgs imobachgs merged commit 275c770 into agama-project:master Dec 18, 2023
2 checks passed
@imobachgs imobachgs deleted the refactor-network-model branch December 18, 2023 10:09
@imobachgs imobachgs mentioned this pull request Dec 21, 2023
imobachgs added a commit that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants