xcp-networkd: dhclient cleanup (v2) by semarie · Pull Request #7162 · xapi-project/xen-api

semarie · 2026-07-02T15:19:03Z

Following up of #7138

The following commits are a first step for doing some cleanup in DHCP client.

The purpose is to abstract the module and clarify function contracts, in order to permit alternative DHCP client to current (unmaintained) dhclient binary from ISC.

psafont · 2026-07-02T15:25:14Z

            if List.mem name (Sysfs.list ()) then (
-              if Dhclient.is_running name then ignore (Dhclient.stop name) ;
-              Ip.flush_ip_addr name
+              Dhclient.stop name ; Ip.flush_ip_addr name


With this the IP is flushed twice

Is it really a concern ? or should a reorder my commits content to avoid that ?

psafont · 2026-07-02T15:26:01Z

-              ignore (Dhclient.stop name) ;
-              Ip.flush_ip_addr name
-            ) ;
+            Dhclient.stop name ;


Previously the IP was only flushed when the DHCP client was running, now it's always flushed

yes, in stop function, the Ip.flush_ip_addr call should be in the if is_running branch.
thanks

psafont · 2026-07-02T15:26:36Z

-                  ignore (Dhclient.stop ~ipv6:true name) ;
+                Dhclient.stop ~ipv6:true name ;
                Sysctl.set_ipv6_autoconf name false ;
                Ip.flush_ip_addr ~ipv6:true name


IP is flushed twice, once before the autoconf.

psafont · 2026-07-02T15:26:57Z

              if List.mem name (Sysfs.list ()) then (
-                if Dhclient.is_running ~ipv6:true name then
-                  ignore (Dhclient.stop ~ipv6:true name) ;
+                Dhclient.stop ~ipv6:true name ;


And again the IP is flushed twice.

psafont · 2026-07-02T15:30:15Z

          | Static6 addrs ->
-              if Dhclient.is_running ~ipv6:true name then
-                ignore (Dhclient.stop ~ipv6:true name) ;
+              Dhclient.stop ~ipv6:true name ;


The IP is always flushed, previously it was never done.

The changes in this commit (the first) seem to be non-functional, unless I missed something

yes, in stop function, the Ip.flush_ip_addr call should be in the if is_running branch.
thanks

semarie · 2026-07-02T15:32:22Z

@psafont I am trying to do incremental changes to avoid one big unreviewable commit.
I agree that we are calling flush twice in several cases, but after "networkd: refactor set_ipv{4,6}_conf functions" commit, it shouldn't be the case anymore.

Also, the PR is currently in draft, and I am still doing some changes and review on it.

semarie · 2026-07-03T08:00:30Z

from previous version, I fixed few cases:

in is_running the pid-file content would contains "1234\n" and int_of_string_opt will always return None, so trim the string before it
have wait_stopped bounded in time (~10 sec before giving up with logging)
missing Ip.set_ipv6_link_local_addr calls in DHCP6 and Autoconf6 to properly initialize the interface with ipv6 address before using it

semarie · 2026-07-03T08:03:49Z

Regarding the regression spotted in previous PR, I think it is due to a race between stop and start where the stopped dhclient was still stopping while the new dhclient was here. The wait_stopped function should take care of that.

@changlei-li I would appreciate if you are able to run the specific test case to confirm the regression is gone

changlei-li · 2026-07-09T07:51:30Z

I rerun the test with this PR, the test still fails, but with a different error. I will have a look and update later.

changlei-li · 2026-07-10T08:23:19Z

-              if Dhclient.is_running name then ignore (Dhclient.stop name) ;
-              Ip.flush_ip_addr name
-            )
+            ()


The test case does host-management-reconfigure, then pif-reconfigure-ip the old management pif DHCP-> none, but the IP on old management pif still exists.

In fact, A simple DHCP4 -> None4 can reproduce this.

I am looking. thanks

it seems at time of set_ipv4_conf call, get_config name is already set to the new value.

2026-07-10T13:45:02.484830+02:00 srt-cogent-nested-1 xcp-networkd: [debug||335 |PIF.reconfigure_ip R:1b3adc84ef6b|network_server] Configuring IPv4 address for xenbr1: "None4" -> "None4"

it makes the deconfiguration step to be wrong.

I am putting the PR in draft for now

I think I have identified the source of the problem.

