Add support for multiple connection files per logical interface #114

koendelaat · 2024-10-11T15:14:36Z

This PR addresses the fact that some interface types generate multiple connection files per logical interface.

Disclaimer: I'm newby with respect to rust, and I struggled with some of the concepts. Please make suggestions on how to improve certain coding aspects.

I've added an openvswitch example that would fail without this fix.

There is a change to the host_config.yaml file, but I tried to make it backwards compatible. For each interface there is an additional field that specifies the connection ids belonging to this logical interface.

An example host_config.yaml:

- hostname: node1
  interfaces:
    - logical_name: br1
      connection_ids:
        - br1-br
      interface_type: ovs-bridge
    - logical_name: bridge0
      connection_ids:
        - bridge0
      mac_address: FE:C4:05:42:8B:AA
      interface_type: linux-bridge
    - logical_name: ovs0
      connection_ids:
        - ovs0-port
        - ovs0-if
      interface_type: ovs-interface
    - logical_name: eth0
      connection_ids:
        - eth0
      mac_address: 0E:4D:C6:B8:C4:72
      interface_type: ethernet
    - logical_name: eth1
      connection_ids:
        - eth1-port
        - eth1
      mac_address: 5c:c7:c9:5e:fb:ec
      interface_type: ethernet

koendelaat · 2024-10-11T15:16:54Z

@atanasdinov Can you give your first thoughts on this change.

Also, please indicate coding errors or weird constructs. As mentioned, I'm a new to rust.

atanasdinov · 2024-10-11T15:21:58Z

Thanks for this contribution! In fact, I was already looking at the culprit behind this and was going to push a fix.

I'll take a look early next week but can you also fix the linter issues so the CI is happy? :)

koendelaat · 2024-10-11T15:42:57Z

Thanks for this contribution! In fact, I was already looking at the culprit behind this and was going to push a fix.

I'll take a look early next week but can you also fix the linter issues so the CI is happy? :)

Addressing the clippy issues now

atanasdinov

Thanks a lot for this contribution! The implementation idea is great - I left some comments about my concerns and some improvements that we can make before merging.

Please let me know what you think, thanks!

atanasdinov · 2024-10-15T16:24:55Z

.dockerignore

@@ -0,0 +1 @@
+target/


We can probably get rid of this file as we never build nmc in a container.

Sure, I added it because the CI builds a container, and when I tried locally this was a speedup

This is interesting, I wasn't aware it could be a speedup locally.

When built locally, a lot of files are created in the target/ directory. When trying to build the dockerimage, all the files in the target/ directory are copied over to the docker build context. This takes "long" for > 1GB of data...

So this was just a quick fix to remediate this issue. I'm also fine by removing it as we don't need the docker image in the end.

src/apply_conf.rs

atanasdinov · 2024-10-16T12:29:51Z

src/apply_conf.rs

+        let connections = &interface
+            .connection_ids
+            .clone()
+            .unwrap_or(vec![interface.logical_name.clone()]);


Is it safer to always include the logical name? Do we anticipate that it will always be present in the connections_id?

I added the logical_name here as a fallback mechanism mainly for backwards compatibility. Although I doubt if we need backwards compatibility (I assume same NMC version is used for generating and applying).

No, the logical name won't be always present in the connection_ids.

See also the example

- logical_name: ovs0 connection_ids: - ovs0-port - ovs0-if interface_type: ovs-interface

Isn't there an ovs0.nmconnection file that we don't copy over with the current implementation?

ovs0.nmconnection is not generated by nmstate. When applying this configuration with the current implementation you'll get an error like described in #113 because the file is missing

src/apply_conf.rs

src/generate_conf.rs

atanasdinov · 2024-10-16T12:55:05Z

src/generate_conf.rs

+                .get(i.name())
+                .cloned()
+                .or_else(|| Some(Vec::new())),


This confirms the initial concern about applying the configurations. We should do one of the following:

Always include the logical name in this list since a connection file will be present for it

Never include the logical name in the list and merge it with these connections when applying

is not true, see previous comment

I'm not saying that either of these is true, but that we should probably do one of those. However, the ovs example you gave can be contradicting if ovs0.nmconnection file is never created.

indeed, ovs0.nmconnection is never created

Signed-off-by: Koen de Laat <[email protected]>

koendelaat · 2024-10-24T11:25:49Z

@atanasdinov, can you please have look at the modifications?

atanasdinov

Sorry for the late response. Thanks for addressing the initial comments - I left a couple more but this is now very close to being ready, great job!

testdata/apply/node1/eth2.nmconnection

src/types.rs

src/generate_conf.rs

src/apply_conf.rs

src/generate_conf.rs

Signed-off-by: Koen de Laat <[email protected]>

src/apply_conf.rs

atanasdinov · 2024-10-30T15:31:59Z

src/generate_conf.rs

        let mut interfaces = extract_interfaces(&net_state);
+        populate_connection_ids(&mut interfaces, &config_files).expect("populate ids");


I believe we're fine to drop this. The test explicitly wants to verify that extract_interfaces works as expected, not necessarily the complete state of the final output.

True, however than in the assert, we've to specify empty vectors in connection_ids. Which is confusing from a developers perspective

atanasdinov · 2024-10-30T15:34:31Z

src/generate_conf.rs

