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

Fix metadata DB & port in Keda connection when usePgbouncer is false #34741

Merged
merged 3 commits into from Oct 7, 2023

Conversation

hussein-awala
Copy link
Member

@hussein-awala hussein-awala commented Oct 3, 2023

closes: #34740

When usePgbouncer and KEDA is activated, KEDA connection should use the metadata port instead of pgbouncer port.

@hussein-awala
Copy link
Member Author

@brokenjacobs this should fix the issue.

Usually we don't create patch releases for the chart, but maybe we can wait a week and release a light patch release if other bugs are reported (to check with @jedcunningham)

@jedcunningham
Copy link
Member

Yep, open to it. We've never really had the need before.

@brokenjacobs
Copy link
Contributor

Please see #34740 the database name is also incorrect. It uses the db name defined in the pgbouncer.ini, not the upstream database name.

@brokenjacobs
Copy link
Contributor

@brokenjacobs this should fix the issue.

Usually we don't create patch releases for the chart, but maybe we can wait a week and release a light patch release if other bugs are reported (to check with @jedcunningham)

Thanks for your work on this!

@hussein-awala
Copy link
Member Author

It uses the db name defined in the pgbouncer.ini, not the upstream database name.

Indeed, I fixed it and I checked all the other values, all looks good!

@eladkal eladkal added this to the Airflow Helm Chart 1.12.0 milestone Oct 4, 2023
@@ -48,7 +49,7 @@ data:
{{- end }}
{{- if and .Values.workers.keda.enabled .Values.pgbouncer.enabled (not .Values.workers.keda.usePgbouncer) }}
{{- with .Values.data.metadataConnection }}
kedaConnection: {{ urlJoin (dict "scheme" .protocol "userinfo" (printf "%s:%s" (.user | urlquery) (.pass | urlquery) ) "host" (printf "%s:%s" $metadataHost $metadataPort) "path" (printf "/%s" $database) "query" $query) | b64enc | quote }}
kedaConnection: {{ urlJoin (dict "scheme" .protocol "userinfo" (printf "%s:%s" (.user | urlquery) (.pass | urlquery) ) "host" (printf "%s:%s" $metadataHost $metadataPort) "path" (printf "/%s" $meadataDatabase) "query" $query) | b64enc | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

typo nit meadataDatabase -> metadataDatabase

@@ -27,7 +27,8 @@
{{- $host := ternary $pgbouncerHost $metadataHost .Values.pgbouncer.enabled }}
{{- $metadataPort := .Values.data.metadataConnection.port | toString }}
{{- $port := ((ternary .Values.ports.pgbouncer $metadataPort .Values.pgbouncer.enabled) | toString) }}
{{- $database := (ternary (printf "%s-%s" .Release.Name "metadata") .Values.data.metadataConnection.db .Values.pgbouncer.enabled) }}
{{- $meadataDatabase := .Values.data.metadataConnection.db }}
{{- $database := (ternary (printf "%s-%s" .Release.Name "metadata") $meadataDatabase .Values.pgbouncer.enabled) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

typo nit. $meadataDatabase -> metadataDatabase

Copy link
Contributor

@brokenjacobs brokenjacobs left a comment

Choose a reason for hiding this comment

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

LGTM!

@hussein-awala hussein-awala changed the title Fix metadata port in Keda connection when usePgbouncer is false Fix metadata DB & port in Keda connection when usePgbouncer is false Oct 7, 2023
@hussein-awala hussein-awala merged commit 38e6607 into apache:main Oct 7, 2023
52 checks passed
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 27, 2023
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 changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

not using pgbouncer connection values still use pgbouncer ports/names for keda in helm chart
5 participants