Interface.make_config is first calling update_config with the new configuration, before iterating on each parameter to call set_dns, set_ipv4_conf, set_ipv4_gateway, etc... each function is also updating the configuration (to a parameter which is already up-to-date). so when set_ipv4_conf is called, the previous configuration is already lost.

by removing the call of update_config from Interface.make_config it makes config to be updated incrementally by each function on the fly (I checked that every function called by Interface.make_config is calling update_config).

but I am unsure if it is the right path as it would have some side-effects : previously an exception in set_dns (function chosen for example, exception possible in Xapi_stdext_unix.Unixext.write_string_to_file) would abort Interface.make_config in the middle but config would be still totally updated (leading system and config state unsynchronized). so I don't know if it would fix bugs or open some new ones.

I pushed a new commit to avoid calling update_config ahead in Interface.make_config.

I take care that each field in interface_config_t to be updated (so added explicitly set_persistent call).

semarie · 2026-07-16T06:51:03Z

Last push:

rebased on master
added one commit to remove update_config from Interface.make_config

semarie · 2026-08-03T07:44:16Z

Last push:

commits rebased

`Interface.make_config` is currently calling `update_config` with the new configuration, before calling each sub-function to apply the configuration. Each of these functions is also calling `update_config` for updating the configuration. Remove the first call inside `Interface.make_config` and let's each sub-function the responsability to update the configuration and apply its on the system. Also, explicity set persistent_i to have each field updated (as before). It should also fixes some cases where exceptions inside sub-function would lead desynchronized state between the configuration (already updated) and the system (not updated). Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

…erface if running and to unconfigure addresses. No functional changes expected (only `Ip.flush_ip_addr` could be called twice instead of once). Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

… return () No functional changes expected. Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

No functional changes expected. Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

…nterface In `set_gateway_interface` and `set_dns_interface` we are removing the configuration file to trigger a restart of Dhclient on the next `ensure_running` call. By setting gateway or DNS interface, it affects the (possibly) running dhclient process and it will need restarting on the old and/or current interface. If the changes are done in A -> B -> A way, the DHCP configuration will be the same, but a restart it still needed. By marking the configuration as dirty, it ensures that the DHCP client will be restarted. No functional changes expected. Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

- use it un Dhclient.stop to return only when it is safe to start a new process again - remove the old configuration file after stopped Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

- deconfigure the previous configuration before configuring the new configuration (instead of unconfiguring all possible configurations) - ignore exception in deconfiguration to ensure configuration is always called Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

semarie · 2026-09-01T12:32:49Z

Last push:

commits rebased

@changlei-li any update on this ?

changlei-li · 2026-09-02T02:00:28Z

The purpose is to abstract the module and clarify function contracts, in order to permit alternative DHCP client to current (unmaintained) dhclient binary from ISC.

I am not clear how the PR is necessary for the purpose, so it's hard for me to feel motivated to review it. Could you help me know more details about it?

Meanwhile, I knew this PR (and its previous v1 version) introduced some regressions. Have you done some related tests: like the basic cases: PIF DHCP <-> None, DHCP <-> Static and also the IPv6 case

semarie · 2026-09-04T07:39:55Z

The purpose of the PR (as the first one) is to abstract the DHCP module and clarify function contracts, in order to permit alternative DHCP client to current (unmaintained) dhclient binary from ISC.

Abstracting the module means to change the module signature to have more "generic" approach which doesn't rely on implementation details, in order to permit having alternate implementation later (and replace the unmaintained one).

In a sense I agree that touching working code could be dangerous. But on the other side, the code is also dusting and technical debt makes new changes more error prone (like the work one the second regression spotted it: each set_... function updating the config while the config was already updated, but not all).
So yes, changing code could introduce regressions, and it is the purpose of having several eyes to spot them before they are committed.

Regarding tests on this PR, I don't recall the ones I conducted (changes are almost 2 months old now).
I will redo a test campaign with the PR.

Thanks.

changlei-li · 2026-09-04T09:17:27Z

-        ignore (stop ~ipv6 interface) ;
-        ignore (start ~ipv6 interface options)
+        internal_stop ~ipv6 ~release:false interface ;
+        start ~ipv6 interface options


Hi @semarie I start a thread here for easy discuss.

