Skip to content

linux: fix uv_available_parallelism using cgroup #4278

Merged
bnoordhuis merged 20 commits intolibuv:v1.xfrom
Ludo171:fix/uv_available_parallelism_cgroup
Feb 28, 2024
Merged

linux: fix uv_available_parallelism using cgroup #4278
bnoordhuis merged 20 commits intolibuv:v1.xfrom
Ludo171:fix/uv_available_parallelism_cgroup

Conversation

@waltoss
Copy link
Copy Markdown
Contributor

@waltoss waltoss commented Jan 9, 2024

PR to fix #4146

@waltoss waltoss mentioned this pull request Jan 9, 2024
Closed
@waltoss
Copy link
Copy Markdown
Contributor Author

waltoss commented Jan 9, 2024

Any idea why the ASAN build is failing ?

bnoordhuis
bnoordhuis reviewed Jan 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I pointed out single instances of style issues but please fix them all.

The ASAN buildbot is known to be problematic at the moment. You can just ignore it.

@vtjnash vtjnash mentioned this pull request Jan 12, 2024
Closed
@waltoss
Copy link
Copy Markdown
Contributor Author

waltoss commented Jan 25, 2024 •
edited
Loading

@bnoordhuis I fixed styling issues and added tests directly in test-platform-output.c. It might be a bit ugly though, happy to have your feedback on it

To test it manually I built it with the following Dockerfile

# Use the official Ubuntu base image
FROM ubuntu:20.04

# Update the package list
RUN apt-get update

# Install cmake and other build dependencies
RUN DEBIAN_FRONTEND=noninteractive apt-get install -y cmake libtool autoconf build-essential

# Fix : for some reason cmake is looking for gmake instead of make
RUN ln -s /usr/bin/make /usr/bin/gmake

# Set the working directory inside the container
WORKDIR /workspace

# Create a user 'libuv' and give ownership of /workspace to 'libuv'
RUN useradd -m libuv && chown -R libuv:libuv /workspace

# Switch to the 'libuv' user
USER libuv

# Copy the current directory contents into the container at /workspace
COPY --chown=libuv:libuv ./ /workspace/

# Run the build steps as 'libuv' user
RUN mkdir -p build
RUN (cd build && cmake .. -DBUILD_TESTING=ON)
RUN cmake --build build

# By default, run a shell
CMD ["/bin/sh", "-c", "/workspace/build/uv_run_tests platform_output"]

And following commands

$ docker build . libuv-test-cgroup
$ docker run --cpus="2" libuv-test-cgroup | grep available

It correctly outputs 2 available cpus

image

bnoordhuis
bnoordhuis reviewed Jan 26, 2024
View reviewed changes
@waltoss
Copy link
Copy Markdown
Contributor Author

waltoss commented Jan 30, 2024

@bnoordhuis have you other feedbacks ?

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Jan 31, 2024

Can you rebase on top of the v1.x branch? It looks like you're a few commits behind.

@waltoss waltoss force-pushed the fix/uv_available_parallelism_cgroup branch from f5bf3c6 to deda40d Compare February 4, 2024 14:19
@waltoss waltoss changed the base branch from master to v1.x February 4, 2024 14:20
@waltoss
Copy link
Copy Markdown
Contributor Author

waltoss commented Feb 4, 2024

Can you rebase on top of the v1.x branch? It looks like you're a few commits behind.

✅

@vtjnash vtjnash mentioned this pull request Feb 7, 2024
Closed
waltoss added 18 commits February 11, 2024 10:38
@waltoss
linux: fix uv_available_parallelism using cgroup
78a33f3 
uv_available_parallelism does not handle container cpu limit
set by systems like Docker or Kubernetes. This patch fixes
this limitation by comparing the amount of available cpus
returned by syscall with the quota of cpus available defined
in cgroup

See issue: libuv#4146
@waltoss
linux: add dockerfile to test cgroup mechanisms
a5a0fb7
@waltoss
vscode: add task to debug tests in container
7a197e3 
Not working yet
@waltoss
refactor(linux): rename cpu_constraint_cpu
4bb8600
@waltoss
refactor(linux): remove vscode debugger
8cd7241
@waltoss
refactor(linux): remove dockerfile to test
8b15c3c
@waltoss
fix(unix): update uv__get_constrained_cpu function signature
2c4b76a
@waltoss
fix(win): declare empty uv__get_constrained_cpu
714f6ab
@waltoss
refactor(linux) move uv__cpu_constraint_cpu to internal definitions
1cac899
@waltoss
style(linux): fix various styling issues
28408c2
@waltoss
text(linux): remove uv__get_constrained_cpu from tests
fc7bcca
@waltoss
test(linux): test available_parallelism with cgroups
bb72f8e
@waltoss
fix(linux): properly cast rc to double
cbffcf4
@waltoss
feat(linux): better cgroup error management
5e682f0
@waltoss
fix(style): undo line return
539f8cb
@waltoss
test(linux): simplify cgroup parallelism tests
718f64c
@waltoss
chore: remove trailing newline
dddf56d
@waltoss
style: align comment
dd4d126
@waltoss waltoss force-pushed the fix/uv_available_parallelism_cgroup branch from deda40d to dd4d126 Compare February 11, 2024 09:39
@waltoss
Copy link
Copy Markdown
Contributor Author

