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
+55 −15
Open

Extend KeepConfiguration= functionality #409

Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 6 additions & 6 deletions abi-compat/jammy_0.107.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<elf-symbol name='_netplan_iter_defs_per_devtype_next' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='_netplan_nameserver_iter_free' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='_netplan_nameserver_iter_next' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='_netplan_netdef_get_critical' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='_netplan_netdef_get_delay_vf_rebind' type='func-type' binding='global-binding' visibility='default-visibility' alias='netplan_netdef_get_delay_virtual_functions_rebind' is-defined='yes'/>
<elf-symbol name='_netplan_netdef_get_embedded_switch_mode' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='_netplan_netdef_get_optional' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
Expand Down Expand Up @@ -58,7 +59,6 @@
<elf-symbol name='netplan_get_global_backend' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='netplan_get_id_from_nm_filename' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='netplan_get_id_from_nm_filepath' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='netplan_netdef_get_keep_configuration' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='netplan_netdef_get_backend' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='netplan_netdef_get_bond_link' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
<elf-symbol name='netplan_netdef_get_bridge_link' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
Expand Down Expand Up @@ -421,7 +421,7 @@
<var-decl name='optional_addresses' type-id='type-id-39' visibility='default' filepath='../src/abi.h' line='203' column='1'/>
</data-member>
<data-member access='public' layout-offset-in-bits='320'>
<var-decl name='keep_configuration' type-id='type-id-42' visibility='default' filepath='../src/abi.h' line='204' column='1'/>
<var-decl name='critical' type-id='type-id-42' visibility='default' filepath='../src/abi.h' line='204' column='1'/>
</data-member>
<data-member access='public' layout-offset-in-bits='352'>
<var-decl name='dhcp4' type-id='type-id-42' visibility='default' filepath='../src/abi.h' line='207' column='1'/>
Expand Down Expand Up @@ -4191,6 +4191,10 @@
<parameter type-id='type-id-181' name='netdef' filepath='../src/types.c' line='590' column='1'/>
<return type-id='type-id-44'/>
</function-decl>
<function-decl name='_netplan_netdef_get_critical' mangled-name='_netplan_netdef_get_critical' filepath='../src/types.c' line='597' column='1' visibility='default' binding='global' size-in-bits='64' elf-symbol-id='_netplan_netdef_get_critical'>
<parameter type-id='type-id-181' name='netdef' filepath='../src/types.c' line='597' column='1'/>
<return type-id='type-id-42'/>
</function-decl>
<function-decl name='_netplan_netdef_get_optional' mangled-name='_netplan_netdef_get_optional' filepath='../src/types.c' line='604' column='1' visibility='default' binding='global' size-in-bits='64' elf-symbol-id='_netplan_netdef_get_optional'>
<parameter type-id='type-id-181' name='netdef' filepath='../src/types.c' line='604' column='1'/>
<return type-id='type-id-42'/>
Expand Down Expand Up @@ -4369,10 +4373,6 @@
<parameter type-id='type-id-106' name='rootdir' filepath='../src/util.c' line='590' column='1'/>
<return type-id='type-id-42'/>
</function-decl>
<function-decl name='netplan_netdef_get_keep_configuration' mangled-name='netplan_netdef_get_keep_configuration' filepath='../src/types.c' line='597' column='1' visibility='default' binding='global' size-in-bits='64' elf-symbol-id='netplan_netdef_get_keep_configuration'>
<parameter type-id='type-id-181' name='netdef' filepath='../src/types.c' line='597' column='1'/>
<return type-id='type-id-43'/>
</function-decl>
<function-decl name='netplan_netdef_get_output_filename' mangled-name='netplan_netdef_get_output_filename' filepath='../src/util.c' line='645' column='1' visibility='default' binding='global' size-in-bits='64' elf-symbol-id='netplan_netdef_get_output_filename'>
<parameter type-id='type-id-181' name='netdef' filepath='../src/util.c' line='645' column='1'/>
<parameter type-id='type-id-106' name='ssid' filepath='../src/util.c' line='645' column='1'/>
Expand Down
15 changes: 10 additions & 5 deletions doc/netplan-yaml.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,14 +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.

- **keep-configuration** (scalar)
- **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.


> 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"
> 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.
> Defaults to "no".
> (not recognized by NetworkManager)

- **dhcp-identifier** (scalar)

Expand Down
10 changes: 5 additions & 5 deletions netplan_cli/cli/commands/apply.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
devices_after_udev = netifaces.interfaces()
# apply some more changes manually
for iface, settings in changes.items():
# rename network interfaces without keep_configuration set
# rename non-critical network interfaces
new_name = settings.get('name')
if new_name:
if len(new_name) >= IF_NAMESIZE:
Expand Down Expand Up @@ -356,7 +356,7 @@ def clear_virtual_links(prev_links, curr_links, devices=[]):
def process_link_changes(interfaces, config_manager: ConfigManager): # pragma: nocover (covered in autopkgtest)
"""
Go through the pending changes and pick what needs special handling.
Only applies to interfaces not having keep_configuration set, which can be safely updated.
Only applies to non-critical interfaces which can be safely updated.
"""

