Arrival-rate executors wrongly report the currently active vus

[na--]

If we run this k6 script:

import { sleep } from 'k6';

export let options = {
    scenarios: {
        constant_arrival: {
            executor: 'constant-arrival-rate',
            rate: 100,
            preAllocatedVUs: 2,
            maxVUs: 100,
            duration: '30s',
            gracefulStop: '0s',
        }, ramping_arrival: {
            executor: 'ramping-arrival-rate',
            startRate: 5,
            timeUnit: '1s',
            preAllocatedVUs: 2,
            startTime: '30s',
            maxVUs: 100,
            stages: [
                { target: 100, duration: '20s' },
                { target: 0, duration: '10s' },
            ],
        },
    }
}

export default function () {
    sleep(1.2);
}

we'll get something like this in the end-of-test summary (and any external outputs):

     vus..................: 198  min=8  max=198
     vus_max..............: 100  min=51 max=100

this is despite the fact that k6 would not have even allocated more than 100 VUs, much less used them, which is obvious both by the vus_max metric info (i.e. "initialized VUs", for which 100 is correct and expected) and by the warnings we see during the script execution itself:

WARN[0003] Insufficient VUs, reached 100 active VUs and cannot initialize more  executor=constant-arrival-rate scenario=constant_arrival
WARN[0048] Insufficient VUs, reached 100 active VUs and cannot initialize more  executor=ramping-arrival-rate scenario=ramping_arrival

This has occurred since k6 v0.27.0, where we introduced arrival-rate executors with #1007, and seems to be caused by allocating VUs mid-test? That is, the problem seems to disappear if we use preAllocatedVUs: 100.

I haven't tested any of the other executors, so it might be a good idea to double-check them with an external output or a k6 stats command mid-test, just in case.

[na--]

I tracked down the issue to ExecutionState.GetUnplannedVU() internally calling es.GetPlannedVU() in the second scenario, when all of the max unplanned VUs had already been initialized by the first scenario and are sitting in the buffer:
https://github.com/k6io/k6/blob/e5d4d82d16c0c86154f017971f4233b43702165d/lib/executor/ramping_arrival_rate.go#L356
https://github.com/k6io/k6/blob/e5d4d82d16c0c86154f017971f4233b43702165d/lib/execution.go#L568-L573

We don't initialize more than 100 VUs, since that is checked by this code: https://github.com/k6io/k6/blob/e5d4d82d16c0c86154f017971f4233b43702165d/lib/executor/ramping_arrival_rate.go#L457-L471

But we because we always call GetPlannedVU() with its second parameter modifyActiveVUCount equal to true, we modify the count of active VUs twice, first in https://github.com/k6io/k6/blob/e5d4d82d16c0c86154f017971f4233b43702165d/lib/execution.go#L536
and second when we actually activate it: https://github.com/k6io/k6/blob/e5d4d82d16c0c86154f017971f4233b43702165d/lib/executor/ramping_arrival_rate.go#L337

I can easily fix this if I just add the same modifyActiveVUCount parameter to GetUnplannedVU() and propagate it forwards instead of always using true, but I wonder if I shouldn't just abolish this second parameter altogether? It will make the executors a bit more verbose, but they would definitely be more understandable and explicit. @mstoykov, @imiric, WDYT?

[mstoykov]

It seems to me that GetUnplannedVU should always either increase or never increase the ActiveVUs.

Given the current logic and how it is only used in arraval-rate, I think it should just "never" increase it so the fix is to just change the second parameter to false in the GetUnplannedVU?

[na--]

Given the current logic and how it is only used in arraval-rate, I think it should just "never" increase it so the fix is to just change the second parameter to false in the GetUnplannedVU?

Yes, that would fix it as well. The externally-controlled executor directly uses InitializeNewVU().

I continue to dislike the optional boolean parameter, but you're right, that might be a better quick fix, for now and a more thorough refactoring can be done in the future, if necessary.