Extend KeepConfiguration= functionality #409

chr4 · 2023-09-19T11:52:14Z

Description

With systemd-v243 (which is the default since focal), the "CriticalConnection" attribute was
revamped into "KeepConfiguration" [1], addressing several issues, especially in high-availability
environents [2][3].

The new "KeepConfiguration" setting can take multiple arguments, according to the documentation
[4]:

Takes a boolean or one of "static", "dhcp-on-stop", "dhcp". When "static", systemd-networkd will
not drop static addresses and routes on starting up process. When set to "dhcp-on-stop",
systemd-networkd will not drop addresses and routes on stopping the daemon. When "dhcp", the
addresses and routes provided by a DHCP server will never be dropped even if the DHCP lease
expires. This is contrary to the DHCP specification, but may be the best choice if, e.g., the
root filesystem relies on this connection. The setting "dhcp" implies "dhcp-on-stop", and "yes"
implies "dhcp" and "static". Defaults to "no".

If I understand the code correctly, it still uses "CriticalConnection" when setting "critical: true" in the netplan configuration.

Closes: https://bugs.launchpad.net/netplan/+bug/1896799

Checklist

Runs make check successfully.
Retains 100% code coverage (make check-coverage).
New/changed keys in YAML format are documented.
(Optional) Adds example YAML for new feature.
(Optional) Closes an open bug in Launchpad.

chr4 · 2023-09-19T11:53:23Z

This is a first draft - not tested yet. Feedback welcome!

chr4 · 2023-09-19T13:35:11Z

Not sure how to fix the Python SIGSEGVs?

See: https://bugs.launchpad.net/netplan/+bug/1896799

daniloegea · 2023-09-21T11:48:59Z

Hello @chr4,

Thank you for bringing this to our attention and also for your PR, it's much appreciated.

Yes, we are still emitting the following configuration when critical: true is used in Netplan:

[DHCP]
CriticalConnection=true

As you already said, According to systemd's NEWS file this option was deprecated some time ago:

The CriticalConnection= setting in .network files is now deprecated,
and replaced by a new KeepConfiguration= setting which allows more
detailed configuration of the IP configuration to keep in place.

So yes, we need to get rid of CriticalConnection in favor of KeepConfiguration.

The problem is that we can't get rid of the critical key in Netplan, because it would break Netplan for everybody using this option.

Instead of replacing it with keep-configuration, I'd suggest a much simpler approach: adapt critical to accept all the new values supported by KeepConfiguration, including true and false.

This would make the patch simpler and wouldn't break Netplan. Also, all those functions and variables renaming wouldn't be necessary.

The second approach would be to keep critical as it is, deprecate it and introduce a new key, such as keep-configuration.

Keeping critical untouched would still work because systemd still accepts CriticalConnection, even though it's not even documented anymore. See https://github.com/systemd/systemd/blob/main/src/network/networkd-network.c#L245

In my opinion we could reuse critical and just change it to accept all the new values accepted by KeepConfiguration.

@slyon @vorlonofportland any thoughts on this?

daniloegea · 2023-09-21T11:55:03Z

src/abi.h

@@ -211,7 +211,7 @@ struct netplan_net_definition {
    /* status options */
    gboolean optional;
    NetplanOptionalAddressFlag optional_addresses;
-    gboolean critical;
+    char* keep_configuration;


To make things simpler, we should use an enum for this option. It has a well defined set of possible values, so it would naturally fit well in an enum:

enum { NETPLAN_CRITICAL_TRUE, NETPLAN_CRITICAL_FALSE, NETPLAN_CRITICAL_STATIC, ..., } CriticalOptions;

Please, check my comments in the summary.

Good idea! Let me revamp

I've been trying to rework this, but I'm not sure if I'm on the right track, as quite a few tests are failing or even segfaulting.

daniloegea

Hi there! Sorry for taking so long to reply. I added a few comments in your most recent commits. I hope it helps.

May I ask you to squash your commits for the next iteration? It will make it a bit easier to read the changes.

Thank you! :)

