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

[Feature][Deployment] Add KEDA autoscaler support for worker deployment when deployed in K8S cluster #13367

Merged
merged 12 commits into from
Jan 14, 2023

Conversation

EricGao888
Copy link
Member

@EricGao888 EricGao888 commented Jan 10, 2023

Purpose of the pull request

image
image

Brief change log

  • Already described above.

Verify this pull request

  • Verified on K8S cluster.

@EricGao888 EricGao888 self-assigned this Jan 10, 2023
@EricGao888 EricGao888 added 3.2.0 for 3.2.0 version feature new feature labels Jan 10, 2023
@EricGao888 EricGao888 added this to the 3.2.0 milestone Jan 10, 2023
@EricGao888 EricGao888 added the miss:docs missing documents in PR label Jan 10, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2023

Codecov Report

Merging #13367 (cba61cd) into dev (8f7b141) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev   #13367      +/-   ##
============================================
- Coverage     39.45%   39.43%   -0.02%     
+ Complexity     4313     4311       -2     
============================================
  Files          1083     1083              
  Lines         40756    40778      +22     
  Branches       4670     4674       +4     
============================================
+ Hits          16079    16081       +2     
- Misses        22890    22910      +20     
  Partials       1787     1787              
Impacted Files Coverage Δ
...er/api/service/impl/TaskGroupQueueServiceImpl.java 43.18% <0.00%> (-12.71%) ⬇️
...heduler/api/service/impl/TaskGroupServiceImpl.java 60.00% <0.00%> (-3.07%) ⬇️
...uler/api/service/impl/TaskInstanceServiceImpl.java 50.34% <0.00%> (-0.36%) ⬇️
...cheduler/api/service/impl/ExecutorServiceImpl.java 47.22% <0.00%> (ø)
...nscheduler/service/process/ProcessServiceImpl.java 30.14% <0.00%> (+0.05%) ⬆️
...scheduler/api/service/impl/ProjectServiceImpl.java 62.90% <0.00%> (+0.11%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@EricGao888
Copy link
Member Author

Action Items for this PR:

  1. Add mysql autoscaler.
  2. Make DB connection configurable in triggers so that users could connect to external db.
  3. Add related docs.

@EricGao888 EricGao888 marked this pull request as ready for review January 13, 2023 12:28
@EricGao888 EricGao888 removed the miss:docs missing documents in PR label Jan 13, 2023
sslmode: "disable"
targetQueryValue: "1"
query: >-
SELECT ceil(COUNT(*)::decimal / {{ .Values.worker.env.WORKER_EXEC_THREADS }})
Copy link
Member

Choose a reason for hiding this comment

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

There is a tricky issue that users can change the worker exec threads through a configmap, yet I don't come out a solution for that case 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I don't have a solution at this moment either. I will add a reminder in docs for users about this.

dbName: {{ .Values.mysql.auth.database }}
username: {{ .Values.mysql.auth.username }}
passwordFromEnv: SPRING_DATASOURCE_PASSWORD
queryValue: "1"
Copy link
Member

Choose a reason for hiding this comment

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

This key queryValue is different from the one in postgresql targetQueryValue, is that expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

This key queryValue is different from the one in postgresql targetQueryValue, is that expected?

Yes, it is. As you could see in the following source code, postgresql and mysql autoscalers have different names for the same parameter.

https://github.com/kedacore/keda/blob/c1611482226aae17b8af1a575f1a629f0c912bd1/pkg/scalers/postgresql_scaler.go#L181-L193

https://github.com/kedacore/keda/blob/c1611482226aae17b8af1a575f1a629f0c912bd1/pkg/scalers/mysql_scaler.go#L211-L223

Comment on lines 71 to 72
{{- else if .Values.externalDatabase.enabled }}
- type: mysql
Copy link
Member

Choose a reason for hiding this comment

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

An externalDatabase is not necessarily mysql 😅 it can be postgresql as well, you might want to use

Suggested change
{{- else if .Values.externalDatabase.enabled }}
- type: mysql
{{- else if .Values.externalDatabase.enabled }}
- type: {{ .Values.externalDatabase.type }}

Copy link
Member Author

Choose a reason for hiding this comment

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

An externalDatabase is not necessarily mysql 😅 it can be postgresql as well, you might want to use

I will use a condition clause here to support both postgresql and mysql. As these two use different autoscalers and there are tiny differences.

which will significantly save the resources.

Worker autoscaling feature is compatible with `postgresql` and `mysql` shipped with `DolphinScheduler official helm chart`. If you
use external database, worker autoscaling feature only supports external `mysql` databases.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have trouble to support both mysql and postgresql now?

Copy link
Member Author

Choose a reason for hiding this comment

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

No trouble. Will add support for postgresql in next commit.

@sonarcloud
Copy link

sonarcloud bot commented Jan 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@EricGao888 EricGao888 merged commit 366f999 into apache:dev Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.2.0 for 3.2.0 version document feature new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][Worker] Use KEDA and HPA for worker auto scaling when deployed in K8S
3 participants