Skip to content
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.

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

Document whether we can restart a stopped service #1263

Closed
sed-i opened this issue Jun 14, 2024 · 4 comments
Closed

Document whether we can restart a stopped service #1263

sed-i opened this issue Jun 14, 2024 · 4 comments
Assignees
Labels
docs Improvements or additions to documentation small item

Comments

@sed-i
Copy link
Contributor

sed-i commented Jun 14, 2024

The docstring is quite minimal:

"""Restart the given service(s) by name."""

In the past we used to:

            if self._container.get_service(self._name).is_running():
                self._container.restart(self._name)
            else:
                self._container.start(self._name)

It would be nice to have it explicitly documented if this is still needed or not.

@dimaqq
Copy link
Contributor

dimaqq commented Jun 15, 2024

The code has a data race smell on the surface at least.

I wonder if it’s better to try: start() except: restart() or maybe the opposite 🤔

@sed-i
Copy link
Contributor Author

sed-i commented Jun 15, 2024

Nice suggestion @dimaqq!
In any case, would be nice to have an updated doc indicating whether we need to guard a restart at all.
If guarding is needed, then perhaps add a kwarg, stopped_ok: bool?

@benhoyt
Copy link
Collaborator

benhoyt commented Jun 16, 2024

This is not needed, and I believe it hasn't been since the restart command was added, see canonical/pebble#58. Restart doesn't try to stop services that aren't running, but it will stop and start any services that are running. See also the docs about replan vs restart at https://github.com/canonical/pebble?tab=readme-ov-file#updating-and-restarting-services

Interestingly, and somewhat related -- there's a little backwards-compatibility shim in Container.restart for old versions of Pebble (quite old by now) that don't yet have restart, to stop the running services first.

@dimaqq Would you be willing to push up a PR to improve on this in the Container.restart docstring, and the underlying pebble.Client.restart_services docstring? Let's try to keep it brief (ideally one sentence), but indicate 1) that it will force a stop and start if they're already, and 2) won't try to stop services that aren't running, just start them.

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 27, 2024

This was done in #1320

@benhoyt benhoyt closed this as completed Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation small item
Projects
None yet
Development

No branches or pull requests

3 participants