Add auto-gomaxprocs optional feature #10498

TomasKohout · 2022-03-29T14:55:28Z

Signed-off-by: Tomas Kohout tomas.kohout1995@gmail.com

Hi,

this PR is about adding a new dependency on go.uber.go/automaxprocs.

Why we need that?

As you all know nodes come with different sizes. For example, we use quite huge nodes with up to 104 CPUs and because we use prometheus-operator, we aren't able to set environment variables to set GOMAXPROCS to the value of cpu.limit. automaxprocs package does this automatically without any additional pain. If there are not limits specified, default value (number of cores) will be left in place.

roidelapluie · 2022-03-29T16:14:43Z

Thanks for your contribution. Until the go runtime figures this out, can we have this behind a feature flag rather than having it enabled by default?

bboreham · 2022-03-29T19:10:54Z

I think having a side-effect caused by _ is a bit too subtle.
I expect adding a feature flag will fix this too - there will be some description associated with the name.

peterbourgon · 2022-03-29T19:29:11Z

I agree with @roidelapluie and @bboreham.

because we use prometheus-operator, we aren't able to set environment variables

Is this a limitation of prometheus-operator specifically, or operators in general?

TomasKohout · 2022-03-29T20:14:25Z

Feature flag added. 🙂

TomasKohout · 2022-03-29T20:15:20Z

I agree with @roidelapluie and @bboreham.

because we use prometheus-operator, we aren't able to set environment variables

Is this a limitation of prometheus-operator specifically, or operators in general?

It depends. Maybe there are some operators which expose env field via theirs CRD. Prometheus-operator doesn't do so.

TomasKohout · 2022-03-29T20:24:11Z

I'll add feature description to the documentation tomorrow.

cmd/prometheus/main.go

peterbourgon · 2022-03-29T20:32:47Z

cmd/prometheus/main.go