To avoid misleading, I don't mean to block you, the background I am asking is:
This PR reduces Dhclient interfaces from 4 functions (remove_conf_file, is_running, stop, ensure_running) to 3 (set_stale, stop, ensure_running). The legacy interfaces leak the mechanism (is-the-pidfile-there, delete-the-conf-file) which is too detail or may be special for one client. Is it the main motivation you want to change?

In my expectation, the abstraction is a named abstraction boundary — a module type (signature) that any client implementation satisfies, with implementations selectable at build/runtime. With it, we can see the final shape. Now I don't know your target interface you're building toward.

Several changes in the diff are behaviour changes, not pure abstraction, for example: is_running rewritten from pidfile-exists to kill pid 0, stop also flush address, removing update_config from make_config, ..., really need to be careful. Are they necessary to fit the abstraction or just improvement?

it is a bit a mix.

yes, having Dhclient signature to have the 3 functions (set_stale, stop, ensure_running) is the purpose of the PR. it is already a trade-off with the way the current module is integrated in the code : ensure_running could be see has already doing too much things for a "low-level" abstraction for any DHCP client. but the current code uses it, so why not.

I agree that some changes are behavior changes and not pure abstraction. Some are here because of multiple iterations on the code (like pidfile-exists to kill pid : it was done as possible source of the regression, fixing possible race), some others to show the proper contract between the Dhclient functions and the caller (like the refactoring of set_ipv{4,6}_conf functions which leads to remove update_config from make_config.

I could try to split the PR in a stack to properly separate commits that could introduce behavior changes and others. I would permit to advance on some parts, while keeping others for more careful review.

thanks

I just created #7256 which is behavior changes free. I will create subsequent PR based on it.

semarie · 2026-09-08T09:37:35Z

putting in draft as it might be superset by others PRs

psafont reviewed Jul 2, 2026

View reviewed changes

semarie force-pushed the dev/srt/master/dhcp-client-cleanup2 branch from f4079bb to 3163b9a Compare July 2, 2026 15:25

psafont reviewed Jul 2, 2026

View reviewed changes

semarie force-pushed the dev/srt/master/dhcp-client-cleanup2 branch 3 times, most recently from 5c6152c to aacfd34 Compare July 3, 2026 07:55

semarie marked this pull request as ready for review July 3, 2026 08:00

semarie requested a review from psafont July 3, 2026 08:05

changlei-li reviewed Jul 10, 2026

View reviewed changes

semarie marked this pull request as draft July 10, 2026 11:49

semarie force-pushed the dev/srt/master/dhcp-client-cleanup2 branch from aacfd34 to 3d254e6 Compare July 16, 2026 06:49

semarie marked this pull request as ready for review July 16, 2026 06:49

semarie requested a review from changlei-li July 20, 2026 08:21

semarie force-pushed the dev/srt/master/dhcp-client-cleanup2 branch from 3d254e6 to 2a454e2 Compare August 3, 2026 07:43

semarie added 7 commits September 1, 2026 11:53

networkd: make Dhclient.stop to stop the DHCP client managing the int…

d23c3ba

…erface if running and to unconfigure addresses. No functional changes expected (only `Ip.flush_ip_addr` could be called twice instead of once). Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

networkd: Dhclient: remove unneeded ignore and make Dhclient.start to…

cf58d3c

… return () No functional changes expected. Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

dhclient: add comments on functions

e1e7299

No functional changes expected. Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

networkd: Dhclient.is_running: check the dhclient process to be gone

6d9bcc4

- use it un Dhclient.stop to return only when it is safe to start a new process again - remove the old configuration file after stopped Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

networkd: Dhclient.stop: allow to not release addresses while stopping

0b97b61

Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

semarie force-pushed the dev/srt/master/dhcp-client-cleanup2 branch from 2a454e2 to d3d6947 Compare September 1, 2026 12:31

changlei-li reviewed Sep 4, 2026

View reviewed changes

semarie marked this pull request as draft September 8, 2026 09:36

Uh oh!

Conversation

semarie commented Jul 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

semarie commented Jul 2, 2026

Uh oh!

semarie commented Jul 3, 2026

Uh oh!

semarie commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changlei-li commented Jul 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

semarie commented Jul 16, 2026

Uh oh!

semarie commented Aug 3, 2026

Uh oh!

semarie commented Sep 1, 2026

Uh oh!

changlei-li commented Sep 2, 2026

Uh oh!

semarie commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

semarie commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

semarie commented Jul 3, 2026 •

edited

Loading