-
Notifications
You must be signed in to change notification settings - Fork 69
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
Introduce incremental workflow scenarios to GHA #369
Introduce incremental workflow scenarios to GHA #369
Conversation
Co-authored-by: Kamesh Akella <[email protected]>
Co-authored-by: Kamesh Akella <[email protected]>
Co-authored-by: Kamesh Akella <[email protected]>
a090880
to
2bf0e8e
Compare
I would like to get the user scalability scenario in as well, @mhajas its almost ready in my local. |
@ahus1 it would be great if you can review some of the changes we(me and Michal) made for these scenarios. Happy to get on a call on Monday for a walk through as well. cc: @mhajas I did push the changes we discussed. PS: Leaving the commit trace as is., if needed we can revert to a previous commit, based on the outcome. I can squash and merge in the end of the review cycle. |
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.
I did a review in another commit. Please review the suggestions, and change/revert them as necessary.
There's also a new FIXME in keycloak-scalability-benchmark.yml
as it IMHO misses to run AuthorizationCode with --users-per-realm
.
For future reviews, please avoid merging the main branch into the PR, as it makes it harder to review the changes in an IDE.
I found that kcb.sh
no longer logged the output to the console, which was a surprise to me and would make it harder for users of the simple runs to analyze their results. Therefore I added 'tee' to have the logs both on the console and in a file.
It might make sense to not log on the console for incremental runs - if you want to do this, feel free to do so.
@@ -135,7 +135,7 @@ if [ "$MODE" = "incremental" ]; then | |||
|
|||
if [[ -n $WARM_UP ]]; then | |||
echo "INFO: Running warm-up phase." | |||
CONFIG_ARGS+=("-Dramp-up=1") | |||
CONFIG_ARGS+=("-Dwarm-up=${WARM_UP}") |
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.
@ahus1 this change was by design., I can explain :)
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.
Please do, I'm curious :-)
@ahus1 thanks so much for the review., and sorry about the merge of main into this, it happened accidentally, will be more careful from next time.
@ahus1 thanks so much for the review., and sorry about the merge of main into this, it happened accidentally, will be more careful from next time. I have updated the kcb.sh script to do warm-up by default every time we run incremental, as we would want that anyways, and it probably will keep things simple. tagging @mhajas as this is a change in how we ought to run the incremental runs. Update the FIXME part in kcb.sh with the duplicated code block to accommodate the right args for AuthorizationCode scenario. I shall work on adding docs for the |
Here is the example output on the console for an incremental run:
|
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.
I've reviewed the changes and ran the scripts locally. This looks ok to be merged. Once it is merged, I'll run additional tests to see if there is the need for further changes.
Did some small adjustments, including disabling sticky sessions. Here are the results of the first run: https://github.com/keycloak/keycloak-benchmark/actions/runs/5255644969/jobs/9495932993 |
Closes #354
Closes #355
@kami619 Do we want to add also user scenario within this PR, or we will do it as a follow-up?