keepalived: fix reload_service to honor the enabled toggle

[Jackie264]

Description:

reload_service() unconditionally sent a signal to an already running instance:

reload_service() {
	process_config
	#SIGHUP is used by keepalived to do init.d reload
	procd_send_signal keepalived
}

This never checked globals.enabled and, more importantly, never opened a new procd instance. Only start_service() (invoked via the "start" action) does that. So toggling globals.enabled from '0' back to '1' through UCI and triggering a reload (as LuCI's apply flow does via the registered reload trigger) left the service stopped: procd_send_signal has nothing to signal when no instance is running, and reload_service() never fell back to actually starting one. Only a full "restart" from the command line worked, because restart calls start_service() through the normal "start" action.

A first attempt at fixing this called "stop" from within reload_service() whenever the service should not be running. That is wrong too: "stop" deregisters the whole service object from procd, including the reload trigger registered in service_triggers(). Once stopped that way, procd no longer knows "keepalived" exists at all, so no future UCI change is ever delivered to it again -- only a manual "start" from the command line can bring it back under procd's management.

Rework reload_service() to:

• keep using the lightweight signal-based reload when an instance is already running and should stay enabled;
• otherwise call "start" (not "stop", and not start_service() directly). "start" goes through rc.common's rc_procd wrapper (procd_open_service/procd_close_service), which is required for procd to actually register the service and its trigger; calling start_service() on its own builds procd's JSON description without ever submitting it over ubus, so nothing is registered. start_service() already returns early without opening an instance when globals.enabled is '0', so routing both the "should be running" and "should not be running" cases through "start" keeps the service registered with procd (and its reload trigger intact) in both states, while still ending up with zero running instances when disabled.

procd_running() (lib/functions/procd.sh) is used to tell an already-running instance from a stopped one, so a plain config tweak on an already-running instance still takes the cheap SIGHUP path instead of a full restart.

Related PR: openwrt/luci#9062

🧪 Run Testing Details

• OpenWrt Version: 25.12.5 r33051-f5dae5ece4 & SNAPSHOT r36403-c5e1634aae
• OpenWrt Target/Subtarget: ipq40xx/generic & qualcommbe/ipq95xx & qualcommax/ipq807x & x86/64
• OpenWrt Device: Linksys MR9000 & Linksys WHW03v2 & Askey SBE1V1K & Linksys MX5300 & AZW EQ

---

✅ Formalities

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

Signed-off-by: Jackie Han jackie.han@gmail.com

[openwrt-ai]

Reviewed 2 new commits.

---

Generated by Claude Code

[openwrt-ai]

nit: the release bump belongs in the same commit as the change that requires it; split this way, c50a61b ships the behaviour change with a stale PKG_RELEASE. Please squash 2688836 into the fix commit.

---

Generated by Claude Code

[stokito]

This probably should be fixed in the procd itself. See how the reload is implemented in the procd https://github.com/openwrt/openwrt/blob/main/package/base-files/files/etc/rc.common#L165
If the service implements the reload function then it will call it. Otherwise it will (re?)start the service.

This function in fact should first check if the service is already started and start it if needed.

So the current implementation is correct.

Actually there are many of other services that should use the same approach
https://github.com/search?q=repo%3Aopenwrt%2Fpackages+reload_service&type=code

reload_service() {
	procd_send_signal "$PROG"
}

In fact, maybe the reload service should not start it. Maybe it was stopped manually but the reload is called from some script.

See docs:

• https://openwrt.org/docs/guide-developer/procd-init-scripts#custom_service_reload
• https://openwrt.org/docs/guide-user/base-system/managing_services

@feckert could you join the discussion? You implemented the reload 0e88bff

[Jackie264]

Thanks for digging into this, and for the links -- the point about reload() defaulting to restart() when a package has no
reload_service() at all is well taken, and I agree in principle that "should a generic reload be allowed to (re)start an instance it finds not running" is a question that ideally gets answered once in rc.common, not reinvented per-package. I don't think this PR is the right place to make that call for the whole tree though -- happy to be corrected if there's appetite for it.

On "maybe reload should not start it, maybe it was stopped manually": that's exactly the scenario I want to avoid, and I believe the current implementation already avoids it, for a reason specific to this package rather than a generic reload argument: keepalived.init now has a UCI-declared globals.enabled knob whose whole purpose is to state the desired run state. reload_service()'s "else start" branch only fires when enabled='1' -- it never fires "just because" the daemon happens to be stopped. A plain /etc/init.d/keepalived stop from the command line (with enabled left at '1' in UCI) fully deregisters the service from procd -- ubus call service list goes from {"keepalived": {}} to {} -- which also tears down the trigger registered in service_triggers(). Once that happens, no subsequent UCI change can reach reload_service() at all, so there's no path by which a manual stop gets silently reversed by an unrelated config change.

The only way to end up "registered but with zero instances" (the state the "else start" branch can act on) is by going through this same reload_service() with enabled='0', which is exactly the disable path this PR is also fixing. So as far as I can tell the "start" branch is only ever reconciling to an explicit UCI value, never guessing.

That said, I'm not 100% sure I'm not missing some other procd trigger path that could reach a running-but-registered-empty state some other way -- if you (or @feckert) know of one, I'd like to hear it, since that's exactly the kind of edge case this should account for.

If the preference is still to keep reload_service() to the conventional signal-only form and not have it start anything, I'm fine with that too -- but then flipping "Enabled" back on in LuCI needs some other explicit action (e.g. LuCI calling start separately after apply, or just documenting that a manual start/restart is required), otherwise the original bug this PR set out to fix (re-enabling from the UI silently does nothing) comes back. Curious what you and @feckert think is the right tradeoff here.

[openwrt-ai]

Reviewed 2 new commits.

---

Generated by Claude Code

[openwrt-ai]

nit: the comment explaining that the bare signal means SIGHUP was dropped with no replacement; without it the reader has to look up _procd_send_signal to see which signal is sent. Keep it.

Suggested change

	procd_send_signal keepalived
	# SIGHUP is used by keepalived to do init.d reload
	procd_send_signal keepalived

---

Generated by Claude Code

[feckert]

@Jackie264 @stokito If the /etc/rc.d symlink is set for keepalived, the init script should start the service. From there, behavior depends on the "global enable" setting:

• If "global enable" is set to 1, the keepalived binary starts with the generated configuration and install the reload trigger on config change for /etc/config/keepalived.
• If "global enable" is set to 0, the binary does not start and not config for keepalived is generated. Instead, only a trigger is installed that reloads the service whenever /etc/config/keepalived is changed.

I think your change adds that.

[Jackie264]

Thanks @feckert, that matches exactly what this PR implements -- enabled=1 starts the daemon and installs the reload trigger, enabled=0 skips starting the daemon but still goes through the same start action (via rc_procd) so the trigger stays installed and future UCI changes can still bring it back. Good to know that lines up with the intended design.

@stokito does this address your concern about reload resurrecting a manually-stopped instance? Per feckert's description, the service's running state is meant to be fully driven by the enabled UCI value rather than by whether it was previously started/stopped some other way, which is what this change enforces.