operator icon indicating copy to clipboard operation
operator copied to clipboard

Document whether we can restart a stopped service

Open sed-i opened this issue 1 year ago • 3 comments

The docstring is quite minimal: https://github.com/canonical/operator/blob/808fb8f79e2e8d14aa3cb4e36081bd1886d68620/ops/model.py#L2221

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.

sed-i avatar Jun 14 '24 17:06 sed-i

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 🤔

dimaqq avatar Jun 15 '24 00:06 dimaqq

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?

sed-i avatar Jun 15 '24 04:06 sed-i

This is not needed, and I believe it hasn't been since the restart command was added, see https://github.com/canonical/pebble/pull/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 avatar Jun 16 '24 22:06 benhoyt

This was done in #1320

benhoyt avatar Sep 27 '24 02:09 benhoyt