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

Assert* failure should leave unit in a failed state - not inactive #10433

Closed
SjonHortensius opened this issue Oct 17, 2018 · 6 comments
Closed

Comments

@SjonHortensius
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I looked into the difference between Condition and Assert and indeed Assert is a bit louder. But I'd like it even louder - if I have a unit with an Assert that fails, I want the unit to be considered failed, in the same way as a failed Exec would.

Describe the solution you'd like
systemctl status should not be running but failed when any unit has a failed assertion. Service should not be inactive but failed.

@poettering
Copy link
Member

So, urks, the docs are actually really borked in this regard. ConditionXYZ= and AssertXYZ= only have an effect on the job that is enqueued for a service, not on the service itself, and I am not sure they should have.

@poettering poettering added this to the v240 milestone Oct 18, 2018
poettering added a commit to poettering/systemd that referenced this issue Oct 29, 2018
… unit state

In the documentation for ConditionXYZ= we claimed that AssertXYZ= would
have an effect on unit state (which is wrong), while at the
documentation for AssertXYZ= we said it only has an effect on the job,
but not the unit (which is right). Let's fix this contradiction, and
only claim the latter.

Also, fix a couple of other things (for example, stop talking about a
"failure state", but let's just expressly called it "the 'failed' state",
as that's the actual name of that state.

Finally, let's emphasize again when the conditions/assertions are
executed, and that they hence are not useful to conditionalize deps.

Fixes: systemd#10433
@poettering
Copy link
Member

Fix waiting in #10569

poettering added a commit to poettering/systemd that referenced this issue Oct 30, 2018
… unit state

In the documentation for ConditionXYZ= we claimed that AssertXYZ= would
have an effect on unit state (which is wrong), while at the
documentation for AssertXYZ= we said it only has an effect on the job,
but not the unit (which is right). Let's fix this contradiction, and
only claim the latter.

Also, fix a couple of other things (for example, stop talking about a
"failure state", but let's just expressly called it "the 'failed' state",
as that's the actual name of that state.

Finally, let's emphasize again when the conditions/assertions are
executed, and that they hence are not useful to conditionalize deps.

Fixes: systemd#10433
@sourcejedi
Copy link
Contributor

sourcejedi commented Oct 30, 2018

@poettering Do you have an example where it might be a problem if Assert*= marks the unit as failed?

I guess one concern is that someone might have written a second unit, which Requires= (and is After=) the first one, and then the second unit will not be started?

Are you mostly concerned about making a breaking change? Or do you think the current behaviour seems more useful? I'm not familiar with Assert*=, but intuitively I would have expected the same behaviour as
SjonHortensius asked for.

@poettering
Copy link
Member

@poettering Do you have an example where it might be a problem if Assert*= marks the unit as failed?

well, it would be a major change of behaviour...

@poettering
Copy link
Member

Are you mostly concerned about making a breaking change? Or do you think the current behaviour seems more useful? I'm not familiar with Assert*=, but intuitively I would have expected the same behaviour as
SjonHortensius asked for.

Well, I can see why people would want that, but this would require a bigger redesign of concepts. Right now many unit types do not actually know a failed state. (for example: target units), and we do allow AssertXYZ= for them. We'd have to introduce a concept there. And sematically, it's a tiny bit surprising I think if unit-type-specific state changes are result of unit-type-generic concepts such as asserts...

@sourcejedi
Copy link
Contributor

sourcejedi commented Oct 30, 2018

Thanks for explaining.

I guess I don't think about the type-specific states (or "result"s) very much. Outside the code (and not considering about the dbus interface), the state transition would mostly look like the opposite of reset-failed :-).

I guess a quick hack would be to change unit_active_state(): if the state would be "dead", but u->assert_result == false, then return "failed" instead. Don't care that the unit-specific state will be "dead" (not "failed"), and any unit-specific result(s) would be "success". IMO it's nicer behaviour, so the main question is whether the change would be worth the risk to existing systems.

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

No branches or pull requests

3 participants