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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] New schedulers #1007

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@na--
Copy link
Member

commented Apr 23, 2019

This pull request totally rewrites the old local executor and a bunch of other things. It builds on #913 and introduces quite a lot of new functionality, but it also has some minor breaking changes.

New features and changes

New schedulers

The biggest change in this PR is the introduction of 7 distinct new VU schedulers:

  • shared-iterations, a.k.a. iterations + vus.
  • constant-looping-vus, a.k.a. duration + vus.
  • variable-looping-vus, a.k.a. stages + vus.
  • manual-execution is similar to variable-looping-vus, but the number of initialized and active VUs at any given time can be controlled by the k6 REST API - it's the only scheduler that could be paused or controlled while it's running (closes #867, closes #1028).
  • per-vu-iterations is 邪 new feature that allows each configured VU to execute an exact number of iterations; the shared iterations have caused confusion before and there are legitimate reasons people want an equal number of iterations for each VU (#663, #627, #378, #381 and probably others).
  • constant-arrival-rate executes a constant number of iterations per second (closes #550).
  • variable-arrival-rate executes a variable number of iterations per second, specified as ramp-ups or ramp-downs stages, similar to how the variable-looping-vus is configured.

These schedulers can be configured via the new execution JS/JSON option. For now, they aren't directly configurable via CLI flags or environment variables.

However, most of the old simple k6 uses where stages, or iterations or duration are specified in any way, including via CLI flags and env vars, are still supported. They are just transparently converted to their respective new execution scheduler configurations. There are some breaking changes here, described in detail in the Breaking Changes section below, but generally only when there is a conflict or ambiguity in the specified configuration.

Other new features

  • Multiple different schedulers can be configured in a single test run, both sequentially and in parallel (mostly closes #239).
  • Each scheduler has a startTime property, which defines at what time relative to the test run start it will start executing.
  • VUs are reused between the different configured schedulers when that's possible.
  • Schedulers have new gracefulStop property that allows for iterations to complete gracefully for some amount of time after the normal scheduler duration is over (closes #879)
  • Validation of the supplied execution configuration is very strict (closes #875 and slightly helps with #883)
  • There is a new feature called execution segments that allows tests to be easily partitioned between multiple k6 instances (implements the execution parts of #997 and helps towards #140)
  • There are separate descriptions and real-time thread-safe progress bars for each individual scheduler.
  • Different schedulers can execute different functions than the default exported one.
  • Different schedulers can have different environment variables.

Example of the new config

import { sleep } from "k6";

export let options = {
  execution: {
    stages_up_down: {
      type: "variable-looping-vus",
      startVUs: 0,
      stages: [
        { duration: "10s", target: 20 },
        { duration: "10s", target: 0 },
      ],
      gracefulRampDown: "0s",
      gracefulStop: "0s",
    },
    stages_down_up: {
      type: "variable-looping-vus",
      startVUs: 20,
      stages: [
        { duration: "10s", target: 0 },
        { duration: "10s", target: 20 },
      ],
      gracefulRampDown: "0s",
      gracefulStop: "0s",
    },
    const_loop_vus: {
      type: "constant-looping-vus",
      vus: 20,
      duration: "20s",
      gracefulStop: "0s",
    },
    constant_arr_rate: {
      type: "constant-arrival-rate",
      rate: 20,
      timeUnit: "1s",
      duration: "10s",
      preAllocatedVUs: 10,
      maxVUs: 20,
      startTime: "20s",
      gracefulStop: "0s",
    },
    variable_arr_rate: {
      type: "variable-arrival-rate",
      startRate: 20,
      timeUnit: "1s",
      preAllocatedVUs: 10,
      maxVUs: 20,
      startTime: "20s",
      stages: [
        { target: 0, duration: "5s" },
        { target: 20, duration: "5s" },
      ],
      gracefulStop: "0s",
    },
    per_vu_iters: {
      type: "per-vu-iterations",
      vus: 20,
      iterations: 10,
      startTime: "30s",
      maxDuration: "10s",
      gracefulStop: "10s",
    },
    shared_iters: {
      type: "shared-iterations",
      vus: 20,
      iterations: 200,
      startTime: "30s",
      maxDuration: "10s",
      gracefulStop: "10s",
    },
    adjustable_at_will: {
      type: "manual-execution",
      vus: 5,
      maxVUs: 10,
      duration: "40s",
    },
  },
};

export default function () {
  console.log(`Start VU ${__VU} ITER ${__ITER}`);
  sleep(0.5 + Math.random() * 0.5);
  console.log(`Done VU ${__VU} ITER ${__ITER}`);
}

Breaking changes

There are a few breaking changes caused by this PR:

  • Execution config options (execution, stages, iterations, duration) from "upper" config layers overwrite execution options from "lower" (i.e. CLI flags > environment variables > JS options > JSON options) config layers. For example, the --iterations CLI flag will overwrite any execution options set as environment variables (e.g. K6_DURATION, K6_STAGES, etc.) or script options (stages: [ /* ... */], execution: { /* ... */ }, etc. ).
  • Using different execution config options on the same level is now a configuration conflict error and will abort the script. For example, executing k6 run --duration 10s --stages 5s:20 script.js won't work (closes #812). The only exception is combining duration and iterations, which will result in a shared-iterations scheduler with the specified non-default maxDuration (#1058).
  • The k6 REST API for controlling script execution (i.e. the k6 pause, k6 scale commands) now only works when a manual-execution scheduler is configured in the execution config. The initial pausing of a test (i.e. k6 run --paused script.js) still works with all scheduler types, but once the test is started with k6 resume (or the corresponding REST API call), it can't be paused again unless only the manual-execution scheduler is used.
  • Previously, running a script with k6 run --paused would have still executed the script's setup() function, if it was present and wasn't disabled with --no-setup... k6 would have paused after executing setup(), but now it will pause before it executes it.
  • The vusMax / K6_VUS_MAX / -m / --max option is deprecated - it was previously used for the control of the initialized VUs by the REST API. Since that has now been restricted to the manual-execution scheduler, the equivalent option there is called maxVUs.
  • Tests with infinite duration are now only possible via the manual-execution scheduler.
  • Previously, all iterations were interruptible - as soon as the specified duration expired, or when VUs were ramped down in a stage, any running iterations were interrupted. Now all schedulers besides the manual-execution one have a gracefulStop period of 30s by default (closes #898). Additionally, variable-looping-vus scheduler has a gracefulRampDown parameter that configures the ramp-down grace period. For those periods, no new iterations will be started by the schedulers, but any currently running iterations will be allowed up to the specified periods to finish their execution.

Broken things and TODOs that remain be fixed in this PR:

  • updating the manual-execution scheduler's parameters via the REST API
  • update the description of this pull request 馃槃
  • forward-port the fixes in #1057 and #1076
  • rename things according to #1007 (comment)
  • support for cloud execution
  • the usage report
  • writing a lot more tests... this PR should hopefully significantly increase the code coverage of k6
  • improving the multi-progressbar alignments and sizing
  • update the release notes and the README
  • whatever @MStoykov says 馃槃

TODOs in the code that will be handled in future pull requests, i.e. for now only issues will be created:

  • handle Ctrl+C graceful shutdown + teardown execution
  • have a special error code for Ctrl+C interrupts
  • tag metrics from different schedulers differently (already described in #796)
  • JavaScript helper functions that could be used to build this new complicated execution configuration, but with a better user experience than manually writing it as a JSON
  • other minor issues and TODOs (have to go through the code diff again to fill in this list)

Things that need to be done shortly before or after this PR is merged in master:

  • a lot of manual testing, especially on different OSes
  • update the documentation (#551)

na-- added some commits Apr 23, 2019

Add the new progressbars and fixed-length formatters
This doesn't include all of the changes in run/cmd.go, since those are quite intertwined with a lot of others.

@na-- na-- requested a review from MStoykov Apr 23, 2019

Show resolved Hide resolved cmd/ui.go Outdated

na-- added some commits Apr 23, 2019

Make sure we wait for and return VUs in all schedulers
Additionally, now getting a VU from the common buffer won't return an error if the context was cancelled.
@codecov

This comment has been minimized.

Copy link

commented Apr 24, 2019

Codecov Report

Merging #1007 into master will decrease coverage by 1.59%.
The diff coverage is 62.2%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1007     +/-   ##
=========================================
- Coverage   72.31%   70.72%   -1.6%     
=========================================
  Files         132      141      +9     
  Lines        9703    10784   +1081     
=========================================
+ Hits         7017     7627    +610     
- Misses       2272     2726    +454     
- Partials      414      431     +17
Impacted Files Coverage 螖
cmd/root.go 34% <酶> (酶) 猬嗭笍
lib/util.go 100% <酶> (酶) 猬嗭笍
lib/testutils/logrus_hook.go 100% <酶> (酶) 猬嗭笍
js/modules/k6/http/limiter.go 100% <酶> (酶) 猬嗭笍
cmd/common.go 50% <酶> (+10%) 猬嗭笍
lib/runner.go 76.47% <酶> (+8.82%) 猬嗭笍
js/runner.go 85.54% <酶> (酶) 猬嗭笍
js/bundle.go 83.33% <酶> (酶) 猬嗭笍
cmd/run.go 10.09% <0%> (+0.87%) 猬嗭笍
lib/scheduler/manual.go 0% <0%> (酶)
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3fc300c...0a3c63f. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Apr 24, 2019

Codecov Report

Merging #1007 into master will decrease coverage by 3.14%.
The diff coverage is 55.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
- Coverage   72.31%   69.17%   -3.15%     
==========================================
  Files         132      141       +9     
  Lines        9703    11039    +1336     
==========================================
+ Hits         7017     7636     +619     
- Misses       2272     2971     +699     
- Partials      414      432      +18
Impacted Files Coverage 螖
cmd/root.go 34% <酶> (酶) 猬嗭笍
lib/util.go 100% <酶> (酶) 猬嗭笍
lib/testutils/logrus_hook.go 100% <酶> (酶) 猬嗭笍
js/modules/k6/http/limiter.go 100% <酶> (酶) 猬嗭笍
cmd/common.go 50% <酶> (+10%) 猬嗭笍
lib/runner.go 76.47% <酶> (+8.82%) 猬嗭笍
js/runner.go 85.54% <酶> (酶) 猬嗭笍
js/bundle.go 83.33% <酶> (酶) 猬嗭笍
cmd/run.go 9.95% <0%> (+0.73%) 猬嗭笍
cmd/collectors.go 0% <0%> (酶) 猬嗭笍
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3fc300c...05c9dcc. Read the comment docs.

Show resolved Hide resolved lib/helpers.go Outdated
Show resolved Hide resolved lib/helpers.go
Show resolved Hide resolved lib/helpers.go
default:
return false
}
})

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 14, 2019

Contributor

I would prefer if everything upto this point is a different method ... something like SortedSteps so we can have the whole function fit on a single page ... To this end I more and more dislike the super long sections of comments in the middle of the functions.
Some are okay(all of them are useful IMO) but at some point a 10 line function is 80 lines of comments in between all the actual code lines. I think that in most cases you can put the comment on top.

This comment has been minimized.

Copy link
@na--

na-- May 14, 2019

Author Member

I would prefer if everything upto this point is a different method ... something like SortedSteps so we can have the whole function fit on a single page

hmm I'd argue that this won't be better of in a separate function - splitting it up doesn't make sense for a few reasons:

  • the code up to here doesn't make logical sense on it's own - it won't be usable for anything else besides the rest of this method
  • the minor trackedStep type is used in both parts of the method, and nowhere else
  • the whole method is only 60 lines of code (without the comments), hardly a monster... and a lot of those lines is actually due to the verbosity of Go - I to implement a sum() function ffs...

To this end I more and more dislike the super long sections of comments in the middle of the functions.

Sometimes I do as well, but in this specific function, as you've seen, the comment below serves the role of visually splitting up the 2 main sections of the function body, besides explaining what the second section actually does.

I think that in most cases you can put the comment on top.

My rule of thumb is for the function/method comments to describe what the function/method does, while the inline comments in its body to describe how it does it. In this case, if I push the inline comments to the top, they won't be useful for users of the function and the function code will be more difficult to understand for future readers.

currentMaxUnplannedVUs[step.configID] = step.MaxUnplannedVUs
}
addCurrentStepIfDifferent() // Add the last step
return consolidatedSteps

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 14, 2019

Contributor

Can we copy the slice at this point so we don't waste space .. although a small amount.
Also maybe preallocating the slice ?

This comment has been minimized.

Copy link
@na--

na-- May 14, 2019

Author Member

I'm confused - how can we pre-allocate the slice when we don't know its eventual size? And what's the wasted space? Are you concerned that the doubling of the slice cap by append()? If that's the case, then I'd say it's not worth it, after all, GetFullExecutionRequirements() is basically used only in once place in the code...


package lib

//TODO

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 14, 2019

Contributor

I would say this is a requirement for this PR

This comment has been minimized.

Copy link
@na--

na-- May 14, 2019

Author Member

Definitely! And I'll add them, along with all of the other missing unit tests, but for the moment I'm prioritizing different things.

Show resolved Hide resolved lib/scheduler/base_config.go Outdated
Show resolved Hide resolved cmd/root.go Outdated
case <-wait:
close(vuOut)
case limiter <- struct{}{}:
go initPlannedVU()

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 15, 2019

Contributor

It would be better (probably not by a lot) to run initConcurrency goroutines which read from range over limiter and just execute the initPlannedVu. you will also need to close limiter.
This is better primarily because it reuses goroutines. In the particular case of initializing VUs ... on my machine 1000 VUs with practically no code take something like 6G to initialize so I doubt the improvement will be significant ... actually I measured it and I couldn't ... reliably detect improvement.

Below is my diff

--- a/core/local/local.go
+++ b/core/local/local.go
@@ -186,24 +186,26 @@ func (e *Executor) Init(ctx context.Context, engineOut chan<- stats.SampleContai
        doneInits := make(chan error, vusToInitialize) // poor man's early-return waitgroup
        //TODO: make this an option?
        initConcurrency := runtime.NumCPU()
-       limiter := make(chan struct{}, initConcurrency)
+       limiter := make(chan struct{})
        subctx, cancel := context.WithCancel(ctx)
        defer cancel()
-
-       initPlannedVU := func() {
-               newVU, err := e.initVU(ctx, logger, engineOut)
-               if err == nil {
-                       e.state.AddInitializedVU(newVU)
-                       <-limiter
-               }
-               doneInits <- err
+       for i := 0; i < initConcurrency; i++ {
+               go func() {
+                       for range limiter {
+                               newVU, err := e.initVU(ctx, logger, engineOut)
+                               if err == nil {
+                                       e.state.AddInitializedVU(newVU)
+                               }
+                               doneInits <- err
+                       }
+               }()
        }

        go func() {
+               defer close(limiter)
                for vuNum := uint64(0); vuNum < vusToInitialize; vuNum++ {
                        select {
                        case limiter <- struct{}{}:
-                               go initPlannedVU()
                        case <-subctx.Done():
                                return
                        }

This comment has been minimized.

Copy link
@na--

na-- May 15, 2019

Author Member

So... should I change it or not? 馃榾

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

yes, I think it is worth the change ... in cases where we have high CPU count with high VUs this would be worth it

return nil
case err := <-doneInits:
if err != nil {
return err

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 15, 2019

Contributor

I am not sure it is worth it to actually wait for all errors and combine them and then return them.
Maybe have a comment if we won't do it

This comment has been minimized.

Copy link
@na--

na-- May 15, 2019

Author Member

I don't think combining them is practical, because after we encounter this first error, we abort the initialization of the remaining VUs by cancelling the context. So any errors they return are likely to be from that, which won't be very useful to users, it will just be noise that obscures the actual error cause.

This comment has been minimized.

Copy link
@na--

na-- May 15, 2019

Author Member

but yeah, I should add a comment that explains this

e.initProgress.Modify(pb.WithHijack(e.getRunStats))

runCtxDone := runCtx.Done()
runScheduler := func(sched lib.Scheduler) {

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 15, 2019

Contributor

Maybe this can be moved in a method?
The freevars reported are :

  1 core/local/local.go|295 col 1| Free identifiers:
  2 core/local/local.go|255 col 45| var engineOut chan<- github.com/loadimpact/k6/stats.SampleContainer
  3 core/local/local.go|257 col 2| var logger.WithFields func(fields github.com/loadimpact/k6/vendor/github.com/sirupsen/logrus.Fields) *github.com/loadimpact/k6/vendor/github.com/sirupsen/logrus.Entry
  4 core/local/local.go|279 col 2| var runCtx context.Context
  5 core/local/local.go|293 col 2| var runCtxDone <-chan struct{}
  6 core/local/local.go|277 col 2| var runResults chan error
  7 core/local/local.go|294 col 23| var sched.GetConfig func() github.com/loadimpact/k6/lib.SchedulerConfig
  8 core/local/local.go|294 col 23| var sched.GetProgress func() *github.com/loadimpact/k6/ui/pb.ProgressBar
  9 core/local/local.go|294 col 23| var sched.Run func(ctx context.Context, engineOut chan<- github.com/loadimpact/k6/stats.SampleContainer) error

So engineOut, runCtx(Done can be taken from it), runResults and sched.

This comment has been minimized.

Copy link
@na--

na-- May 15, 2019

Author Member

You missed logger, so 5 arguments in total... would taking this out as a method really make the code more readable? I don't mind doing it, I'm just unsure if having to jump between the two would be easier when reading the code.

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

given that you move 30+ lines of code out of a 100 line function ... yes :)

for range e.schedulers {
err := <-runResults
if err != nil && firstErr == nil {
firstErr = err

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 15, 2019

Contributor

Again .. maybe combine all the errors from the different schedulers ?

This comment has been minimized.

Copy link
@na--

na-- May 15, 2019

Author Member

Same reason here as with the initialization - the first error aborts the script, all of the rest of the errors would likely be caused by that and obscure the actually important error. And to be honest, because of the better execution config validation, this error shouldn't ever happen. If it happens, something has very seriously gone wrong 馃槃 It's not a panic() mostly because of the terrible UX of panics...

executor, err := local.New(&lib.MiniRunner{}, logrus.StandardLogger())
require.NoError(t, err)
engine, err := core.NewEngine(executor, lib.Options{}, logrus.StandardLogger())
require.NoError(t, err)

This comment has been minimized.

Copy link
@MStoykov

MStoykov May 16, 2019

Contributor

Maybe have this 4 lines of code as a separate function as this is the nth (might be kth as n is quite big but you get the point) time I see it

@na-- na-- referenced this pull request May 16, 2019

Open

CSV API #1021

Show resolved Hide resolved lib/execution_segment.go Outdated
@na--

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

During a few discussions, we decided that some of the things in this PR should be renamed. Since the changes are quite extensive, I'm posting a partial list here, before I change the PR description and code:

  1. The new schedulers should be called executors, both in the code and in the documentation, because:
  • it's a better name - while there's a scheduling element in them (although even that is execeution-related, i.e. GetExecutionRequirements(*ExecutionSegment) []ExecutionStep), their main job is actually executing iterations according to rules defined by their type and options
  • schedulers would somewhat conflict with the scheduling of tests in the cloud service
  • the JS/JSON option is called execution
  1. Consequently, even though that matters only to k6 developers, to avoid confusion, the current local executor should be renamed to something different:
  • still not clear what the name should be, but it actually has more to do with scheduling things than the current "schedulers", so maybe ExecutorScheduler or something like that...
  • the current ExecutorState would probably be better off as ExecutionState, since that more accurately describes it
  • the lib.Executor interface probably should be scrapped altogether, since after the execution segments, it's very unlikely that we will reuse it to implement the native k6 distributed execution
  1. It would be better to rename "incomplete iterations" to "interrupted iterations", much clear what happened. Thinking about it, it may also be worth it to have a separate counter for iterations that had an error. It won't be in this PR, but it may make sense to also show that when we get to implementing #877?
  2. Rename the manual-execution executor to something better - external-control, externally-controlled, manually-controlled, something else?

Other details that need to be updated:

  • update the architecture wiki page to the list of tasks in the issue
  • add a new issue about changes and additions to the metrics and tags k6 emits, given the new executors - probably not in this PR, but likely immediately after; this includes tagging metrics with the executor ID by default, maybe split the vus metric by executor, a new metric for insufficient VUs by the arrival-rate executors
  • fix a bug where ctrl+c doesn't work when using k6 scale in the
  • remove the ERRO interrupted context message error message that's sometimes shown when an iteration is interrupted
  • k6 inspect doesn't fail when there are validation errors in options

@legander legander referenced this pull request May 31, 2019

Merged

Feature/options #9

5 of 5 tasks complete
@MStoykov
Copy link
Contributor

left a comment

As a whole ... way too many changes in the same PR. I am still under the believe that we could've done half the things in different PR with the caveat that we would change things many times but ... I digress.

No big architectural problems, but I have some places where I would like to try to change things which I will do while writing some tests.
I still think that you are using way too many lambdas everywhere and specifically in the manual executor which is by far the worst of code I have seen you write ;) . I strictly prefer if that code gets cut down to smaller functions and methods which is the case always .. I do truly believe that methods that read as story are much better even if they mean there will be a couple of functions/method with a single call to them .

Also ... I will definitely need a second review after I write some tests and actually see if I can not do some things differently

Show resolved Hide resolved lib/executor.go
}

increment := new(big.Rat).Sub(to, from)
increment.Denom().Mul(increment.Denom(), big.NewInt(numParts))

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

Can you change this to

// increment = (to -from)/ numParts
increment = new(big.Rat).Mul(new(big.Rat).Sub(to, from), big.NewRat(1, numParts))

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

I'll add the comment, but I don't see why we should basically double the number of bigint allocations and operations. Due to the cumbersome big APIs, I honestly don't find the one-liner you propose much more readable than the denominator manipulation I'm currently doing.

}

if from.Cmp(to) != 0 {
return nil, fmt.Errorf("Expected %s and %s to be equal", from, to)

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

Is this basically for asserting that the above algorithm has done it's job ?

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

I'll remove when I push the comprehensive unit tests for the execution segments.

if err != nil {
return nil, err
}
results[i] = segment

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

You can directly set it above without adding another variable

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

I could, but then I'd have to also add var err error above, which makes the code slightly harder to read. Whereas having a segment variable makes the code slightly easier to read.

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

I would argue that I(as a golang developer) am perfectly aware why you would add var err error but adding another variable not named err to me sounds like a thing that you intend on using/modifying so for me this is more confusing.

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

Oh come on, it's more than a bit of a stretch that having an extra properly-named variable in this "huge" 7-line loop is somehow confusing... I mean, if I didn't decide to be clever and pre-allocate the slice and just used append(), we wouldn't even be having this bikeshedding session...

Show resolved Hide resolved lib/scheduler/manual.go
// buffer that the user originally specified in the JS config.
startMaxVUs := segment.Scale(mex.startConfig.MaxVUs.Int64)
vuHandles := make([]*manualVUHandle, startMaxVUs)
activeVUsCount := new(int64)

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

I think this should be a field of ManualExecution as well as maxVUs and startMaxVus ... maybe just segment as well.

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

maybe, though I don't think that will substantially simplify this whole mess...

// Keep track of the progress
maxVUs := new(int64)
*maxVUs = startMaxVUs
progresFn := func() (float64, string) {

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

Again ... I think this should be a method but it will be easier of some of the things it reference are fields like maxVUs, activeVUsCount and so on ..

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

hmm I see, that could work and is a good enough reason for moving some of the variables to the struct 馃憤

return err
}
defer func() {
err = handleConfigChange(currentControlConfig, ManualExecutionControlConfig{})

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

Because of this line I now even more want this method to be A lot smaller than it currently is ... like maybe just get this defer and the loop below and put it in another function where if someone sees this at the top they know that the err here is named return.
Although I am pretty sure that the majority of the lambda definitions above can be moved to methods/function and as such this will be less required :)

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

I think you can just have the below loop in a separate function and have this not be in defer but just after the call to the loop function

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

I agree that moving a lot of the lambdas and variables to methods and properties of the struct is probably worth it in this case. I'll try to refactor it so after the renames...

updateConfigEvent.err <- err
return err
}
currentControlConfig = updateConfigEvent.newConfig

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

I don't like this ... are you certain we can't just configLock/Unlock around every time we get mex.currentControlConfig and use it instead ? As far as I can see there are not a lot of cases where we touch it

This comment has been minimized.

Copy link
@na--

na-- Jun 3, 2019

Author Member

At this point, mex.currentControlConfig is just saved so that the GetCurrentConfig() method can function correctly, so

just configLock/Unlock around every time we get mex.currentControlConfig and use it instead

this would just add unnecessary synchronization in the code

)
}

mex.configLock.Lock()

This comment has been minimized.

Copy link
@MStoykov

MStoykov Jun 3, 2019

Contributor

Can you add a comment explaining that this is to safeguard between a race of this and hasStarted being closed and us getting into the default case with the idea that it isn't

@na--

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

As a whole ... way too many changes in the same PR. I am still under the believe that we could've done half the things in different PR with the caveat that we would change things many times but ... I digress.

Even with the benefit of hindsight, I still don't see how I could have split this up further into separate PRs. Though I'll probably leave implementing the exec and env executor options in a separate PR, since they are somewhat unrelated to the rest.

No big architectural problems, but I have some places where I would like to try to change things which I will do while writing some tests.

馃憤

I still think that you are using way too many lambdas everywhere and specifically in the manual executor which is by far the worst of code I have seen you write ;) . I strictly prefer if that code gets cut down to smaller functions and methods which is the case always .. I do truly believe that methods that read as story are much better even if they mean there will be a couple of functions/method with a single call to them .

In the smaller executors I could argue that splitting the Run() methods won't make them more readable, but I agree about the the manual executor - it ended up being an unreadable monstrosity 馃槥 I'll try to refactor it into something more readable when I'm fixing the bug where it doesn't abort at Ctrl+C while scaling VUs.

Also ... I will definitely need a second review after I write some tests and actually see if I can not do some things differently

馃憤

@na--

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

After #1057 is merged into master, the changes from it need to be ported here as well, probably before any renaming...

@na--

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

The inclusion of the derived execution config in the archives' consolidated metadata.json options by k6 v0.24.0 was an even bigger problem than I originally realized... 馃う鈥嶁檪 馃槶 #1057 was not enough to fix the issue, because:

  • the default option values for the startVUs in the variable-looping-vus executor are different, causing the reflect.DeepEqual check employed there to falsely trigger
  • between k6 v0.24.0 and this PR, I've renamed and consolidated the two interruptible and iterationTimeout options to the single new gracefulStop option; even though those option values would be null in the metadata.json options, the validation of the execution JSON is very strict and doesn't allow any unknown fields, which interruptible and iterationTimeout will be.

So, a more radical solution than #1057 is needed. Here's my proposal:

  1. Make a new pull request that will be merged in master before v0.25.0 is released that will make k6 disregard any execution options it sees. We will always derive them anew from the shortcut options (iterations, duration, stages). This way we'll still have the nice deprecation and validation warnings we currently show, but none of the issues. We don't actually use any of the execution config itself in the current master anyway, it's only used for validation and all of the actual new execution logic is in this PR...
  2. Make a separate PR (or add to this one), probably after v0.25.0 is released (since we should probably do it after #1059 is merged), that would modify the archive loading logic. We should delete the execution options saved in metadata.json before they are parsed for any archives that don't have a k6version field. That field was added in #1057 (so ~ k6 < v0.25.0), which also fortuitously fixed the bug of saving the derived execution in archives...

I think this should be enough to fully fix the issue without any more unexpected negatives. And even though 2) would be a huge hack, it will be relatively self-contained and understandable, or at least more so than the previous "fix"...

na-- added a commit that referenced this pull request Jul 9, 2019

Disable the execution mismatch check, but keep the warnings
Full description of the issue and rationale for the change can be found here: #1007 (comment)

na-- added a commit that referenced this pull request Jul 9, 2019

Disable the execution mismatch check, but keep the warnings
Full description of the issue and rationale for the change can be found here: #1007 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.