daniloegea · 2023-11-01T15:34:20Z

src/types.c

@@ -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;


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.

+1 for using NETPLAN_CRITICAL_UNSET here.

daniloegea · 2023-11-01T15:45:13Z

src/parse.c

@@ -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)}, \


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

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

daniloegea · 2023-11-01T15:46:36Z

src/networkd.c

+        case NETPLAN_CRITICAL_DHCP_ON_STOP:
+            g_string_append(s, "KeepConnection=dhcp-on-stop\n");
+            break;
+        default: g_assert_not_reached(); // LCOV_EXCL_LINE


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.

daniloegea · 2023-11-01T15:56:29Z

python-cffi/netplan/netdef.py

@@ -75,8 +75,8 @@ def set_name(self) -> str:
        return _string_realloc_call_no_error(lambda b: lib.netplan_netdef_get_set_name(self._ptr, b, len(b)))

    @property
-    def keep_configuration(self) -> str:
-        return _string_realloc_call_no_error(lambda b: lib.netplan_netdef_get_keep_configuration(self._ptr, b, len(b)))
+    def critical(self) -> bool:


I wonder if we could keep the Python binding untouched and return False if critical is false (or unset) and True if it's set to any of the other values...

Although we probably should change it to return the enum values. You can see an example of enums in the Python code here https://github.com/chr4/netplan/blob/feat/keep-configuration/python-cffi/netplan/_utils.py#L27

That's an interesting thought. I feel like we should not diverge the bindings from the native API behavior, though. But we need to think about how we can handle that in a backwards compatible way.

slyon

Thanks a lot for your contribution to Netplan @chr4! And for the initial review from @daniloegea

Overall, I think this is going into the right direction, but could you please have a look at #424 ? Maybe we should make it a two-step approach to land that first (and fix the bug at hand), then work on the extension of the new setting as an additional step.

Danilo's suggestions are good and should be addressed. I like the approach of keeping the critical: setting in Netplan and just extending it to accept "bool or scalar" eventually. So we can keep backwards compatibility and support the new usecase going forward.

I left some additional remarks inline, which need some more work. Especially:

Any changes to src/parse.c seem to been lost. We need those.
unit-test and CI failures. We want it to be (mostly) green. Danilo gave some good advice of how to approach this. You might also want to run make check-coverage locally, to test on your own, outside the CI.

slyon · 2023-11-27T16:22:33Z

src/types.c

@@ -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;


+1 for using NETPLAN_CRITICAL_UNSET here.

slyon · 2023-11-27T16:23:03Z

doc/netplan-yaml.md

@@ -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)


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.

slyon · 2023-11-27T16:36:26Z

src/abi.h

+    NETPLAN_CRITICAL_STATIC,
+    NETPLAN_CRITICAL_DHCP,
+    NETPLAN_CRITICAL_DHCP_ON_STOP,
+} NetplanCriticalOption;


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.

slyon · 2023-11-27T16:38:54Z

src/networkd.c

+        case NETPLAN_CRITICAL_DHCP_ON_STOP:
+            g_string_append(s, "KeepConnection=dhcp-on-stop\n");
+            break;
+        default: g_assert_not_reached(); // LCOV_EXCL_LINE


slyon · 2023-11-27T16:41:09Z

src/util-internal.h

+NETPLAN_INTERNAL NetplanCriticalOption
 _netplan_netdef_get_critical(const NetplanNetDefinition* netdef);


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.

slyon · 2023-11-27T16:43:15Z

doc/netplan-yaml.md

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


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.

slyon · 2023-11-27T16:50:35Z

src/parse.c

@@ -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)}, \


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

slyon · 2023-11-27T16:50:45Z

src/networkd.c

+        case NETPLAN_CRITICAL_DHCP_ON_STOP:
+            g_string_append(s, "KeepConnection=dhcp-on-stop\n");
+            break;
+        default: g_assert_not_reached(); // LCOV_EXCL_LINE


