cli: correctly obtain GOMEMLIMIT env var value#101498
cli: correctly obtain GOMEMLIMIT env var value#101498craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
this seems like the easiest fix, so makes sense to go with it. but i also wonder if the GoMemLimit flag should be changed to use the EnvVar field:
diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go
index 0c06a8bfb85..571b16939ba 100644
--- a/pkg/cli/cliflags/flags.go
+++ b/pkg/cli/cliflags/flags.go
@@ -171,6 +171,7 @@ and 1GiB) or a percentage of physical memory (e.g. .25). If left unspecified,
defaults to 2.25x of --max-sql-memory (subject to max-go-memory + 1.15x --cache
not exceeding 90% of available RAM). Set to 0 to disable the soft memory limit
(not recommended).`,
+ EnvVar: "GOMEMLIMIT",
}
TSDBMem = FlagInfo{
also, i think there could be a test for this in cli/interactive_tests
4518607 to
e6612bd
Compare
On a quick glance, that EnvVar flag assumes that it's for "internal" vars, with COCKROACH_ prefix (see
I made an attempt to add the corresponding test, but I don't think I did it right (the test times out for me locally before and after the fix with |
knz
left a comment
There was a problem hiding this comment.
What you did wrong in your test is not using the start_server command like in other tests.
e6612bd to
97d5765
Compare
This commit fixes how the value of GOMEMLIMIT env var is obtained in case `--max-go-memory` is not specified. Previously, we were using `envutil.Env*` method which assumes that the env var is internal (i.e. has `COCKROACH_` prefix), so it would result in a panic on start up. Now we properly obtain the value as for external env var. Release note (bug fix): CockroachDB 23.1.0 alpha and beta versions previously would panic on `start` command when `GOMEMLIMIT` env var was set and `--max-go-memory` flag wasn't specified, and this is now fixed.
97d5765 to
2e95c3b
Compare
|
I was actually missing |
|
TFTRs! bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting assignees, but backport branch blathers/backport-release-23.1-101498 is ready: Post "https://api.github.com/repos/cockroachdb/cockroach/issues/101564/assignees": read tcp 169.254.8.1:56047->140.82.114.5:443: read: connection reset by peer Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This commit fixes how the value of GOMEMLIMIT env var is obtained in case
--max-go-memoryis not specified. Previously, we were usingenvutil.Env*method which assumes that the env var is internal (i.e. hasCOCKROACH_prefix), so it would result in a panic on startup. Now we properly obtain the value as for external env var.Fixes: #https://github.com/cockroachlabs/support/issues/2228.
Epic: None
Release note (bug fix): CockroachDB 23.1.0 alpha and beta versions previously would panic on
startcommand whenGOMEMLIMITenv var was set and--max-go-memoryflag wasn't specified, and this is now fixed.