Skip to content

feat(scaletest): add scaletest-runner template #9662

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
Merged
merged 36 commits into from
Sep 15, 2023
Merged

feat(scaletest): add scaletest-runner template #9662

merged 36 commits into from
Sep 15, 2023

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Sep 13, 2023 •
edited
Loading

This template is used on big.cdr.dev to run an automated scaletest.

https://big.cdr.dev/templates/scaletest-runner

Closes #9571

Some samples:

image

image

image

@mafredri mafredri requested review from johnstcn and mtojek September 13, 2023 14:07
@mafredri mafredri marked this pull request as ready for review September 13, 2023 14:07
mtojek
mtojek reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

First review round. It looks awesome, and I do like that it will be hosted publicly!

scaletest/templates/scaletest-runner/main.tf
option {
name = "Large"
value = "kubernetes-large"
icon = "/emojis/1f434.png" # Horse.
Copy link
Member

Choose a reason for hiding this comment

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

nit: could be an elephant :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, true. I just matched the icons set on the current template on big.

scaletest/templates/scaletest-runner/main.tf Outdated
order = 2
type = "bool"
name = "Dry-run"
default = false
Copy link
Member

Choose a reason for hiding this comment

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

In general, it is safer to enable dry-run by default.

BUT there is something to consider:
Ephemeral parameters are reset on every build, which means that if we intend to setup any workspace schedule/autobuild, default values will be picked up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I made it default true. I don't think we need to have these perfectly figured out right now, we'll be iterating on this as we detect new needs 👍🏻

johnstcn
johnstcn reviewed Sep 14, 2023
View reviewed changes
scaletest/setup/scaletest-sa.yaml Outdated
Copy link
Member

@johnstcn johnstcn Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This should probably be incorporated into scaletest/terraform instead

EDIT: If you prefer, I can incorporate this when I'm going through and updating the tf config instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'd appreciate that! Should I remove it from this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Nah you can leave it.

mafredri
mafredri commented Sep 14, 2023
View reviewed changes
scaletest/templates/scaletest-runner/Dockerfile
@@ -0,0 +1,34 @@
# This image is used to run scaletest jobs and, although it is inside
# the template directory, it is built separately and pushed to
# gcr.io/coder-dev-1/scaletest-runner:latest.
Copy link
Member Author

Choose a reason for hiding this comment

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

Self-review: I ended up keeping this file close to where it's used, however, it may be confusing if someone tries to edit it in the template editor and there are no changes.

Perhaps I should move it elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Well, in theory we could use our CI to build and push it...

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, that was the idea behind the "Future improvements" below. 👍🏻 IMO not the highest priority right now, though.

@mafredri mafredri force-pushed the mafredri/scaletest-runner-template branch from 126bc44 to 047c784 Compare September 14, 2023 16:03
@mafredri
Copy link
Member Author

mafredri commented Sep 14, 2023 •
edited
Loading

@mtojek @johnstcn I believe I've amended all feedback, and added some small tweaks:

  • Additional metadata column showing previous phase (useful to know what happened before we e.g. waiting to establish baseline)
  • An ephemeral cleanup strategy that defaults to always cleaning up workspaces
  • Status and phase updates now store a timestamp as well, may be useful for debugging
  • Always run clean up in prepare phase, in case resources were left after previous scale test (this could skew the graphs slightly, but via future Grafana annotations we know to ignore the data)

There will probably be a lot of tweaks to this template as we start adding features and improvements.

image image

@mafredri mafredri requested review from johnstcn and mtojek September 14, 2023 16:38
@mafredri mafredri force-pushed the mafredri/scaletest-runner-template branch from 87fb6f5 to 5f5d3b1 Compare September 14, 2023 19:09
@mafredri mafredri force-pushed the mafredri/scaletest-runner-template branch from bb045c7 to c9205ce Compare September 14, 2023 19:13
johnstcn
johnstcn approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Nice work!

mtojek
mtojek approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Awesome 👍

@mafredri mafredri enabled auto-merge (squash) September 15, 2023 13:19
@mafredri mafredri merged commit bc97eaa into main Sep 15, 2023
@mafredri mafredri deleted the mafredri/scaletest-runner-template branch September 15, 2023 13:23
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@johnstcn johnstcn johnstcn approved these changes

@mtojek mtojek mtojek approved these changes

Assignees

@mafredri mafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow running scaletests via one-click method
3 participants
@mafredri @johnstcn @mtojek