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

container: init service for linux and mac #4331

Closed
wants to merge 1 commit into from

Conversation

michaelCTS
Copy link

@michaelCTS michaelCTS commented Aug 11, 2023

Description

Evolved from the original PR based on #2630 by @MaeIsBad, this PR introduces 3 major options:

virtualisation.containers

From the original PR and nixpkgs, allows managing regstristries.conf (used by podman and others, but not docker) declaratively. containers.conf is modified by the aforementioned tools and managing by home-manager is considered out of scope of this PR.

virtualisation.oci-containers

Inspired by nixpkgs, a simple method to enable containers which will pick the appropriate/supported oci-container implementation of your platform. ATM only linux and mac are supported by activating virtualisation.podman.

virtualisation.oci-containers.enable = true and you're off.

virtualisation.podman

Enables support for podman on linux and mac. Linux is more simple as it activates as systemd user service and that's that.

Mac is the difficult child in this relationship as it requires a VM to function. I opted to support basic, declarative VM management using virtualisation.podman.machines.<name>. podman stores its VM configs in JSON and outputs them using the CLI in JSON as well. Since python is my forté and not bash + jq, I wrote a helper called podmactl which is called at activation of a new home-manager generation.

It diffs the existing machines from requested machines (JSON generated by nix) and the required machines are generated and only one is active (podman doesn't allow multiple running machines).

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@michaelCTS michaelCTS force-pushed the docker-service branch 3 times, most recently from 5465fc3 to 93b57c1 Compare August 11, 2023 11:35
@MaeIsBad
Copy link
Contributor

I'd be glad to review this once it gets out of the draft stage and help with maintenance.

One thing to note is that for a while the regular nixos virtualisation.podman.enable option already installs a user systemd socket you can use

modules/lib/maintainers.nix Outdated Show resolved Hide resolved

toml = pkgs.formats.toml { };
in {
meta.maintainers = [ lib.hm.maintainers.bad ];
Copy link
Member

Choose a reason for hiding this comment

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

There is no such maintainer listed anywhere, as far as I can see?

Copy link
Contributor

@MaeIsBad MaeIsBad Aug 17, 2023

Choose a reason for hiding this comment

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

I think this is a leftover from my original PR, where I added myself as a maintainer.
I don't feel confident in being the sole maintainer but I would be happy to become a second maintainer for this module if @michaelCTS wants to add me. You can reuse the maintainer info from my original PR here

Copy link
Author

Choose a reason for hiding this comment

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

I'll let you decide on that once the PR is ready @MaeIsBad . Then you can have an idea of what you're getting yourself into 😁

Copy link
Author

Choose a reason for hiding this comment

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

@MaeIsBad it's ready for review. Once you've had a look at it, feel free to decide if you want to be a maintainer.

Comment on lines 11 to 26
# Provides a fake "docker" binary mapping to podman
dockerCompat = pkgs.runCommandNoCC
"${podmanPackage.pname}-docker-compat-${podmanPackage.version}" {
outputs = [ "out" "man" ];
inherit (podmanPackage) meta;
} ''
mkdir -p $out/bin
ln -s ${podmanPackage}/bin/podman $out/bin/docker

mkdir -p $man/share/man/man1
for f in ${podmanPackage.man}/share/man/man1/*; do
basename=$(basename $f | sed s/podman/docker/g)
ln -s $f $man/share/man/man1/$basename
done
'';
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should be a feature of the podman package in Nixpkgs, similar to how the yt-dlp package has a withAlias argument. For example,

pkgs.podman.override { inherit (cfg) extraPackages enableDockerAlias; }

Perhaps it would be possible to add this functionality to the podman package in Nixpkgs?

Copy link
Author

Choose a reason for hiding this comment

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

It's a good idea. I'll let somebody else handle that due to a lack of time on my end 😇

@rycee
Copy link
Member

rycee commented Aug 16, 2023

Thanks a lot! It would be great to have this functionality. I started adding a few comments. Will continue when I find some more time.

@michaelCTS
Copy link
Author

Thanks for the review @rycee . For now, I made only minimal changes after recovering @MaeIsBad's PR. There might be bigger changes coming tomorrow (I get to work on this once a week) as I have a look at how virtualisation.podman.enable currently works on nixpkgs. CNI isn't used for networking anymore and there may be more changes to incorporate.

@michaelCTS
Copy link
Author

@rycee I pushed some changes for mac and introduced oci-containers just like nixpkgs which K900 on matrix pointed me to.

ATM, the mac changes can't be fully tested as they generated an endless recursion error when generating the machines.json file on 1ec2829#diff-c4ec42715ced2d6da0509ab4842b23451cacb94de96de6265d9fc86e16a67800R242

You can find the reasoning in the commit message 1ec2829 (used by script called by launchd to CRUD podman machines and start one). If you can spot why it's recursing endlessly, help would be much obliged.

@michaelCTS michaelCTS force-pushed the docker-service branch 2 times, most recently from 195f108 to 126ef80 Compare September 4, 2023 22:23
@michaelCTS
Copy link
Author

Alright, I was dumb and hadn't pulled master in a year. Rebased the branch and the infinite recursion problem went away + was able to build docs again. What's left now is to test it live, but it's unclear how to do so. How would one go about doing that? Create a new user and somehow setup a channel that points to a local directory? Maybe I missed that point in the docs.

@MaeIsBad
Copy link
Contributor

MaeIsBad commented Sep 5, 2023

With flakes you can point the flake input by using git+file:///home/user/home-manager. That's how I test changes to home-manager

@michaelCTS michaelCTS marked this pull request as ready for review September 8, 2023 15:13
@michaelCTS
Copy link
Author

Thanks @MaeIsBad . I don't use flakes, so had to find another way. Turned out it was

cd $proj
nix-shell -A install
home-manager -I home-manager=$proj switch

The PR is ready for review. I'll leave comments where I'm unsure about stuff.

As for the commit messages: only noticed those while checking of the list. What to do? Just squash everything? I tried to add many comments in the code, so a huge commit might be acceptable?
Otherwise I can rebase and reword each commit. Whatever is prefered.

@michaelCTS michaelCTS requested review from rycee and MaeIsBad September 8, 2023 15:18
Comment on lines 16 to 22
# Unimplemented as podman modifies the file and it would have to be merged
# not replaced when activating a new config
# containersConf.settings = mkOption {
# type = toml.type;
# default = { };
# description = "The {file}`containers.conf` configuration.";
# };
Copy link
Author

Choose a reason for hiding this comment

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

Present in nixpkgs https://github.com/NixOS/nixpkgs/blob/230262cda5dc5b704aae91273e2676a9ac65c81e/nixos/modules/virtualisation/containers.nix#L31-L35

Podman modifies the files in the user's home, outside of home-manager. Fixing that in this PR is out of scope, so not sure if this comment (and related commented out code) should stay or just be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the standard way of doing this in nix is to just put the file there with an optional "mutable" boolean option(eg. users.mutableUsers). I don't think podman breaks if it's unable to write to that file by itself

Copy link
Author

Choose a reason for hiding this comment

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

I'll give that a try and see if podman complains :)

Copy link
Author

Choose a reason for hiding this comment

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

Not sure where to put this. Wanted unit testing so putting it in the podman.nix didn't seem right.

Comment on lines 110 to 116
# enableNvidia = mkOption {
# type = types.bool;
# default = false;
# description = ''
# Enable use of NVidia GPUs from within podman containers.
# '';
# };
Copy link
Author

Choose a reason for hiding this comment

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

Requires virtualisation.containers.containersConf.settings which is a hassle to implement and out of scope (see comment on the options in containers.nix). Remove comment or keep?

Comment on lines 184 to 186
virtualisation.containers = {
enable = true; # Enable common /etc/containers configuration
# containersConf.settings = lib.optionalAttrs cfg.enableNvidia {
# engine = {
# conmon_env_vars =
# [ "PATH=${lib.makeBinPath [ pkgs.nvidia-podman ]}" ];
# runtimes.nvidia =
# [ "${pkgs.nvidia-podman}/bin/nvidia-container-runtime" ];
# };
# };
};
Copy link
Author

Choose a reason for hiding this comment

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

Requires virtualisation.containers.containersConf.settings which is a hassle to implement and out of scope (see comment on the options in containers.nix).
Delete comment or keep?

Comment on lines 171 to 175
apply = machines:
assert ((lib.lists.count (machine: machine.active)
(lib.attrsets.attrValues machines)) == 1);
machines;
};
Copy link
Author

@michaelCTS michaelCTS Sep 8, 2023

Choose a reason for hiding this comment

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

Unsure how to do this with a proper error message e.g Only one machine may be active at a time. Also unsure how to test this. nmt executes a script, but the assert is executed before the script is even run.

Comment on lines 16 to 22
# Unimplemented as podman modifies the file and it would have to be merged
# not replaced when activating a new config
# containersConf.settings = mkOption {
# type = toml.type;
# default = { };
# description = "The {file}`containers.conf` configuration.";
# };
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the standard way of doing this in nix is to just put the file there with an optional "mutable" boolean option(eg. users.mutableUsers). I don't think podman breaks if it's unable to write to that file by itself

options.virtualisation.oci-containers = {

enable = lib.mkEnableOption
"a convenience option to enable containers in platform-agnostic manner";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what "convenience option" means here? I think this should just be "containers"

Copy link
Author

Choose a reason for hiding this comment

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

This is taken from nixpkgs https://github.com/NixOS/nixpkgs/blob/ec68439bdc77ee684a4b2846f03677c5c50920a9/nixos/modules/virtualisation/oci-containers.nix#L335-L349

The problem with containers is that it used by podman. I'm not versed enough in nix and nixpkgs to not create a recursion problem with containers.enable using containers.backend to enable podman which itself then sets containers.enable and configures the settings and other stuff. I gave it a shot and immediately ran into recursion errors.

Since nixpkgs doesn't bother with that, I erred on the side of "they probably know better what they're doing that some 1 year nix scrub like me".

modules/virtualisation/podman/podmactl/.gitignore Outdated Show resolved Hide resolved
description =
"This machine should be started. Only one machine can be active at a time";
};
qemu_binary = mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

nix options are typically written using camelCase, same for the rest of this file

Copy link
Author

Choose a reason for hiding this comment

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

This is specifically because it's exported into a JSON which is used by the python script to apply the machine config. There's a comment a few lines up that mentions this.

I'll have a look at the rest of the file and see if I didn't it respect it outside of machine options.

modules/virtualisation/podman/podman.nix Outdated Show resolved Hide resolved
modules/virtualisation/podman/podman.nix Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels out of scope for a home-manager module, maybe you should consider moving this to a separate project..? Up to the home-manager devs to decide

Copy link
Author

Choose a reason for hiding this comment

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

This is what I considered too, but then fixing stuff would require modifying another project and creating a PR to update the version here. But I agree, it's up to the home-manager devs since I don't know what they prefer. @rycee (don't know who else is a maintainer), input is very welcome here.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it should be moved into its own repository, if you don't want it under your own account, then perhaps it could be hosted under nix-community, since it may be useful for other Nix related projects?

rycee pushed a commit that referenced this pull request Dec 23, 2023
This module is a continuation of #2630 by MaeIsBad.

It also adds a module `virtualisation.oci-containers` that is
equivalent to the one in NixOS. Basically it allows a simple toggle to
activate oci-container services and commands.

We also support Podman on mac. Note, Podman requires a VM on mac,
which has to be started before any Podman commands can be executed.
Users might sometimes require VMs that use different architectures
than the default VM started by Podman. Thus, they get the option to
define the VM(s) that will be initialized and started by podman.

Since Podman has to start a machine, it's best to do it using launchd.
The configuration of the machines requires a JSON, generated from an
attrset in Home Manager, which is where Python script comes into play
to take care of diff-ing the `podman machine list` to CRUD them.

PR #4331

Co-authored-by: MaeIsBad <26093674+MaeIsBad@users.noreply.github.com>
rycee pushed a commit that referenced this pull request Dec 23, 2023
This module is a continuation of #2630 by MaeIsBad.

It also adds a module `virtualisation.oci-containers` that is
equivalent to the one in NixOS. Basically it allows a simple toggle to
activate oci-container services and commands.

We also support Podman on mac. Note, Podman requires a VM on mac,
which has to be started before any Podman commands can be executed.
Users might sometimes require VMs that use different architectures
than the default VM started by Podman. Thus, they get the option to
define the VM(s) that will be initialized and started by podman.

Since Podman has to start a machine, it's best to do it using launchd.
The configuration of the machines requires a JSON, generated from an
attrset in Home Manager, which is where Python script comes into play
to take care of diff-ing the `podman machine list` to CRUD them.

PR #4331

Co-authored-by: MaeIsBad <26093674+MaeIsBad@users.noreply.github.com>
@rycee
Copy link
Member

rycee commented Dec 23, 2023

Finally had a chance to look at this and overall looks very nice. I made some minor cleanups and squashed the commits but unfortunately was not allowed to push to this branch. Would you mind resetting the PR to faa4b16

The only thing I'm concerned about is the inclusion of the Python project, I would feel more comfortable with it being a separate project and pulled from Nixpkgs. Then I imagine it could be used elsewhere as well, perhaps in nix-darwin or devenv?

@michaelCTS
Copy link
Author

michaelCTS commented Jan 2, 2024

Thanks for the review @rycee and happy new year!

If it's not an issue, I'll move the python code out to a repo I own and later try to get it into nixpkgs or nix-community or something. My experience with nixpkgs hasn't been good as PRs seem to take a considerable amount of time to be merged if they are merged at all, and I don't want that to hold up this PR.

My mac isn't with me at the moment, so the earliest I can test the changes will be this Sunday, 2024-01-07.

Cheers

Edit: I still need to rebase and merge the commits, but it should be ready for testing by somebody with a mac.

michaelCTS pushed a commit to michaelCTS/podmactl that referenced this pull request Jan 2, 2024
modules/misc/news.nix Outdated Show resolved Hide resolved
@michaelCTS michaelCTS force-pushed the docker-service branch 2 times, most recently from ea86266 to f660ee3 Compare February 14, 2024 08:22
michaelCTS pushed a commit to michaelCTS/home-manager that referenced this pull request Feb 14, 2024
This module is a continuation of nix-community#2630 by MaeIsBad.

It also adds a module `virtualisation.oci-containers` that is
equivalent to the one in NixOS. Basically it allows a simple toggle to
activate oci-container services and commands.

We also support Podman on mac. Note, Podman requires a VM on mac,
which has to be started before any Podman commands can be executed.
Users might sometimes require VMs that use different architectures
than the default VM started by Podman. Thus, they get the option to
define the VM(s) that will be initialized and started by podman.

Since Podman has to start a machine, it's best to do it using launchd.
The configuration of the machines requires a JSON, generated from an
attrset in Home Manager, which is where Python script comes into play
to take care of diff-ing the `podman machine list` to CRUD them.

PR nix-community#4331

Co-authored-by: MaeIsBad <26093674+MaeIsBad@users.noreply.github.com>
@michaelCTS
Copy link
Author

Alright, I moved out podmactl to https://github.com/michaelCTS/podmactl . After multiple tests, it seems quite functional on mac m1.

This should be ready to merge now 🙂

This module is a continuation of nix-community#2630 by MaeIsBad.

It also adds a module `virtualisation.oci-containers` that is
equivalent to the one in NixOS. Basically it allows a simple toggle to
activate oci-container services and commands.

We also support Podman on mac. Note, Podman requires a VM on mac,
which has to be started before any Podman commands can be executed.
Users might sometimes require VMs that use different architectures
than the default VM started by Podman. Thus, they get the option to
define the VM(s) that will be initialized and started by podman.

Since Podman has to start a machine, it's best to do it using launchd.
The configuration of the machines requires a JSON, generated from an
attrset in Home Manager, which is where Python script comes into play
to take care of diff-ing the `podman machine list` to CRUD them.

PR nix-community#4331

Co-authored-by: MaeIsBad <26093674+MaeIsBad@users.noreply.github.com>
@terlar terlar mentioned this pull request Feb 15, 2024
6 tasks
Comment on lines +210 to +215
ExecStart = [
"${cfg.finalPackage}/bin/podman"
"$LOGGING"
"system"
"service"
];
Copy link
Member

Choose a reason for hiding this comment

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

This will cause the service file to contain one ExecStart for each entry in the list. I assume you want to run this as a single command?

Suggested change
ExecStart = [
"${cfg.finalPackage}/bin/podman"
"$LOGGING"
"system"
"service"
];
ExecStart = lib.concatStringsSep " " [
"${cfg.finalPackage}/bin/podman"
"$LOGGING"
"system"
"service"
];

Service = {
Type = "exec";
KillMode = "process";
Environment = ''LOGGING=" --log-level=info"'';
Copy link
Member

Choose a reason for hiding this comment

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

Generally speaking it is best to make the Environment attributes have list values to allow for merging:

Suggested change
Environment = ''LOGGING=" --log-level=info"'';
Environment = [ ''LOGGING="--log-level=info"'' ];

@rycee
Copy link
Member

rycee commented Mar 6, 2024

Sorry about the delay. I had a chance to try this out. But I don't know how to get it to work. Firstly, the generated systemd service file seems to be invalid due to generating multiple ExecStart fields. Once I turned into a single command I get an error

Error: command required for rootless mode with multiple IDs:
       exec: "newuidmap": executable file not found in $PATH

I didn't do any special configuration, just added

virtualisation.podman.enable = true;

in my HM configuration. Do I need some additional configuration to get the service up?

@michaelCTS
Copy link
Author

I'm pretty sure I copied the podman systemd unit from nixos, but that was months ago. I'll have to apply the changes you proposed. As for newuidmap, it must've worked on my nixos because it was installed globally somehow.

podman requires the binary with a setuid. NixOS/nixpkgs#138423 has more information. Will have to test this on a non-nixos system again.

@michaelCTS
Copy link
Author

I'm sorry, I'll have to close this. The podman update to 5.0 made changes to podman machine that I'd have to integrate and I've since moved away from trying to support Mac as my company has another solution.

Anybody is free to try and take this over. I'll leave my branch up.

@michaelCTS michaelCTS closed this Apr 15, 2024
@jlbribeiro
Copy link

@michaelCTS Thank you so so much for your work on this PR (and the reviewers for their feedback on it)!

My humble suggestion/request for whoever picks up this wonderful work (unless #4801 gets merged before): I think macOS/Darwin support should not block the merge of this module, as the original @MaeIsBad PR this one is based off and #4336 indicate, this has been a need for a while (at least for me).

Imho, NixOS should be the priority, given it would enable proper rootless podman, since NixOS/nixpkgs#207050 and NixOS/nixpkgs#259770 are open, and the discussion in containers/podman#20573 seems to point in the direction of using systemd --user units, and not system systemd + User= (previously discussed in containers/podman#12778).

After supporting NixOS, generic Linux systems, and only then, macOS/Darwin (which will always be a bit more complex). Linux-only support would already be a great improvement on QoL regarding podman on Nix-managed systems.
(With that said, taking the time to design an API that is extension-friendly (so as to allow supporting other platforms) is always good.)

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.

5 participants