github workflow: use GOMEMLIMIT to limit mem usage
#610
Conversation
Remove the comment and switch from `GOGC` to `GOMEMLIMIT` as the project uses Go 1.21. Signed-off-by: Ivan Valdes <[email protected]>
@fuweid, I can work on it in another PR, do you have a reference of how to define a large runner? |
|
@ivanvc you can try to use REF: etcd-io/etcd@3f6a5c0 |
Thanks, I'll try it out. |
We should consider to use larger runner for the following tests which used to run longer than other workflows,
|
| # | ||
| # REF: https://github.com/actions/runner-images/issues/6680#issuecomment-1335778010 | ||
| GOGC=30 CPU=4 ENABLE_RACE=true make test | ||
| GOMEMLIMIT=2048MiB CPU=4 ENABLE_RACE=true make test |
There was a problem hiding this comment.
Based on the Suggested uses in https://tip.golang.org/doc/gc-guide,
Don't use the memory limit when deploying to an execution environment you don't control.Don't set a memory limit to avoid out-of-memory conditions when a program is already close to its environment's memory limits.
Also the default value for GOGC is 100, which should be good enough.
What we can do is to try to use larger runner.
So can we just remove the configuration? I mean we don't use GOGC, nor GOMEMLIMIT?
There was a problem hiding this comment.
The original issue seems to imply jobs are being killed due to excessive resource usage: actions/runner-images#6680 and based on discussion / linked issues it appears to be ongoing.
However I don't actually know if we are still impacted by this, based on the comments we were historically.
Agree let's try large runner for longer jobs and see if we can just remove it altogether. We can always reinstate the limit if necessary.
There was a problem hiding this comment.
These changes would change the approach for this PR. I pushed them to #612.
There was a problem hiding this comment.
The GOGC was used to prevent from unexpected kill. I think the issue is still here if we don't use larger size runner.
Yeah, I prefer to abort this one and use #612.
There was a problem hiding this comment.
Would using the large runner just for the race test case make sense? And leave the others with the regular runner? I can change the implementation if that sounds good.
There was a problem hiding this comment.
SGTM - 4cpu race (amd64 +arm64) and windows tests are the ones that need extra resources.
|
Closing in favor of #612. |
Remove the comment and switch from
GOGCtoGOMEMLIMITas the project uses Go 1.21.Fixes #609.