openwrt / packages

Community maintained packages for OpenWrt. Documentation for submitting pull requests is in CONTRIBUTING.md
GNU General Public License v2.0
3.94k stars 3.45k forks source link

ddns-scripts: dynamic_dns_updater.sh executes multiple times for each DNS name #23135

Closed McGiverGim closed 4 months ago

McGiverGim commented 7 months ago

Maintainer: @chris5560, @maxberger, @dibdot Environment: Xiaomi AX3600, OpenWrt snapshot

Description: I've four DNS names to update with the ddns scripts (they are two, but one for IPv4 and the other for IPv6, so four in total). Looking at the "processes" of my machine, I can see the four scripts under execution:

 5537 root      1712 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
 5538 root      1704 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
 5539 root      1712 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
 5540 root      1704 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start

all is ok, but sometimes, the scripts appear repeated, for example:

16880 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
16882 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
16913 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
16914 root      1724 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
16915 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
16916 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
16925 root      1720 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
16926 root      1712 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
16927 root      1704 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
16940 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
16944 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
16945 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
16946 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
16947 root      1704 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
16948 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
16950 root      1712 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
16953 root      1696 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
16975 root      1724 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
16976 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
16977 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
16984 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
16985 root      1724 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
16986 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
16987 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
16997 root      1704 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
16998 root      1720 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
16999 root      1712 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
17000 root      1696 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
17005 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
17006 root      1724 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
17009 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
17025 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
17026 root      1724 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
17027 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
17029 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
17054 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
17070 root      1704 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
17073 root      1704 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start
17140 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org -- start
17141 root      1724 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S xxxx_mywire_org_ipv6 -- start
17142 root      1716 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org -- start
17143 root      1708 S    {dynamic_dns_upd} /bin/sh /usr/lib/ddns/dynamic_dns_updater.sh -v 0 -S wg_xxxx_mywire_org_ipv6 -- start

I don't know what is the culprit that make repeat the execution of the scripts so I can't help with the origin, only the result.

McGiverGim commented 7 months ago

I think I've been able to reproduce the problem... Executing:

service ddns reload

reproduces the problem.

McGiverGim commented 7 months ago

More info: looking at the code, there is a killall command:

killall -1 dynamic_dns_updater.sh 2>/dev/null

https://github.com/openwrt/packages/blob/f9c90fce0aad9ebec04e09b10843da3571c09f83/net/ddns-scripts/files/usr/lib/ddns/dynamic_dns_updater.sh#L92C3-L92C48

This command seems to produce the problem. If I remove the -1 from the command it seems to work, but executing with it I can reproduce the problem.

maxberger commented 7 months ago

Kill -1 is sighup, kill without -1 defaults to sigterm

I would need to check the script to see what it should do in each case. Killall is probably a bad idea in case someone runs multiple updates (like you do)

McGiverGim commented 7 months ago

I don't see it a bad idea. It simply tries to "kill" all instances to restart all of them. I saw the -1 was added some years ago, the strange thing is that this bug was not noticed, maybe when added it worked and other change starting to produce this behaviour later? I don't know too much about Linux, so I can't see the difference between sighup and sigterm, but in the worst case, removing the -1 seems to work.

McGiverGim commented 7 months ago

I've done more tests and it's the killall with the -1 who duplicates the processes. It seems when it receives the sighup it creates another process without dying.

McGiverGim commented 5 months ago

@maxberger some advance on this? If not, at least as workaround, we can remove the "-1", it seems to work and not duplicates the processes.

Ansuel commented 4 months ago

@McGiverGim we should investigate each command that the script run... some process are probably not reacting and getting stuck with a respawn loop as you are seeing

feckert commented 4 months ago

Not against the author of this. But the ddns-scripts package is overloaded and quite a mess to work with when I look at the source code. I think it would be done very differently today. It starts with the fact that I would not choose a shell script for this. Fixing something there is dangerous!

In the meantime, I've reached the point where I'm considering a rewrite. The whole thing can be made much leaner and tidier.

Ansuel commented 4 months ago

@feckert I agree it was done in really old times where openwrt had little utility tech to handle these kind of thing. The shell script are done with correct logic and concepts but in the time with many contributors putting hands and fixes the original solid logic was a bit missed and now it's on life support.

Would love to see a rewrite but little time now :(

Thing is that i think the json base and how all the require things for a ddns are ok so i think only a rewrite of the thing is needed without making change to all the args it does use.

feckert commented 4 months ago

@Ansuel From my point of view, I would have to make one refactoring commit and then put it on a new base. I don't know if there's enough time, but I would be willing to change and do this. I already had the pleasure of changing the handling to json a few years ago.

McGiverGim commented 4 months ago

I know is out of this "refactor" but as suggestion for the future, the major part of DDNS providers let update the IPv4 and IPv6 at the same time. It will be interesting to have this possibility in OpenWrt too: update both with only one DDNS rule, now it needs to have two, one for IPv4 and the other for IPv6. But as I say, is only a suggestion for the future. It's more complicated that what it seems.

srd424 commented 1 month ago

I've just hit this bug but I'm not sure I understand why converting from SIGHUP -> SIGTERM fixes it? There's specific code to trap HUP and react appropriately .. if we just kill everything, what restarts it?

On my 23.05 machine, making this change just seems to leave no updater processes running at all?

srd424 commented 1 month ago

I'm not actually sure why separate reload logic is necessary, given that all the reload code is trying to do is start a new process and let the old one exit?