Skip to content

handlers: restart only the wireguard vpn updater supervisor process (fixes #47)#83

Open
DarshiniKurasa wants to merge 1 commit into
openwisp:masterfrom
DarshiniKurasa:change-restart-wireguard-updater-47
Open

handlers: restart only the wireguard vpn updater supervisor process (fixes #47)#83
DarshiniKurasa wants to merge 1 commit into
openwisp:masterfrom
DarshiniKurasa:change-restart-wireguard-updater-47

Conversation

@DarshiniKurasa

Copy link
Copy Markdown

Summary

This PR improves deployment reliability by stopping Supervisor from restarting unrelated processes.
Replaces the global supervisorctl reload handler with a targeted restart of the WireGuard VPN updater supervisor process to avoid unnecessary restarts of unrelated services.

What was changed

  • Added a new handler in handlers/main.yml:
    • Runs: supervisorctl restart openwisp-flask-vpn-updater-{{ openwisp2_wireguard_vpn_uuid }}
    • Uses register, failed_when, and changed_when logic so:
      • The play does not fail if the process doesn't exist
      • The task reports changed only when a restart succeeds
  • Updated notify statements in tasks to use the new handler:
    • tasks/flask.yml
    • tasks/pip.yml
    • tasks/uwsgi.yml
  • Kept verify.yml unchanged to avoid altering test behavior unnecessarily

Why

Reloading supervisor triggers a restart of all managed processes, creating avoidable downtime.
Restarting only the affected process is safer, more efficient, and aligns with the request in issue #47.

How to verify

Run local QA checks:

run-qa-checks

(Already tested locally, passed successfully)

Closes #47

Happy to update the PR if needed. Thanks for reviewing!

Replace supervisorctl reload with a targeted restart of
openwisp-flask-vpn-updater-{{ openwisp2_wireguard_vpn_uuid }} and add safe
register/failed_when/changed_when logic to avoid failing when the process
is absent. Update task notifications accordingly.

Fixes : openwisp#47

@nemesifier nemesifier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @DarshiniKurasa for looking at #47, the goal (don't restart unrelated supervisor processes) is legitimate. Unfortunately the implementation as written breaks a more important case, so it can't be merged in this form.

Blocking bug: a brand new supervisor program is never loaded

In this role the only thing that loads the updater's supervisor program config is the reload supervisor handler. Look at tasks/flask.yml: the task that templates flask_vpn_updater_{{ uuid }}.conf into the supervisor conf dir notifies that same handler. supervisorctl reload re-reads all configs and brings the new program up.

Your replacement runs supervisorctl restart openwisp-flask-vpn-updater-{{ uuid }}. But restart does not read config files: on a fresh install the program does not exist yet, so supervisor returns ERROR (no such process), which your handler explicitly swallows via failed_when. Net result: on first install (and on any run where the supervisor program config itself changes), the updater is never loaded and never starts, silently.

restart only restarts an already-known program. Picking up a new or changed program config requires reread + update (or reload).

Suggested approach

Split into two handlers so each notify does the right thing:

  • For changes to the supervisor config template (the vpn_updater.j2 task): notify a handler that runs supervisorctl reread && supervisorctl update. update only restarts programs whose config changed, which is exactly the "do not touch unrelated processes" behavior #47 asks for, and it correctly loads a brand new program.
  • For changes to code/deps only (the script, vpn_updater.py, pip installs, uwsgi.ini): notify a restart <name> handler like the one you wrote.

If you want to keep it to one handler, supervisorctl reread && supervisorctl update alone is safer than restart, since update covers both the new-program and changed-config cases and leaves unrelated programs alone.

Also

This branch is out of date with main: the handler was renamed to Reload supervisor (capital R) and the notifies updated accordingly, so this needs a rebase before it will even apply cleanly.

Right problem, wrong tool. Switch to reread/update and I am happy to take another look.

@openwisp-companion

Copy link
Copy Markdown

Hi @DarshiniKurasa 👋,

This is a friendly reminder that this pull request has had no activity for 7 days since changes were requested.

We'd love to see this contribution merged! Please take a moment to:

  • Address the review feedback
  • Push your changes
  • Let us know if you have any questions or need clarification

If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it.

Note: within 7 more days, the linked issue will be unassigned to allow other contributors to work on it.

Thank you for your contribution! 🙏

@openwisp-companion

Copy link
Copy Markdown

Hi @DarshiniKurasa 👋,

This pull request has been marked as stale due to 14 days of inactivity after changes were requested.

As a result, any linked issues are being unassigned from you so other contributors can pick them up.

However, you can still continue working on this PR! If you push new commits or respond to the review feedback:

  • The issue will be reassigned to you
  • Your contribution is still very welcome

If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[change] Instead of reloading supervisor after changes, restart only the affected process

2 participants