Skip to content

xcp-networkd: dhclient cleanup (v2) #7162

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

Open
semarie wants to merge 8 commits into
base: master
Choose a base branch
from
Open

xcp-networkd: dhclient cleanup (v2) #7162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0a4fcbb
Interface.make_config: do not update update_config in advance
semarie Jul 10, 2026
6fa6d28
networkd: make Dhclient.stop to stop the DHCP client managing the int…
semarie Jun 19, 2026
a3c1652
networkd: Dhclient: remove unneeded ignore and make Dhclient.start to…
semarie Jul 2, 2026
3c129dd
dhclient: add comments on functions
semarie Jun 19, 2026
f27ea41
dhclient: add `set_stale` (and remove `remove_conf_file`) in public i…
semarie Jun 19, 2026
3b0fbde
networkd: Dhclient.is_running: check the dhclient process to be gone
semarie Jun 19, 2026
3d3e2ee
networkd: Dhclient.stop: allow to not release addresses while stopping
semarie Jun 19, 2026
2a454e2
networkd: refactor set_ipv{4,6}_conf functions
semarie Jun 19, 2026
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
162 changes: 80 additions & 82 deletions ocaml/networkd/bin/network_server.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,27 +218,27 @@ let reset_state () =
config := Network_config.read_management_conf reset_order

