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

proposal: cmd/go: build option to set env variable defaults #40870

Closed
networkimprov opened this issue Aug 18, 2020 · 21 comments
Closed

proposal: cmd/go: build option to set env variable defaults #40870

networkimprov opened this issue Aug 18, 2020 · 21 comments

Comments

@networkimprov
Copy link

networkimprov commented Aug 18, 2020

Users are asking for a way to hard-code runtime environment variables, because their end-users cannot set them on app invocation.

I suggest a go build/test/run command-line option. Its use needn't be restricted to Go-specific variables. Existence of a variable in the environment at runtime would override any build-time default.

go build -env 'GODEBUG=asyncpreemptoff=1,madvdontneed=1;MYVAR=x' // semicolon separated list

#37569 (comment) @elgatito
#40722 (comment) @gwillem

I considered a new stdlib API, but that opens the possibility of conflict between packages. A way to prevent that is to restrict the API to package main, but that seems unprecedented.

EDIT: fix example to use semicolon delimiter

@gopherbot gopherbot added this to the Proposal milestone Aug 18, 2020
@networkimprov networkimprov changed the title proposal: build option to set env variable defaults proposal: cmd/go: build option to set env variable defaults Aug 18, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 18, 2020
@elgatito
Copy link

I would like to add that priorities for options setters should be:
Environment -> Build -> Defaults.

@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

If users are asking for this, it means we have failed at the way we configure programs.
Let's correct our mistakes instead of embracing and building atop our failures.
What specific values do users want to hard-code into the binaries and why?
What should we do to make it so those don't need to be set?

On top of that, program configuration belongs in programs not on build command lines.
Instead of go build -env 'GOGC=50' a program can call runtime.SetGCPercent(50).

If the problem is bugs that can be disabled with GODEBUG settings,
perhaps we should have debug.Set("asyncpreempt", "1") or something like that
and let programs call it during init.

/cc @aclements

@elgatito
Copy link

elgatito commented Sep 3, 2020

@rsc

What specific values do users want to hard-code into the binaries and why?

In my case, I need to set values for "asyncpreemptoff" and "madvdontneed" for specific builds, no difference, is it "set on build" or "set by code".

As a develop, I want to be sure program runs the way I plan it, giving users ability to override that with environment settings.
Not always we have the control how the program is ran.

@aclements
Copy link
Member

