Skip to content

Add NM dhcp settings to network model#2189

Merged
teclator merged 2 commits intoagama-project:masterfrom
jcronenberg:network_dhcp_settings
Mar 28, 2025
Merged

Add NM dhcp settings to network model#2189
teclator merged 2 commits intoagama-project:masterfrom
jcronenberg:network_dhcp_settings

Conversation

@jcronenberg
Copy link
Contributor

Problem

For the wicked2nm migration we require some additional dhcp settings, which are currently missing.

Solution

Add all the necessary values to the model. As these are pretty specific and unlikely to really be nedded by agama, I just added these to the network model and not the network settings and also took care that these shouldn't interfere in any way with the "normal" agama operation.

Testing

  • Extended unit tests
  • Tested manually

pub ip6_privacy: Option<i32>,
}

#[skip_serializing_none]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not checked deeply but I guess we should skip serializing it in general if it is the default...
that is, default send_hostname and unset client_id and iaid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for the model it isn't as important as for e.g. the settings. I was basing client_id etc. mostly on MacAddress which also isn't skipped if unset. But if you prefer it not being serialized, I'm not against it.

@teclator
Copy link
Contributor

It is already quite useful as we probably will need to expose these settings too, we were already planning to add the per connection hostname dhcp settings at least.

@jcronenberg jcronenberg force-pushed the network_dhcp_settings branch 2 times, most recently from 1f300c4 to 0160503 Compare March 25, 2025 12:46
}

if let Some(dhcp4_settings) = &ip_config.dhcp4_settings {
ipv4_dbus.insert("dhcp-send-hostname", dhcp4_settings.send_hostname.into());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just noticed that recent NM versions transitioned to a new property dhcp-send-hostname-v2 for NM > 1.52 (which isn't in SLFO yet, only Factory). I would of course like to adopt this new property as early as possible, but what is agama's requirement when it comes to NM compatability?

https://networkmanager.dev/docs/api/latest/settings-ipv4.html (see dhcp-send-hostname-v2 property docs)
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/commit/51ea910cc223bab4c8fa489b192001cfc809e500 (relevant commit)

Copy link
Contributor

@teclator teclator Mar 25, 2025

Choose a reason for hiding this comment

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

Once it is available in both I guess we are fine adopting it. There could be a problem if the seleted product to be installed has a different NM version which does not support the option.

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

@jcronenberg jcronenberg force-pushed the network_dhcp_settings branch from 0160503 to f0c7c60 Compare March 27, 2025 13:27
@jcronenberg jcronenberg force-pushed the network_dhcp_settings branch from f0c7c60 to 3a315a3 Compare March 27, 2025 13:29
@teclator teclator merged commit 8525cdd into agama-project:master Mar 28, 2025
1 check failed
@imobachgs imobachgs mentioned this pull request Apr 22, 2025
imobachgs added a commit that referenced this pull request Apr 22, 2025
@jcronenberg jcronenberg deleted the network_dhcp_settings branch May 14, 2025 15:13
imobachgs added a commit that referenced this pull request Jun 3, 2025
## Problem

Agama doesn't detect and handle different NetworkManager versions. I
know that currently this isn't an issue for agama itself, however I
think it may become one in the future. Also for wicked2nm it represents
a problem because when it is run on Leap15, it tries to send values that
are not present due to the old NM version. See
openSUSE/wicked2nm#24

## Solution

Parse NM version via the semver crate and then act accordingly when
populating dbus values. I haven't checked all the values and when they
were introduced, but I checked for Leap15 and also I mentioned in #2189
that `dhcp-send-hostname` is deprecated in favor of
`dhcp-send-hostname-v2`

## Testing

- *Added a new unit test*
- *Tested manually*
- *Tested with wicked2nm*
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.

2 participants