+) -> anyhow::Result<()> {
+    config
+        .iter()
+        .filter(|(filename, _)| filename != "lo.nmconnection")


Would this cause an issue with validate_connection_ids? 👀

No, the current implementation removes the type=loopback interfaces. So this code would fail for lo.nmconnection because it cannot find a matching interface definition.

This is however not perfect I realize now, there could be more loopback connections with each their own connection file.

Precisely because we filter it out, I'd expect the validate_connection_ids to return an error and abort the generation in the cases where there are loopbacks.

I've tried to implement a better filter for loopback connections.

However I'm wondering why we exclude loopback connections at all. Current code will silently remove any loopback configuration during generation and therefor not apply any loopback connection.

src/generate_conf.rs

Signed-off-by: Koen de Laat <[email protected]>

koendelaat · 2024-11-05T08:10:41Z

@atanasdinov, I've fixed the linter issues. Any other comments?

src/generate_conf.rs

Co-authored-by: Atanas Dinov <[email protected]> Signed-off-by: Koen de Laat <[email protected]>

Add support for multiple connection files per logical interface

33aad3b

Fix formatting

9a8b97d

koendelaat force-pushed the fix/multiple_connection_files branch from a6e276f to 094bacb Compare October 11, 2024 15:50

Fix clippy issues

425f1f5

koendelaat force-pushed the fix/multiple_connection_files branch from 094bacb to 425f1f5 Compare October 11, 2024 15:51

koendelaat marked this pull request as ready for review October 14, 2024 12:58

atanasdinov self-requested a review October 15, 2024 07:56

atanasdinov requested changes Oct 16, 2024

View reviewed changes

Address review comments

4ad178b

Signed-off-by: Koen de Laat <[email protected]>

koendelaat requested a review from atanasdinov October 21, 2024 11:20

koendelaat force-pushed the fix/multiple_connection_files branch 2 times, most recently from fcaaa98 to 1bda6b4 Compare October 21, 2024 11:24

Rework generate_config

13a168b

Signed-off-by: Koen de Laat <[email protected]>

koendelaat force-pushed the fix/multiple_connection_files branch from 1bda6b4 to 13a168b Compare October 21, 2024 11:28

atanasdinov reviewed Oct 28, 2024

View reviewed changes

testdata/apply/node1/eth2.nmconnection Show resolved Hide resolved

src/types.rs Outdated Show resolved Hide resolved

src/generate_conf.rs Outdated Show resolved Hide resolved

src/apply_conf.rs Outdated Show resolved Hide resolved

src/generate_conf.rs Outdated Show resolved Hide resolved

Proper error handling

d7a5d2b

Signed-off-by: Koen de Laat <[email protected]>

koendelaat force-pushed the fix/multiple_connection_files branch from 3b1424d to d7a5d2b Compare October 30, 2024 14:03

koendelaat requested a review from atanasdinov October 30, 2024 14:09

atanasdinov reviewed Oct 30, 2024

View reviewed changes

koendelaat added 3 commits October 31, 2024 17:54

Filter loopback connections

ed6ded9

Signed-off-by: Koen de Laat <[email protected]>

Add check for empty connection_ids

8e27a63

Signed-off-by: Koen de Laat <[email protected]>

Fix linting

68418be

Signed-off-by: Koen de Laat <[email protected]>

atanasdinov reviewed Nov 5, 2024

View reviewed changes

src/generate_conf.rs Outdated Show resolved Hide resolved

Refactor populate_connection_ids

cd7f3b3

Co-authored-by: Atanas Dinov <[email protected]> Signed-off-by: Koen de Laat <[email protected]>

koendelaat requested a review from atanasdinov November 8, 2024 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple connection files per logical interface #114

Add support for multiple connection files per logical interface #114

koendelaat commented Oct 11, 2024

koendelaat commented Oct 11, 2024 •

edited

Loading

atanasdinov commented Oct 11, 2024

koendelaat commented Oct 11, 2024

atanasdinov left a comment

atanasdinov Oct 15, 2024

koendelaat Oct 16, 2024

atanasdinov Oct 17, 2024

koendelaat Oct 18, 2024

atanasdinov Oct 16, 2024

koendelaat Oct 16, 2024

atanasdinov Oct 17, 2024

koendelaat Oct 18, 2024

atanasdinov Oct 16, 2024

koendelaat Oct 16, 2024

atanasdinov Oct 17, 2024

koendelaat Oct 18, 2024

koendelaat commented Oct 24, 2024

atanasdinov left a comment

atanasdinov Oct 30, 2024

koendelaat Oct 31, 2024

atanasdinov Oct 30, 2024

koendelaat Oct 31, 2024

atanasdinov Oct 31, 2024

koendelaat Oct 31, 2024

koendelaat commented Nov 5, 2024

		let mut interfaces = extract_interfaces(&net_state);
		populate_connection_ids(&mut interfaces, &config_files).expect("populate ids");

		@@ -0,0 +1 @@
		target/

Add support for multiple connection files per logical interface #114

Are you sure you want to change the base?

Add support for multiple connection files per logical interface #114

Conversation

koendelaat commented Oct 11, 2024

koendelaat commented Oct 11, 2024 • edited Loading

atanasdinov commented Oct 11, 2024

koendelaat commented Oct 11, 2024

atanasdinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koendelaat commented Oct 24, 2024

atanasdinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koendelaat commented Nov 5, 2024

koendelaat commented Oct 11, 2024 •

edited

Loading