These are called GODEBUG because they're meant for debugging. If you have to set them programmatically, it suggests we've done something wrong (in the case of madvdontneed, maybe Linux did something wrong, but that's another story). It's possible we could put some of these in the runtime/debug package as @rsc suggested, but I'd like to understand better why you need to change them first.

@elgatito
Copy link

elgatito commented Sep 3, 2020

@aclements In my case I have build targets for linux hardware, running on kernel 3.14, and without madvdontneed it acts differently.

I would accept any way, build options or runtime/debug calls.

@aclements
Copy link
Member

Sorry, but that doesn't really answer my question. Why do you need these options? How does it behave differently and why is it important that you change that?

@networkimprov
Copy link
Author

Some end-users cannot set env vars (stated at top). GODEBUG can't investigate or workaround problems in those cases.

Besides the issues linked at top, 1.14 & 1.15 break CIFS & FUSE. Fixes are in for 1.16 but won't be backported.
#38836 #39237 #40846

@rsc
Copy link
Contributor

rsc commented Sep 18, 2020

@networkimprov, lucky for us this build option wouldn't be backported either, so it isn't any better than a general fix to the EINTR problem. Let's solve that instead.

@rsc
Copy link
Contributor

rsc commented Sep 18, 2020

It sounds like the real problem is that there are two runtime bugs that have not been adequately addressed, and too many people are working around them by setting GODEBUG settings. The two workarounds that are being overused are madvdontneed=1, for various perceived or actual out-of-memory conditions on Linux; and asyncpreemptoff=1, for various problems with spurious EINTR returns from Linux.

For madvdontneed=1, I believe the state of the world is as @aclements summarized in #33376 (comment). Go is doing the same thing as Java and Node here. It should not be necessary to set that. It is unfortunate that Linux tools like top/htop do not subtract out the lazyfree total from RSS in the display, but the OS knows what it can take away and will do that rather than OOM. That comment also says that container memory limits understand what Go is doing here too, so this should not be causing container OOMs either. If someone is seeing problems here, we need to get more information about their exact setup to help understand it better. Hence the (unanswered) questions to @elgatito above and more importantly on #37569. If there are other cases where using MADV_FREE is causing OOMs, please file details in a different issue and we will try to get to the bottom of that.

For asyncpreemptoff=1, the problem is that preemption has caused some (arguably buggy) kernel behavior returning EINTR where we weren't checking for it. And now I believe we are checking. If there are more places to check, let's fix those.

In both cases, fixing the specific bugs is more effective and will be easier than adding a new build option we will have to support for all time.

Let's fix the real bugs.

@networkimprov
Copy link
Author

Certainly fix the bugs, but why discount the value of letting end-users run apps with a preset GODEBUG (or other env) value? Why not a runtime/debug API for it?

That is the only way you can possibly get debug info from certain sites.

@rsc
Copy link
Contributor

rsc commented Sep 18, 2020

It's just more API surface that we are stuck with forever, and it's API surface that shouldn't even exist.

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 23, 2020
@rsc
Copy link
Contributor

rsc commented Sep 30, 2020

Based on the discussion above, this seems like a likely decline.

@networkimprov
Copy link
Author

networkimprov commented Oct 7, 2020

You just got another inquiry about this: #41818 (comment)

@elgatito
Copy link

elgatito commented Oct 7, 2020

@rsc I understand, you want to stay clear and avoid having environment variables hell, but, again, with the change of MADV_FREE behavior in Go, when running on hosts with old kernels (like 3.17) we get OOM from time to time.
I'm not sure it's related to Go really, but I'd like to control such things in my binary, without shell scripts.

@blinkinglight
Copy link

I think, each env variable should have method with its own documentation, instead of comment block in env variables. Because how madvdontneed=1 and production related to word goDEBUG? Also, I think, that defaults should be reviewed and who really needs performance ought to have ability to tune it.

@rsc
Copy link
Contributor

rsc commented Oct 7, 2020

@elgatito, the answer for MADV_FREE is being discussed in #37569. It may be that we should just stop trying to use it, at least on Android, but possibly everywhere. That's separate from this discussion.

@blinkinglight, these are meant to be temporary knobs, not permanent API. If lots of people need one, we're doing something wrong and we should fix it, as I commented above.

@rsc
Copy link
Contributor

rsc commented Oct 7, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Oct 7, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Oct 7, 2020
@networkimprov
Copy link
Author

A workaround on unix might be to check os.GetEnv() for the necessary variable, set it with os.SetEnv(), then restart the app:

unix.Exec(os.Args[0], os.Args[1:], os.Environ())

https://godoc.org/golang.org/x/sys/unix#Exec

@neilalexander
Copy link
Contributor

neilalexander commented Oct 15, 2020

For what it's worth, MADV_FREE/MADV_DONTNEED does create a rather significant perceptual problem with resource usage. When users look at top/htop and see the resident set size inflated and seemingly never decreasing, the first thing they usually do is point the finger towards the developers because "your program is using all of my memory"!

It would be nice to have a way to choose between perception vs optimisation in cases like this that don't require setting environment variables.

Not to mention that, for things built using gobind/gomobile, environment variables aren't exactly straight-forward.

@gopherbot
Copy link

Change https://golang.org/cl/267100 mentions this issue: runtime: default to MADV_DONTNEED on Linux

gopherbot pushed a commit that referenced this issue Nov 2, 2020
In Go 1.12, we changed the runtime to use MADV_FREE when available on
Linux (falling back to MADV_DONTNEED) in CL 135395 to address issue
 #23687. While MADV_FREE is somewhat faster than MADV_DONTNEED, it
doesn't affect many of the statistics that MADV_DONTNEED does until
the memory is actually reclaimed under OS memory pressure. This
generally leads to poor user experience, like confusing stats in top
and other monitoring tools; and bad integration with management
systems that respond to memory usage.

We've seen numerous issues about this user experience, including
 #41818, #39295, #37585, #33376, and #30904, many questions on Go
mailing lists, and requests for mechanisms to change this behavior at
run-time, such as #40870. There are also issues that may be a result
of this, but root-causing it can be difficult, such as #41444 and
 #39174. And there's some evidence it may even be incompatible with
Android's process management in #37569.

This CL changes the default to prefer MADV_DONTNEED over MADV_FREE, to
favor user-friendliness and minimal surprise over performance. I think
it's become clear that Linux's implementation of MADV_FREE ultimately
doesn't meet our needs. We've also made many improvements to the
scavenger since Go 1.12. In particular, it is now far more prompt and
it is self-paced, so it will simply trickle memory back to the system
a little more slowly with this change. This can still be overridden by
setting GODEBUG=madvdontneed=0.

Fixes #42330 (meta-issue).

Fixes #41818, #39295, #37585, #33376, #30904 (many of which were
already closed as "working as intended").

Change-Id: Ib6aa7f2dc8419b32516cc5a5fc402faf576c92e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/267100
Trust: Austin Clements <austin@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@networkimprov
Copy link
Author

@aclements @rsc @ianlancetaylor

Here is another case #41099 (comment) describing the need to disable async preemption for all instances of an application (it trips up a required third-party library).

In that light it may be worth reconsidering this proposal...

@golang golang locked and limited conversation to collaborators Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants