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

Speed up TestAppServersHA #12128

Merged
merged 4 commits into from
Apr 26, 2022
Merged

Speed up TestAppServersHA #12128

merged 4 commits into from
Apr 26, 2022

Conversation

rosstimothy
Copy link
Contributor

@rosstimothy rosstimothy commented Apr 20, 2022

Allow test cases to be run in parallel and allow app servers to
be spawned in parallel to reduce test time from ~99s to ~25s.

From master:

while go test ./integration -run TestAppServersHA  -count=1 -race; do :; done                                                                                                    
ok  	github.com/gravitational/teleport/integration	98.516s
ok  	github.com/gravitational/teleport/integration	99.719s
ok  	github.com/gravitational/teleport/integration	97.247s
ok  	github.com/gravitational/teleport/integration	97.509s
ok  	github.com/gravitational/teleport/integration	98.961s
ok  	github.com/gravitational/teleport/integration	98.864s

With changes in this PR:

 while go test ./integration -run TestAppServersHA  -count=1 -race; do :; done                                                                                             
ok  	github.com/gravitational/teleport/integration	25.519s
ok  	github.com/gravitational/teleport/integration	25.889s
ok  	github.com/gravitational/teleport/integration	23.359s
ok  	github.com/gravitational/teleport/integration	22.605s
ok  	github.com/gravitational/teleport/integration	23.169s
ok  	github.com/gravitational/teleport/integration	24.411s

@github-actions github-actions bot requested review from r0mant and xacrimon April 20, 2022 19:10
integration/app_integration_test.go Show resolved Hide resolved
integration/app_integration_test.go Outdated Show resolved Hide resolved
results := make(chan result, len(configs))
for _, conf := range configs {
go func(cfg *service.Config) {
dataDir, err := os.MkdirTemp("", "cluster-"+i.Secrets.SiteName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this logic reuse StartApp above? Seems like most of logic here is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially tried to launch StartApp in a goroutine for each app, but it turns out TeleInstance is racy. StartApp, StartNode, StartDatabase, etc all update i.tempDirs and i.Nodes without using a mutex. I opted to add StartApps and handle updating the TeleInstance fields in a way that wasn't racy instead of adding a mutex to TeleInstance and finding all the possible races.

The downside is as you mentioned StartApp and StartApps are nearly identical. If you think it would be better to remove StartApps and go down the mutex route I'm happy to make the change.

Allow test cases to be run in parrallel and allow app servers to
be spawned in parrallel to reduce test time from ~99s to ~25s.
@rosstimothy rosstimothy force-pushed the tross/speed_up_app_servers_ha branch from 54f670e to 198d994 Compare April 26, 2022 17:16
@rosstimothy
Copy link
Contributor Author

I further simplified the tests in 198d994 to hopefully get CI to pass. This also shaved some more time off the test:

while go test ./integration -run TestAppServersHA -count=1 -race; do :; done
ok  	github.com/gravitational/teleport/integration	19.709s
ok  	github.com/gravitational/teleport/integration	19.889s
ok  	github.com/gravitational/teleport/integration	20.801s
ok  	github.com/gravitational/teleport/integration	20.360s
ok  	github.com/gravitational/teleport/integration	20.451s
ok  	github.com/gravitational/teleport/integration	19.464s
ok  	github.com/gravitational/teleport/integration	20.956s
ok  	github.com/gravitational/teleport/integration	20.930s
ok  	github.com/gravitational/teleport/integration	19.739s

@rosstimothy rosstimothy merged commit 71dea2d into master Apr 26, 2022
@rosstimothy rosstimothy deleted the tross/speed_up_app_servers_ha branch April 26, 2022 19:05
rosstimothy added a commit that referenced this pull request Apr 26, 2022
* Speed up TestAppServersHA

Allow test cases to be run in parrallel and allow app servers to
be spawned in parrallel to reduce test time from ~99s to ~20s.

(cherry picked from commit 71dea2d)
rosstimothy added a commit that referenced this pull request Apr 26, 2022
* Speed up TestAppServersHA

Allow test cases to be run in parrallel and allow app servers to
be spawned in parrallel to reduce test time from ~99s to ~20s.

(cherry picked from commit 71dea2d)
@rosstimothy
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
branch/v8
branch/v9

Questions ?

Please refer to the Backport tool documentation

rosstimothy added a commit that referenced this pull request Apr 28, 2022
* Speed up TestAppServersHA

Allow test cases to be run in parrallel and allow app servers to
be spawned in parrallel to reduce test time from ~99s to ~20s.

(cherry picked from commit 71dea2d)
rosstimothy added a commit that referenced this pull request Apr 28, 2022
* Speed up TestAppServersHA

Allow test cases to be run in parrallel and allow app servers to
be spawned in parrallel to reduce test time from ~99s to ~20s.

(cherry picked from commit 71dea2d)
@webvictim webvictim mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants