[feature] Added SIGUSR2 support to force config reload #245
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a CLI option Sequence Diagram(s)sequenceDiagram
participant CLI as openwisp-config CLI
participant OS as Operating System
participant Agent as openwisp-config Agent
participant FS as File System
participant Config as Configuration Subsystem
CLI->>OS: invoke --force-update
OS->>Agent: send SIGUSR2
activate Agent
Agent->>Agent: handle_sigusr2()
Agent->>Agent: log "forced reload"
Agent->>FS: remove CONFIGURATION_CHECKSUM
Agent->>FS: remove PERSISTENT_CHECKSUM
Agent->>Agent: set FORCE_RELOAD=1
deactivate Agent
loop Agent main loop
Agent->>Agent: check FORCE_RELOAD
alt FORCE_RELOAD == 1
Agent->>Config: update_configuration()
Config->>FS: apply configuration changes
Agent->>Agent: FORCE_RELOAD=0
else
Agent->>Agent: configuration_changed()?
alt changed
Agent->>Config: update_configuration()
Config->>FS: apply configuration changes
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @openwisp-config/files/openwisp.agent:
- Around line 981-982: The trap references an undefined handler handle_sigusr2,
so add a function named handle_sigusr2 (placed near handle_sigusr1) that logs
receipt of SIGUSR2 and sets FORCE_RELOAD=1 so the trap can call it; mirror the
pattern used by handle_sigusr1 (use logger with a descriptive message, same
tag/priority) and ensure the function name matches the trap declaration.
- Around line 967-976: When FORCE_RELOAD is 1 the script skips
configuration_changed but does not remove cached checksums; modify the
FORCE_RELOAD branch around the FORCE_RELOAD check so that before calling
update_configuration it removes checksum files/directories under
/tmp/openwisp/checksum and /etc/openwisp/checksum (use safe rm -f/rm -rf
semantics) then calls update_configuration and resets FORCE_RELOAD=0; also
normalize indentation in that entire block (use consistent spaces) and keep the
existing configuration_changed() path unchanged.
- Line 960: Define the missing signal handler function handle_sigusr2 similar to
handle_sigusr1: implement handle_sigusr2() { FORCE_RELOAD=1; } (optionally
include a brief log like echo or logger), so the existing trap line `trap
handle_sigusr2 USR2` will set FORCE_RELOAD to 1 when SIGUSR2 is received.
🧹 Nitpick comments (1)
openwisp-config/files/openwisp.agent (1)
973-975: Consider using idiomatic shell pattern for return value check.The current pattern works but can be simplified using
if configuration_changed; thenfor better readability. Note thatconfiguration_changedreturns 1 when changed and 0 when unchanged, so the logic would need adjustment.♻️ Alternative approach (optional)
else - configuration_changed - if [ "$?" -eq "1" ]; then + if configuration_changed; [ "$?" -eq "1" ]; then update_configuration fi fiOr keep the current approach but capture the return value explicitly for clarity:
else configuration_changed - if [ "$?" -eq "1" ]; then + local changed=$? + if [ "$changed" -eq "1" ]; then update_configuration fi fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp-config/files/openwisp.agent
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: QA-Checks and Tests
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
dce4df0 to
7d6cb05
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for forcing an immediate configuration reload via the SIGUSR2 signal, allowing administrators to bypass the checksum validation and trigger an immediate update of the agent configuration.
Changes:
- Added FORCE_RELOAD flag initialization and logic to skip checksum check when flag is set
- Added trap handler for SIGUSR2 signal to enable forced reload capability
- Modified main loop conditional logic to check FORCE_RELOAD flag before configuration_changed check
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @openwisp-config/files/openwisp.agent:
- Around line 981-982: The trap references a missing function handle_sigusr2
which will cause a runtime error; add a handler function named handle_sigusr2
(placed near handle_sigusr1) that logs receipt of SIGUSR2 via logger (include a
descriptive message, tag, and priority) and sets FORCE_RELOAD=1 so the rest of
the script knows to force a configuration reload; ensure the function name
exactly matches handle_sigusr2 referenced by the trap.
🧹 Nitpick comments (2)
openwisp-config/files/openwisp.agent (2)
968-975: Inconsistent indentation.The indentation mixes tabs and spaces, which is inconsistent with the rest of the file. The existing codebase uses single-tab indentation.
972-975: Consider capturing exit code explicitly.Using
$?on a separate line is fragile if code is added between the command and the check. Consider capturing the exit code in a variable for clarity.♻️ Suggested improvement
- configuration_changed - if [ "$?" -eq "1" ]; then + configuration_changed + local changed=$? + if [ "$changed" -eq "1" ]; then update_configuration fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp-config/files/openwisp.agent
🔇 Additional comments (1)
openwisp-config/files/openwisp.agent (1)
960-960: LGTM!The
FORCE_RELOADvariable is correctly initialized before entering the main loop.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @openwisp-config/files/openwisp.agent:
- Around line 972-981: The block mixes tabs/spaces and uses a fragile
separate-$? check; normalize indentation to consistent spaces and replace the
separate exit-code test by testing configuration_changed directly (e.g., use "if
configuration_changed; then" semantics) so you call update_configuration only
when configuration_changed returns success, keep FORCE_RELOAD logic unchanged
(check FORCE_RELOAD == 1 then call update_configuration and reset it), and
ensure all lines use the same indentation style and level around FORCE_RELOAD,
update_configuration, and configuration_changed.
- Around line 917-922: handle_sigusr2 currently only sets FORCE_RELOAD=1 and
uses spaces for indentation; update handle_sigusr2 to also remove checksum files
by running safe removals (e.g., rm -f) for /tmp/openwisp/checksum and
/etc/openwisp/checksum before setting FORCE_RELOAD, ensuring the removals
tolerate missing files and do not abort on error, and convert the function body
indentation to tabs to match the file style; keep the existing logger call and
variable names (handle_sigusr2, FORCE_RELOAD) so the behavior is preserved while
ensuring checksum cleanup on forced reload.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp-config/files/openwisp.agent
🔇 Additional comments (3)
openwisp-config/files/openwisp.agent (3)
965-965: LGTM!The
FORCE_RELOADvariable is correctly initialized before the main loop begins.
986-992: LGTM!The USR2 trap setup and teardown correctly mirrors the existing USR1 pattern, ensuring the signal is handled during the sleep interval and ignored otherwise.
917-992: Address missing tests and documentation before merge.The PR checklist indicates that tests and documentation are not done. Consider adding:
- Unit/integration tests to verify SIGUSR2 triggers the forced reload.
- Documentation describing how users can send SIGUSR2 (e.g.,
kill -USR2 $(pidof openwisp.agent)).The optional CLI flags (
--check-conf,--force-conf) mentioned in issue #181 could be deferred to a follow-up PR.
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
b82f6bb to
ff3c544
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
ff3c544 to
206e15e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @openwisp-config/files/openwisp.agent:
- Around line 972-981: The script checks FORCE_RELOAD in the loop but never
initializes it; add FORCE_RELOAD=0 before the main loop (the while true block)
so the test [ "$FORCE_RELOAD" -eq "1" ] is safe on the first iteration; locate
the while true loop and insert FORCE_RELOAD=0 just above it, leaving the
existing logic around configuration_changed and update_configuration unchanged
(references: FORCE_RELOAD, configuration_changed, update_configuration).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp-config/files/openwisp.agent
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Upload Packages
- GitHub Check: Agent
🔇 Additional comments (2)
openwisp-config/files/openwisp.agent (2)
917-923: LGTM!The handler correctly logs the signal, removes both checksum files as required by issue #181, and sets the
FORCE_RELOADflag. This aligns with the existinghandle_sigusr1pattern.
985-996: LGTM!The trap registration and clearing for USR2 follows the established pattern for USR1. The inline comment with the example command is helpful for users.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
206e15e to
0154554
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
0154554 to
5e55e70
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @openwisp-config/files/openwisp.agent:
- Around line 917-922: In handle_sigusr2(), before setting FORCE_RELOAD=1,
remove the stale checksum files (/tmp/openwisp/checksum and
/etc/openwisp/checksum) so update_configuration() cannot read a stale md5 and
abort with "Configuration checksum mismatch!"; reference the handler name
handle_sigusr2 and ensure deletion occurs prior to triggering the forced reload
logic that relies on FORCE_RELOAD and update_configuration().
- Around line 975-984: When FORCE_RELOAD is set, we skip configuration_changed()
so get_checksum() never runs and REMOTE_CHECKSUM stays empty; modify the
forced-reload branch to call get_checksum (the same function used by
configuration_changed) before calling update_configuration() so REMOTE_CHECKSUM
is populated, then clear FORCE_RELOAD as before; reference update_configuration,
configuration_changed, get_checksum, handle_sigusr2 and REMOTE_CHECKSUM to
locate and update the logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp-config/files/openwisp.agent
🔇 Additional comments (2)
openwisp-config/files/openwisp.agent (2)
965-968: LGTM!Good placement of initialization before the loop with clear explanatory comments.
988-999: LGTM!Signal trap registration and cleanup is correctly symmetric for both USR1 and USR2. The example command in the comment is a helpful addition for users.
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
5e55e70 to
97efd99
Compare
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
e06bb40 to
71c2278
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks good, I will test it manually asap, in the meantime see my comments below.
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
71c2278 to
dc7e034
Compare
Viscous106
force-pushed
the
agent-sigusr2-reload
branch
from
dc7e034 to
b8ee597
Compare
nemesifier
changed the title
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Viscous106 I did a round of manual testing and it's working as expected, thanks! 👍
I did some minor changes, waiting for the CI Build before merging.
github-project-automation
bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp-config/files/openwisp.agent`:
- Around line 12-22: The current AGENT_PID variable from pgrep may contain
multiple space-separated PIDs; instead of passing the quoted string to kill,
split and iterate over each PID and call kill -USR2 on each one. Modify the
block that sets AGENT_PID=$(pgrep -P 1 -f openwisp-config) to check for empty
result, then loop over the tokens in AGENT_PID (for pid in $AGENT_PID; do ...
done), verify each pid is non-empty and numeric before running kill -USR2
"$pid", and update the echo to report which PIDs were signalled (or a summary)
from the existing --force-update case.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp-config/files/openwisp.agent
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Upload Packages
🔇 Additional comments (3)
openwisp-config/files/openwisp.agent (3)
928-935: LGTM!The signal handler correctly removes both checksum files, which ensures
configuration_changed()will detect a difference and triggerupdate_configuration(). This approach is simpler and more robust than maintaining a separate flag, as suggested by the reviewer@nemesifier.
991-996: Implementation is acceptable with documented limitation.The traps are only active during the sleep interval. Signals received during
configuration_changed()orupdate_configuration()execution will be ignored until the next sleep phase. This is a reasonable trade-off that avoids complexity from handling signals mid-operation.The inline comment on line 993 could be slightly expanded to clarify this timing behavior for maintainers, though this is optional.
997-999: LGTM!Both signal traps are correctly reset after the sleep interval, maintaining consistency with the trap setup and ensuring signals don't interfere with configuration operations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| --force-update) | ||
| # Check if agent is running and send SIGUSR2 to force configuration reload | ||
| AGENT_PID=$(pgrep -P 1 -f openwisp-config) | ||
| if [ -z "$AGENT_PID" ]; then | ||
| echo "Error: openwisp-config agent is not running" >&2 | ||
| exit 1 | ||
| fi | ||
| kill -USR2 "$AGENT_PID" | ||
| echo "Sent SIGUSR2 to openwisp-config agent (PID: $AGENT_PID) to force configuration update" | ||
| exit 0 | ||
| ;; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual file to confirm the code snippet
cat -n openwisp-config/files/openwisp.agent | sed -n '10,25p'Repository: openwisp/openwisp-config
Length of output: 624
🏁 Script executed:
# Test pgrep behavior - can it return multiple PIDs?
echo "=== Testing pgrep with multiple potential matches ==="
# Create a test to see pgrep output format
pgrep -f "bash" 2>/dev/null | head -3 | while IFS= read -r line; do echo "Line: [$line]"; done
echo ""
# Count how many PIDs pgrep returns
PIDS=$(pgrep -f "bash" 2>/dev/null)
PID_COUNT=$(echo "$PIDS" | wc -w)
echo "Number of PIDs returned: $PID_COUNT"
echo "PIDS content: [$PIDS]"Repository: openwisp/openwisp-config
Length of output: 166
🏁 Script executed:
# Test kill behavior with quoted multi-PID string
echo "=== Testing kill with quoted multi-PID string ==="
PIDS="1 2 3"
echo "Variable PIDS contains: $PIDS"
echo "Running: kill -0 \"\$PIDS\""
kill -0 "$PIDS" 2>&1 || echo "Command failed (as expected if multiple PIDs)"Repository: openwisp/openwisp-config
Length of output: 287
🏁 Script executed:
# Check if there's any documentation about single vs. multiple agent instances
rg -i "agent" openwisp-config/files/openwisp.agent | head -10Repository: openwisp/openwisp-config
Length of output: 659
🏁 Script executed:
# Look for comments about process management and PID handling
rg -B2 -A2 "pgrep|pidof" openwisp-config/files/openwisp.agentRepository: openwisp/openwisp-config
Length of output: 300
Fix pgrep output handling: iterate over PIDs instead of passing quoted string to kill.
When pgrep -P 1 -f openwisp-config returns multiple PIDs (space-separated), passing the quoted variable to kill fails because kill interprets the entire string as a single invalid PID argument. The correct approach is to iterate over the individual PIDs.
Suggested fix
--force-update)
# Check if agent is running and send SIGUSR2 to force configuration reload
- AGENT_PID=$(pgrep -P 1 -f openwisp-config)
- if [ -z "$AGENT_PID" ]; then
+ AGENT_PIDS=$(pgrep -P 1 -f openwisp-config)
+ if [ -z "$AGENT_PIDS" ]; then
echo "Error: openwisp-config agent is not running" >&2
exit 1
fi
- kill -USR2 "$AGENT_PID"
- echo "Sent SIGUSR2 to openwisp-config agent (PID: $AGENT_PID) to force configuration update"
+ for pid in $AGENT_PIDS; do
+ kill -USR2 "$pid" 2>/dev/null
+ done
+ echo "Sent SIGUSR2 to openwisp-config agent (PID(s): $AGENT_PIDS) to force configuration update"
exit 0
;;🤖 Prompt for AI Agents
In `@openwisp-config/files/openwisp.agent` around lines 12 - 22, The current
AGENT_PID variable from pgrep may contain multiple space-separated PIDs; instead
of passing the quoted string to kill, split and iterate over each PID and call
kill -USR2 on each one. Modify the block that sets AGENT_PID=$(pgrep -P 1 -f
openwisp-config) to check for empty result, then loop over the tokens in
AGENT_PID (for pid in $AGENT_PID; do ... done), verify each pid is non-empty and
numeric before running kill -USR2 "$pid", and update the echo to report which
PIDs were signalled (or a summary) from the existing --force-update case.
2 participants
This commit adds a trap for SIGUSR2 in the main agent loop. When received, it sets a
FORCE_RELOADflag to bypass the checksum check and force an immediate configuration update.Testing:
I manually verified the logic using a local simulation script to confirm that the signal sets the flag and the loop prioritizes the reload.
Closes #181
Checklist
Reference to Existing Issue
#181