slyon · 2023-11-27T16:52:54Z

python-cffi/netplan/netdef.py

@@ -75,8 +75,8 @@ def set_name(self) -> str:
        return _string_realloc_call_no_error(lambda b: lib.netplan_netdef_get_set_name(self._ptr, b, len(b)))

    @property
-    def keep_configuration(self) -> str:
-        return _string_realloc_call_no_error(lambda b: lib.netplan_netdef_get_keep_configuration(self._ptr, b, len(b)))
+    def critical(self) -> bool:


That's an interesting thought. I feel like we should not diverge the bindings from the native API behavior, though. But we need to think about how we can handle that in a backwards compatible way.

chr4 force-pushed the feat/keep-configuration branch 6 times, most recently from f66728c to 49a24de Compare September 19, 2023 13:26

chr4 force-pushed the feat/keep-configuration branch from 74723e0 to 914a7df Compare September 19, 2023 15:34

chr4 added 2 commits September 19, 2023 19:52

Migrate CriticalConnection (deprecated) to KeepConfiguration

e71e865

See: https://bugs.launchpad.net/netplan/+bug/1896799

Make netplan_netdef_get_keep_configuration() public

7a1914e

chr4 force-pushed the feat/keep-configuration branch from 914a7df to 7a1914e Compare September 19, 2023 17:52

daniloegea added the community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. label Sep 21, 2023

daniloegea reviewed Sep 21, 2023

View reviewed changes

chr4 force-pushed the feat/keep-configuration branch from fa13edc to 7c237c6 Compare September 22, 2023 22:03

[squashme] revamp keep-configuration option backwards compatible

82442d8

chr4 force-pushed the feat/keep-configuration branch from 7c237c6 to bd9306a Compare September 22, 2023 22:26

[squashme] howto pyhon enum plz?

8ae3b07

chr4 force-pushed the feat/keep-configuration branch from bd9306a to 8ae3b07 Compare September 22, 2023 22:28

daniloegea requested changes Nov 1, 2023

View reviewed changes

slyon mentioned this pull request Nov 27, 2023

Migrate CriticalConnection (deprecated) to KeepConfiguration (LP: #1896799) #424

Merged

3 tasks

slyon changed the title ~~Migrate CriticalConnection (deprecated) to KeepConfiguration~~ Extend KeepConfiguration functionality Nov 27, 2023

slyon changed the title ~~Extend KeepConfiguration functionality~~ Extend KeepConfiguration= functionality Nov 27, 2023

slyon requested changes Nov 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend KeepConfiguration= functionality #409

Extend KeepConfiguration= functionality #409

chr4 commented Sep 19, 2023 •

edited

chr4 commented Sep 19, 2023

chr4 commented Sep 19, 2023

daniloegea commented Sep 21, 2023

daniloegea Sep 21, 2023 •

edited

chr4 Sep 22, 2023

chr4 Oct 13, 2023

daniloegea left a comment

daniloegea Nov 1, 2023

slyon Nov 27, 2023

daniloegea Nov 1, 2023

slyon Nov 27, 2023

daniloegea Nov 1, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

daniloegea Nov 1, 2023

slyon Nov 27, 2023

slyon left a comment

slyon Nov 27, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

slyon Nov 27, 2023

		NETPLAN_INTERNAL NetplanCriticalOption
		_netplan_netdef_get_critical(const NetplanNetDefinition* netdef);

Extend KeepConfiguration= functionality #409

Are you sure you want to change the base?

Extend KeepConfiguration= functionality #409

Conversation

chr4 commented Sep 19, 2023 • edited

Description

Checklist

chr4 commented Sep 19, 2023

chr4 commented Sep 19, 2023

daniloegea commented Sep 21, 2023

daniloegea Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniloegea 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

slyon 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

chr4 commented Sep 19, 2023 •

edited

daniloegea Sep 21, 2023 •

edited