Skip to content

Conversation

@Epicsteve2
Copy link
Contributor

Adds horizontal pod autoscaler to the helm chart.
A horizontal pod autoscaler simple will simply scale the number of Trino pods based off of CPU usage.

@cla-bot
Copy link

cla-bot bot commented Feb 16, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link

cla-bot bot commented Feb 16, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: autoscaling/v2 is out, but it went General Availability for Kubernetes 1.23. I feel like that version is too new to include in this helm chart.

For reference, AWS doesn't even support Kubernetes 1.22 yet

@cla-bot cla-bot bot added the cla-signed label Feb 17, 2022
@Epicsteve2
Copy link
Contributor Author

Epicsteve2 commented Feb 17, 2022

CI failed because I forgot to commit my changes for removing query.max-total-memory-per-node. 😅

I squashed it to 1 commit, but let me know if it should be a separate commit.
I do notice that #18 is almost ready to merge now. Should I just wait for that?

@hashhar
Copy link
Member

hashhar commented Feb 17, 2022

@Epicsteve2 The linked PR has been merged, can you rebase accordingly?

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % comment

@Epicsteve2
Copy link
Contributor Author

Looks like I forgot to bump the chart minor version (0.4.0 -> 0.5.0), so I updated that as well.

@hashhar
Copy link
Member

hashhar commented Feb 17, 2022

@Epicsteve2 Do most projects bump chart version for each change? I'd prefer to change them in batches when we explicitly decide to release some of those changes together.

@Epicsteve2
Copy link
Contributor Author

@Epicsteve2 Do most projects bump chart version for each change? I'd prefer to change them in batches when we explicitly decide to release some of those changes together.

Oh, yeah good point. I'll revert the change again. My apologies 😅

@hashhar hashhar merged commit 90be7de into trinodb:main Feb 17, 2022
@hashhar
Copy link
Member

hashhar commented Feb 17, 2022

Thanks for the contribution @Epicsteve2. I plan to cut a new release once #19, #12 and #17 make in.

@Epicsteve2 Epicsteve2 deleted the horizontal-pod-autoscaler branch February 17, 2022 18:36
@laserninja
Copy link

@Epicsteve2 What happens if the CPU usage for a node dips below the threshold and the node is still serving a query? Will the query fail?

@Epicsteve2
Copy link
Contributor Author

Epicsteve2 commented May 3, 2022

What happens if the CPU usage for a node dips below the threshold and the node is still serving a query? Will the query fail?

@laserninja
Yeah it kills the query when that happens 😭

Ideally, Trino wouldn’t kill queried pods when scaling down. I'm not sure if that's an issue being worked on for Trino, but it could be fixed in a future version of Trino

One way that could possibly be dealt with is to setup query retries for now. (I haven't tested this though 😅)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants