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

Move most capabilities out of the core.Engine in preparation for removal #2813

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

na--
Copy link
Member

@na-- na-- commented Dec 8, 2022

This built on top of #2812 and is the penultimate PR before #1889 🎉 🤞

It splits off as much of the capabilities of the Engine as it's possible to do and keep most of its tests (mostly) intact 😅 Commits should be atomic and it might be easier to review the PR commit by commit. I can even split this into smaller PRs, if needs be, though I think grouping these commits together in one PR and completely removing the Engine in another PR makes the most sense.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2022

Codecov Report

Merging #2813 (d0eb9d3) into master (daa866a) will increase coverage by 0.06%.
The diff coverage is 94.16%.

@@            Coverage Diff             @@
##           master    #2813      +/-   ##
==========================================
+ Coverage   76.78%   76.84%   +0.06%     
==========================================
  Files         226      225       -1     
  Lines       16848    16807      -41     
==========================================
- Hits        12936    12915      -21     
+ Misses       3084     3066      -18     
+ Partials      828      826       -2     
Flag Coverage Δ
ubuntu 76.65% <94.16%> (-0.03%) ⬇️
windows 76.52% <93.43%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/v1/setup_teardown_routes.go 58.82% <60.00%> (+0.92%) ⬆️
api/v1/status_routes.go 51.02% <87.50%> (-1.07%) ⬇️
api/v1/routes.go 55.31% <90.00%> (ø)
core/engine.go 85.36% <92.85%> (-8.98%) ⬇️
metrics/engine/engine.go 84.82% <94.87%> (+4.05%) ⬆️
api/server.go 82.60% <100.00%> (+1.65%) ⬆️
api/v1/group_routes.go 57.14% <100.00%> (-3.73%) ⬇️
api/v1/metric_routes.go 77.77% <100.00%> (-1.54%) ⬇️
api/v1/status.go 100.00% <100.00%> (ø)
api/v1/status_jsonapi.go 100.00% <100.00%> (ø)
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@olegbespalov
Copy link
Contributor

@na-- For staring the code review could you please resolve conflicts, thanks!

api/server.go Outdated Show resolved Hide resolved
cmd/run.go Outdated Show resolved Hide resolved
@na-- na-- force-pushed the move-things-to-execution branch 3 times, most recently from a42bdc7 to 9f050f7 Compare January 24, 2023 10:38
@na-- na-- force-pushed the prune-the-engine branch 2 times, most recently from 96162e1 to 30908a1 Compare January 24, 2023 10:48
Base automatically changed from move-things-to-execution to master January 24, 2023 10:57
codebien
codebien previously approved these changes Jan 24, 2023
Comment on lines +31 to +32
// TODO: reduce the control surface as much as possible? For example, if
// we refactor the Runner API, we won't need to send the Samples channel.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, do we have an issue with it? I didn't find much searching by Runner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think we have a dedicated issue for that 🤔 We have a few code TODOs like this one, and some tangentially related issues like #1048 and #2869, but nothing dedicated 🤔

The problem is that we (or at least I) don't have a clear vision for how it should refactored yet... For example, I know that we need to change the name, since Runner is super confusing when it is essentially a "factory" for VUs... 😅 But since I don't have a clear idea how we can refactor it, I also don't know into what to rename it 😞

So yeah, this is why we have the TODO here and in the Runner:

k6/lib/runner.go

Lines 41 to 85 in 1d99b0b

// A Runner is a factory for VUs. It should precompute as much as possible upon
// creation (parse ASTs, load files into memory, etc.), so that spawning VUs
// becomes as fast as possible. The Runner doesn't actually *do* anything in
// itself, the ExecutionScheduler is responsible for wrapping and scheduling
// these VUs for execution.
//
// TODO: Rename this to something more obvious? This name made sense a very long
// time ago.
type Runner interface {
// Creates an Archive of the runner. There should be a corresponding NewFromArchive() function
// that will restore the runner from the archive.
MakeArchive() *Archive
// Spawns a new VU. It's fine to make this function rather heavy, if it means a performance
// improvement at runtime. Remember, this is called once per VU and normally only at the start
// of a test - RunOnce() may be called hundreds of thousands of times, and must be fast.
NewVU(ctx context.Context, idLocal, idGlobal uint64, out chan<- metrics.SampleContainer) (InitializedVU, error)
// Runs pre-test setup, if applicable.
Setup(ctx context.Context, out chan<- metrics.SampleContainer) error
// Returns json representation of the setup data if setup() is specified and run, nil otherwise
GetSetupData() []byte
// Saves the externally supplied setup data as json in the runner
SetSetupData([]byte)
// Runs post-test teardown, if applicable.
Teardown(ctx context.Context, out chan<- metrics.SampleContainer) error
// Returns the default (root) Group.
GetDefaultGroup() *Group
// Get and set options. The initial value will be whatever the script specifies (for JS,
// `export let options = {}`); cmd/run.go will mix this in with CLI-, config- and env-provided
// values and write it back to the runner.
GetOptions() Options
SetOptions(opts Options) error
// Returns whether the given name is an exported and executable
// function in the script.
IsExecutable(string) bool
HandleSummary(context.Context, *Summary) (map[string]io.Reader, error)
}

I also have some very vague ideas on how to refactor setup/teardown/handleSummary handling so they don't require their own dedicated methods, and also how to refactor the archive handling to be somewhat separate, but everything is super vague at the moment and probably full of problems 😞 Someone needs to start doing it, so we can hit the real issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to create a new issue after all: #2873

Even though we don't have the whole picture, it doesn't hurt to have a place to collect problems that need to be fixed

imiric
imiric previously approved these changes Jan 26, 2023
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have major comments, nicely done 👍 And sorry it took so long to review it 😓

metrics/engine/engine.go Outdated Show resolved Hide resolved
core/engine.go Outdated Show resolved Hide resolved
core/engine.go Show resolved Hide resolved
core/engine_test.go Show resolved Hide resolved
After this commit, the only place the Engine is used in the api/ package tests, and only in a few places that will be easy to refactor after we remove it completely.
@na-- na-- merged commit 1268fad into master Jan 27, 2023
@na-- na-- deleted the prune-the-engine branch January 27, 2023 09:30
na-- added a commit that referenced this pull request Jan 30, 2023
This is the first commit from #2815, plus an extra test that checks the exit code when aborting a `k6 run --paused` test with Ctrl+C. 

Unfortunately, it turned out that 19dd505 from #2813 introduced a regression. Interrupting the k6 process with a signal should result in a `105` exit code (`ExternalAbort`), but with that commit k6 exited with `103` if k6 was `--paused`. The good news is that the fix was already done in the first commit of #2815, which was entirely self-sufficient and can be moved to this separate PR. As a bonus, this makes reviewing everything even easier... 

So, some details about the commit itself... In short, there is no need to have 2 separate `Run()` and `Init()` methods in the `execution.Scheduler`. Instead, Run() can start the VU initialization itself. As a bonus, this immediately makes the error handling around init errors much more in line with other error handling, allowing us to resolve the bug, but also to respect `--linger` and to try and execute `handleSummary()` if there were problems. Except the first init that is used to get the exported options, of course, that one is still special.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants