[systemd-devel] [PATCH] core: collapse JOB_RELOAD on an inactive unit into JOB_NOP

[Uoti Urpala]

On Fri, 2014-08-15 at 12:50 +0200, Lennart Poettering wrote:
> On Fri, 15.08.14 05:09, Uoti Urpala (uoti.urpala at pp1.inet.fi) wrote:
> 
> > > > Before this commit systemctl reload on an inactive unit with a queued
> > > > start job would block until the unit had started if the unit supported
> > > > reload, but return failure immediately if the unit didn't.
> > > 
> > > This sounds like correct behaviour to me.
> > > 
> > > 1) I think the rule should be to return only after the unit is in the
> > > desired state, and the desired operation or something equivalent has
> > > been executed. This is useful, so that callers can rely on the fact that
> > 
> > What is your "desired state" for reload then?  
> 
> *operating* with the new configuration loaded.

The problem with this is that it's common for things updating
configuration to be separate from things using the daemon. If something
changes, the configuration update part wants to guarantee that
subsequent requests, *if any*, use the new configuration, but does not
itself make any such requests; as such, blocking for the service to be
up only causes unnecessary delays and sometimes deadlocks. Ensuring that
the service is up belongs to different code paths that actually make
requests to the daemon. And they do that whether there's been a reload
or not, so they need to handle it regardless of reload behavior anyway.



> > > > Additionally systemctl reload-or-try-restart (and systemctl force-reload)
> > > > would block until the unit had started if the unit supported reload, but
> > > > return *success* immediately if the unit didn't.
> > > 
> > > This actually also sounds like correct behaviour to me.
> > > 
> > > "try-restart" means that we try to restart a service, but only if it is
> > > already running. If we are not running this is not considered an
> > > error. If you then add the "reload-or-" to the mix, this means that we
> > > will try to reload it if the unit supports it, instead of restarting it.
> > > 
> > > So, if a start is already queued, we'll wait for it to finish, so that
> > > the configuration is fully in effect afterwards. But if the thing is not
> > > running at all, then we will not consider that a problem at all and
> > > return success.
> > 
> > I don't think this makes sense. Reload is a "softer" alternative; if I
> > say "reload-or-try-restart", that should be considered as asking less
> > from systemd - full restart is not necessary, just reload is enough if
> > that option is available. Asking for less should not be a reason to
> > block longer!
> 
> "try-restart" also waits for any already queued start job to finish, if
> there's any, before it returns, hence "reload-or-try-restart" and
> "try-restart" actually block for the same time... Not following here...

"try-restart" does *not* wait for a queued start job to finish.
job_type_collapse() changes JOB_TRY_RESTART to JOB_NOP if
UNIT_IS_INACTIVE_OR_DEACTIVATING(), which is true if the job is just
queued and not yet being executed.



> > After an operation that changes configuration,
> > "reload-or-try-restart" (or just "reload" if we know reload is in fact
> > supported by the daemon and sufficient for the configuration change in
> > question) should guarantee that further requests use the new
> > configuration. It should not unnecessarily block until the daemon is
> > actually running if it's being started; anything which relies on it
> > being up should test for that separately, independently of configuration
> > change handling. And there's no reason why implementing lighter-weight
> > reloads to replace full restarts should lead to systemd blocking
> > longer.
> 
> No. It *should* wait until any start request that is already queued is
> complete... Again, it's about the *outcome*, and about being *positive*,
> really. Nothing else. We should not make things fail, if we already
> *know* that there's alerady something going on that makes the thing we
> are trying to do succeed.

>From the configuration change point of view, the outcome *is* positive
as soon as it's guaranteed that no further requests will be processed
with the old configuration. So systemd should return *success* at that
point. This does not "make things fail"; rather, it prevents failure
from deadlocks caused by unnecessary blocking.


> I think most of the confusion here comes from the fact that sysv service
> restarts don't care about ordering at all, really, and we do. But the

No, my objections to the current behavior are not related to sysv
semantics at all.

> answer to that is not to weaken the current strong semantics of
> blocking, but simply not to request blocking operation at all, i.e. use
> --no-block, and just queue things in, instead of waiting for them.

If you want to block for things other than the configuration change
itself, such as the service being up, that should be separate from
changing configuration.


