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

Worker pids limit #276

Closed
wants to merge 3 commits into from
Closed

Conversation

candleindark
Copy link
Collaborator

@candleindark candleindark commented Nov 24, 2023

This PR provides a workaround solution to set the PID limit of the worker service container. This workaround is needed because currently Podman Compose, as of version 1.0.6, doesn't support setting of the PID limit by the pid_limit key or the deploy.reservations.pids key.

This solution provides docker-compose.dev.no-worker.yml to bring up all services in Datalad-Registry except the worker service and worker-up.sh to bring up the worker service with option to choose the PID limit using the --pids-limit option.

Note:

  1. A request has been made to the podman-compose project to add support of setting the PID limit of a service container. (Add support of setting PID limit containers/podman-compose#806)
  2. Additionally, this workaround will not be needed as soon as Podman is update to version 4.4 or later, https://docs.podman.io/en/v4.4/markdown/podman-update.1.html#pids-limit-limit.

@candleindark candleindark marked this pull request as draft November 24, 2023 05:54
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (85987c9) 98.73% compared to head (74673c2) 98.73%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #276   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          47       47           
  Lines        2215     2215           
=======================================
  Hits         2187     2187           
  Misses         28       28           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@candleindark candleindark marked this pull request as ready for review November 24, 2023 08:59
@yarikoptic
Copy link
Member

Thank you for tracking it all down. Good that you shared the ps output which finally let us see the actual reason for the need for so many PIDs - they were not picked up. We had similar issue awhile back in buildbot setup where we also had clients start many of processes in docker container. Pasting here some references from my reply on matrix

Here is the commit where I added a simple init process to fix it datalad/buildbot@3dc7cb1
Here more on that zombie ripper https://github.com/phusion/baseimage-docker/tree/master#whats-inside-the-image

So I think we should not bother raising PID limit, but rather add this init process into the setup

@yarikoptic
Copy link
Member

yarikoptic commented Nov 28, 2023

ok, if easy to rebase image to be on top of that https://github.com/phusion/baseimage-docker/tree/master -- go ahead.

Otherwise -- we could just copy that https://github.com/phusion/baseimage-docker/blob/master/image/bin/my_init here, and use it instead of plain "bash -c" to start celery worker. Something like

diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml
index 97a9974..f40bc43 100644
--- a/docker-compose.dev.yml
+++ b/docker-compose.dev.yml
@@ -37,10 +37,11 @@ services:
       db:
         condition: service_healthy
     command: [
-      "bash", "-c",
-      "celery -A datalad_registry.make_celery:celery_app worker --loglevel INFO --pool prefork"
+      "my_init", "--skip-startup-files", "--skip-runit", "--",
+      "celery", "-A", "datalad_registry.make_celery:celery_app", "worker", "--loglevel", "INFO", "--pool", "prefork"
     ]
     volumes:
+      - ./bin/my_init:/usr/local/sbin/my_init
       - ${WORKER_PATH_AT_HOST}/data/cache:/data/cache
     environment:
       <<: *env

@candleindark
Copy link
Collaborator Author

This approach has been abandon. We will ensure the Docker image for the project has a correct init process instead which is accomplished by #282.

@candleindark candleindark deleted the worker-pids-limit branch February 26, 2024 16:36
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.

2 participants