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

Dinit support #42

Merged
merged 9 commits into from
Dec 13, 2024
Merged

Dinit support #42

merged 9 commits into from
Dec 13, 2024

Conversation

MagicRB
Copy link
Collaborator

@MagicRB MagicRB commented Oct 5, 2024

No description provided.

This was referenced Oct 5, 2024
@pizzapim
Copy link
Collaborator

Have pushed some fixes I found so far here: https://github.com/pizzapim/NixNG/tree/dinit-support

@MagicRB
Copy link
Collaborator Author

MagicRB commented Oct 15, 2024

I don't quite like that we cannot have multiple env files, maybe that can be patched into dinit?

@MagicRB
Copy link
Collaborator Author

MagicRB commented Oct 15, 2024

Well, i patched it in, it compiles, ill test later

@MagicRB
Copy link
Collaborator Author

MagicRB commented Oct 15, 2024

@pizzapim i merged your changes and added a multiple env-file patch to dinit

@MagicRB
Copy link
Collaborator Author

MagicRB commented Oct 15, 2024

I rebased, though maybe i broke something

@pizzapim
Copy link
Collaborator

Two things I'm noticing now:

  1. Apparently the chpst from busybox does not implement the -b flag, so those instances would have to be fixed if we swap to dinit
  2. runit still seems to be used somewhere even if you enable dinit

@MagicRB
Copy link
Collaborator Author

MagicRB commented Oct 15, 2024

  1. Well most of the modules use the runit chpst i think, so that should be fine
  2. See 1

We do have to make sure to eventually remove the call to chpst now that we have tmpfilesd, we can have the service manager to drop the priviledges

@pizzapim
Copy link
Collaborator

I mean, runit is still being added to environment.systemPackages in modules/environment.nix. I don't think this is necessary at all?

@MagicRB
Copy link
Collaborator Author

MagicRB commented Oct 15, 2024

Oh yeah that's an oversight. The actual runit module should do that conditionally only, it really shouldn't be present always.

@pizzapim
Copy link
Collaborator

Also, seems like dumb-init has a hidden dependency on runit because it expects system.activation."runit" to be ran.

@@ -0,0 +1,36 @@
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/di/dinit/package.nix

useful even if just so that you don't need to do version bumps yourself

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll switch it after we switch to 24.11 for the nixpkgs base.


patches = [
./dinit-dont-shutdown-system.patch
./dinit-allow-multiple-env-files.patch

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm sure upstream would accept this... maybe i'll PR that to help out

};

patches = [
./dinit-dont-shutdown-system.patch

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you run dinit with the -s flag so i believe this is not necessary - it is definitely possible to run dinit without having it handle shutdown though, patch not needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thing is we want it to do process reaping and properly waiting on services to come down, but not call the kernel to shutdown. So a yes and no situation

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i think that is what -s does...

 --system, -s                 run as the system service manager
 --system-mgr, -m             run as system manager (perform shutdown etc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've tested that itll try ro do shutdown without that patch and we do use -s so im a bit confused in regards to the docs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my experiments i ran an instance of dinit during stage1, shut it down, then launched a dinit again for stage2 and everything worked as i expected it to

from my perspective it worked as i am describing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, ill retest during the weekend and finish this PR up. Thanks for input

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aanderse

[STOPPD] nginx
Sending TERM/KILL to all processes...
2024/12/13 15:20:25 [notice] 100#100: signal 15 (SIGTERM) received from 1, exiting
2024/12/13 15:20:25 [notice] 100#100: signal 17 (SIGCHLD) received from 101
2024/12/13 15:20:25 [notice] 100#100: worker process 101 exited with code 0
2024/12/13 15:20:25 [notice] 100#100: signal 29 (SIGIO) received
2024/12/13 15:20:25 [notice] 100#100: signal 17 (SIGCHLD) received from 102
2024/12/13 15:20:25 [notice] 100#100: worker process 102 exited with code 0
2024/12/13 15:20:25 [notice] 100#100: exit
Turning off swap...
/sbin/swapoff: No such file or directory
Unmounting disks...
/bin/umount: No such file or directory
Issuing shutdown via kernel...

that's what I get without the patch

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is we need this bit to execute https://github.com/davmac314/dinit/blob/master/src/shutdown.cc#L434 but not the shutdown, i.e. we need process reaping but not shutdown. Without the process reaping dinit exits immediately and then the container runtime will SIGKILL the rest of the processes.

let
ownerParts = lib.splitString ":" owner;
user = builtins.elemAt ownerParts 0;
group = builtins.elemAt ownerParts 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having a : in the owner will throw a very non helpful error, index out of bounds.

@MagicRB
Copy link
Collaborator Author

MagicRB commented Dec 13, 2024

I'm gonna merge this, it seems to be good enough and it being in a branch is making my life more difficult in my dotfiles repo.

@MagicRB MagicRB marked this pull request as ready for review December 13, 2024 14:36
@MagicRB MagicRB merged commit ef2867a into master Dec 13, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants