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

Add Concurrent.cpu_requests that is cgroups aware. #1058

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

heka1024
Copy link
Contributor

In K8s environments, cpu requests are often more useful than limits, but concurrent-ruby currently lacks a related method, so I've added it. Below are links to articles about the usefulness of cpu requests.

@eregon
Copy link
Collaborator

eregon commented Jul 30, 2024

Could you summarize in one/a few sentences why one should use cpu requests instead of cpu limits?

@heka1024
Copy link
Contributor Author

heka1024 commented Jul 30, 2024

@eregon CPU limits can lead to throttling and reduced performance under high load. So, as Tim Hockin suggested, I've set only requests in my application at company. I think this is a quiet common pattern and it'll be nice if we can provide method to get cpu requests.

@eregon
Copy link
Collaborator

eregon commented Aug 1, 2024

I've read quickly through the blog posts, so it seems the limit/quota is a hard limit (don't use more CPU even if idle) and this requests/shares/weight is like a relative share for each pod/workload on the node, so can use more of the CPU if available. Makes sense.

@eregon
Copy link
Collaborator

eregon commented Aug 1, 2024

@byroot @casperisfine Could you review this too?

if RbConfig::CONFIG["target_os"].include?("linux")
if cgroups_v2?
# Ref: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2254-cgroup-v2#phase-1-convert-from-cgroups-v1-settings-to-v2
weight = File.read('/sys/fs/cgroup/cpu.weight').to_f
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably fine, but in compute_cpu_quota we only ever read files we tested for existence prior.

Here we check /sys/fs/cgroup/cpu.max and assume it means /sys/fs/cgroup/cpu.weight exists? Maybe that's a correct assertion, but I personally don't know if it's true.

I think ultimately it would be simpler to not extract cgroups_v1? / cgroups_v2? and check if /sys/fs/cgroup/cpu.weight exists here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, this seems safer

weight = File.read('/sys/fs/cgroup/cpu.weight').to_f
((((weight - 1) * 262142) / 9999) + 2) / 1024
elsif cgroups_v1?
File.read('/sys/fs/cgroup/cpu/cpu.shares').to_f / 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue here.

Copy link
Contributor

@byroot byroot left a comment

Choose a reason for hiding this comment

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

I'd undo the "extract method" for extra safety, but other than that LGTM.

@heka1024
Copy link
Contributor Author

heka1024 commented Aug 4, 2024

@byroot @eregon I'd undo the "extract method" and replace it with file existence check. Could you review this once again?

@eregon eregon merged commit 6f7c91a into ruby-concurrency:master Aug 5, 2024
14 checks passed
@heka1024 heka1024 deleted the cpu-requests branch August 6, 2024 14:50
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.

3 participants