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

Add Keda autoscaling for ballista in k8s #586

Merged
merged 7 commits into from
Jun 28, 2021

Conversation

edrevo
Copy link
Contributor

@edrevo edrevo commented Jun 18, 2021

Which issue does this PR close?

Closes #585.

What changes are included in this PR?

This PR provides very primitive support for auto-scaling: it works like an on-off switch. If there are no active tasks, the number of executors drops to 0. If there are active tasks, the number of executors will increase to the maximum number of allowed executors configured in Keda.

Further work could refine this so that the number of executors is proportional to the amount of outstanding tasks or something like that, but it is not trivial since we may end up killing executors that hold valuable shuffle data even if the aren't processing anything.

Changes in this PR:

  • The scheduler now implements the necessary gRPC interface for Keda.
  • k8s user guide has been updated

I have tested this manually and seen it scale to 0 and back up when there is a query.

Are there any user-facing changes?

No

@edrevo
Copy link
Contributor Author

edrevo commented Jun 18, 2021

cc @andygrove

@edrevo
Copy link
Contributor Author

edrevo commented Jun 18, 2021

apache-rat license violation: ballista/rust/scheduler/proto/keda.proto
Error: Process completed with exit code 1.

What's the policy here? This file comes from the Keda repo

@codecov-commenter
Copy link

Codecov Report

Merging #586 (580e692) into master (51e5445) will decrease coverage by 0.05%.
The diff coverage is 17.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
- Coverage   76.02%   75.96%   -0.06%     
==========================================
  Files         156      156              
  Lines       27063    27086      +23     
==========================================
+ Hits        20575    20577       +2     
- Misses       6488     6509      +21     
Impacted Files Coverage Δ
ballista/rust/scheduler/src/lib.rs 19.17% <0.00%> (-1.65%) ⬇️
ballista/rust/scheduler/src/main.rs 0.00% <0.00%> (ø)
ballista/rust/scheduler/src/state/mod.rs 71.01% <54.54%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51e5445...580e692. Read the comment docs.

@houqp
Copy link
Member

houqp commented Jun 21, 2021

3rd party work should have their original license and copyright maintained, see https://www.apache.org/legal/src-headers.html#3party. I think adding a header like below should be good enough:

   Copyright 2020 The KEDA Authors.

   and others that have contributed code to the public domain.

   Licensed under the Apache License, Version 2.0 (the "License"); 
   you may not use this file except in compliance with the License.
   You may obtain a copy of the License at.

       http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
   See the License for the specific language governing permissions and
   limitations under the License.

Copied from the bottom of https://github.com/kedacore/keda/blob/main/LICENSE.

@edrevo
Copy link
Contributor Author

edrevo commented Jun 23, 2021

merge conflicts resovled

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM. I haven't used keda before and haven't tested these changes but they look reasonable.

@andygrove
Copy link
Member

Hi @edrevo could you rebase this one when you get a chance.

@edrevo
Copy link
Contributor Author

edrevo commented Jun 28, 2021

merge conflicts resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autoscaling ballista executors in k8s
4 participants