@@ -186,6 +189,9 @@ func (c *flagConfig) setFeatureListOptions(logger log.Logger) error {
 			case "promql-per-step-stats":
 				c.enablePerStepStats = true
 				level.Info(logger).Log("msg", "Experimental per-step statistics reporting")
+			case "auto-gomaxprocs":
+				c.enableAutoGOMAXPROCS = true
+				level.Info(logger).Log("msg", "Automatically set GOMAXPROCS to cpu.limits when running in container.")


If this feature flag is enabled, but Prometheus is not running in a container, will the maxprocs.Set call (below) fail and emit a warning?

It'll not fail nor emit warning, it'll leave the default which is equal to number of CPUs.

I'm not sure under which conditions would Set fail. I've added warning for user to know why the GOMAXPROCS is not set when this feature is enabled.

cmd/prometheus/main.go

roidelapluie · 2022-03-30T07:32:45Z

This LGTM besides my comment. Can you please add a note in the Feature Flags documentation?
Thanks!

TomasKohout · 2022-03-30T08:00:10Z

Note added. I've tried to describe it, but I'm not sure that it's described good enough. 🙂 Could you have an additional look please?

bwplotka

Great, thanks! 💪

bwplotka · 2022-03-30T08:18:30Z

Do you mind singing commit for DCO bot to work? (Instructions can be found when you click on DCO failure)

TomasKohout · 2022-03-30T08:21:45Z

Yay, sure thing!

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

Co-authored-by: Peter Bourgon <peterbourgon@users.noreply.github.com> Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

Co-authored-by: Julien Pivotto <roidelapluie@gmail.com> Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

roidelapluie · 2022-03-30T10:50:18Z

Thanks!

TomasKohout · 2022-03-30T11:22:09Z

That was fast! I should thank you for help. 🙂

SuperQ · 2022-04-21T11:17:07Z

@TomasKohout I'm not sure where you get the idea that the Prometheus-Operator can't inject env vars.

From the CRD API docs

containers:

Containers allows injecting additional containers or modifying operator generated containers. This can be used to allow adding an authentication proxy to a Prometheus pod or to change the behavior of an operator generated container. Containers described here modify an operator generated container if they share the same name and modifications are done via a strategic merge patch. The current container names are: prometheus, config-reloader, and thanos-sidecar. Overriding containers is entirely outside the scope of what the maintainers will support and by doing so, you accept that this behaviour may break at any time without notice.

You should easily be able to something like this

containers:
- name: prometheus
  env:
  - name: GOMAXPROCS
    value: 16

TomasKohout · 2022-04-21T11:27:06Z

@SuperQ well, that sounds nice, but why should I do something that could break at any time without notice?

Edit: IMHO it's better for prometheus to determine GOMAXPROCS automatically from cpu.limits, sorry for misleading information. ;-)

bboreham · 2022-04-21T12:37:22Z

May I suggest editing the PR title and description to match what was merged?
E.g. "Add auto-gomaxprocs optional feature" for the title.

diana-borbe-consensys · 2022-07-06T10:03:23Z

Can you please make auto-gomaxprocs configurable from the helm chart also?

bboreham · 2022-07-06T10:19:51Z

No helm chart in this repo. Probably best to ask wherever the one you use is maintained.

TomasKohout changed the title ~~add dependency on go.uber.org/automaxprocs~~ Add dependency on go.uber.org/automaxprocs Mar 29, 2022

peterbourgon reviewed Mar 29, 2022

View reviewed changes

roidelapluie reviewed Mar 29, 2022

View reviewed changes

cmd/prometheus/main.go Outdated Show resolved Hide resolved

roidelapluie reviewed Mar 29, 2022

View reviewed changes

cmd/prometheus/main.go Outdated Show resolved Hide resolved

roidelapluie reviewed Mar 30, 2022

View reviewed changes

cmd/prometheus/main.go Outdated Show resolved Hide resolved

bwplotka approved these changes Mar 30, 2022

View reviewed changes

TomasKohout force-pushed the main branch from c0c0015 to 501f6ef Compare March 30, 2022 08:23

TomasKohout and others added 10 commits March 30, 2022 10:30

add dependency on go.uber.org/automaxprocs

288d9ad

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

add dependency on go.uber.org/automaxprocs

b1dde95

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

add dependency on go.uber.org/automaxprocs

9d43237

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

add dependency on go.uber.org/automaxprocs

ec0de40

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

Update cmd/prometheus/main.go

210545c

Co-authored-by: Peter Bourgon <peterbourgon@users.noreply.github.com> Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

Update cmd/prometheus/main.go

6fc842c

Co-authored-by: Peter Bourgon <peterbourgon@users.noreply.github.com> Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

suggestions

ad52aa9

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

imports

9a43d8d

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

Update cmd/prometheus/main.go

1d64b8c

Co-authored-by: Julien Pivotto <roidelapluie@gmail.com> Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

an attempt to make documentation

24ff369

Signed-off-by: Tomas Kohout <tomas.kohout1995@gmail.com>

TomasKohout force-pushed the main branch from 501f6ef to 24ff369 Compare March 30, 2022 08:30

roidelapluie merged commit c0fd228 into prometheus:main Mar 30, 2022

roidelapluie mentioned this pull request Apr 6, 2022

cut v2.35.0-rc0 #10543

Merged

TomasKohout changed the title ~~Add dependency on go.uber.org/automaxprocs~~ Add auto-gomaxprocs optional feature Apr 21, 2022

jan--f mentioned this pull request Jun 13, 2023

OCPBUGS-13153: Limit the value of GOMAXPROCS on node-exporter. openshift/cluster-monitoring-operator#1996

Merged

5 tasks

dongjiang1989 mentioned this pull request May 9, 2024

feat(env): add auto go maxprocs prometheus/alertmanager#3837

Merged

jnoordsij mentioned this pull request May 20, 2024

[grafana] Set GOMAXPROCS and GOMEMLIMIT environment variables based on container resources grafana/helm-charts#3138

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auto-gomaxprocs optional feature #10498

Add auto-gomaxprocs optional feature #10498

TomasKohout commented Mar 29, 2022

roidelapluie commented Mar 29, 2022

bboreham commented Mar 29, 2022

peterbourgon commented Mar 29, 2022

TomasKohout commented Mar 29, 2022

TomasKohout commented Mar 29, 2022

TomasKohout commented Mar 29, 2022

peterbourgon Mar 29, 2022

TomasKohout Mar 30, 2022 •

edited

Loading

roidelapluie commented Mar 30, 2022

TomasKohout commented Mar 30, 2022

bwplotka left a comment

bwplotka commented Mar 30, 2022

TomasKohout commented Mar 30, 2022

roidelapluie commented Mar 30, 2022

TomasKohout commented Mar 30, 2022

SuperQ commented Apr 21, 2022

TomasKohout commented Apr 21, 2022 •

edited

Loading

bboreham commented Apr 21, 2022

diana-borbe-consensys commented Jul 6, 2022

bboreham commented Jul 6, 2022

Add auto-gomaxprocs optional feature #10498

Add auto-gomaxprocs optional feature #10498

Conversation

TomasKohout commented Mar 29, 2022

roidelapluie commented Mar 29, 2022

bboreham commented Mar 29, 2022

peterbourgon commented Mar 29, 2022

TomasKohout commented Mar 29, 2022

TomasKohout commented Mar 29, 2022

TomasKohout commented Mar 29, 2022

peterbourgon Mar 29, 2022

Choose a reason for hiding this comment

TomasKohout Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

roidelapluie commented Mar 30, 2022

TomasKohout commented Mar 30, 2022

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Mar 30, 2022

TomasKohout commented Mar 30, 2022

roidelapluie commented Mar 30, 2022

TomasKohout commented Mar 30, 2022

SuperQ commented Apr 21, 2022

TomasKohout commented Apr 21, 2022 • edited Loading

bboreham commented Apr 21, 2022

diana-borbe-consensys commented Jul 6, 2022

bboreham commented Jul 6, 2022

TomasKohout Mar 30, 2022 •

edited

Loading

TomasKohout commented Apr 21, 2022 •

edited

Loading