if this will be merged, we can go on and cleanup the artifacts from old solutions - WINCH signal, mixed killmode, PID mgmt in imperative containers, restart as stop/start...

cc @arianvp here.

I tested this locally and can confirm that it fixes the regression of #6212 introduced in 90a3908.

This seems fine. though im a bit confused by why it was originally introduced.

I also don't understand why we need machinectl poweroff to turn off a container; but maybe this is what you're referring to with:

WINCH signal, mixed killmode, PID mgmt in imperative containers, restart as stop/start...

From my understanding, systemd-nspawn should be able to just run in the unit file, and then when the unit is stopped, systemd-nspawn should stop automatically... E.g. make this unit look more like the upstream systemd-nspawn@.service unit

#  SPDX-License-Identifier: LGPL-2.1+
#
#  This file is part of systemd.
#
#  systemd is free software; you can redistribute it and/or modify it
#  under the terms of the GNU Lesser General Public License as published by
#  the Free Software Foundation; either version 2.1 of the License, or
#  (at your option) any later version.

[Unit]
Description=Container %i
Documentation=man:systemd-nspawn(1)
Wants=modprobe@tun.service modprobe@loop.service modprobe@dm-mod.service
PartOf=machines.target
Before=machines.target
After=network.target systemd-resolved.service modprobe@tun.service modprobe@loop.service modprobe@dm-mod.service
RequiresMountsFor=/var/lib/machines

[Service]
# Make sure the DeviceAllow= lines below can properly resolve the 'block-loop' expression (and others)
ExecStart=systemd-nspawn --quiet --keep-unit --boot --link-journal=try-guest --network-veth -U --settings=override --machine=%i
KillMode=mixed
Type=notify
RestartForceExitStatus=133
SuccessExitStatus=133
Slice=machine.slice
Delegate=yes
TasksMax=16384
@SERVICE_WATCHDOG@

# Enforce a strict device policy, similar to the one nspawn configures when it
# allocates its own scope unit. Make sure to keep these policies in sync if you
# change them!
DevicePolicy=closed
DeviceAllow=/dev/net/tun rwm
DeviceAllow=char-pts rw

# nspawn itself needs access to /dev/loop-control and /dev/loop, to implement
# the --image= option. Add these here, too.
DeviceAllow=/dev/loop-control rw
DeviceAllow=block-loop rw
DeviceAllow=block-blkext rw

# nspawn can set up LUKS encrypted loopback files, in which case it needs
# access to /dev/mapper/control and the block devices /dev/mapper/*.
DeviceAllow=/dev/mapper/control rw
DeviceAllow=block-device-mapper rw

[Install]
WantedBy=machines.target

So I think, by merging this change, we can completely get rid of the machinectl stop command, as doing systemctl stop container@blah should be enough to shut down the container.

This is the same issue we're fixing as documented here: systemd/systemd#2770

@danbst this looks good - can you do the above mentioned cleanups in this PR, too?

@arianvp omg, there was an upstream issue for this! Interesting, they mentioned RemainAfterExit, I think we can't get rid of it in NixOS, as it will break NixOS restart logic.

@flokli ok, I'll try cleanup that

We'll have to keep KillMode=mixed by the way. it's also used upstream. I asked for the reason and this was the answer:

michich, when the kernel wants to stop properly the system (for instance, because you pressed the power button) it sends
SIGTERM to init, wait for some time, then powers down
to emulate that for a container, you send SIGTERM to the main process (the init of the container)
then you wait a bit
then you SIGKILL everything, which is the equivalent for a container of shutting down

@danbst poke ;-)

@flokli I'm debugging tests, they fail on nixos-container destroy command

Cleanups (from commit message):

* machinectl terminate during container start was a hack, probably wrong one
* use "systemctl restart" consistently for declarative and imperative containers
* fix "nixos-container terminate" command. We have to notify systemd we want
  unit to be stopped, because otherwise it will be started back due to
  restart on failure policy.
* add a bit docs to stop/terminate/destroy, to clarify which one does what
* add missing "nixos-container restart" docs

@arianvp I was not able to remove machinectl poweroff, for some reason it doesn't poweroff machine, but just kills it.

Also, with this PR machinectl terminate behavior had changed. I think I'll add release notes, to clarify this bit. nixos-container terminate/destroy works though.

@GrahamcOfBorg test containers-imperative

@arianvp kind ping :)

@danbst @arianvp may I ask if there are any updates here? :)

I'm okay with this approach for now if we are adamant on getting it out for 20.03

If not, I would like to wait until we complete the 245 release, and see if the issues I encountered in the linked systemd issue goes away. If we keep using --keep-unit with the inclusion of --kill-signal=SIGTRM3 then systemctl stop container@foo should be sufficient to kill the container, without the machinectl terminate call

I'm not sure which approach is referred to by "this", whether it's this PR, or the one referencing it (#84608). I don't think any of these two should be backported to 20.03. It's too late for that, and I don't want to cause breakages that late in the release cycle.

Ping @danbst; this is still an issue in master, and it would be nice to have this fixed there at least.

Hello @arianvp and @danbst, are there any news on this one?

lIt would be so great to have some kind of fix. Today was another day where I woke up to all my containers being down after the nightly system update.

I marked this as stale due to inactivity. → More info