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.

Sign up for GitHub

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

Jump to bottom

Fix ramping arrival rate unplanned vus #1500

Merged
merged 6 commits into from
Jul 3, 2020
Merged

Fix ramping arrival rate unplanned vus #1500

merged 6 commits into from
Jul 3, 2020

Conversation

mstoykov
Copy link
Collaborator

No description provided.

@mstoykov mstoykov requested review from imiric and na-- June 10, 2020 13:39
@mstoykov mstoykov added this to the v0.27.0 milestone Jun 10, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2020 •
edited

Codecov Report

Merging #1500 into new-schedulers will increase coverage by 0.02%.
The diff coverage is 90.14%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           new-schedulers    #1500      +/-   ##
==================================================
+ Coverage           77.27%   77.29%   +0.02%     
==================================================
  Files                 162      162              
  Lines               13058    13095      +37     
==================================================
+ Hits                10090    10122      +32     
- Misses               2452     2455       +3     
- Partials              516      518       +2
Impacted Files Coverage Δ
lib/executor/constant_arrival_rate.go 96.66% <88.57%> (-0.85%) ⬇️
lib/executor/ramping_arrival_rate.go 96.65% <91.66%> (-0.21%) ⬇️
lib/executor/vu_handle.go 93.69% <0.00%> (-1.81%) ⬇️
stats/cloud/collector.go 78.08% <0.00%> (-0.62%) ⬇️
lib/testutils/minirunner/minirunner.go 81.48% <0.00%> (+3.70%) ⬆️

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 447f892...07dea12. Read the comment docs.

mstoykov
mstoykov commented Jun 11, 2020
View reviewed changes
lib/executor/common_test.go Outdated
@@ -79,6 +79,6 @@ func initializeVUs(
for i := uint64(0); i < number; i++ {
vu, err := es.InitializeNewVU(ctx, logEntry)
require.NoError(t, err)
es.AddInitializedVU(vu)
es.ReturnVU(vu, false)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because AddInitializedVU adds 1 to the number of initializedVus .. this also seemed like the only place it is used, so maybe we can remove it :)

Copy link
Member

Choose a reason for hiding this comment

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

It turns out that this broke the externally-controlled tests, so I reverted it. Also, it's used here: https://github.com/loadimpact/k6/blob/447f892cc2c4073290e25b220307851d14aaae95/core/local/local.go#L203

imiric
imiric approved these changes Jun 17, 2020
View reviewed changes
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.

This makes sense, though it took me a while to follow the test 😅

lib/executor/ramping_arrival_rate.go Outdated Show resolved Hide resolved
na--
na-- requested changes Jun 23, 2020
View reviewed changes
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

The general initialization approach seems good, but I think the logic of how it's used is still wrong, as I've described in my inline comment

lib/executor/ramping_arrival_rate.go Outdated Show resolved Hide resolved
lib/executor/ramping_arrival_rate.go Outdated Show resolved Hide resolved
mstoykov and others added 4 commits July 1, 2020 11:46
@mstoykov @na--
VariableArrivalRate: don't block on unplannedVU init if possible
94b88a0 
This make it so that if an unplannedVU needs to be started we wait for
ANY free VU instead of only for that unplannedVU to start the next
iteration.

Also puts in place some guards against initing multiple unplannedVUs
@mstoykov @na--
Make certain no unitilized VU will be left unreturned
6a23e86
@na--
Fix ramping VU initialization
502a550
@na--
Fix tests and constant-arrival-rate VU init
223ed80
@na-- na-- force-pushed the fixRampingArrivalRateUnplannedVUs branch from 0719cfd to 223ed80 Compare July 2, 2020 09:40
@na-- na-- mentioned this pull request Jul 2, 2020
Open
@na-- na-- requested a review from imiric July 2, 2020 09:44
mstoykov
mstoykov commented Jul 2, 2020
View reviewed changes
Copy link
Collaborator Author

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

👍 but I can't approve it as I am the author 🙄

@na-- na-- force-pushed the fixRampingArrivalRateUnplannedVUs branch from a8076d6 to 9780e43 Compare July 2, 2020 12:36
imiric
imiric suggested changes Jul 2, 2020
View reviewed changes
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.

Hey, sorry, I think there's at least one blocker here (the initVU comment). But let me know if I'm wrong 😄

lib/executor/constant_arrival_rate.go Show resolved Hide resolved
lib/executor/constant_arrival_rate.go Outdated Show resolved Hide resolved
lib/executor/constant_arrival_rate.go Show resolved Hide resolved
@mstoykov mstoykov merged commit 89361f6 into new-schedulers Jul 3, 2020
@mstoykov mstoykov deleted the fixRampingArrivalRateUnplannedVUs branch July 3, 2020 12:20
@mstoykov mstoykov mentioned this pull request Jul 7, 2020
Closed
@na-- na-- mentioned this pull request Jul 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@na-- na-- na-- approved these changes

@imiric imiric imiric approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.27.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mstoykov @codecov-commenter @imiric @na--