waltoss commented Feb 11, 2024

@bnoordhuis I rebased on v1.x

bnoordhuis
bnoordhuis approved these changes Feb 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

@libuv/collaborators can one of you give this a quick high-level look-over?

santigimeno
santigimeno approved these changes Feb 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of comments.

@waltoss
Copy link
Copy Markdown
Contributor Author

waltoss commented Feb 27, 2024

@santigimeno @bnoordhuis any idea when this can be merged ?

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Feb 28, 2024

I'll go ahead and merge it. Thanks for the pull request.

@bnoordhuis bnoordhuis merged commit 6b56200 into libuv:v1.x Feb 28, 2024
giordano pushed a commit to JuliaLang/libuv that referenced this pull request Aug 26, 2024
@waltoss @giordano
linux: fix uv_available_parallelism using cgroup (libuv#4278)
96e8499 
uv_available_parallelism does not handle container cpu limit
set by systems like Docker or Kubernetes. This patch fixes
this limitation by comparing the amount of available cpus
returned by syscall with the quota of cpus available defined
in the cgroup.

Fixes: libuv#4146
(cherry picked from commit 6b56200)
@giordano giordano mentioned this pull request Aug 26, 2024
Merged
giordano pushed a commit to JuliaLang/libuv that referenced this pull request Aug 31, 2024
@waltoss @giordano
linux: fix uv_available_parallelism using cgroup (libuv#4278)
c57e7f0 
uv_available_parallelism does not handle container cpu limit
set by systems like Docker or Kubernetes. This patch fixes
this limitation by comparing the amount of available cpus
returned by syscall with the quota of cpus available defined
in the cgroup.

Fixes: libuv#4146
(cherry picked from commit 6b56200)
giordano added a commit to giordano/Yggdrasil that referenced this pull request Aug 31, 2024
@giordano
[LibUV] Includes a couple of backports from our fork
fa1bc18 
This includes

* JuliaLang/libuv#38, backport of
  libuv/libuv#4521
* JuliaLang/libuv#39, backport of
  libuv/libuv#4278
@giordano giordano mentioned this pull request Aug 31, 2024
Merged
giordano added a commit to giordano/Yggdrasil that referenced this pull request Aug 31, 2024
@giordano
[LibUV] Includes a couple of backports from our fork
026f269 
This includes

* JuliaLang/libuv#38, backport of
  libuv/libuv#4521
* JuliaLang/libuv#39, backport of
  libuv/libuv#4278
giordano added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Aug 31, 2024
@giordano
[LibUV] Includes a couple of backports from our fork (#9337)
ae9c13f 
This includes

* JuliaLang/libuv#38, backport of
  libuv/libuv#4521
* JuliaLang/libuv#39, backport of
  libuv/libuv#4278
@giordano giordano mentioned this pull request Aug 31, 2024
Merged
giordano added a commit to JuliaLang/julia that referenced this pull request Aug 31, 2024
@giordano
[LibUV_jll] Update to new build (#55647)
d81a45b 
Corresponding PR to Yggdrasil:
JuliaPackaging/Yggdrasil#9337.

This build includes backports of
libuv/libuv#4278 (useful for for #46226) and
libuv/libuv#4521 (useful for #55592)
KristofferC pushed a commit to JuliaLang/julia that referenced this pull request Sep 12, 2024
@giordano
[LibUV_jll] Update to new build (#55647)
f2d5525 
Corresponding PR to Yggdrasil:
JuliaPackaging/Yggdrasil#9337.

This build includes backports of
libuv/libuv#4278 (useful for for #46226) and
libuv/libuv#4521 (useful for #55592)
kshyatt pushed a commit to JuliaLang/julia that referenced this pull request Sep 12, 2024
@giordano @kshyatt
[LibUV_jll] Update to new build (#55647)
cc38d49 
Corresponding PR to Yggdrasil:
JuliaPackaging/Yggdrasil#9337.

This build includes backports of
libuv/libuv#4278 (useful for for #46226) and
libuv/libuv#4521 (useful for #55592)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@santigimeno santigimeno santigimeno approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] - unix available_parallelism does not handle container cpu limits

3 participants

@waltoss @bnoordhuis @santigimeno