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

Also support mysql Keda trigger in scaledObject #36166

Closed
2 tasks done
claneys opened this issue Dec 11, 2023 · 1 comment · Fixed by #36167
Closed
2 tasks done

Also support mysql Keda trigger in scaledObject #36166

claneys opened this issue Dec 11, 2023 · 1 comment · Fixed by #36167
Assignees
Labels
area:helm-chart Airflow Helm Chart type:new-feature Changelog: New Features

Comments

@claneys
Copy link
Contributor

claneys commented Dec 11, 2023

Official Helm Chart version

1.11.0 (latest released)

Apache Airflow version

2.7.3

Kubernetes Version

1.25

Helm Chart configuration

postgresql:
  enabled: false

data:
  metadataConnection:
    host: an.ip.to.mysql
    port: 3306
    protocol: mysql

workers:
  keda:
    enabled: true
    # Query to use for KEDA autoscaling. Must return a single integer.
    # cf: https://github.com/apache/airflow/blob/2.7.3/chart/values.yaml#L531
    query: >-
      SELECT ceil(COUNT(*) / {{ .Values.config.celery.worker_concurrency }})
      FROM task_instance
      WHERE (state='running' OR state='queued')
      {{- if eq .Values.executor "CeleryKubernetesExecutor" }}
      AND queue != '{{ .Values.config.celery_kubernetes_executor.kubernetes_queue }}'
      {{- end }}

What happened

Airflow currently support two SQL database backend with postgresql and mysql. PostgreSQL being the default one it's also the trigger used by default by the Keda ScaledObject created when enabling autoscaling. Which do not work if using a mysql database. Query has to be changed a bit and mysql trigger parameters aren't the same (pretty sneaky little changes within key names) and most of all the connection string used isn't the correct one when addressing to mysql.

Rendered connection string: mysql://user:password@host:port/db_name
connection string that work: user:password@tcp(host:port)/db_name

What you think should happen instead

Have the trigger correctly set using values provided by the user to connect to the mysql database.
Use the already existing kedaConnection key to setup the correct connection string for mysql and as if using the Pgbouncer provide the KEDA_DB_CONN to the worker as environment variable.

The correct connectionString

How to reproduce

disable internal postgresql instance and connect to a mysql one. Then

$ kubectl get scaledobjects.keda.sh
NAME                  SCALETARGETKIND      SCALETARGETNAME       MIN   MAX   TRIGGERS   AUTHENTICATION   READY   ACTIVE   FALLBACK   PAUSED    AGE
airflow-worker   apps/v1.Deployment   airflow-worker   0     10    postgresql                       False    False    False      Unknown   2d22h

Anything else

I wrote a PR that supports both cases hoping that it will satisfying you but not sure if it's the best option. I'll be glad to discuss and improve it.

I saw the issue #34863 already opened and found that it has broader expection that only support the mysql db backend, so I opened this one.

Thanks

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@claneys claneys added area:helm-chart Airflow Helm Chart kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Dec 11, 2023
Copy link

boring-cyborg bot commented Dec 11, 2023

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

claneys added a commit to claneys/airflow that referenced this issue Dec 11, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Dec 13, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Dec 15, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
@hussein-awala hussein-awala added type:new-feature Changelog: New Features and removed kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Dec 17, 2023
claneys added a commit to claneys/airflow that referenced this issue Dec 18, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Dec 19, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Dec 20, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Dec 27, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Dec 28, 2023
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 3, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 8, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 9, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 11, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 15, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 16, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 17, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 18, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 19, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
claneys added a commit to claneys/airflow that referenced this issue Jan 20, 2024
apache#36166

Signed-off-by: Romain Forlot <romain.forlot@dailymotion.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants