You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/11/22 15:58:55 UTC
[GitHub] [airflow] pgvishnuram opened a new pull request #19749: pgbouncer service enhancements
pgvishnuram opened a new pull request #19749:
URL: https://github.com/apache/airflow/pull/19749
Problem:
pgbouncer does not support additional volume and volume mounts this lacks feature to add client side tls certificates.
Additionally pgbouncer MetricExporter sslmode is hardcoded at all times, this breaks if user tries to set sslmode=require
How does this PR fix the problem above:
This PR adds ability to utilise existing labels and allow to use new labels from values.yaml
* Added support for extraVolumes and Volumemounts
* Added seperate sslmode for metrics exporter
* Fixed condition for pgbouncer volume mounts
* Added test cases
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pgvishnuram commented on a change in pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
pgvishnuram commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r766759154
##########
File path: chart/tests/test_pgbouncer.py
##########
@@ -427,7 +453,7 @@ def test_default_exporter_secret(self):
def test_exporter_secret_with_overrides(self):
connection = self._get_connection(
{
- "pgbouncer": {"enabled": True},
+ "pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "require"}},
Review comment:
Yesterday i applied all the suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r766757477
##########
File path: chart/tests/test_pgbouncer.py
##########
@@ -427,7 +453,7 @@ def test_default_exporter_secret(self):
def test_exporter_secret_with_overrides(self):
connection = self._get_connection(
{
- "pgbouncer": {"enabled": True},
+ "pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "require"}},
Review comment:
The expected value down on line 470 needs to be changed to `require`. I can't suggest it through the UI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#issuecomment-989813814
@pgvishnuram Can you rebase on main branch & resolve conflicts please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r765860292
##########
File path: chart/values.schema.json
##########
@@ -3004,6 +3004,16 @@
],
"default": null
},
+ "extraVolumes": {
+ "description": "Mount additional volumes into PgBouncer.",
+ "type": "array",
+ "default": []
+ },
+ "extraVolumeMounts": {
+ "description": "Mount additional volumes into PgBouncer.",
+ "type": "array",
+ "default": []
Review comment:
```suggestion
"default": [],
"items": {
"$ref": "https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/v1.22.0-standalone-strict/volumemount-v1.json"
}
```
##########
File path: chart/values.schema.json
##########
@@ -3004,6 +3004,16 @@
],
"default": null
},
+ "extraVolumes": {
+ "description": "Mount additional volumes into PgBouncer.",
+ "type": "array",
+ "default": []
Review comment:
```suggestion
"default": [],
"items": {
"$ref": "https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/v1.22.0-standalone-strict/volume-v1.json"
}
```
##########
File path: chart/tests/test_pgbouncer.py
##########
@@ -427,7 +453,7 @@ def test_default_exporter_secret(self):
def test_exporter_secret_with_overrides(self):
connection = self._get_connection(
{
- "pgbouncer": {"enabled": True},
+ "pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "disable"}},
Review comment:
```suggestion
"pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "require"}},
```
The intent of this test is to test with non-default values. It also needs to change in the assert below, but I can't suggest it via the UI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r766782204
##########
File path: chart/tests/test_pgbouncer.py
##########
@@ -427,7 +453,7 @@ def test_default_exporter_secret(self):
def test_exporter_secret_with_overrides(self):
connection = self._get_connection(
{
- "pgbouncer": {"enabled": True},
+ "pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "require"}},
Review comment:
As I said, I can't suggest it through the UI - it's too far from other changes.
I've just pushed a commit that fixes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r765784635
##########
File path: chart/values.schema.json
##########
@@ -3004,6 +3004,16 @@
],
"default": null
},
+ "extraVolumes": {
+ "description": "Mount additional volumes into PgBouncer.",
+ "type": "array",
+ "default": []
+ },
+ "extraVolumeMounts": {
+ "description": "Mount additional volumes into PgBouncer.",
Review comment:
Needs fixing typo .. volumeMounts instead of volumes ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r765785468
##########
File path: chart/values.schema.json
##########
@@ -3004,6 +3004,16 @@
],
"default": null
},
+ "extraVolumes": {
+ "description": "Mount additional volumes into PgBouncer.",
+ "type": "array",
+ "default": []
+ },
+ "extraVolumeMounts": {
+ "description": "Mount additional volumes into PgBouncer.",
Review comment:
or maybe not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r754492675
##########
File path: chart/tests/test_pgbouncer.py
##########
@@ -427,7 +455,7 @@ def test_default_exporter_secret(self):
def test_exporter_secret_with_overrides(self):
connection = self._get_connection(
{
- "pgbouncer": {"enabled": True},
+ "pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "disable"}},
Review comment:
```suggestion
"pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "require"}},
```
The intent of this test is to ensure non-defaults can be set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r756273288
##########
File path: chart/values.schema.json
##########
@@ -2685,6 +2685,16 @@
],
"default": null
},
+ "extraVolumes": {
+ "description": "Mount additional volumes into pgbouncer.",
+ "type": "array",
+ "default": []
+ },
+ "extraVolumeMounts": {
+ "description": "Mount additional volumes into pgbouncer.",
Review comment:
```suggestion
"description": "Mount additional volumes into PgBouncer.",
"type": "array",
"default": []
},
"extraVolumeMounts": {
"description": "Mount additional volumes into PgBouncer.",
```
```suggestion
"description": "Mount additional volumes into pgbouncer.",
"type": "array",
"default": []
},
"extraVolumeMounts": {
"description": "Mount additional volumes into pgbouncer.",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pgvishnuram commented on pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
pgvishnuram commented on pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#issuecomment-989816465
okay sure . let me re-verify them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r765783993
##########
File path: chart/tests/test_pgbouncer.py
##########
@@ -427,7 +455,7 @@ def test_default_exporter_secret(self):
def test_exporter_secret_with_overrides(self):
connection = self._get_connection(
{
- "pgbouncer": {"enabled": True},
+ "pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "require"}},
Review comment:
```suggestion
"pgbouncer": {"enabled": True, "metricsExporterSidecar": {"sslmode": "disable"}},
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil closed pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil closed pull request #19749:
URL: https://github.com/apache/airflow/pull/19749
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r756272679
##########
File path: chart/values.schema.json
##########
@@ -2754,6 +2764,17 @@
}
}
]
+ },
+ "sslmode": {
+ "description": "SSL mode for metricsExporterSidecar",
Review comment:
```suggestion
"description": "SSL mode for ``metricsExporterSidecar``",
```
```suggestion
"description": "SSL mode for metricsExporterSidecar",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19749: pgbouncer service enhancements
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#discussion_r765783355
##########
File path: chart/tests/test_pgbouncer.py
##########
@@ -266,6 +266,34 @@ def test_command_and_args_overrides_are_templated(self):
assert ["RELEASE-NAME"] == jmespath.search("spec.template.spec.containers[0].command", docs[0])
assert ["Helm"] == jmespath.search("spec.template.spec.containers[0].args", docs[0])
+ def test_should_add_extra_volume_and_extra_volume_mount(self):
+ docs = render_chart(
+ values={
+ "pgbouncer": {
+ "enabled": True,
+ "extraVolumes": [
+ {
+ "name": "pgbouncer-client-certificates",
+ "secret": {"secretName": "pgbouncer-client-tls-certificate"},
+ }
+ ],
+ "extraVolumeMounts": [
+ {"name": "pgbouncer-client-certificates", "mountPath": "/etc/pgbouncer/certs"}
+ ],
+ },
+ },
+ show_only=["templates/pgbouncer/pgbouncer-deployment.yaml"],
+ )
+
+ print(docs[0])
+
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19749:
URL: https://github.com/apache/airflow/pull/19749#issuecomment-989852056
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham merged pull request #19749: Chart: PgBouncer service enhancements
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #19749:
URL: https://github.com/apache/airflow/pull/19749
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org