feat(env): auto set GOMAXPROCS by go.uber.org/automaxprocs #6576
Conversation
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
|
Amazing @dongjiang1989 🥳 |
|
Thanks for working on this @dongjiang1989 |
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
| @@ -183,6 +185,13 @@ func run(fs *flag.FlagSet) int { | |||
| stdlog.Fatal(err) | |||
| } | |||
|
|
|||
| l := func(format string, a ...interface{}) { | |||
There was a problem hiding this comment.
I think it should be behind a feature flag, wdyt?
There was a problem hiding this comment.
We still don't have feature flags tho 😅
But I agree that this needs to be configurable, e.g. what's the side-effect of having gomaxprocs auto enabled during development? If I run the operator in my machine, will it take over all cores?
There was a problem hiding this comment.
Read the CGroup value in the container environment to identify the CPU quota of the container, calculate the actual number of cores, and automatically set the number of GOMAXPROCS threads.
Like: runtime.GOMAXPROCS(maxProcs)
There was a problem hiding this comment.
Well!
I think it's a fair and simple feature flag implementation.
I'm ok with that, because it's also the same approach we have in Prometheus.
Let's see what the other maintainers think about it.
There was a problem hiding this comment.
I have another PR open for a few weeks implementing feature-flags as well, but following Kubernetes feature-flags approach. #6491
Differently from Prometheus' approach, K8s allows us maintainers to graduate feature flags slowly while still letting users disable feature flags that we might enable by default in the future.
In my opinion, this gives us the flexibility to enable features when we feel it has achieved enough maturity, while also providing safe-guards to users if they break something in production
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
|
I'd be fine using automaxprocs by default without a feature flag. Unlike Prometheus, the operator (almost) always runs on Kubernetes and we should provide the best experience for folks setting CPU limits.
the same as today: the Go runtime will set GOMAXPROCS to the number of cores available from your laptop. |
Yes, it makes sense. |
|
"almost" = except when you do local development |
Thanks. Got it. I'll revert this feature flag. |
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
There was a problem hiding this comment.
the same as today: the Go runtime will set GOMAXPROCS to the number of cores available from your laptop.
I had a wrong understanding of this configuration, I don't know how I thought that but I had the impression it was configuring how many CPU cores it would take priority over other processes in my machine 😅
|
thanks! |
Description
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Auto set GOMAXPROCS.
add dependency on go.uber.org/automaxprocs
ref: #6560
Type of change
What type of changes does your code introduce to the Prometheus operator? Put an
xin the box that apply.CHANGE(fix or feature that would cause existing functionality to not work as expected)FEATURE(non-breaking change which adds functionality)BUGFIX(non-breaking change which fixes an issue)ENHANCEMENT(non-breaking change which improves existing functionality)NONE(if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)Verification
Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.
Changelog entry
Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.