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

Open
networkimprov opened this issue Aug 18, 2020 · 11 comments
Open

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

networkimprov opened this issue Aug 18, 2020 · 11 comments
Labels
Projects
Milestone

Comments

@networkimprov
Copy link

@networkimprov 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
@gopherbot gopherbot added the Proposal label 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 Aug 18, 2020
@elgatito
Copy link

@elgatito elgatito commented Aug 19, 2020

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

@rsc
Copy link
Contributor

@rsc 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 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

@aclements aclements commented Sep 3, 2020

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 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

@aclements aclements commented Sep 3, 2020

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

@networkimprov networkimprov commented Sep 3, 2020

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 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 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

@networkimprov networkimprov commented Sep 18, 2020

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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.