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.

Sign up for GitHub

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

fix: Parallelism CPU calculation inside Kubernetes and Docker with limits #799

Merged
merged 8 commits into from
Feb 1, 2025
+16 −7
Merged

fix: Parallelism CPU calculation inside Kubernetes and Docker with limits #799

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ce318b
fix: parallelism CPU calculation inside k8s
daniel-sampliner Jan 30, 2025
6af478f
refactor: style
daniel-sampliner Jan 31, 2025
b426613
fix: clean error message when unexpected cpu.cfs_quota_us
daniel-sampliner Jan 31, 2025
9bd7a26
fix: suppress unnecessary error
daniel-sampliner Jan 31, 2025
4a77613
chore: Add docs and move vars inside their if's
MaxymVlasov Jan 31, 2025
70eefc8
fix: Docker CPU limits
MaxymVlasov Jan 31, 2025
97d7f3a
Apply suggestions from code review
MaxymVlasov Jan 31, 2025
c6331c0
Merge branch 'master' into k8s-parallelism
MaxymVlasov Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions hooks/_common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,14 +197,15 @@ function common::is_hook_run_on_whole_repo {
function common::get_cpu_num {
local -r parallelism_ci_cpu_cores=$1

local cpu_quota cpu_period cpu_num
local millicpu

if [[ -f /sys/fs/cgroup/cpu/cpu.cfs_quota_us &&
! -f /proc/sys/fs/binfmt_misc/WSLInterop ]]; then # WSL have cfs_quota_us, but WSL should be checked as usual Linux host
# Inside K8s pod or DinD in K8s
millicpu=$(< /sys/fs/cgroup/cpu/cpu.cfs_quota_us)
cpu_quota=$(< /sys/fs/cgroup/cpu/cpu.cfs_quota_us)

if [[ $millicpu -eq -1 ]]; then
if [[ $cpu_quota -eq -1 ]]; then
# K8s no limits or in DinD
if [[ -n $parallelism_ci_cpu_cores ]]; then
if [[ ! $parallelism_ci_cpu_cores =~ ^[[:digit:]]+$ ]]; then
Expand Down Expand Up @@ -233,7 +234,13 @@ function common::get_cpu_num {
return
fi

echo $((millicpu / 1000))
cpu_period=$(< /sys/fs/cgroup/cpu/cpu.cfs_period_us)
cpu_num=$((cpu_quota / cpu_period))
if ((cpu_num < 1)); then
echo 1
else
echo $cpu_num
fi
daniel-sampliner marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

@daniel-sampliner Do you think that there's no need for echo $((millicpu / 1000)) as e.g. a fallback if /sys/fs/cgroup/cpu/cpu.cfs_period_us does not exist (in case it may not exist at all)? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I don't believe it's possible for cpu.cfs_period_us to not exist if cpu.cfs_quota_us does exist and is > 0. But I also don't know for certain.

It definitely can't hurt to fall back to something if cpu.cfs_period_us doesn't exist, but I don't think millicpu / 1000 would be the right value. Perhaps like this to default to 1 CPU when cpu.cfs_period_us doesn't exist

Suggested change
cpu_period=$(< /sys/fs/cgroup/cpu/cpu.cfs_period_us)
cpu_num=$((cpu_quota / cpu_period))
if ((cpu_num < 1)); then
echo 1
else
echo $cpu_num
fi
cpu_period=$(< /sys/fs/cgroup/cpu/cpu.cfs_period_us || echo -1)
cpu_num=$((cpu_quota / cpu_period))
[[ $cpu_num -lt 1 ]] && echo 1 || echo $cpu_num

Copy link
Collaborator

@yermulnik yermulnik Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, the above commit suggestion makes sense to me.
@MaxymVlasov WDYT?

return
fi

Expand Down
Loading