You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "andormarkus (via GitHub)" <gi...@apache.org> on 2024/08/01 08:56:46 UTC
[PR] fix: Fixing Helm chart flower ingress service reference [airflow]
andormarkus opened a new pull request, #41179:
URL: https://github.com/apache/airflow/pull/41179
closes #41175
### What happened
We are sub charting the official airflow chart and for this reason we need to use `fullnameOverride: "airflow"` settings to achieve the desired k8s resource names. However when we are using it the flower ingress becomes invalid because it want's to connect to non existing service name. Root cause are in the ingress.yaml. The webserver ingress can handle it with no problem.
Incorrect code section
`flower-ingress.yaml` - [code section](https://github.com/apache/airflow/blob/daccc75b064c735636027b20f3edb82da69ffab1/chart/templates/flower/flower-ingress.yaml#L75)
```yaml
spec:
rules:
- http:
paths:
- backend:
service:
name: {{ $.Release.Name }}-flower
port:
name: flower-ui
```
While in the correct code section
`webserver-ingress.yaml` - [code section](https://github.com/apache/airflow/blob/daccc75b064c735636027b20f3edb82da69ffab1/chart/templates/webserver/webserver-ingress.yaml#L83)
```yaml
spec:
rules:
- http:
paths:
- backend:
service:
name: {{ $fullname }}-webserver
port:
name: airflow-ui
```
To overcome this issue we need to run this command on every deployment
```bash
kubectl patch ingress airflow-flower-ingress \
--namespace "airflow" \
--type='json' \
-p='[{"op": "replace", "path": "/spec/rules/0/http/paths/0/backend/service/name", "value":"airflow-flower"}]'
```
### What you think should happen instead
The following code should be updated to match how webserver ingress works
`flower-ingress.yaml` - [code section](https://github.com/apache/airflow/blob/daccc75b064c735636027b20f3edb82da69ffab1/chart/templates/flower/flower-ingress.yaml#L75)
```yaml
spec:
rules:
- http:
paths:
- backend:
service:
name: {{ $fullname }}-flower
port:
name: flower-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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "andormarkus (via GitHub)" <gi...@apache.org>.
andormarkus commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2343275404
Hi @romsharon98
Sorry for the late reply I was on extended vacation.
I have run into to a problem when I added the `"fullnameOverride": "test-basic"` it looks like the `chart/templates/_helpers.yaml` is not executed when I run the tests.
Here are my tests
`helm_tests/webserver/test_ingress_web.py`
```python
def test_backend_service_name(self):
docs = render_chart(
values={"ingress": {"web": {"enabled": True}}},
show_only=["templates/webserver/webserver-ingress.yaml"],
)
assert "release-name-webserver" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
def test_backend_service_name_with_fullname_override(self):
docs = render_chart(
values={"fullnameOverride": "test-basic",
"ingress": {"web": {"enabled": True}}},
show_only=["templates/webserver/webserver-ingress.yaml"],
)
assert "test-basic-webserver" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
```
`helm_tests/webserver/test_ingress_flower.py`
```python
def test_backend_service_name(self):
docs = render_chart(
values={"ingress": {"enabled": True}, "flower": {"enabled": True}},
show_only=["templates/flower/flower-ingress.yaml"],
)
assert "release-name-flower" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
def test_backend_service_name_with_fullname_override(self):
docs = render_chart(
values={"fullnameOverride": "test-basic",
"ingress": {"enabled": True}, "flower": {"enabled": True}},
show_only=["templates/flower/flower-ingress.yaml"],
)
assert "test-basic-flower" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
```
I run the test using the following command
```shell
breeze testing helm-tests --helm-test-package webserver
```
```shell
FAILED helm_tests/webserver/test_ingress_flower.py::TestIngressFlower::test_backend_service_name_with_fullname_override - AssertionError: assert equals failed
'test-basic-flower' 'release-name-flower'
FAILED helm_tests/webserver/test_ingress_web.py::TestIngressWeb::test_backend_service_name_with_fullname_override - AssertionError: assert equals failed
'test-basic-webserver' 'release-name-webserver'
```
```
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "andormarkus (via GitHub)" <gi...@apache.org>.
andormarkus commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2262647209
Can someone help my why this test failing?
[https://github.com/apache/airflow/actions/runs/10195595534/job/28204650305?pr=41179](https://github.com/apache/airflow/actions/runs/10195595534/job/28204650305?pr=41179)
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2373801674
> Hi @romsharon98
>
>
>
> When is the helm chart 1.16.0 scheduled for release?
>
>
>
> Thanks,
>
> Andor
I think @jedcunningham will know better
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2264911838
> Can someone help my why this test failing? https://github.com/apache/airflow/actions/runs/10195595534/job/28204650305?pr=41179
rebase solved 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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "andormarkus (via GitHub)" <gi...@apache.org>.
andormarkus commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2264978663
Hi @romsharon98
I'm happy to add helm test for the modified ingress object, however the helm test framework is fully missing from the helm chart and we need to add the automated test runs to the CI/CD pipelines as well.
Helm testing [documentation](https://helm.sh/docs/topics/chart_tests/),
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2349915115
> Hi @romsharon98
>
> Sorry for the late reply I was on extended vacation.
>
> I have run into to a problem when I added the `"fullnameOverride": "test-basic"` it looks like the `chart/templates/_helpers.yaml` is not executed when I run the tests.
>
> Here are my tests `helm_tests/webserver/test_ingress_web.py`
>
> ```python
> def test_backend_service_name(self):
> docs = render_chart(
> values={"ingress": {"web": {"enabled": True}}},
> show_only=["templates/webserver/webserver-ingress.yaml"],
> )
>
> assert "release-name-webserver" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
>
> def test_backend_service_name_with_fullname_override(self):
> docs = render_chart(
> values={"fullnameOverride": "test-basic",
> "ingress": {"web": {"enabled": True}}},
> show_only=["templates/webserver/webserver-ingress.yaml"],
>
> )
>
> assert "test-basic-webserver" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
> ```
>
> `helm_tests/webserver/test_ingress_flower.py`
>
> ```python
> def test_backend_service_name(self):
> docs = render_chart(
> values={"ingress": {"enabled": True}, "flower": {"enabled": True}},
> show_only=["templates/flower/flower-ingress.yaml"],
> )
>
> assert "release-name-flower" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
>
> def test_backend_service_name_with_fullname_override(self):
> docs = render_chart(
> values={"fullnameOverride": "test-basic",
> "ingress": {"enabled": True}, "flower": {"enabled": True}},
> show_only=["templates/flower/flower-ingress.yaml"],
>
> )
>
> assert "test-basic-flower" == jmespath.search("spec.rules[0].http.paths[0].backend.service.name", docs[0])
> ```
>
> I run the test using the following command
>
> ```shell
> breeze testing helm-tests --helm-test-package webserver
> ```
>
> ```shell
> FAILED helm_tests/webserver/test_ingress_flower.py::TestIngressFlower::test_backend_service_name_with_fullname_override - AssertionError: assert equals failed
> 'test-basic-flower' 'release-name-flower'
> FAILED helm_tests/webserver/test_ingress_web.py::TestIngressWeb::test_backend_service_name_with_fullname_override - AssertionError: assert equals failed
> 'test-basic-webserver' 'release-name-webserver'
> ```
for this to be applied you need to set `useStandardNaming` to be `true`
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "andormarkus (via GitHub)" <gi...@apache.org>.
andormarkus commented on code in PR #41179:
URL: https://github.com/apache/airflow/pull/41179#discussion_r1774736678
##########
helm_tests/webserver/test_ingress_web.py:
##########
@@ -200,3 +200,27 @@ def test_can_ingress_hosts_be_templated(self):
"cc.example.com",
"dd.example.com",
] == jmespath.search("spec.rules[*].host", docs[0])
+
+ def test_backend_service_name(self):
+ docs = render_chart(
+ values={"ingress": {"web": {"enabled": True}}},
+ show_only=["templates/webserver/webserver-ingress.yaml"],
+ )
+
+ assert "release-name-webserver" == jmespath.search(
+ "spec.rules[0].http.paths[0].backend.service.name", docs[0]
+ )
+
+ def test_backend_service_name_with_fullname_override(self):
+ docs = render_chart(
Review Comment:
Hi @romsharon98,
Thank you for your input!
Due to our company's design guidelines, we had to create a sub-chart from the official Airflow chart. This causes all resources to inherit the `release_name` as a prefix, which is something we want to avoid. To ensure clearer and more manageable Kubernetes resource names, we use the `fullnameOverride` parameter to customize the resource naming.
The goal of the `test_backend_service_name_with_fullname_override` is to verify that when we don't want the `release_name` as a prefix, our resources instead use the `fullnameOverride` as the desired prefix.
Hope this clarifies the approach!
##########
helm_tests/webserver/test_ingress_web.py:
##########
@@ -200,3 +200,27 @@ def test_can_ingress_hosts_be_templated(self):
"cc.example.com",
"dd.example.com",
] == jmespath.search("spec.rules[*].host", docs[0])
+
+ def test_backend_service_name(self):
+ docs = render_chart(
+ values={"ingress": {"web": {"enabled": True}}},
+ show_only=["templates/webserver/webserver-ingress.yaml"],
+ )
+
+ assert "release-name-webserver" == jmespath.search(
+ "spec.rules[0].http.paths[0].backend.service.name", docs[0]
+ )
+
+ def test_backend_service_name_with_fullname_override(self):
+ docs = render_chart(
Review Comment:
Hi @romsharon98,
Due to our company's design guidelines, we had to create a sub-chart from the official Airflow chart. This causes all resources to inherit the `release_name` as a prefix, which is something we want to avoid. To ensure clearer and more manageable Kubernetes resource names, we use the `fullnameOverride` parameter to customize the resource naming.
The goal of the `test_backend_service_name_with_fullname_override` is to verify that when we don't want the `release_name` as a prefix, our resources instead use the `fullnameOverride` as the desired prefix.
Hope this clarifies the approach!
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on code in PR #41179:
URL: https://github.com/apache/airflow/pull/41179#discussion_r1759913022
##########
helm_tests/webserver/test_ingress_web.py:
##########
@@ -200,3 +200,27 @@ def test_can_ingress_hosts_be_templated(self):
"cc.example.com",
"dd.example.com",
] == jmespath.search("spec.rules[*].host", docs[0])
+
+ def test_backend_service_name(self):
+ docs = render_chart(
+ values={"ingress": {"web": {"enabled": True}}},
+ show_only=["templates/webserver/webserver-ingress.yaml"],
+ )
+
+ assert "release-name-webserver" == jmespath.search(
+ "spec.rules[0].http.paths[0].backend.service.name", docs[0]
+ )
+
+ def test_backend_service_name_with_fullname_override(self):
+ docs = render_chart(
Review Comment:
Sorry for confusing you.
I think for checking we don't break change here we need to apply the tests to use `Release.name` and we can achieve it by sending the function `render_chart` in the first parameter the name of the release like that:
`render_chart('test-basic', values=..)`
and we can remove `fullnameOverride` and `useStandardNaming` from the values.
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "andormarkus (via GitHub)" <gi...@apache.org>.
andormarkus commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2351167355
Hi @romsharon98
All checks are now passing.
Thanks,
Andor
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "andormarkus (via GitHub)" <gi...@apache.org>.
andormarkus commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2373732178
Hi @romsharon98
When is the helm chart 1.16.0 scheduled for release?
Thanks,
Andor
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2265016260
You can add your tests [here](https://github.com/apache/airflow/blob/main/helm_tests/other/test_flower.py)
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 merged PR #41179:
URL: https://github.com/apache/airflow/pull/41179
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "andormarkus (via GitHub)" <gi...@apache.org>.
andormarkus commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2350272360
Hi @romsharon98
Thank you so much the help. Now my tests are passing.
Please let me know If I need to more test of where to adjust the code?
Thanks,
Andor
--
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
Re: [PR] fix: Fixing Helm chart flower ingress service reference [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on PR #41179:
URL: https://github.com/apache/airflow/pull/41179#issuecomment-2350811178
> Hi @romsharon98
>
>
>
> Thank you so much the help. Now my tests are passing.
>
> Please let me know If I need to more test of where to adjust the code?
>
>
>
> Thanks,
>
> Andor
You have static checks fail.
Better work with pre commit to see those errors before you commit.
You can see how to install it here
https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#installing-pre-commit-hooks
--
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