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.

Sign up for GitHub

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

Jump to bottom

Extend KeepConfiguration= functionality #409

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Extend KeepConfiguration= functionality #409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e71e865
Migrate CriticalConnection (deprecated) to KeepConfiguration
chr4 Sep 19, 2023
7a1914e
Make netplan_netdef_get_keep_configuration() public
chr4 Sep 19, 2023
82442d8
[squashme] revamp keep-configuration option backwards compatible
chr4 Sep 22, 2023
8ae3b07
[squashme] howto pyhon enum plz?
chr4 Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions doc/netplan-yaml.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,19 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`.
> (networkd backend only) Allow the specified interface to be configured even
> if it has no carrier.

- **critical** (bool)
- **critical** (scalar)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'd need to keep this as "(bool or scalar)", making use of handle_generic_bool() internally in parse.c, checking for the error condition in the GError** error parameter. Then falling back to "scalar" (and validating the input to be one of the accepted strings) if a bool cannot be parsed.


> Designate the connection as "critical to the system", meaning that special
> care will be taken by to not release the assigned IP when the daemon is
> restarted. (not recognized by NetworkManager)
> This option sets the systemd KeepConfiguration option.
> (not recognized by NetworkManager)
>
> When set to "static", static addresses and routes won't be dropped on
> starting up process.
> When set to "dhcp-on-stop", addresses and routes won't be dropped when
> stopping the daemon.
> When set to "dhcp", addresses and routes provided by a DHCP server will
> never be dropped even if the DHCP lease expires, implies "dhcp-on-stop".
> When set to "yes", "dhcp" and "static" is implied.
Comment on lines +382 to +388
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Did you double-check if something similar to KeepConfiguration= exists for NetworkManager these days? I'd like to align the wording between the two backends. If it does not exist it might be fine re-using the systemd-networkd terminology here. I need to think about this a bit more.

> Defaults to "no".

- **dhcp-identifier** (scalar)

Expand Down
3 changes: 2 additions & 1 deletion python-cffi/netplan/_build_cffi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
typedef struct netplan_net_definition NetplanNetDefinition;
typedef enum { ... } NetplanBackend;
typedef enum { ... } NetplanDefType;
typedef enum { ... } NetplanCriticalOption;

// TODO: Introduce getters for .address/.lifetime/.label to avoid exposing the raw struct
typedef struct {
Expand Down Expand Up @@ -112,7 +113,7 @@
ssize_t _netplan_netdef_get_embedded_switch_mode(const NetplanNetDefinition* netdef, char* out_buffer, size_t out_buf_size);
gboolean _netplan_netdef_get_sriov_vlan_filter(const NetplanNetDefinition* netdef);
guint _netplan_netdef_get_vlan_id(const NetplanNetDefinition* netdef);
gboolean _netplan_netdef_get_critical(const NetplanNetDefinition* netdef);
NetplanCriticalOption _netplan_netdef_get_critical(const NetplanNetDefinition* netdef);
gboolean _netplan_netdef_is_trivial_compound_itf(const NetplanNetDefinition* netdef);
int _netplan_state_get_vf_count_for_def(
const NetplanState* np_state, const NetplanNetDefinition* netdef, NetplanError** error);
Expand Down
10 changes: 9 additions & 1 deletion src/abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ typedef enum {
NETPLAN_OPTIONAL_STATIC = 1<<4,
} NetplanOptionalAddressFlag;

typedef enum {
NETPLAN_CRITICAL_FALSE,
NETPLAN_CRITICAL_TRUE,
NETPLAN_CRITICAL_STATIC,
NETPLAN_CRITICAL_DHCP,
NETPLAN_CRITICAL_DHCP_ON_STOP,
} NetplanCriticalOption;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: we might just want to call that NetplanCritical to stay more in line with the other enums. Everything is an "Option", so that suffix feels kind of redundant.

PS: please also add and use a NETPLAN_CRITICAL_UNSET option, as suggested by @daniloegea

Note: changing the gboolean type to an enum should not break our ABI, as both will be mapped to a gint in the end.


/* Fields below are valid for dhcp4 and dhcp6 unless otherwise noted. */
typedef struct dhcp_overrides {
gboolean use_dns;
Expand Down Expand Up @@ -211,7 +219,7 @@ struct netplan_net_definition {
/* status options */
gboolean optional;
NetplanOptionalAddressFlag optional_addresses;
gboolean critical;
NetplanCriticalOption critical;

/* addresses */
gboolean dhcp4;
Expand Down
12 changes: 11 additions & 1 deletion src/netplan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,8 +742,18 @@ _serialize_yaml(

if (def->optional)
YAML_NONNULL_STRING_PLAIN(event, emitter, "optional", "true");
if (def->critical)

if (def->critical == NETPLAN_CRITICAL_TRUE) {
YAML_NONNULL_STRING_PLAIN(event, emitter, "critical", "true");
} else if (def->critical == NETPLAN_CRITICAL_FALSE) {
YAML_NONNULL_STRING_PLAIN(event, emitter, "critical", "false");
} else if (def->critical == NETPLAN_CRITICAL_STATIC) {
YAML_NONNULL_STRING_PLAIN(event, emitter, "critical", "static");
} else if (def->critical == NETPLAN_CRITICAL_DHCP) {
YAML_NONNULL_STRING_PLAIN(event, emitter, "critical", "dhcp");
} else if (def->critical == NETPLAN_CRITICAL_DHCP_ON_STOP) {
YAML_NONNULL_STRING_PLAIN(event, emitter, "critical", "dhcp-on-stop");
}

if (def->ignore_carrier)
YAML_NONNULL_STRING_PLAIN(event, emitter, "ignore-carrier", "true");
Expand Down
21 changes: 17 additions & 4 deletions src/networkd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ write_bond_parameters(const NetplanNetDefinition* def, GString* s)
if (def->bond_params.selection_logic)
g_string_append_printf(params, "\nAdSelect=%s", def->bond_params.selection_logic);
if (def->bond_params.all_members_active)
g_string_append_printf(params, "\nAllSlavesActive=%d", def->bond_params.all_members_active); /* wokeignore:rule=slave */
g_string_append_printf(params, "\nAllSlavesActive=%d", def->bond_params.all_members_active); /* wokeignore:rule=slave */
if (def->bond_params.arp_interval) {
g_string_append(params, "\nARPIntervalSec=");
if (interval_has_suffix(def->bond_params.arp_interval))
Expand Down Expand Up @@ -393,7 +393,7 @@ write_bond_parameters(const NetplanNetDefinition* def, GString* s)
g_string_append_printf(params, "\nGratuitousARP=%d", def->bond_params.gratuitous_arp);
/* TODO: add unsolicited_na, not documented as supported by NM. */
if (def->bond_params.packets_per_member)
g_string_append_printf(params, "\nPacketsPerSlave=%d", def->bond_params.packets_per_member); /* wokeignore:rule=slave */
g_string_append_printf(params, "\nPacketsPerSlave=%d", def->bond_params.packets_per_member); /* wokeignore:rule=slave */
if (def->bond_params.primary_reselect_policy)
g_string_append_printf(params, "\nPrimaryReselectPolicy=%s", def->bond_params.primary_reselect_policy);
if (def->bond_params.resend_igmp)
Expand Down Expand Up @@ -921,8 +921,21 @@ netplan_netdef_write_network_file(
g_string_append(network, "\n[DHCP]\n");
}

if (def->critical)
g_string_append_printf(network, "CriticalConnection=true\n");
switch (def->critical) {
case NETPLAN_CRITICAL_TRUE:
g_string_append(s, "KeepConnection=true\n");
break;
case NETPLAN_CRITICAL_STATIC:
g_string_append(s, "KeepConnection=static\n");
break;
case NETPLAN_CRITICAL_DHCP:
g_string_append(s, "KeepConnection=dhcp\n");
break;
case NETPLAN_CRITICAL_DHCP_ON_STOP:
g_string_append(s, "KeepConnection=dhcp-on-stop\n");
break;
default: g_assert_not_reached(); // LCOV_EXCL_LINE
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the reasons it's failing tests is because the case for the false option is missing here. See my comment below about the enum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

}

if (def->dhcp4 || def->dhcp6) {
if (def->dhcp_identifier)
Expand Down
4 changes: 2 additions & 2 deletions src/types.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ reset_netdef(NetplanNetDefinition* netdef, NetplanDefType new_type, NetplanBacke

netdef->optional = FALSE;
netdef->optional_addresses = 0;
netdef->critical = FALSE;
netdef->critical = NETPLAN_CRITICAL_FALSE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably shouldn't initialize critical with false.

According to the documentation, KeepConfiguration defaults to "dhcp-on-stop" when systemd-networkd is running in initrd, "yes" when the root filesystem is a network filesystem, and "no" otherwise.

Initializing it to false would imply setting it to false even when the default is something else. I suggest adding a new entry in the enum, something like NETPLAN_CRITICAL_UNSET, and use it as the default value. Then in the switch-case you added, you'd also have an option for the false case and and an option that does nothing for the unset case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for using NETPLAN_CRITICAL_UNSET here.


netdef->dhcp4 = FALSE;
netdef->dhcp6 = FALSE;
Expand Down Expand Up @@ -594,7 +594,7 @@ _netplan_netdef_get_vlan_id(const NetplanNetDefinition* netdef)
return netdef->vlan_id;
}

gboolean
NetplanCriticalOption
_netplan_netdef_get_critical(const NetplanNetDefinition* netdef)
{
g_assert(netdef);
Expand Down
2 changes: 1 addition & 1 deletion src/util-internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ _netplan_state_get_vf_count_for_def(const NetplanState* np_state, const NetplanN
NETPLAN_INTERNAL gboolean
_netplan_netdef_get_sriov_vlan_filter(const NetplanNetDefinition* netdef);

NETPLAN_INTERNAL gboolean
NETPLAN_INTERNAL NetplanCriticalOption
_netplan_netdef_get_critical(const NetplanNetDefinition* netdef);
Comment on lines +105 to 106
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This is NETPLAN_INTERNAL API, as also desribed by the _netplan prefix. It's supposed to be used by Netplan tools/CLI only, not by external consumers. Although, changing the signature is an API breaking change, I think it's fine as we're the only ones affected.


NETPLAN_INTERNAL gboolean
Expand Down
2 changes: 1 addition & 1 deletion tests/generator/test_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ def test_dhcp_critical_true(self):
LinkLocalAddressing=ipv6

[DHCP]
CriticalConnection=true
KeepConfiguration=true
'''})

def test_dhcp_identifier_mac(self):
Expand Down
Loading