Add support for exec/env/tags executor options #1428

imiric · 2020-04-30T14:00:30Z

This enables configuration of environment variables, metric tags and non-default function execution for each executor. See the example in #1300.

Closes #1300

See #1300

core/local/local.go

imiric · 2020-04-30T14:07:56Z

core/local/local_test.go

+		}
+	}
+	export function nonDefault() {}`,
+			"error while initializing executor per_vu_iters: function 'nonDefaultErr' not found in exports"},


Should we add more test cases here? Test other executors as well?

lib/executor/constant_arrival_rate.go

lib/runner.go

core/local/local.go

lib/executor/constant_arrival_rate.go

lib/executor/vu_handle.go

core/local/local.go

This leaves executor init before VU init, but moves the config validation to a more suitable place before the engine/ExecutionScheduler is initialized. It now also properly reports errors from more than one executor. Resolves #1428 (comment)

…onfig Resolves: - #1428 (comment) - #1428 (comment)

Not sure if this is an improvement honestly, but I didn't find a way to set a default value for null.String.

This now also grabs all exported functions, including setup/teardown. Partially resolves: - #1428 (comment) - #1428 (comment)

codecov-io · 2020-05-07T10:54:13Z

Codecov Report

Merging #1428 into new-schedulers will increase coverage by 0.36%.
The diff coverage is 87.95%.

@@                Coverage Diff                 @@
##           new-schedulers    #1428      +/-   ##
==================================================
+ Coverage           76.34%   76.71%   +0.36%     
==================================================
  Files                 162      163       +1     
  Lines               12936    12977      +41     
==================================================
+ Hits                 9876     9955      +79     
+ Misses               2545     2511      -34     
+ Partials              515      511       -4

Impacted Files	Coverage Δ
cmd/archive.go	`26.19% <0.00%> (ø)`
cmd/cloud.go	`9.02% <0.00%> (ø)`
cmd/run.go	`9.28% <0.00%> (ø)`
lib/executors.go	`94.11% <ø> (+3.92%)`	⬆️
lib/testutils/minirunner/minirunner.go	`77.77% <33.33%> (-3.00%)`	⬇️
js/runner.go	`82.68% <75.00%> (-0.65%)`	⬇️
lib/executor/externally_controlled.go	`62.32% <85.71%> (+3.09%)`	⬆️
js/bundle.go	`89.33% <86.36%> (+0.68%)`	⬆️
cmd/config.go	`83.58% <100.00%> (+10.56%)`	⬆️
js/modules/k6/k6.go	`100.00% <100.00%> (+8.51%)`	⬆️
... and 23 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 7dcbfe1...b2b8648. Read the comment docs.

This now also grabs all exported functions, including setup/teardown. Partially resolves: - #1428 (comment) - #1428 (comment)

cmd/config.go

core/local/local.go

lib/executor/shared_iterations.go

js/runner.go

na--

see #1428 (comment) - the rest are minor nitpicks, but tags and environment variables should not cross over between executors.

Basically, VUs in each executor have to get {the global --tag/--env values + whatever was configured for that specific executor}, no crossover.

Resolves #1428 (comment)

This was done in e693715 to ensure exec validation would fail before VUs are initialized, but now that this validation is done much earlier, this is no longer needed. Resolves #1428 (comment)

The minor optimization wasn't worth the messiness. Resolves #1428 (comment)

Resolves #1428 (comment)

mstoykov · 2020-05-11T13:11:51Z

js/modules/k6/k6.go

 	tags := state.Options.RunTags.CloneTags()
+	for k, v := range state.Tags {
+		tags[k] = v
+	}


Hm ... shouldn't it just be state.Tags.CloneTags() ? And have the state.Options.RunTags be cloned to state.Tags along all the others ?

Not sure I'm following, sorry. Can you elaborate on that?

Options.RunTags are the globally defined tags, whereas state.Tags was added here to pass on the custom executor tags. There's an overriding hierarchy here, which we might want to document.

I am just of the opinion you can put the overridden final tags inside state.Tags and use them everywhere where state.Options.RunTags are being used currently instead of merging them everywhere

Aha, I see. I'll look into that, thanks.

@mstoykov See d15bfe9 for this change.

I'm a bit apprehensive about it, since it could break some previous overriding, and it couldn't be done in httpext.MakeRequest, otherwise TestRequestAndBatch/Params/tags/tags-precedence would fail. Also now WS metrics will be tagged with optional system tags (iter, vu, etc.), whereas previously they weren't. Arguably an improvement, but a bit worrying that something else didn't inadvertently change and we're not testing for it.

I'd leave this in anyway, but there's now a race condition in CI I thought was fixed by e33400e, which passed locally, but apparently not, or I broke it again. :-/

I don't know ... I think this is once again too big of change ... and at this point, I think it was a mistake to try and implement both exec, tags, and env at the same time. Especially tags seem to be problematic, mostly because they were problematic even before that. I don't know what is the correct change, but am fine with you reverting the changes ... or not - I am of the opinion that this probably fixes some tiems when vu/iter weren't present in metrics they should've been in.

The race conditions on the other hand are problematic and will need to be fixed :(

The race condition seems to be between https://github.com/loadimpact/k6/blob/d15bfe93177fabfd80527045a270930b2f609eed/lib/executor/variable_looping_vus.go#L579 and https://github.com/loadimpact/k6/blob/d15bfe93177fabfd80527045a270930b2f609eed/lib/executor/variable_looping_vus.go#L587, where apparently by https://golang.org/src/sync/waitgroup.go?s=3500:3527#L124 you need to synchronize the Add and Wait ... which I agree with and it is something that my WIP change does for variable arrival rate ... I think ...

So maybe this isn't a problem with this PR :D

Yeah, while it doesn't seem related to this PR, the raciness was exhibited with the new tests, which is good I suppose, since I didn't find anything wrong with the tests themselves.

So I applied the same defer reordering fix we did in a previous PR for the arrival-rate execs to the VLV one, and it seemed to fix the issue locally at least (although that could be flaky...). See 60b2ffe. Let's wait for CI... \o/ Seems to have fixed it.

imiric · 2020-05-11T16:15:48Z

core/local/local_test.go

+				}
+			}
+		case <-done:
+			require.Equal(t, 8, gotSampleTags, "received wrong amount of samples with expected tags")


So a couple of issues with this test:

It's too large, and possibly testing too much functionality. I'd be up for splitting it, but I think we'd end up with more LOCs overall. Any ideas on simplifying, or splitting criteria and/or where to move it?

This assertion is far too brittle (i.e. easily susceptible to changes), and the counter approach is not very reliable. Any better ideas? :)

mstoykov

I have a few more nitpicks and in general things that I do not like, but all of those can be fixed at a later time, and this seems to be in a working state? Although we should definitely do more manual testing.

List of things I don't like (in no particular order): (while I am okay debating them, implementing them in this PR is in no way required)

the whole exports field in the js package, haven't tried it but IMO we don't need it, we just need the VU to have its function as a field, and even that is probably not a requirement. As I mentioned previously my primary problem is that in the past we had stupid bugs because of a goja.Value/goja.Object was used in another VM that isn't the one it was created in. Looking at this - this will predominantly be problematic in the tests.
the more I look at the more it seems to me the whole merging of tags/env should just happen in one place ... maybe even not in the runner (although that is okay as well) and just to get propagated through the state for everywhere else in the VU to use.
Not really happy with us polluting even more lib and core/local with dependencies to js
something that I forgot while I was writing the above 🤦

mstoykov · 2020-05-12T09:23:06Z

core/local/local_test.go

+	opts := runner.GetOptions()
+	opts.Hosts = map[string]net.IP{
+		"httpbin.local": net.ParseIP("127.0.0.1"),
+	}
+	require.NoError(t, runner.SetOptions(opts))


nit: Does this not work if it's just in the text options above ?

Indeed 🤦 Fixed in 8c5d8df.

mstoykov · 2020-05-12T09:31:25Z

lib/executor/externally_controlled.go

 func newManualVUHandle(
 	parentCtx context.Context, state *lib.ExecutionState,
-	localActiveVUsCount *int64, initVU lib.InitializedVU, logger *logrus.Entry,
+	localActiveVUsCount *int64, initVU lib.InitializedVU,
+	config *BaseConfig, logger *logrus.Entry,


nit: maybe have this be a method on *externallyControlledRunState as there are now 5 parameters that are its fields ...

Good idea, should be resolved by 438b422.

lib/executor/vu_handle.go

This avoids having to read state.Options in some cases, as that should be done only once in runFn. Resolves #1428 (comment)

This reverts commit 25a26ab. Concurrent calls to getVUActivationParams would cause race conditions when reading from BaseConfig and several tests would fail, so this needs to be done only once per executor, and we need this RunContext overriding after all.

This avoids having to read state.Options in some cases, as that should be done only once in runFn. Resolves #1428 (comment)

Resolves #1428 (comment)

This applies the same fixes as eb13303 and d01e2c1 for the VLV executor, to ensure activeVUsWG.Wait() is called last, after the context has been cancelled. It resolves some race conditions exhibited by the TestExecutionSchedulerRun* tests (e.g. [1]). [1]: https://circleci.com/gh/loadimpact/k6/10139

imiric · 2020-05-12T15:07:15Z

This should be ready now, hopefully free from race conditions...

Let me know if I should clean the commits up a bit, some of them could be squashed.

Also: I haven't tested tagging of thresholds. Besides that, is there anything else taggable that might be affected by this?

mstoykov

What I said in my previous comment - LGTM!

We need to test and document this a lot now ... as this is probably the single biggest change to the executing of VUs we've done in a long time.

na--

LGTM in general, though I found a few minor issues that I fixed in #1453 - if you don't see any issues with the changes, feel free to 👍 and merge that PR here, and this in new-schedulers afterwards.

This would previously fail without the fixes in #1453.

Ivan Mirić added 4 commits April 30, 2020 15:28

Add support for non-default function execution

e693715

See #1300

Add support for executor-specific env vars

4913ad0

See #1300

Add support for executor-specific tags

71a7180

See #1300

Ignore funlen linter for PerVUIterations.Run

702f80d

imiric requested review from mstoykov and na-- April 30, 2020 14:00

imiric commented Apr 30, 2020

View reviewed changes

core/local/local.go Outdated Show resolved Hide resolved

imiric commented Apr 30, 2020

View reviewed changes

core/local/local.go Outdated Show resolved Hide resolved

imiric commented Apr 30, 2020

View reviewed changes

lib/executor/constant_arrival_rate.go Outdated Show resolved Hide resolved

na-- requested changes Apr 30, 2020

View reviewed changes

Ivan Mirić added 2 commits May 4, 2020 12:42

Move script function names to lib/consts package

f7bc0fa

Validate executor config earlier

a185924

This leaves executor init before VU init, but moves the config validation to a more suitable place before the engine/ExecutionScheduler is initialized. It now also properly reports errors from more than one executor. Resolves #1428 (comment)

imiric pushed a commit that referenced this pull request May 6, 2020

Use helper function for VUActivationParams, simplify by passing BaseC…

add949b

…onfig Resolves: - #1428 (comment) - #1428 (comment)

Ivan Mirić added 2 commits May 6, 2020 18:00

Use helper function for VUActivationParams, simplify by passing BaseC…

f7e0214

…onfig Resolves: - #1428 (comment) - #1428 (comment)

Refactor exec fallback

1eafd11

Not sure if this is an improvement honestly, but I didn't find a way to set a default value for null.String.

imiric force-pushed the feat/1007-1300-env-tags-exec-options branch from 4d47445 to 1eafd11 Compare May 6, 2020 16:00

imiric pushed a commit that referenced this pull request May 7, 2020

Check exports with lambda instead of passing object

9c1d408

This now also grabs all exported functions, including setup/teardown. Partially resolves: - #1428 (comment) - #1428 (comment)

Check exports with lambda instead of passing object

d90078d

This now also grabs all exported functions, including setup/teardown. Partially resolves: - #1428 (comment) - #1428 (comment)

imiric force-pushed the feat/1007-1300-env-tags-exec-options branch from 9c1d408 to d90078d Compare May 7, 2020 12:57

imiric requested a review from na-- May 7, 2020 13:12

na-- reviewed May 8, 2020

View reviewed changes

cmd/config.go Outdated Show resolved Hide resolved

core/local/local.go Outdated Show resolved Hide resolved

lib/executor/shared_iterations.go Show resolved Hide resolved

js/runner.go Outdated Show resolved Hide resolved

na-- requested changes May 8, 2020

View reviewed changes

Ivan Mirić added 6 commits May 11, 2020 14:54

Fix custom exec tags not leaking between executors

ae5ef34

Resolves #1428 (comment)

Revert moving executor init before VU init

60c3c09

This was done in e693715 to ensure exec validation would fail before VUs are initialized, but now that this validation is done much earlier, this is no longer needed. Resolves #1428 (comment)

Get new VUActivationParams instance per VU in some executors

25a26ab

The minor optimization wasn't worth the messiness. Resolves #1428 (comment)

Add custom executor tags to WebSocket metrics

80bff4d

Add custom executor tags to checks and group metrics

06aa32d

Remove superfluous use of fmt.Sprintf

d25c71a

Resolves #1428 (comment)

mstoykov reviewed May 11, 2020

View reviewed changes

imiric force-pushed the feat/1007-1300-env-tags-exec-options branch from b23b02a to a90e8f2 Compare May 11, 2020 15:59

imiric commented May 11, 2020

View reviewed changes

mstoykov approved these changes May 12, 2020

View reviewed changes

imiric pushed a commit that referenced this pull request May 12, 2020

Merge RunTags into state.Tags and reuse that everywhere

d15bfe9

This avoids having to read state.Options in some cases, as that should be done only once in runFn. Resolves #1428 (comment)

Ivan Mirić added 7 commits May 12, 2020 16:39

Define httpbin.local host in NoCrossover test

8c5d8df

Cleanup NoCrossover test a bit

28032b6

Merge RunTags into state.Tags and reuse that everywhere

4cf716c

This avoids having to read state.Options in some cases, as that should be done only once in runFn. Resolves #1428 (comment)

Move newManualVUHandle function to EC RunState

438b422

Resolves #1428 (comment)

Minor simplification of VU activation in vuHandle

56c0a89

Resolves #1428 (comment)

imiric force-pushed the feat/1007-1300-env-tags-exec-options branch from d15bfe9 to 60b2ffe Compare May 12, 2020 14:49

imiric requested review from mstoykov and na-- May 12, 2020 15:05

mstoykov approved these changes May 15, 2020

View reviewed changes

na-- added a commit that referenced this pull request May 18, 2020

Fix a bunch of minor issues and repetitions in #1428

0c94462

na-- mentioned this pull request May 18, 2020

Fix a bunch of minor issues and repetitions #1453

Merged

na-- approved these changes May 18, 2020

View reviewed changes

na-- and others added 3 commits May 18, 2020 12:08

Fix a bunch of minor issues and repetitions in #1428

a44509d

Fix __ENV polution

e1a5c0a

Tweak NoCrossover test to better cover VU reuse scenarios

b2b8648

This would previously fail without the fixes in #1453.

imiric merged commit 270fd91 into new-schedulers May 18, 2020

imiric deleted the feat/1007-1300-env-tags-exec-options branch May 18, 2020 11:03

This was referenced May 18, 2020

Extra env/tags/exec options for executors #1300

Closed

add internal to solve import cycle #1454

Closed

mstoykov added this to the v0.27.0 milestone Jul 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for exec/env/tags executor options #1428

Add support for exec/env/tags executor options #1428

imiric commented Apr 30, 2020

imiric Apr 30, 2020

codecov-io commented May 7, 2020 •

edited

Loading

na-- left a comment

mstoykov May 11, 2020

imiric May 11, 2020

mstoykov May 11, 2020

imiric May 11, 2020

imiric May 12, 2020 •

edited

Loading

mstoykov May 12, 2020

mstoykov May 12, 2020

mstoykov May 12, 2020

imiric May 12, 2020 •

edited

Loading

imiric May 11, 2020

mstoykov left a comment

mstoykov May 12, 2020

imiric May 12, 2020

mstoykov May 12, 2020

imiric May 12, 2020

imiric commented May 12, 2020

mstoykov left a comment •

edited

Loading

na-- left a comment

Add support for exec/env/tags executor options #1428

Add support for exec/env/tags executor options #1428

Conversation

imiric commented Apr 30, 2020

Choose a reason for hiding this comment

codecov-io commented May 7, 2020 • edited Loading

Codecov Report

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric commented May 12, 2020

mstoykov left a comment • edited Loading

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

codecov-io commented May 7, 2020 •

edited

Loading

imiric May 12, 2020 •

edited

Loading

imiric May 12, 2020 •

edited

Loading

mstoykov left a comment •

edited

Loading