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

Expose cpu quota #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Expose cpu quota #28

wants to merge 2 commits into from

Conversation

ovesh
Copy link

@ovesh ovesh commented Apr 23, 2020

This PR exposes the api for querying the cpu quota.

The specific use case is when we need to specify the concurrency level (via cli arg) for a non-golang subprocess.

Avishai Weissberg added 2 commits April 12, 2020 10:30
@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #28 into master will decrease coverage by 1.82%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   91.91%   90.09%   -1.83%     
==========================================
  Files           8        9       +1     
  Lines         198      202       +4     
==========================================
  Hits          182      182              
- Misses         13       17       +4     
  Partials        3        3              
Impacted Files Coverage Δ
cpuquota/cpuquota.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e393bb0...5f4d8ed. Read the comment docs.

@jcorbin
Copy link
Contributor

jcorbin commented Apr 24, 2020

Would your use case be able to instead just use runtime.GOMAXPROCS(-1) to get the effective value separately from this library?

@ovesh
Copy link
Author

ovesh commented Apr 24, 2020

Would your use case be able to instead just use runtime.GOMAXPROCS(-1) to get the effective value separately from this library?

@jcorbin
To my understanding, no. runtime.GOMAXPROCS(-1) won't return an accurate CPU allocation for a containerized application. It returns the global runtime.gomaxprocs, which I verified is the total number of cores on the host and not the CPU allocation.

Am I misunderstanding your comment?

@jcorbin
Copy link
Contributor

jcorbin commented Apr 24, 2020

Would your use case be able to instead just use runtime.GOMAXPROCS(-1) to get the effective value separately from this library?

@jcorbin
To my understanding, no. runtime.GOMAXPROCS(-1) won't return an accurate CPU allocation for a containerized application. It returns the global runtime.gomaxprocs, which I verified is the total number of cores on the host and not the CPU allocation.

Am I misunderstanding your comment?

So our automaxprocs library works be changing that value. Any time after that, you'll get the newly affected value ( unless something else has changed it of course, which should be rare ). It adds a little bit of logic like "round down" and minimum clamping. But it will be better than the total number of cores available.

@ovesh
Copy link
Author

ovesh commented Apr 24, 2020

So our automaxprocs library works be changing that value

I'd like to get the value without setting it for the current process.

@CLAassistant
Copy link

CLAassistant commented Jun 13, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Avishai Weissberg seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@piyush-singh
Copy link

This PR would resolve #29 which would be useful for us here at cockroach. Any chance we can revive this?

@prashantv
Copy link
Contributor

Please see my comment here: #29 (comment)

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.

5 participants