isc-dhcp: don't crash if the network is restarted

[pprindeville]

📦 Package Details

Maintainer: me

Description:
Previously if the network interfaces (bridges and vlans especially) went away and dhcpd was listening on them, it would die ignominiously. Add support for restarting the service is the interfaces it was listening on get kicked out from under it.

---

🧪 Run Testing Details

• OpenWrt Version: HEAD
• OpenWrt Target/Subtarget: x86_64/generic
• OpenWrt Device: pc-engines-apu6

---

✅ Formalities

• I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

If your PR contains a patch:

N/A

cc: @nmeyerhans @Alphix @oldium @stintel

[pprindeville]

I'm not sure that we need add_interface_trigger or service_triggers or both, as it wasn't getting the job done.

Anyone got an opinion?

[pprindeville]

cc: @aparcar @neheb How's your procd-fu?

[pprindeville]

cc: @oldium Moving the conversation here. I don't think I need to worry about interfaces not being present, since this is usually run on lan (and not, for instance, a modemmanager 4G/5G connection where you have to wait for carrier, etc).

[oldium]

cc: @oldium Moving the conversation here. I don't think I need to worry about interfaces not being present, since this is usually run on lan (and not, for instance, a modemmanager 4G/5G connection where you have to wait for carrier, etc).

The point is really not to worry about interfaces not being up, and:

1. start with those interfaces that are up, or
2. add procd service definition first when all expected interfaces are up, not earlier.

The procd triggers ensure that you get notified (i.e. start_service being called) at the time when some interface goes up or down (and it is up to you to decide whether it is enough to start - meaning to define the service via procd_* or not), the netdev param in the service definition ensures that the service definition gets reevaluated when something about the interface changes (not sure what exactly 😅).

Just keep in mind that the service definition does not currently contain the configuration file, so if you regenerate the config file as part of the service definition (i.e. in the start_service), the procd will not reload/restart the service automatically, because it sees the same executable and the same parameters in the service definition. There is a way to tell procd to see the config with procd_set_param file (mentioned here), in which case you will get a reload request and you can send a signal.

In that context I am not sure about the service_reload implementation, because it calls start_service. The example here shows just sending the signal - I think it also expects the procd_set_param file value when a configuration file is used.

[pprindeville]

Does procd_add_reload_interface_trigger want an interface or a device (i.e. Linux interface name)?

[pprindeville]

@oldium With this latest version we don't die, but we still see that last line which is bothersome. Would adding a delay somewhere help, and if so, where?

Jan  7 17:37:20 OpenWrt2 dhcpd: Internet Systems Consortium DHCP Server 4.4.3-P1
Jan  7 17:37:20 OpenWrt2 dhcpd: Copyright 2004-2022 Internet Systems Consortium.
Jan  7 17:37:20 OpenWrt2 dhcpd: All rights reserved.
Jan  7 17:37:20 OpenWrt2 dhcpd: For info, please visit https://www.isc.org/software/dhcp/
Jan  7 17:37:20 OpenWrt2 dhcpd: Wrote 0 deleted host decls to leases file.
Jan  7 17:37:20 OpenWrt2 dhcpd: Wrote 0 new dynamic host decls to leases file.
Jan  7 17:37:20 OpenWrt2 dhcpd: Wrote 0 leases to leases file.
Jan  7 17:37:20 OpenWrt2 dhcpd: Server starting service.
Jan  7 17:37:34 OpenWrt2 dhcpd: receive_packet failed on eth1: Network is down

Also, do we want to reload every time someone touches /etc/config/dhcp? That sounds bad. There are plenty of times that I modify it in advance of a reboot or explicit restart (sometimes via crontab), wanting to exactly time machines getting different parameters, etc.

Trying to not make too many changes at once and break things...

[oldium]

@oldium With this latest version we don't die, but we still see that last line which is bothersome. Would adding a delay somewhere help, and if so, where?

Jan  7 17:37:20 OpenWrt2 dhcpd: Internet Systems Consortium DHCP Server 4.4.3-P1
Jan  7 17:37:20 OpenWrt2 dhcpd: Copyright 2004-2022 Internet Systems Consortium.
Jan  7 17:37:20 OpenWrt2 dhcpd: All rights reserved.
Jan  7 17:37:20 OpenWrt2 dhcpd: For info, please visit https://www.isc.org/software/dhcp/
Jan  7 17:37:20 OpenWrt2 dhcpd: Wrote 0 deleted host decls to leases file.
Jan  7 17:37:20 OpenWrt2 dhcpd: Wrote 0 new dynamic host decls to leases file.
Jan  7 17:37:20 OpenWrt2 dhcpd: Wrote 0 leases to leases file.
Jan  7 17:37:20 OpenWrt2 dhcpd: Server starting service.
Jan  7 17:37:34 OpenWrt2 dhcpd: receive_packet failed on eth1: Network is down

Also, do we want to reload every time someone touches /etc/config/dhcp? That sounds bad. There are plenty of times that I modify it in advance of a reboot or explicit restart (sometimes via crontab), wanting to exactly time machines getting different parameters, etc.

Trying to not make too many changes at once and break things...

Ok, checked the code again. This is what is happening:

1. When trigger is activated, it reloads, because here and here.
2. When reload is called, the inner reload_service is called.
3. It seems you are correct in calling start_service to update the service definition from the reload_service (with one obvious example here).
4. The procd then evaluates the new service definition.

The procd part is also important in the way how it behaves when the service definition is updated:

1. If the configuration change is detected (command line, file's content MD5...), the service is restarted inside procd code (in this function).
2. Restart means either sending a pre-defined signal or sending SIGTERM and starting a new instance.

So in case you actually add configuration to files (with procd_set_param file), the situation might get worse (if I read the code correctly):

1. If you defined a signal (via procd_set_param reload_signal), the change in command-line arguments would only send the signal. This is bad.
2. You do not have reload_signal defined, so in case the command-line changes, you will get SIGTERM. So if you add config file into the configuration, you might be terminated instead of reloaded via your signal.

Note: You are changing the command line (the $dhcp_ifs is dynamic), so you need a restart when the command-line changes.

So I think it would be beneficial for you to evaluate the configuration changes by yourself - generate a new configuration and compare the content with the existing one. You can always overwrite the old content after making the check. And if the content changed, send the signal. This way any unrelated changes to dhcp or other triggers would not cause any reload/restart. And changes to command line will cause the physical process restart (this will be handled by procd itself).

[pprindeville]

@oldium I'm hoping that my last changes to libubox get merged soon (they're minor) and I can push out the Kea backend to replace ISC-DHCP.

There are a couple of directives that Kea doesn't support that ISC-DHCP did, but bugs have been filed for them and I'll support them as they get released. @fxdupont