> > > > Finaly reload on an inactive unit without a queued start job would
> > > > reurn failure, but try-restart on the same job would return success.
> > > 
> > > Well, the "try-" prefix is supposed to be the thing "apply only if
> > > already running, but don't consider it an error if it isn't". We could of
> > > course add "try-reload" to the mix, so that you have a "reload" version
> > > that will not fail if the daemon is not running, but I think that's not
> > > particularly interested....
> > 
> > I think such "try-reload" semantics are the actually useful use-case,
> > and what just "reload" should do as the non-try semantics do not seem
> > that useful.
> 
> Well, I disagree on this. I think it's a good idea to make things fail
> if people ask a service to reload that isn't actually running. If people
> want to tape ove that case (which is totally acceptable, and also has
> good usecases), then they should use "try-reload", and all is good.
> 
> But I think this discussion is really moot. There are two usecases here,
> both valid, we just have different opinions which one is more relevant.
> 
> At this time, these two verbs have been defined like this since systemd
> was introduced, and we actually inherited their names from sysv, so we
> really shouldn't change their effect anymore.

This isn't true. The reload semantics were changed in 6a371e23ee
("systemd: treat reload failure as failure"), after v208. Before that
reload succeeded on stopped units. It's unclear what the rationale for
this commit was, as the commit does not match the bug report given as
explanation: the bug report talks about the case where the ExecReload
command returns an error, but the commit changes a case where the
ExecReload command is not executed at all and as such does not return
any error. The commit message also says "documented to fail", but the
only reference to documentation seems to be for the not-stopped case
where ExecReload actually *is* executed and fails.



> > > > Also fixes deadlocks on boot if a unit calls systemctl reload,
> > > > reload-or-try-restart or force-reload on a unit ordered later
> > > > in the boot sequence during startup.
> > > 
> > > Well, for cases like that, please consider using --no-block, which will
> > > just queue the job, but not wait for it. Or use
> > > --job-mode=ignore-dependencies or so, even though that's a horrid
> > > invention...
> > 
> > That's not safe - with --no-block the daemon may still process requests
> > with outdated configuration after the configuration change operation
> > returns. The above semantics for "reload" make this work completely
> > naturally without such problems. The problems are only caused by the
> > inappropriate blocking for unit start in current systemd; the start time
> > of the daemon is unrelated to the configuration change operations, and
> > the added blocking has no benefit whatsoever, it only causes problems.
> > Waiting for start should be done with a separate operation if needed.
> 
> Can you elaborate on what the actual reason this popped up? I am really
> not convinced that the semantics you propose would be in any way an
> improvement, since they are much weaker. Where exactly would this be
> beneficial?

Any case where you want to change the configuration of some service, not
because you necessarily want to make a request to the service using the
new configuration, but because of some external event or admin request
that means the service should start behaving differently. This is
generally the case for any configuration change you'd make from event
hooks. You want to make sure that old configuration will not be used
after you return, so --no-block would be wrong; but your own code is not
going to make any requests to the daemon either, so it does not care
whether the daemon is running or not, and blocking until the daemon runs
only has the potential to cause deadlocks.

In what case do you consider the block-until-start semantics of reload
to be particularly useful? If you explicitly start the service, you'd do
that with a blocking "start", so having a subsequent "reload" block is a
no-op. If something else starts the service, is "reload" really the
right tool to wait? And do you really always reload before using - if
you don't, you'd need another check regardless of reload behavior.


> Again, sysv made no restrictions at all, when you could run
> services. You could invoke /sbin/service at *any* time and it would do
> something. It was left to the caller to know when exactly it was OK to
> call it, and when it wasn't. Just using "systemctl" instead of

Restricting reload semantics to only the configuration change part
should make it pretty much safe to call at any time you have changed
configuration - either it succeeds immediately because the service is
inactive, waits for currently executing startup of service to finish and
then calls reload, or immediately calls reload for an active service; in
no case can it cause a deadlock.



P.S. not directly related to the above, but something I noticed in the
code: is the job_type_is_redundant() behavior for JOB_RELOAD and
JOB_RESTART really safe? It returns true if the current state of the
unit is UNIT_RELOADING / UNIT_ACTIVATING. Couldn't events go as follows:
* unit begins restart
* daemon reads configuration
* configuration is changed externally
* a new restart is requested, but is considered redundant because state
is still ACTIVATING
* unit finishes restart, but is still using old configuration
Though the job_type_is_redundant() check in transaction.c is only done
if tr->anchor_job != k, which I guess would limit the above only to
cases of propagated effects or such.