let set_gateway_interface _dbg name =
(* Remove dhclient conf (if any) for the old and new gateway interfaces.
* This ensures that dhclient gets restarted with an updated conf file when
(* Mark the DHCP configuration as stale for the old and new gateway interfaces.
* This ensures that DHCP client will be restarted with an updated conf file when
* necessary. *)
( match !config.gateway_interface with
| Some old_iface when name <> old_iface ->
Dhclient.remove_conf_file name ;
Dhclient.remove_conf_file old_iface
Dhclient.set_stale name ;
Dhclient.set_stale old_iface
| _ ->
()
) ;
debug "Setting gateway interface to %s" name ;
config := {!config with gateway_interface= Some name}

let set_dns_interface _dbg name =
(* Remove dhclient conf (if any) for the old and new DNS interfaces.
* This ensures that dhclient gets restarted with an updated conf file when
(* Mark the DHCP configuration as stale for the old and new DNS interfaces.
* This ensures that DHCP client will be restarted with an updated conf file when
* necessary. *)
( match !config.dns_interface with
| Some old_iface when name <> old_iface ->
Dhclient.remove_conf_file name ;
Dhclient.remove_conf_file old_iface
Dhclient.set_stale name ;
Dhclient.set_stale old_iface
| _ ->
()
) ;
Expand Down Expand Up @@ -474,6 +474,17 @@ module Interface = struct
)
()

let config_to_dhcp_options config =
let gateway =
Option.fold ~none:[]
~some:(fun n -> [`gateway n])
config.gateway_interface
in
let dns =
Option.fold ~none:[] ~some:(fun n -> [`dns n]) config.dns_interface
in
gateway @ dns

let get_ipv4_addr dbg name =
Debug.with_thread_associated dbg (fun () -> Ip.get_ipv4 name) ()

Expand All @@ -482,33 +493,27 @@ module Interface = struct
(fun () ->
debug "Configuring IPv4 address for %s: %s" name
(conf |> Rpcmarshal.marshal typ_of_ipv4 |> Jsonrpc.to_string) ;
update_config name {(get_config name) with ipv4_conf= conf} ;
let previous_config = get_config name in
let previous = previous_config.ipv4_conf in
update_config name {previous_config with ipv4_conf= conf} ;
(* deconfigure previous *)
Xapi_stdext_pervasives.Pervasiveext.ignore_exn (fun () ->
match previous with
| None4 ->
()
| DHCP4 ->
if conf <> DHCP4 then Dhclient.stop name
| Static4 _ -> (
match conf with Static4 _ -> () | _ -> Ip.flush_ip_addr name
)
) ;
(* configure conf *)
match conf with
| None4 ->
if List.mem name (Sysfs.list ()) then (
if Dhclient.is_running name then ignore (Dhclient.stop name) ;
Ip.flush_ip_addr name
)
()

@changlei-li changlei-li Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@semarie semarie Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am looking. thanks

@semarie semarie Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@semarie semarie Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@semarie semarie Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

| DHCP4 ->
let gateway =
Option.fold ~none:[]
~some:(fun n -> [`gateway n])
!config.gateway_interface
in
let dns =
Option.fold ~none:[]
~some:(fun n -> [`dns n])
!config.dns_interface
in
if not (Dhclient.is_running name) then (* Remove any static IPs *)
Ip.flush_ip_addr name ;
let options = gateway @ dns in
Dhclient.ensure_running name options
Dhclient.ensure_running name (config_to_dhcp_options !config)
| Static4 addrs ->
if Dhclient.is_running name then (
ignore (Dhclient.stop name) ;
Ip.flush_ip_addr name
) ;
(* the function is meant to be idempotent and we want to avoid
CA-239919 *)
let cur_addrs = Ip.get_ipv4 name in
Expand Down Expand Up @@ -569,53 +574,47 @@ module Interface = struct
else (
debug "Configuring IPv6 address for %s: %s" name
(conf |> Rpcmarshal.marshal typ_of_ipv6 |> Jsonrpc.to_string) ;
update_config name {(get_config name) with ipv6_conf= conf} ;
let previous_config = get_config name in
let previous = previous_config.ipv6_conf in
update_config name {previous_config with ipv6_conf= conf} ;
(* deconfigure previous *)
Xapi_stdext_pervasives.Pervasiveext.ignore_exn (fun () ->
match previous with
| None6 ->
()
| Linklocal6 ->
if conf <> Linklocal6 then Ip.flush_ip_addr ~ipv6:true name
| DHCP6 ->
if conf <> DHCP6 then Dhclient.stop ~ipv6:true name
| Autoconf6 ->
if conf <> Autoconf6 then (
Sysctl.set_ipv6_autoconf name false ;
Ip.flush_ip_addr ~ipv6:true name
)
| Static6 _ -> (
match conf with
| Static6 _ ->
()
| _ ->
Ip.flush_ip_addr ~ipv6:true name
)
) ;
(* configure conf *)
match conf with
| None6 ->
if List.mem name (Sysfs.list ()) then (
if Dhclient.is_running ~ipv6:true name then
ignore (Dhclient.stop ~ipv6:true name) ;
Sysctl.set_ipv6_autoconf name false ;
Ip.flush_ip_addr ~ipv6:true name
)
()
| Linklocal6 ->
if List.mem name (Sysfs.list ()) then (
if Dhclient.is_running ~ipv6:true name then
ignore (Dhclient.stop ~ipv6:true name) ;
Sysctl.set_ipv6_autoconf name false ;
Ip.flush_ip_addr ~ipv6:true name ;
Ip.set_ipv6_link_local_addr name
)
Ip.set_ipv6_link_local_addr name
| DHCP6 ->
let gateway =
Option.fold ~none:[]
~some:(fun n -> [`gateway n])
!config.gateway_interface
in
let dns =
Option.fold ~none:[]
~some:(fun n -> [`dns n])
!config.dns_interface
in
if Dhclient.is_running ~ipv6:true name then
ignore (Dhclient.stop ~ipv6:true name) ;
Sysctl.set_ipv6_autoconf name false ;
Ip.flush_ip_addr ~ipv6:true name ;
Ip.set_ipv6_link_local_addr name ;
let options = gateway @ dns in
ignore (Dhclient.ensure_running ~ipv6:true name options)
Dhclient.ensure_running ~ipv6:true name
(config_to_dhcp_options !config)
| Autoconf6 ->
if Dhclient.is_running ~ipv6:true name then
ignore (Dhclient.stop ~ipv6:true name) ;
Ip.flush_ip_addr ~ipv6:true name ;
Ip.set_ipv6_link_local_addr name ;
Sysctl.set_ipv6_autoconf name true
(* Cannot link set down/up due to CA-89882 - IPv4 default route
cleared *)
cleared *)
| Static6 addrs ->
if Dhclient.is_running ~ipv6:true name then
ignore (Dhclient.stop ~ipv6:true name) ;
Sysctl.set_ipv6_autoconf name false ;
(* add the link_local and clean the old one only when needed *)
let cur_addrs =
let addrs = Ip.get_ipv6 name in
Expand Down Expand Up @@ -910,20 +909,19 @@ module Interface = struct
List.iter
(function
| ( name
, ( {
ipv4_conf
; ipv4_gateway
; ipv6_conf
; ipv6_gateway
; ipv4_routes
; dns
; mtu
; ethtool_settings
; ethtool_offload
; _
} as c
) ) ->
update_config name c ;
, {
ipv4_conf
; ipv4_gateway
; ipv6_conf
; ipv6_gateway
; ipv4_routes
; dns
; mtu
; ethtool_settings
; ethtool_offload
; persistent_i
} ) ->
exec (fun () -> set_persistent dbg name persistent_i) ;
exec (fun () ->
match dns with
| None ->
Expand Down
Loading
Loading