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

Simplify the vuHandle implementation #1492

Closed
na-- opened this issue Jun 9, 2020 · 1 comment
Closed

Simplify the vuHandle implementation #1492

na-- opened this issue Jun 9, 2020 · 1 comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 executors lower prio refactor

Comments

@na--
Copy link
Member

na-- commented Jun 9, 2020

As I mentioned in #1479 (review), I don't like how complex the vuHandle implementation has gotten. True, its job is very tricky, so I am a bit skeptical about my gut feeling we can simplify it... It has to deal with a few gnarly synchronization issues:

  1. The gracefulStop / gracefulRampDown handling, which can require a VU that is in the process of gracefully shutting down to resume again, without ever returning the VU to the pool, if we suddenly ramp up. And we don't know how long the last iteration it's executing is going to be, so it can end at basically any time in that process, or not at all... 😭
  2. The VU deactivation, which, because of the way we stop their goja JS runtime (by necessity), happens a bit after the VU context is cancelled...
  3. The unpredictable nature of the externally-controlled executor...

I initially thought that this will finally be a job for sync.Cond, but because of the context we require for JS execution and deactivation, it really doesn't seem to be of much use, though I might be missing something 😞 That said, it might not be totally out of place in some bigger refactoring, for example if we use the changed atomic more broadly. That is, if instead of just signalling that something has changed, we use a few different values to signal the current state Stopped / Running / GracefullyStopping that the executor is supposed to be in... But that has issues of its own, namely something has to cancel the VU context...

Something else to consider... For variable-looping-vus executor, we can know the precise time each VU is supposed to run, including the gracefulStop value. It will become easier to calculate once we zip rawStepEvents and gracefulLimitEvents in a single plan, to address #1473. So, instead of having a resident goroutine for each VU we want to run (i.e. a stopped vuHandle), we can launch new goroutines every time we ramp up, and give 2 unique contexts to each of them, somewhat similar to what we do with the executor contexts, maxDurationCtx, regDurationCtx, cancel := getDurationContexts()... That would still leave the problem of what to do with the externally-controlled executor though...

And it also won't solve the issues with rapid ramp-down, ramp-up events. In these, we may have a situation where a VU is in the process of gracefully stopping, but it's still executing its last iteration. And if, before the gracefulRampDown time expires, we ramp up to that VU again, the VU never has to stop. So, in this case, what we do on the iteration length - for short iteration, we would have returned the VU to the buffer, but for iterations longer than the gap, it should be as if there was never any interruption in the VU running at all. This is the trickiest issue that I can't think of any other way besides a "vuHandle" to solve... 😞

So yeah, leaving this here for some future time, if we have any bright ideas... Or, we can close this a few months from now and just use my ramblings above as a documentation/justification of the current approach 😅

@na-- na-- added refactor lower prio executors evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels Jun 9, 2020
mstoykov added a commit that referenced this issue Jun 17, 2020
this is ... mostly a complete rewrite of vuHandle now using an atomically updated state to work as a state machine. 

There is still a fast path which provides all the speed, but now it also always returns any VU it has got and doesn't return it in cases where this isn't required - there was a start after a gracefulStop, but before it actually stopped. 

closes #1492
@na--
Copy link
Member Author

na-- commented Jun 18, 2020

Closed by #1506 🎉

@na-- na-- closed this as completed Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 executors lower prio refactor
Projects
None yet
Development

No branches or pull requests

1 participant