merge steps in VLV executors for more stability #1496
Conversation
Codecov Report
@@ Coverage Diff @@
## new-schedulers #1496 +/- ##
==================================================
+ Coverage 76.79% 76.80% +0.01%
==================================================
Files 163 163
Lines 13039 13028 -11
==================================================
- Hits 10013 10006 -7
+ Misses 2515 2509 -6
- Partials 511 513 +2
Continue to review full report at Codecov.
|
lib/executor/variable_looping_vus.go
Outdated
| // TODO ... thinking about it gracefulExecutionSteps in a lot of cases will lead to nothing so maybe | ||
| // we should skip them here? | ||
| // TODO better name ... zip is not correct, and combine seems even worse ... merge is not great as | ||
| // well though |
There was a problem hiding this comment.
merge sounds clear enough to me :)
Though I wonder if we can avoid this process, and build the merged execution plan from the start.
Right now getRawExecutionSteps() is called twice: once with zeroEnd==true to build the initial rawExecutionSteps, and a second time in GetExecutionRequirements() with zeroEnd==false to build the gracefulExecutionSteps.
There's likely a way we can avoid this merge step and do a single pass over the stages that produces the same merged plan as this function. What do you think?
There was a problem hiding this comment.
Yeah, I just noticed this and another potential optimization we can do with HasWork(). Not sure if we should tackle this now, or if I shouldn't just make a new performance issue about them...
There was a problem hiding this comment.
I think that is possible future refactor ;) I really want to get this done with so we can know it's not flaky or buggy, while - yes I would argue that this can be done much earlier, possibly with even better names, and even less code :D
Also fix VLV not respecting gracefulStop since 270fd91, add test for it and try to make some other VLV tests more stable. The original code (pre 270fd91) did go through all the raw steps (ramp up/down) and the graceful stop ones concurrently and when the raw steps ended it will return from Run, but will start a goroutine to go through the remainign graceful stop ones. The most important of which ( at least in my current understanding) being the last one which is the actuall gracefulStop end which will make all VUs stop. The reason why this actually worked is that it also waiting on all activeVUs to have ended being active, being returned in the new terminology, before it actually cancels the context for all of them. So if the VUs manage to end iterations in this time the executor will end earlier if not the gracefuStop will make them. 270fd91 broke the returning of VUs which lead to the executor never returning and moving the cancel "before" the waiting for activeVUs fixed that at the cost of gracefulStop not being taken into account, but with no tests, nobody noticed. Here I basically revert that especially because vuHandle now returns VUs when stopped. fixes #1473
Co-authored-by: na-- <n@andreev.sh>
Also fix VLV not respecting gracefulStop since 270fd91, add test for it
and try to make some other VLV tests more stable.
The original code (pre 270fd91) did go through all the raw steps
(ramp up/down) and the graceful stop ones concurrently and when the raw
steps ended it will return from Run, but will start a goroutine to go
through the remainign graceful stop ones. The most important of which (
at least in my current understanding) being the last one which is the
actuall gracefulStop end which will make all VUs stop. The reason why
this actually worked is that it also waiting on all activeVUs to have
ended being active, being returned in the new terminology, before it
actually cancels the context for all of them. So if the VUs manage to
end iterations in this time the executor will end earlier if not the
gracefuStop will make them.
270fd91 broke the returning of VUs which lead to the executor never
returning and moving the cancel "before" the waiting for activeVUs fixed
that at the cost of gracefulStop not being taken into account, but with
no tests, nobody noticed.
Here I basically revert that especially because vuHandle now returns VUs
when stopped.
fixes #1473