Skip to content

fix(up): devservices up will not restart supervisor if it is already running #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hubertdeng123 merged 3 commits into from
Jun 3, 2025
Merged

fix(up): devservices up will not restart supervisor if it is already running #284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47dac92
supervisorctl supports reloading config and restarting processes
hubertdeng123 May 31, 2025
1f8fb91
add another note
hubertdeng123 May 31, 2025
3c995d0
catch another exception and handle gracefully
hubertdeng123 Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions devservices/utils/supervisor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,21 @@ def start_supervisor_daemon(self) -> None:
try:
client = self._get_rpc_client()
client.supervisor.getState()
# Supervisor is already running, restart it since config may have changed
client.supervisor.restart()
# Supervisor is already running, run supervisord update to update config and restart running processes
# Notes:
# - xmlrpc.client.reloadConfig does not work well here as config changes don't appear to be reloaded, so we use `supervisorctl update` instead
# - processes that are edited/added will not be automatically started
subprocess.run(
["supervisorctl", "-c", self.config_file_path, "update"],
check=True,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
)
Comment on lines +191 to +196
Copy link
Copy Markdown
Contributor

@IanWoodard IanWoodard Jun 3, 2025

Choose a reason for hiding this comment

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

Shouldn't we also catch a subprocess error below now?

Copy link
Copy Markdown
Member Author

@hubertdeng123 hubertdeng123 Jun 3, 2025

Choose a reason for hiding this comment

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

yep


# Wait for supervisor to be ready after restart
# Wait for supervisor to be ready after config reload
self._wait_for_supervisor_ready()
return
except xmlrpc.client.Fault as e:
except (xmlrpc.client.Fault, subprocess.CalledProcessError) as e:
capture_exception(e, level="info")
pass
except (SupervisorConnectionError, socket.error, ConnectionRefusedError):
Expand Down
9 changes: 7 additions & 2 deletions tests/utils/test_supervisor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,13 @@ def test_start_supervisor_daemon_already_running(
}
supervisor_manager.start_supervisor_daemon()
assert mock_rpc_client.return_value.supervisor.getState.call_count == 2
assert mock_rpc_client.return_value.supervisor.restart.call_count == 1
assert mock_subprocess_run.call_count == 0
mock_subprocess_run.assert_called_with(
["supervisorctl", "-c", supervisor_manager.config_file_path, "update"],
check=True,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
)
mock_subprocess_run.assert_called_once()


@mock.patch("devservices.utils.supervisor.subprocess.run")
Expand Down
Loading