-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix externally-controlled maxVUs scaling #1517
Conversation
if updateErr := executor.UpdateConfig(r.Context(), newConfig); err != nil { | ||
apiError(rw, "Config update error", updateErr.Error(), http.StatusInternalServerError) | ||
if updateErr := executor.UpdateConfig(r.Context(), newConfig); updateErr != nil { | ||
apiError(rw, "Config update error", updateErr.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstoykov We need that errorcheck
linter ;)
Also note the change to a 400 response code here, which is what was expected by the test, and will probably make sense in most cases.
@@ -479,6 +479,7 @@ func (rs *externallyControlledRunState) handleConfigChange(oldCfg, newCfg Extern | |||
executionState.ReturnVU(rs.vuHandles[i].initVU, false) | |||
} | |||
rs.vuHandles[i] = nil | |||
executionState.ModInitializedVUsCount(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this the actual issue of not scaling down ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes :D initializedVUs
wasn't decremented anywhere. Though not sure if this is the right approach or place for it :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be done when we're not returning the VU back to the buffer pool though, like this:
if i < rs.startMaxVUs {
// return the initial planned VUs to the common buffer
executionState.ReturnVU(rs.vuHandles[i].initVU, false)
} else {
executionState.ModInitializedVUsCount(-1)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To demonstrate, use the following options:
export let options = {
scenarios: {
extc1: {
executor: "externally-controlled",
vus: 5,
maxVUs: 10,
duration: "10s",
},
const_loop_vus: {
executor: "constant-vus",
vus: 10,
duration: "20s",
startTime: "10s",
gracefulStop: "0s",
},
},
};
see the 00/00 VUs
in the description here:
running (14.1s), 00/00 VUs, 139 complete and 15 interrupted iterations
extc1 ✓ [======================================] 05/10 VUs 10s
const_loop_vus ✗ [======>-------------------------------] 10 VUs 04.1s/20s
c03b3e7
to
c1244db
Compare
This replicates more closely what the local ExecutionScheduler does, and is needed for correct assertions in externally-controlled tests.
8d04ad7
to
a246679
Compare
a246679
to
fc48c46
Compare
Codecov Report
@@ Coverage Diff @@
## new-schedulers #1517 +/- ##
==================================================
+ Coverage 76.64% 77.23% +0.58%
==================================================
Files 163 163
Lines 13044 13046 +2
==================================================
+ Hits 9998 10076 +78
+ Misses 2532 2454 -78
- Partials 514 516 +2
Continue to review full report at Codecov.
|
Phew, I think I'll see if I can apply this same pattern to other executor tests to stabilize them. We should preferably have something like a |
@@ -479,6 +479,7 @@ func (rs *externallyControlledRunState) handleConfigChange(oldCfg, newCfg Extern | |||
executionState.ReturnVU(rs.vuHandles[i].initVU, false) | |||
} | |||
rs.vuHandles[i] = nil | |||
executionState.ModInitializedVUsCount(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be done when we're not returning the VU back to the buffer pool though, like this:
if i < rs.startMaxVUs {
// return the initial planned VUs to the common buffer
executionState.ReturnVU(rs.vuHandles[i].initVU, false)
} else {
executionState.ModInitializedVUsCount(-1)
}
@na-- See 64f30fc with that change. Though there are still some inconsistencies:
import http from 'k6/http';
import { sleep } from 'k6';
export let options = {
scenarios: {
adjustable_at_will: {
executor: 'externally-controlled',
vus: 0,
maxVUs: 10,
duration: '10m',
},
constant_vus: {
executor: 'constant-vus',
vus: 20,
duration: '2m',
},
}
};
export default function() {
sleep(0.2);
}
It's somewhat confusing that the API returns the global status, but can only modify externally-controlled parameters. Not sure how big of a problem this would be in practice, though it would be best to avoid any such inconsistencies. Maybe transparently convert any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I haven't tried it locally
#1357 strikes again 😞 |
hmm I think we should just document this, and fix it in the v2 REST API, whenever that is implemented... |
OK, I'll mention it in the release notes and the new docs somewhere. |
Closes #1511
I'm not sure this is the right fix, and the test is a bit hacky, but let me know.