changes = {}
Expand Down Expand Up @@ -384,9 +384,9 @@ def process_link_changes(interfaces, config_manager: ConfigManager): # pragma:
# Skip interface if it already has the correct name
logging.debug('Skipping correctly named interface: {}'.format(newname))
continue
if netdef.keep_configuration:
# Skip interfaces with keep_configuration set, as we should not take them down in order to rename
logging.warning('Cannot rename {} ({} -> {}) at runtime (needs reboot), due to keep_configuration being set'
if netdef.critical:
# Skip interfaces defined as critical, as we should not take them down in order to rename
logging.warning('Cannot rename {} ({} -> {}) at runtime (needs reboot), due to being critical'
.format(netdef.id, current_iface_name, newname))
continue

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;
char* keep_configuration;
NetplanCriticalOption critical;

/* addresses */
gboolean dhcp4;
Expand Down
14 changes: 12 additions & 2 deletions 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->keep_configuration)
YAML_STRING_PLAIN(def, event, emitter, "keep-configuration", def->keep_configuration);

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
19 changes: 16 additions & 3 deletions src/networkd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,13 +916,26 @@ netplan_netdef_write_network_file(
}
}

if (def->dhcp4 || def->dhcp6 || def->keep_configuration) {
if (def->dhcp4 || def->dhcp6 || def->critical) {
/* NetworkManager compatible route metrics */
g_string_append(network, "\n[DHCP]\n");
}

if (def->keep_configuration)
g_string_append_printf(network, "KeepConfiguration=%s\n", def->keep_configuration);
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
2 changes: 1 addition & 1 deletion src/parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2785,7 +2785,7 @@ static const mapping_entry_handler dhcp6_overrides_handlers[] = {
{"accept-ra", YAML_SCALAR_NODE, {.generic=handle_accept_ra}, netdef_offset(accept_ra)}, \
{"activation-mode", YAML_SCALAR_NODE, {.generic=handle_activation_mode}, netdef_offset(activation_mode)}, \
{"addresses", YAML_SEQUENCE_NODE, {.generic=handle_addresses}, NULL}, \
{"keep-configuration", YAML_SCALAR_NODE, {.generic=handle_netdef_bool}, netdef_offset(keep_configuration)}, \
{"critical", YAML_SCALAR_NODE, {.generic=handle_netdef_bool}, netdef_offset(critical)}, \
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the new critical option is not a boolean anymore, we'll need to update the parser. You'll need to create a new function that checks what option the user set and store one of the enum variants in the variable. You can base your implementation on the handle_auth_methods function for example https://github.com/chr4/netplan/blob/feat/keep-configuration/src/parse.c#L947

And as we want to still accept the boolean values so we'll not break existing configurations out there, we'll need to accept all the boolean values accepted by netplan. See https://github.com/chr4/netplan/blob/feat/keep-configuration/src/parse.c#L402

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like any changes to src/parse.c have been lost in a rebase?

{"ignore-carrier", YAML_SCALAR_NODE, {.generic=handle_netdef_bool}, netdef_offset(ignore_carrier)}, \
{"dhcp4", YAML_SCALAR_NODE, {.generic=handle_netdef_bool}, netdef_offset(dhcp4)}, \
{"dhcp6", YAML_SCALAR_NODE, {.generic=handle_netdef_bool}, netdef_offset(dhcp6)}, \
Expand Down
9 changes: 8 additions & 1 deletion 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;
FREE_AND_NULLIFY(netdef->keep_configuration);
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,6 +594,13 @@ _netplan_netdef_get_vlan_id(const NetplanNetDefinition* netdef)
return netdef->vlan_id;
}

NetplanCriticalOption
_netplan_netdef_get_critical(const NetplanNetDefinition* netdef)
{
g_assert(netdef);
return netdef->critical;
}

gboolean
_netplan_netdef_get_optional(const NetplanNetDefinition* netdef)
{
Expand Down
3 changes: 3 additions & 0 deletions src/util-internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ _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 NetplanCriticalOption
_netplan_netdef_get_critical(const NetplanNetDefinition* netdef);

NETPLAN_INTERNAL gboolean
_netplan_netdef_get_optional(const NetplanNetDefinition* netdef);

Expand Down
7 changes: 0 additions & 7 deletions src/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,13 +1018,6 @@ netplan_netdef_match_interface(const NetplanNetDefinition* netdef, const char* n
return TRUE;
}

char *
netplan_netdef_get_keep_configuration(const NetplanNetDefinition* netdef)
{
g_assert(netdef);
return netdef->keep_configuration;
}

ssize_t
netplan_netdef_get_set_name(const NetplanNetDefinition* netdef, char* out_buffer, size_t out_buf_size)
{
Expand Down
4 changes: 2 additions & 2 deletions tests/generator/test_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,12 +467,12 @@ def test_bond_arp_ip_targets_multi_pass(self):
Bond=bond0
'''})

def test_dhcp_keep_configuration_true(self):
def test_dhcp_critical_true(self):
self.generate('''network:
version: 2
ethernets:
engreen:
keep-configuration: yes
critical: yes
''')

self.assert_networkd({'engreen.network': '''[Match]
Expand Down
8 changes: 4 additions & 4 deletions tests/test_libnetplan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,15 +581,15 @@ def test_backend(self):
self.assertEqual(state['eth0'].backend, 'networkd')
self.assertEqual(state['eth1'].backend, 'NetworkManager')

def test_keep_configuration(self):
def test_critical(self):
state = state_from_yaml(self.confdir, '''network:
ethernets:
eth0:
keep-configuration: true
critical: true
eth1: {}''')

self.assertTrue(state['eth0'].keep_configuration)
self.assertFalse(state['eth1'].keep_configuration)
self.assertTrue(state['eth0'].critical)
self.assertFalse(state['eth1'].critical)

def test_eq(self):
state = state_from_yaml(self.confdir, '''network:
Expand Down