-
Notifications
You must be signed in to change notification settings - Fork 43
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
MM-60630: Automated loadtests configuration templates #860
base: master
Are you sure you want to change the base?
Conversation
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.
Good stuff, left some thoughts, nothing truly blocking though,
{ | ||
"Rate": 5.4, | ||
"Percentage": 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.
So, did we settle on the single rate for these? Is the multi-rate used anywhere at this point?
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 is what we used in the ceiling tests and I'm afraid what we've been using everywhere since.
}, | ||
"NumUsersInc": 8, | ||
"NumUsersDec": 8, | ||
"RestTimeSec": 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.
Just noting this doesn't make too much sense if UpdateIntervalMs
is still at 2s.
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.
Hmmm, you're right. I'm really afraid of touching this now that we've tested it works, but I can run some more tests in the following days if needed.
{ | ||
"AWSProfile": "", | ||
"AWSRegion": "us-east-1", | ||
"AWSAvailabilityZone": "", |
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.
Eventually, I think we'd want to try and use the same AZ, if possible, to contain costs unless it becomes a nightmare of flakiness. Trying other regions may help with that.
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 seen the flakiness happening with every region, that's why I decided not to add it here. We should review it, because it's important, but still flaky for automation :(
"MetricsInstanceType": "t3.xlarge", | ||
"AgentInstanceCount": 6, | ||
"AgentInstanceType": "c7i.xlarge", | ||
"ElasticSearchSettings": { |
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.
Do we have plans to use ES? We have 20 million posts after all. My rationale here is the more we test, the higher the chance of spotting regressions in the PRs.
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.
What we don't have for testing ES is an index for the 20M posts. I agree we should add it at some point, but this first iteration was planned without it.
examples/config/ci/deployer.json
Outdated
"AdminEmail": "[email protected]", | ||
"AdminUsername": "sysadmin", | ||
"AdminPassword": "Sys@dmin-sample1", | ||
"LoadTestDownloadURL": "https://github.com/mattermost/mattermost-load-test-ng/releases/download/v1.23.0-rc1/mattermost-load-test-ng-v1.23.0-rc1-linux-amd64.tar.gz", |
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.
Will this need constant updating?
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 set of config will need maintaining from our side, yes. I can add a step in the release process to review this. Also, I need to change this to the final release, not the RC-1.
"PR": "undefined", | ||
"SHA": "undefined" |
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.
Are these filled in later? Why the undefined literal vs an empty string?
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, they are! It doesn't really matter what we put here, it could be another literal or an empty string, nothing special about undefined
, just what I first wrote :)
Summary
This PR adds the needed config templates for CI to run the automated load-tests.
It also formats cluster.tf (there's always some formatting to do there) and removes the
lts
version of thelinux-aws-tools
in the proxy, which was still there.Ticket Link
https://mattermost.atlassian.net/browse/MM-60630