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

`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-07-16T06:51:03Z

Last push:

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

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 added 8 commits July 16, 2026 08:47

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

8d26fde

…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…

fafed9a

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

dhclient: add comments on functions

78e4806

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

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

6336899

- 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

dc51d4f

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

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

semarie commented Jul 3, 2026 •

edited

Loading