Revert "units: do not soft-reboot before soft-reboot.target reached"

[poettering]

Reverts #32880

Do not merge as is, this is just a placeholder.

I think the issue should be fixed differently, so that all types of reboot/shutdown are handled in the same fashion.

(also, only the 2nd commit in the reverted PR i have a problem with. the first commit is fine)

[poettering]

So I am not sure what a better fix would be.

One idea: just replace the Before=soft-reboot.target and Conflicts=soft-reboot.target lines in systemd-journald.service to point to systemd-soft-reboot.service, i.e. the actual services that do the deed.

Or alternatively, we should change the killing spree logic during soft reboot to be closer to the killing spree logic during regular shutdown, i.e. do a SIGTERM first.

dunno.

[bluca]

I think the issue should be fixed differently, so that all types of reboot/shutdown are handled in the same fashion.

There's another PR to bring others in line too, but it doesn't seem appropriate for post-RC given it doesn't fix any bugs (unlike the change that was merged, which does fix actual issues), we can just take that for 257

[poettering]

There's another PR to bring others in line too, but it doesn't seem appropriate for post-RC given it doesn't fix any bugs (unlike the change that was merged, which does fix actual issues), we can just take that for 257

i am not convinced that is desirable.

let's not do hotfixes that look like we migh tneed to revert them later.

[bluca]

There's another PR to bring others in line too, but it doesn't seem appropriate for post-RC given it doesn't fix any bugs (unlike the change that was merged, which does fix actual issues), we can just take that for 257

i am not convinced that is desirable.

let's not do hotfixes that look like we migh tneed to revert them later.

We need a fix - doesn't have to be this one ofc - so if you have an alternative in mind, that is appropriate for this stage (self contained, doesn't break compat, usual stuff) I'll happily review a PR that implements it

[YHNdnzj]

So I am not sure what a better fix would be.

One idea: just replace the Before=soft-reboot.target and Conflicts=soft-reboot.target lines in systemd-journald.service to point to systemd-soft-reboot.service, i.e. the actual services that do the deed.

I think using targets as synchronization points is more common practice, also that's what's documented in systemd.special. We order the service that carries out the switch-root action after the corresponding target too.

As mentioned, switching the order for normal shutdown types shouldn't be necessary, but there's no harm in doing that either. I'm fine with merging the other PR to make things consistent.

[bluca]

Indeed, all our docs say to use targets for sync points, so switching the rest (for v257) seems like the best solution to me.

[poettering]

Sorry, but these changes make little sense to me, and by any means, this kind of stuff should not be done this late in a cycle.

[bluca]

Sorry, but these changes make little sense to me, and by any means, this kind of stuff should not be done this late in a cycle.

The RC stage is explicitly and solely for fixing bugs - and this did fix a bug. Again, if there are alternatives, let's see how they look like and then we can pick one.

[poettering]

I think using targets as synchronization points is more common practice, also that's what's documented in systemd.special. We order the service that carries out the switch-root action after the corresponding target too.

well, usually targets pull in services, and are ordered after them. It's the exception that targets pull in something and order themselves before. It happens, and it is OK, but it's certainly much less used than the other way round.

However, that doesn't matter much, this is a question of compat: we should not, in the 3rd rc willy-nilly order this around, as if changing the milestones had no effect on compatibility.

Previously, the order was very clear:

1. shutdown.target comes first.
2. the service that then issues the operation comes next (i.e. systemd-poweroff.service and so on)
3. The final target (i.e. poweroff.target and so on) comes after that (and is generally never reached, if the service did what it was supposed to do).

So this gives people one generic target to order things before, and one specific target + service for the method of shutdown.

I am now very confused what the new logic is supposed to be though as soft-reboot is now handled very differently from the others, and hence I don't know at all what means what anymore?

but also, why?

For making such a change to the dependency tree you need very strong reasons. After all other software is plugging into the ordering here, using our generic milestones. And if you break compat like that, you must have really good reasons. So what are those?

[YHNdnzj]

For making such a change to the dependency tree you need very strong reasons. After all other software is plugging into the ordering here, using our generic milestones. And if you break compat like that, you must have really good reasons. So what are those?

So that all jobs that have either Before=soft-reboot.target or Before=systemd-soft-reboot.service will be correctly synchronized. As mentioned, we tend to recommend people to use targets as sync points, and currently that would be icky, since the target might never be reached (as you said), so it's not a valid sync point.

Plus, I really don't know where this has broken compat? Both would work correctly now.

[YHNdnzj]

So this gives people one generic target to order things before, and one specific target + service for the method of shutdown.

I am now very confused what the new logic is supposed to be though as soft-reboot is now handled very differently from the others, and hence I don't know at all what means what anymore?

Let's start with some basic facts first. Your observation in #32880 (comment) is wrong, as the deps are moved rather than dropped. It's important to get this straight first before further claims.

Secondly, please point out where exactly this has broken compat. The PR made both target and service valid sync point, so this should be purely an enhancement.

[poettering]

Secondly, please point out where exactly this has broken compat. The PR made both target and service valid sync point, so this should be purely an enhancement.

we do allow people to implement their own shutdown routines, and people do for weird power sources. they generally insert their stuff in parallel to systemd-poweroff.service, i.e. pulled in by poweroff.service, and ordered before it. That stuff is suddenly wrong.

[poettering]

sorry, the implications of such a change of ordering are not clear at all.

And this stuff is wildly incomplete if it only touches soft reboot, and nothing else.

This should not be done this late in the cycle. There are simply mechanisms to fix this, that are less intrusive, such as ordering the journal service before systemd-soft-reboot.service rather than soft-reboot.target.

Hence, sorry, but this stuff should not have been hurried in like this, and I am sure the relevant commits should be backed out, and maybe reposted for v257 (though I am not too convinced even then, but let's have the discussion on a separate PR).

I mean, the thing is, this is a major reordering of things, and we do advertise that people can plug things betwwen our milestones, or replace our services and so on, but if they do that, they suddenly will run into the fact that two units that are relevant milestones suddenly changed order.

[bluca]

Secondly, please point out where exactly this has broken compat. The PR made both target and service valid sync point, so this should be purely an enhancement.

we do allow people to implement their own shutdown routines, and people do for weird power sources. they generally insert their stuff in parallel to systemd-poweroff.service, i.e. pulled in by poweroff.service, and ordered before it. That stuff is suddenly wrong.

How is a change in the softreboot target going to affect systemd-poweroff and units ordered with it?

[YHNdnzj]

Secondly, please point out where exactly this has broken compat. The PR made both target and service valid sync point, so this should be purely an enhancement.

we do allow people to implement their own shutdown routines, and people do for weird power sources. they generally insert their stuff in parallel to systemd-poweroff.service, i.e. pulled in by poweroff.service, and ordered before it. That stuff is suddenly wrong.

Hmm? Isn't that inherently racy? Why not plug that into systemd-shutdown, instead?

And also:

How is a change in the softreboot target going to affect systemd-poweroff and units ordered with it?

[yuwata]

Still I believe the original change is good. But actually the change was made after -rc2 and that's too late. Let's revert it at least now, and continue discussion. I'd like to re-push the change after v256-final.
Full version of the revert is in #32975.

[bluca]

Replaced by other PR