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

nixos/oci-containers can depend on arbitrary systemd services #82102

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CMCDragonkai
Copy link
Member

Motivation for this change

Fixes #81814

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@CMCDragonkai
Copy link
Member Author

This adds dependsOnService.

It's the same as dependsOn, except it doesn't focus solely on other containers. It allows you to wait on any arbitrary service.

I use this for creating a one-shot docker-pull.service that pulls in the containers once and which gets used by many different services.

@CMCDragonkai
Copy link
Member Author

I also found that I need a separation between requires and after.

If I have too many containers using dependsOnService, they all get put into requires which attempts to start a service. And this can hit this problem: https://serverfault.com/questions/845471/service-start-request-repeated-too-quickly-refusing-to-start-limit/845473#845473

So I need to instead have another afterServices and not just dependsOnServices. At this point, one may just separate into requireServices and afterServices.

@Ma27
Copy link
Member

Ma27 commented Mar 9, 2020

👎 you can add more services by overriding the systemd service with systemd.services.docker.requires = [].

@CMCDragonkai
Copy link
Member Author

That's not what I need. My specific containers need to depend on specific services. Not that docker depends on some service. It would not solve my problem.

@Ma27
Copy link
Member

Ma27 commented Mar 11, 2020

Oof, I totally misinterpreted the patch, sorry!

One question though: is there any reason why you don't do systemd.services.docker-container.after = []?

Also, I think that if we merge this, we should probably provide options for after and requires as you suggested.

@CMCDragonkai
Copy link
Member Author

Yea I have it in my own custom module. You won't know the name of the container ahead of time. It would be a leaky abstraction.

Copy link
Contributor

@Rakesh4G Rakesh4G left a comment

Choose a reason for hiding this comment

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

Hi @CMCDragonkai ,

We can add this.

git diff                                                                                                                                                                           /projects/github/nixpkgs_matrixai
diff --git a/nixos/modules/virtualisation/docker-containers.nix b/nixos/modules/virtualisation/docker-containers.nix
index 31d6b1ef745..59f32352da5 100644
--- a/nixos/modules/virtualisation/docker-containers.nix
+++ b/nixos/modules/virtualisation/docker-containers.nix
@@ -184,16 +184,31 @@ let
           '';
         };
 
-        dependsOnService = mkOption {
+        requiresServices = mkOption {
           type = with types; listOf str;
           default = [];
           description = ''
-            Define which other services this one depends on. They will be added to both After and Requires for the unit.
+            Define which other services this one depends on. They will be added to only Requires for the unit.
           '';
           example = literalExample ''
             services.docker-containers = {
               node1 = {
-                dependsOnService = [ "ncsd.service" ];
+                requiresServices = [ "ncsd.service" ];
+              }
+            }
+          '';
+        };
+
+        afterServices = mkOption {
+          type = with types; listOf str;
+          default = [];
+          description = ''
+            Define which other services this one will delay until they have started. They will be added to only the After for the unit.
+          '';
+          example = literalExample ''
+            services.docker-containers = {
+              node1 = {
+                afterServices = [ "ncsd.service" ];
               }
             }
           '';
@@ -223,8 +238,8 @@ let
     mkAfter = map (x: "docker-${x}.service") container.dependsOn;
   in rec {
     wantedBy = [] ++ optional (container.autoStart) "multi-user.target";
-    after = [ "docker.service" "docker.socket" ] ++ mkAfter ++ container.dependsOnService;
-    requires = after;
+    after = [ "docker.service" "docker.socket" ] ++ mkAfter ++ container.afterServices;
+    requires = [ "docker.service" "docker.socket" ] ++ mkAfter ++ container.requiresServices;
     path = [ pkgs.docker ];
 
     preStart = ''

@CMCDragonkai
Copy link
Member Author

This is now updated with 2 additional options of afterServices and requiresServices thus making docker container services alot more flexible in their interaction with other systemd services!

@CMCDragonkai
Copy link
Member Author

This is ready to be merged.

@CMCDragonkai
Copy link
Member Author

This has been updated since docker-containers.nix module has changed oci-containers.nix.

Someone please merge this.

@CMCDragonkai
Copy link
Member Author

Who's responsible for this module?

@CMCDragonkai CMCDragonkai changed the title nixos/docker-containers can depend on arbitrary systemd services nixos/oci-containers can depend on arbitrary systemd services Aug 11, 2020
@CMCDragonkai
Copy link
Member Author

Updated name of this PR and name of the commit message.

@infinisil
Copy link
Member

Wouldn't it be better to make naming deterministic, such that systemd.services.*.{requires,after} can be used directly?

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 24, 2020 via email

@infinisil
Copy link
Member

Oh wait they already are, never mind. But then you can just do this instead:

{
  systemd.services.docker-foo.after = [ "docker-bar.service" ];
}

Same for requires. No need for an additional option.

I now see that the problem with this is that this might change from docker to podman in the future, but then I propose this change:

diff --git a/nixos/modules/virtualisation/oci-containers.nix b/nixos/modules/virtualisation/oci-containers.nix
index a46dd65eb49..894d9f8a784 100644
--- a/nixos/modules/virtualisation/oci-containers.nix
+++ b/nixos/modules/virtualisation/oci-containers.nix
@@ -310,7 +310,7 @@ in {
 
   config = lib.mkIf (cfg.containers != {}) (lib.mkMerge [
     {
-      systemd.services = mapAttrs' (n: v: nameValuePair "${cfg.backend}-${n}" (mkService n v)) cfg.containers;
+      systemd.services = mapAttrs' (n: v: nameValuePair "oci-${n}" (mkService n v)) cfg.containers;
     }
     (lib.mkIf (cfg.backend == "podman") {
       virtualisation.podman.enable = true;

Then you can do

{
  systemd.services.oci-foo.after = [ "oci-bar.service" ];
}

Without worrying about changing backends.

@stale
Copy link

stale bot commented Feb 21, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 21, 2021
@CMCDragonkai
Copy link
Member Author

@infinisil if you prefer that, can you send up a PR to ensure the OCI has deterministic naming then? Then we don't have to add an additional option to the container modules.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 30, 2021
@stale
Copy link

stale bot commented Sep 26, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 26, 2021
@mbrgm
Copy link
Member

mbrgm commented Nov 15, 2021

Still relevant.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 15, 2021
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 19, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank marked this pull request as draft March 25, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using docker-containers with multiple containers on the same image results in parallel redundant pulls
6 participants