Fix `ctrlaltdel` and `1` for stopit and reboot files #50

ericonr · 2020-07-13T20:15:03Z

Fixes #47

Since the halt program seems to use init directly, I don't think anything else needs to be changed.

Needs proper testing as well.

ahesford · 2020-07-18T03:47:21Z

The biggest problem, to me, is that runit/3 does chmod 100 /run/runit/reboot if that file exists, which has the effect of moving the reboot trigger from "only when executable" to "whenever the file is present". I completely overlooked that and wondered why runit seemed to be ignoring permissions until @ericonr pointed out the stage-three chmod.

There is no reason for /run/runit/reboot to exist in stage 1, so it doesn't not need to be created there. Likewise, if we are changing the default behavior of /run/runit/stopit to a no-op by removing the execute bit, that file doesn't need to be created in runit/1. Those files can be created in ctrlaltdel with permissions 0100.

Note that runit-init will create the stopit or reboot files with the right permissions, so shutdown and reboot will still work as expected using shutdown, halt (or its symlinks reboot and poweroff) or init (which is really what's called by halt).

I propose:

Remove the two install lines in 1 that you currently have.
Add two lines to ctrlaltdel:

install -m 0100 /dev/null /run/runit/stopit
install -m 0100 /dev/null /run/runit/reboot

Remove the chmod 100 /run/runit/reboot in 3.

This should make our use of runit consistent with the documentation.

NOTE: If we're not creating /run/runit/stopit with permissions 0100 in stage 1, we should be certain that nothing depends on the default behavior of runit halting or rebooting the system when sent SIGCONT.

ericonr · 2020-07-18T07:07:30Z

This is now WIP, I added commits with @ahesford's changes, will need to rebase them properly before merging. I agree with the suggestions regarding how we should do it.

NOTE: If we're not creating /run/runit/stopit with permissions 0100 in stage 1, we should be certain that nothing depends on the default behavior of runit halting or rebooting the system when sent SIGCONT.

grepping for SIGCONT and -CONT in void-runit and void-packages didn't yield anything of interest. I guess it could make sense to have an INSTALL.msg for it, though. I would expect a user who depends on this behavior to have an idea of what they are doing, but it's good to be sure. Something like "runit now requires 100 perms in /run/runit/reboot in order to reboot`.

ahesford

I think this should be fine. I definitely would not add an INSTALL.msg about this. It's bound to create confusion among the majority who are not relying on the existing permissions and, again, normal reboot/shutdown/ctrl-alt-del behavior should be unaffected.

Like you said, people relying on this configuration are probably knowledgeable enough to debug any changes.

ctrlaltdel

ahesford

The changes look good if they are squashed.

ericonr · 2020-07-18T23:07:44Z

Squashed.

Duncaen · 2020-07-18T23:20:56Z

The upstream examples all seem to create at least the stopit file on boot. Not sure if there is a good reason, I can only think of issues with creating new files in some edge cases, but not sure how relevant this is for tmpfs /run, chmodding an existing file has less ways of failing.

ericonr · 2020-07-19T00:24:23Z

So install stopit with 000 perms?

I would prefer it if both files got configured the same way, so if stopit is created during 1, so should reboot.

ahesford · 2020-07-19T00:26:02Z

I would either create both stopit and reboot files with mode 0 in1 as originally proposed, or create neither as currently implemented in this branch. No need to create one ahead of time but leave the other to be created on demand.

Even though I advocated for the on-demand scheme you put together, I don't' have strong convictions about the choice.

The 100 permission in /run/runit/stopit made it so that signaling runit with SIGCONT would shut the system down. To achieve the correct behavior, we should create the stopit and reboot files with 000 perms, and allow their permissions to be set correctly by calls to `init 0` or `init 6` or by /etc/runit/ctrlaltdel. 1

runit uses the permissions in the /etc/runit/reboot file to determine whether it's going to perform a halt or reboot action. This conditional in 3 meant that touching the reboot file, even with 000 perms, would lead to a reboot, which goes agains what is expected according to the runit documentation.

ahesford

I think this should do the trick. @Duncaen, are you OK with this?

/run/runit/{stopit,reboot} should have their perms set to 100 by ctrlaltdel, before runit signals itself with SIGCONT, in order to trigger a reboot. This script can be changed by a user to perform different actions. The comments in it explain what each file is for.

ericonr · 2020-07-20T05:16:46Z

Fixed typo in commit message.

sbromberger · 2023-12-26T19:30:59Z

Sorry to resurrect this thread, but I'm wondering what the rationale was for changing stopit perms to 000 from 100. This breaks control of void containers when SIGCONT is used on stop commands. Is there a particular reason this change was made?

That is:

NOTE: If we're not creating /run/runit/stopit with permissions 0100 in stage 1, we should be certain that nothing depends on the default behavior of runit halting or rebooting the system when sent SIGCONT.

Management of containers depends on runit halting when sent SIGCONT (unless there's another way to do it that I'm missing....)

leahneukirchen · 2024-01-01T16:59:29Z

SIGCONT is not the proper signal to shutdown a container. SIGINT should work as well.

Note that you need --cap-add=sys_boot for this to work.

sbromberger · 2024-01-01T17:11:11Z

SIGCONT is not the proper signal to shutdown a container. SIGINT should work as well.

Do you have a reference for this? lxc/lxc-ci#408 discusses it in a bit of detail, and it was (at least when I started using LXD with void) the only way to get lxc stop to work properly.

leahneukirchen · 2024-01-01T17:39:18Z

SIGINT will trigger a reboot, which results in a shutdown of a container. You can test this easily.

sbromberger · 2024-01-01T18:14:27Z

I just want to make sure we're talking about the same thing here: I'm not referring to Docker containers; I'm referring to incus containers. SIGINT indeed attempts a reboot with incus (but fails the reboot stage, so it has the effect of a shutdown, but with error logs). There is no --cap-add option for starting incus containers, which leads me to think you're talking about Docker.

leahneukirchen · 2024-01-01T18:31:44Z

Can you share the error? If it terminates, it has the appropriate capabilities, else it would hang forever.

sbromberger · 2024-01-01T18:47:05Z

[root@elemental ~]# incus start test
[root@elemental ~]# incus stop test
Error: Failed restarting instance: The instance is already running
Try `incus info --show-log test` for more info

...
lxc test 20240101184607.290 INFO     error - ../src/lxc/error.c:lxc_error_set_and_log:34 - Child <15487> ended on signal Hangup(1)
lxc test 20240101184607.310 INFO     conf - ../src/lxc/conf.c:run_script_argv:340 - Executing script "/usr/local/bin/incusd callhook /var/lib/incus "default" "test" stopns" for container "test"
lxc test 20240101184607.429 INFO     conf - ../src/lxc/conf.c:lxc_map_ids:3603 - Caller maps host root. Writing mapping directly
lxc test 20240101184607.429 NOTICE   utils - ../src/lxc/utils.c:lxc_drop_groups:1368 - Dropped supplimentary groups
lxc test 20240101184607.455 INFO     conf - ../src/lxc/conf.c:run_script_argv:340 - Executing script "/usr/local/bin/incusd callhook /var/lib/incus "default" "test" stop" for container "test"
lxc test 20240101184607.513 ERROR    conf - ../src/lxc/conf.c:run_buffer:321 - Script exited with status 1
lxc test 20240101184607.513 ERROR    start - ../src/lxc/start.c:lxc_end:985 - Failed to run lxc.hook.post-stop for container "test"
lxc test 20240101184607.513 WARN     start - ../src/lxc/start.c:lxc_end:987 - Container will be stopped instead of rebooted
lxc test 20240101184607.514 ERROR    af_unix - ../src/lxc/af_unix.c:lxc_abstract_unix_recv_fds_iov:218 - Connection reset by peer - Failed to receive response
lxc test 20240101184607.514 ERROR    commands - ../src/lxc/commands.c:lxc_cmd_rsp_recv_fds:128 - Failed to receive file descriptors for command "get_state"

It's a standard incus image with the following config applied:

incus config set test raw.lxc="lxc.signal.halt=SIGINT"

leahneukirchen · 2024-01-01T21:29:00Z

Indeed lxd doesn't seem to like that it tries to reboot. You can make ctrlaltdel not do chmod 100 /run/runit/reboot, then it works just as well as SIGCONT.

sbromberger · 2024-01-01T22:59:02Z

The way I currently do it is to chmod 100 /run/runit/stopit which allows me to use what I've used from the start; namely SIGCONT. I noticed that new images weren't shutting down properly, which is why I investigated and when I discovered this PR which is the source of the breakage. I was hoping that we might be able to revisit this particular PR so that things will "just work" properly again.

ahesford mentioned this pull request Jul 18, 2020

Wrong information in ctrlaltdel #47

Closed

ahesford reviewed Jul 18, 2020

View reviewed changes

ctrlaltdel Outdated Show resolved Hide resolved

ctrlaltdel Outdated Show resolved Hide resolved

ericonr force-pushed the ctrl branch from 2519c29 to 2ad73ad Compare July 18, 2020 17:22

ahesford reviewed Jul 18, 2020

View reviewed changes

ericonr force-pushed the ctrl branch from 2ad73ad to 526d7b4 Compare July 18, 2020 19:06

ericonr added 2 commits July 20, 2020 00:28

ericonr force-pushed the ctrl branch from 526d7b4 to 8c7ef95 Compare July 20, 2020 03:30

ahesford approved these changes Jul 20, 2020

View reviewed changes

ericonr force-pushed the ctrl branch from 8c7ef95 to 0946572 Compare July 20, 2020 05:16

Duncaen approved these changes Jul 20, 2020

View reviewed changes

ahesford merged commit 05c6e86 into void-linux:master Jul 20, 2020

sbromberger mentioned this pull request Jan 2, 2024

Migrate the creation of runit control files from stage 1 script to core-services #113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ctrlaltdel` and `1` for stopit and reboot files #50

Fix `ctrlaltdel` and `1` for stopit and reboot files #50

ericonr commented Jul 13, 2020

ahesford commented Jul 18, 2020

ericonr commented Jul 18, 2020

ahesford left a comment

ahesford left a comment

ericonr commented Jul 18, 2020

Duncaen commented Jul 18, 2020 •

edited

Loading

ericonr commented Jul 19, 2020

ahesford commented Jul 19, 2020 •

edited

Loading

ahesford left a comment

ericonr commented Jul 20, 2020

sbromberger commented Dec 26, 2023 •

edited

Loading

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024 •

edited

Loading

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024 •

edited

Loading

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024

Fix ctrlaltdel and 1 for stopit and reboot files #50

Fix ctrlaltdel and 1 for stopit and reboot files #50

Conversation

ericonr commented Jul 13, 2020

ahesford commented Jul 18, 2020

ericonr commented Jul 18, 2020

ahesford left a comment

Choose a reason for hiding this comment

ahesford left a comment

Choose a reason for hiding this comment

ericonr commented Jul 18, 2020

Duncaen commented Jul 18, 2020 • edited Loading

ericonr commented Jul 19, 2020

ahesford commented Jul 19, 2020 • edited Loading

ahesford left a comment

Choose a reason for hiding this comment

ericonr commented Jul 20, 2020

sbromberger commented Dec 26, 2023 • edited Loading

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024 • edited Loading

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024 • edited Loading

leahneukirchen commented Jan 1, 2024

sbromberger commented Jan 1, 2024

Fix `ctrlaltdel` and `1` for stopit and reboot files #50

Fix `ctrlaltdel` and `1` for stopit and reboot files #50

Duncaen commented Jul 18, 2020 •

edited

Loading

ahesford commented Jul 19, 2020 •

edited

Loading

sbromberger commented Dec 26, 2023 •

edited

Loading

sbromberger commented Jan 1, 2024 •

edited

Loading

sbromberger commented Jan 1, 2024 •

edited

Loading