Aborting test during VU initializaiton doesn't get propagated to outputs

[mstoykov]

In the new-schedulers(new-executors) there are a lot changes around this and we either broke it or made it more likely, but if during the initialization of the VUs you figure something is wrong and ^C to exit, the metric outputs won't realize that as they won't be running yet.

This is specifically bad with the cloud output as there a test will be created and then no aborted, but it is actually a general design problem.

• Code where collectors are Inited
• Code where engine.Init is called after that
• Code inside engine.Init that initializes VUs
• Code that call Collector.Run after which stoping k6 will propaged it was stopped to the output/collectors

Possible fixes are:

1. Move the initialization of VUs after the call to Collector.Run - somewhat fast, not a big change, but
   1.1. There is still time after the Init, but before it was run
   1.2. runMetricsEmission shouldn't run before the test actually start as it emits VUs count ... so it will probably be bigger change :(
2. Make Collector.Init takes a context as well and uses that to figure out when the test has ended. Probably also remove the context from Run? This will be a bigger code change but will be a better fix IMO

[imiric]

I took a look at this, and forgive my ignorance, but is there a reason collectors couldn't be initialized after VUs? Something like c096ec0 .

This seems like the easiest fix to ^C during VU initialization, the outputs wouldn't be initialized unless that succeeds.

In the case of the cloud collector it also means that it wouldn't know about the test until it's about to run, which might be a reason to not do this, but just wanted to confirm with you.

Otherwise, I'll likely go with option 2. from the proposed fixes.

[imiric]

I experimented with the second suggestion of passing a context to Init() and removing it from Run(), but it gets messy as it would need to be stored on the Collector, a separate goroutine would be needed to react on the context cancel, and it would require additional synchronization with the main goroutine to wait for the "cleanup" to complete.

So I abandoned that in favor of a simple Cleanup() hook that gets run on the first ^C. See 4619f64. This avoids any context or sync changes, and works nicely to propagate the status for the cloud collector (tested on staging). I prefer this small addition to the lib.Collector interface (even though it's worthless to most collectors, but then again so is Link() ;)), instead of changing how the context is used. I still think doing the collector initialization in Engine.startBackgroundProcesses() would avoid this altogether, though.

Let me know if either approach would address this issue and I'll create a PR.

[na--]

I took a look at this, and forgive my ignorance, but is there a reason collectors couldn't be initialized after VUs? Something like c096ec0 .

This seems like the easiest fix to ^C during VU initialization, the outputs wouldn't be initialized unless that succeeds.

In the case of the cloud collector it also means that it wouldn't know about the test until it's about to run, which might be a reason to not do this, but just wanted to confirm with you.

Hmm I'm not sure this is viable, even as a short term fix? We show the output, including the cloud test run URL in the UI output: field, which happens before the VUs are initialized (and before their new progressbar)...

Regarding the other proposed solutions, I think that I prefer to leave changing the Collector interface to v0.28.0... And, if possible, we should strive to simplify the interface, not add methods to it (#1075).

[imiric]

we should strive to simplify the interface, not add methods to it

Agreed, though 4619f64 seemed like the simplest fix for this, at the expense of some additional (but trivial) cleanup we'll have to do anyway. @mstoykov's second proposal would also require a slight change to the Init() signature, but it turned out to be more complicated when I experimented with it because of the additional required synchronization, so I abandoned it.

But OK, let's revisit this for v0.28.0 and hopefully it will be more straightforward then.

[na--]

Closed by #1869