enhance(starlark): make execution step limit configurable #947
Conversation
5,000 was too few to enable the example added to the testdata to work, so was 6,000. I chose 7,500 arbitrarily after a test at 10,000 and both worked. In the long term, this should probably be configurable so as not to require recompilation. For now, this kicks the can down the road while allowing this build matrix use case to exist.
Codecov Report
@@ Coverage Diff @@
## main #947 +/- ##
==========================================
+ Coverage 70.79% 70.85% +0.05%
==========================================
Files 311 311
Lines 12852 12858 +6
==========================================
+ Hits 9099 9110 +11
+ Misses 3288 3283 -5
Partials 465 465
|
There was a problem hiding this comment.
You might want to have a "Starlark execution environment" config struct where this limit and inevitably other stuff gets shoved. It would perhaps include the environment name, e.g.
thread := &starlark.Thread{Name: "templated-base"}Maybe something like
type StarklarkEnv struct {
executionStepLimit uint64
name string
}Also, there's still a hardcoded step limit in the Render method on around line 41-42.
wass3r
dismissed
their stale review
we should honor this request #943 (comment) for attribution
ecrupper
force-pushed
the
enhance/configurable-starlark-exec-limit
branch
from
82e5593 to
294a893
Compare
…:go-vela/server into enhance/configurable-starlark-exec-limit
colindean
left a comment
colindean
left a comment
There was a problem hiding this comment.
Lookin' good.
I would like to see that StarlarkExecutionConfig struct idea, but maybe I'll do that in a follow up PR.
|
@colindean what is the use case for controlling the thread name? or was that just arbitrarily chosen as an example for making it more extensible? |
|
Arbitrarily chosen. Wouldn't be surprised if there's other things in there that end up needing to be tuned |
|
For that config could also include things for controlling the inevitable subprocess, like a timeout, memory limits, etc |
Ref: #943
Since we don't have a good idea on how to potentially calculate a limit, it's a good idea to make this value configurable on start up, rather than incrementing it whenever we run into problems.
Default is set to 7500, though I'm open to other ideas on that 👍 .