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 2022/07/05 14:26:50 UTC
[GitHub] [airflow] zartstrom opened a new pull request, #24846: Bind log server on worker to IPv6 address (#24755)
zartstrom opened a new pull request, #24846:
URL: https://github.com/apache/airflow/pull/24846
<!--
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 an existing issue, reference it using one of the following:
closes: #24755
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 changes, an 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 a newsfragment file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] potiuk commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1232157760
> To me it looks like Gunicorn only throws a sys.exit(1) on this error: https://github.com/benoitc/gunicorn/blob/master/gunicorn/sock.py#L198
Then it needs proper fix and detection. Just make it work automatically.
--
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] eladkal commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914050368
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
awesome - then maybe we just need to clarify it in the PR template
--
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] potiuk commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1232256621
Just make a PR :) discusing over the code where we can comment it directly and you can iterate is way better than disucssing copy&pasted code without context. The worst that is going to happen, we will agree to change the approach, but by doing "real code" change you will see immediately if it works, how it fits in etc.
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914053912
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
> Yep, either works. It has to be unique, and it's used to build a link in the release notes, otherwise it really doesn't matter.
Comment: This I did not realise. We do not actually have to use it to build the link - we can get it from the actual PR# (which is in the subject) - I am not sure if it matters that much though - if we are cool with having sometimes link to issue and sometimes to PR, I am cool as well. And it does help to use newsfragment ID in case somoene (me again) forget about disabling "rebase & merge" :P
--
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] eladkal commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914061211
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
already in progress
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914045274
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
Actually - It does not matter in the long run - it is important they are unique between merges. The newsfragements get deleted when release notes are prepared and the numbers are gone. In case of GitHub PR and Issue share the same number space (so there is never a PR with the same number that Issue). It's actually a bit easier to use Issue number than PR number (when you have an issue) because you actually KNOW it before you make a PR. Otherwsie the process of creating newsfragment is a bit cumbersome:
1) create a branch
2) make a draft PR
3) add newsframent and amand the PR (this is the first moment you know PR #)
4) push ameneded commit and un-draft PR
I think using issue id brings us no problems and helps with making the process simpler (when you have an issue)
1) create a branch with issue # newsfragment
2) create a PR
@jedcunningham - wdyt?
--
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] ashb commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r913934882
##########
airflow/utils/serve_logs.py:
##########
@@ -138,10 +142,14 @@ def serve_logs():
wsgi_app = create_app()
worker_log_server_port = conf.getint('logging', 'WORKER_LOG_SERVER_PORT')
- options = {
- 'bind': f"0.0.0.0:{worker_log_server_port}",
- 'workers': 2,
- }
+
+ # Make sure to have both an IPv4 and an IPv6 interface.
+ # Gunicorn can bind multiple addresses, see https://docs.gunicorn.org/en/stable/settings.html#bind.
+ options = [
+ GunicornOption("bind", f"0.0.0.0:{worker_log_server_port}"),
+ GunicornOption("bind", f"[::]:{worker_log_server_port}"),
Review Comment:
Does this cause any problems if ipv6 isn't enabled?
--
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] saimon46 commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
saimon46 commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1231578766
Hello! We have Airflow version 2.3.4 and this is not working if in the machine IPv6 is not available or disabled. We end up having an error during worker startup and the IPv4 bind doesn't even start because the entire gunicorn process fails. The bind should be configurable and not hardcoded in the code.
```
[2022-08-30 12:09:00 +0000] [1754859] [INFO] Starting gunicorn 20.1.0
[2022-08-30 12:09:00 +0000] [1754859] [ERROR] Retrying in 1 second.
[2022-08-30 12:09:01 +0000] [1754859] [ERROR] Retrying in 1 second.
[2022-08-30 12:09:02 +0000] [1754859] [ERROR] Retrying in 1 second.
[2022-08-30 12:09:03 +0000] [1754859] [ERROR] Retrying in 1 second.
[2022-08-30 12:09:04,388: INFO/MainProcess] Connected to amqp://**:**@**:**//
[2022-08-30 12:09:04,403: INFO/MainProcess] mingle: searching for neighbors
[2022-08-30 12:09:04 +0000] [1754859] [ERROR] Retrying in 1 second.
[2022-08-30 12:09:05,457: INFO/MainProcess] mingle: sync with 3 nodes
[2022-08-30 12:09:05,457: INFO/MainProcess] mingle: sync complete
[2022-08-30 12:09:05,486: INFO/MainProcess] celery@** ready.
[2022-08-30 12:09:05,543: INFO/MainProcess] Events of group {task} enabled by remote.
[2022-08-30 12:09:05 +0000] [1754859] [ERROR] Can't connect to ('::', 8793)
```
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914045274
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
Actually - It does not matter in the long run - it is important they are unique between merges. The newsfragements get deleted when release notes are prepared and the numbers are gone. In case of GitHub - PRs and Issues share the same `issue/pr number` space (so there is never a PR with the same number as another Issue). It's actually a bit easier to use Issue number than PR number (when you have an issue) because you actually KNOW it before you make a PR. Otherwsie the process of creating newsfragment is a bit cumbersome:
1) create a branch
2) make a draft PR
3) add newsframent and amend the PR (this is the first moment you know PR #)
4) push ameneded commit and un-draft PR
I think using issue id brings us no problems and helps with making the process simpler (when you have an issue)
1) create a branch with issue # newsfragment
2) create a PR
I believe we should be cool with either PR# or Issue# - both will work just fine.
@jedcunningham - wdyt?
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914054360
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
> awesome - then maybe we just need to clarify it in the PR template
As usual @eladkal ... PRs are most welcome :P :P :P
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914045274
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
Actually - It does not matter in the long run - it is important they are unique between merges. The newsfragements get deleted when release notes are prepared and the numbers are gone. In case of GitHub - PRs and Issues share the same `issue/pr number` space (so there is never a PR with the same number as another Issue). It's actually a bit easier to use Issue number than PR number (when you have an issue) because you actually KNOW it before you make a PR. Otherwsie the process of creating newsfragment is a bit cumbersome:
1) create a branch
2) make a draft PR
3) add newsframent and amand the PR (this is the first moment you know PR #)
4) push ameneded commit and un-draft PR
I think using issue id brings us no problems and helps with making the process simpler (when you have an issue)
1) create a branch with issue # newsfragment
2) create a PR
@jedcunningham - wdyt?
--
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] notatallshaw-work commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
notatallshaw-work commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1233421255
This is all new to me, but yes I believe that's the case as long as the OS supports [dualstack](https://docs.python.org/3/library/socket.html#socket.has_dualstack_ipv6) and IPV6_V6ONLY has not been set on the socket options:
* https://man7.org/linux/man-pages/man7/ipv6.7.html
* https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ipv6-socket-options
Thus it would be actually simpler to change the code to:
```python
if socket.has_dualstack_ipv6():
bind_option = GunicornOption("bind", f"[::]:{worker_log_server_port}")
else:
bind_option = GunicornOption("bind", f"0.0.0.0:{worker_log_server_port}")
options = [
bind_option,
GunicornOption("workers", 2),
]
```
Note though that `socket.has_dualstack_ipv6` was introduced in Python 3.8, if Airflow still supports Python 3.7 then this code may need to be vendored: https://github.com/python/cpython/blob/v3.10.6/Lib/socket.py#L853
--
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] potiuk commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1231615586
Feel free to make a PR to change it @saimon46. It is in `serve_logs.py`. You are likely the best person to test it and provide a fix as you will be able to test if the fix fixes your problem (And you can apply it locally before Airlfow release so there are mutual benefits).
Looking forward to your PR - happy to review 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] boring-cyborg[bot] commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1178175536
Awesome work, congrats on your first merged pull request!
--
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] eladkal commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914036784
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
I think the number here is wrong? It should be PR number not issue number I believe
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914045274
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
Actually - It does not matter in the long run - it is important they are unique between merges. The newsfragements get deleted when release notes are prepared and the numbers are gone. In case of GitHub - PRs and Issues share the same `issue/pr number` space (so there is never a PR with the same number as another Issue). It's actually a bit easier to use Issue number than PR number (when you have an issue) because you actually KNOW it before you make a PR. Otherwsie the process of creating newsfragment is a bit cumbersome:
1) create a branch
2) make a draft PR
3) add newsframent and amend the PR (this is the first moment you know PR #)
4) push ameneded commit and un-draft PR
I think using issue id brings us no problems and helps with making the process simpler (when you have an issue)
1) create a branch with issue # newsfragment
2) create a PR
@jedcunningham - wdyt?
--
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] boring-cyborg[bot] commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1175126119
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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 diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914049503
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
Yep, either works. It has to be unique, and it's used to build a link in the release notes, otherwise it really doesn't matter.
--
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] saimon46 commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
saimon46 commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1232516745
I confirm that the test to check that IPv6 is bindable is the best approach. In our case the function that @notatallshaw-work posted works fine. Around that function we should add or not the IPv6 bind in the gunicorn options.
@notatallshaw-work are you already working on a PR?
--
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] notatallshaw-work commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
notatallshaw-work commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1233139654
> I confirm that the test to check that IPv6 is bindable is the best approach. In our case the function that @notatallshaw-work posted works fine. Around that function we should add or not the IPv6 bind in the gunicorn options. @notatallshaw-work are you already working on a PR?
I am discussing with my company on the process to contribute to open source projects, we would like to but we just need to make sure we get the policy right on our end. It may be a week or so until we have it ironed out, feel free to use the above code in any way to submit a PR yourself if you would like it to land quicker than I can provide one.
Also I was playing around with a more generic version, with the idea that someone in the future *might* have only IPv6 enabled and IPv4 disabled:
```python
import socket
def get_bindable_addresses(port=8000):
address_list = []
addrinfo = socket.getaddrinfo(None, port, 0, socket.SOCK_STREAM, 0, socket.AI_PASSIVE)
for addr in addrinfo:
family = addr[0]
test_address = addr[-1][:2]
try:
socket.socket(family=family).bind(test_address)
except OSError as e:
print(f'WARN: Failed to bind {test_address} using IP family {family}: {e!s}')
continue
if family == socket.AF_INET:
address_list.append(f'{test_address[0]}:{test_address[1]}')
elif family == socket.AF_INET6:
address_list.append(f'[{test_address[0]}]:{test_address[1]}')
else:
print(f'WARN: Unrecognized IP address family {family!r}')
return address_list
bindable_addresses = get_bindable_addresses()
if not bindable_addresses:
raise RuntimeError(...)
...
```
But maybe that's overkill, whatever you think is best if you submit a PR.
--
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] potiuk commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1231791442
There is no particular reason why you should configure it - you can configure it by enabling/disabling the IPV6/IPV4 in the deployment of yours and Airflow should use whatever is available. There is no point in having two places where you can configure it (deployment and airflow).
--
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] potiuk commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1231789365
The best will be handle error gracefully simply rather than fail (this was the original intention and we believed it worked this way).. Adding new configuration should only be neeeded if absolutely necessary.
--
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] saimon46 commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
saimon46 commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1231646705
@potiuk I perfectly agree on this. I'm going to work on a PR but before I start I'm writing here 2 possible solutions for this problem:
- airflow logging configuration to define the bind address, same as the port. Maybe bind it to localhost is some cases can be useful.
- simple airflow logging configuration that enables or disables IPv6 compatibility on gunicorn.
- automatically discover if the machine has IPv6 interface and if it's enabled and eventually bind gunicorn on it.
What do you thing would be the best? The first gives more flexibility to the sysadmin same as the second, the third is automatic yes, but still you loose in configuration flexibility.
--
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] notatallshaw-work commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
notatallshaw-work commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1232250864
One approach I guess is to test if it's possible to bind to IPv6?
I played around with the socket library and came up with this test(tested on both machines that can bind and can not bind to IPv6):
```python
import socket
def ipv6_bindable(port=8000):
try:
socket.socket(family=socket.AF_INET6).bind(('::', port))
except OSError as e:
if e.args[0] == 97: # Address family not supported by protocol
return False
raise
return 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
[GitHub] [airflow] potiuk merged pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24846:
URL: https://github.com/apache/airflow/pull/24846
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914050160
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
BTW. In `pip` they even follow a bit more strict approach (but I actually started to like ours where we only add newsfragments when we really need it) - whenever there is an "insignificant" change - they have a semi-generated unique empty random number for newsfragment - so practically, the number we have for newsfragment does not have to be connected with issue or pr or anything else - it is enough that it is unique until it gets deleted at release time.
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914045274
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
Actually - It does not matter in the long run - it is important they are unique between merges. The newsfragements get deleted when release notes are prepared and the numbers are gone. In case of GitHub - PRs and Issues share the same `issue/pr number` space (so there is never a PR with the same number as another Issue). It's actually a bit easier to use Issue number than PR number (when you have an issue) because you actually KNOW it before you make a PR. Otherwsie the process of creating newsfragment is a bit cumbersome:
1) create a branch
2) make a draft PR
3) add newsframent and amend the PR (this is the first moment you know PR #)
4) push ameneded commit and un-draft PR
I think using issue id brings us no problems and helps with making the process simpler (when you have an issue)
1) create a branch with issue # newsfragment
2) create a PR
I believe we should be cool with either PR or Issue#
@jedcunningham - wdyt?
--
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] potiuk commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r914045274
##########
newsfragments/24755.improvement.rst:
##########
@@ -0,0 +1 @@
+Log server on worker binds IPv6 interface.
Review Comment:
Actually - It does not matter in the long run - it is important they are unique between merges. The newsfragements get deleted when release notes are prepared and the numbers are gone. In case of GitHub - PRs and Issues share the same `issue/pr number` space (so there is never a PR with the same number that Issue). It's actually a bit easier to use Issue number than PR number (when you have an issue) because you actually KNOW it before you make a PR. Otherwsie the process of creating newsfragment is a bit cumbersome:
1) create a branch
2) make a draft PR
3) add newsframent and amand the PR (this is the first moment you know PR #)
4) push ameneded commit and un-draft PR
I think using issue id brings us no problems and helps with making the process simpler (when you have an issue)
1) create a branch with issue # newsfragment
2) create a PR
@jedcunningham - wdyt?
--
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] potiuk commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1233398354
No worries. It can wait :). One benefit of the "auto-configuration" is that it might be treated as a bug-fix so it might be added any time (and I anticipate one-two bugfix releases following 2.4.0 in a few weeks time.
I believe when you bind to IPv6 address and IPV4 is there, the OS will automatically map IPV4 port to IPV6 port (unless you specify `IPV6 bind only`) - this is at least what https://www.reddit.com/r/codehunter/comments/tgnalu/dual_ipv4_and_ipv6_support_in_flask_applications/ suggest.
So I think we can simplify it all by simply binding to IPV6 if it is bindable and IPV4 if not. I think we cannot "just" bind to IPV6 because if - for whatever reason - IPV6 support is not enabled at all, it will fail.
--
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] notatallshaw-work commented on pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
notatallshaw-work commented on PR #24846:
URL: https://github.com/apache/airflow/pull/24846#issuecomment-1232156413
FYI also affected by this.
@potiuk Are you suggesting a preferred PR would be to catch this exception and try binding to IPv4 only?
To me it looks like Gunicorn only throws a `sys.exit(1)` on this error: https://github.com/benoitc/gunicorn/blob/master/gunicorn/sock.py#L198
Possible to catch `SystemExit` but it's not the best behavior as you can't tell if that happened for some other reason.
--
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] saimon46 commented on a diff in pull request #24846: Bind log server on worker to IPv6 address (#24755)
Posted by GitBox <gi...@apache.org>.
saimon46 commented on code in PR #24846:
URL: https://github.com/apache/airflow/pull/24846#discussion_r958431564
##########
airflow/utils/serve_logs.py:
##########
@@ -138,10 +142,14 @@ def serve_logs():
wsgi_app = create_app()
worker_log_server_port = conf.getint('logging', 'WORKER_LOG_SERVER_PORT')
- options = {
- 'bind': f"0.0.0.0:{worker_log_server_port}",
- 'workers': 2,
- }
+
+ # Make sure to have both an IPv4 and an IPv6 interface.
+ # Gunicorn can bind multiple addresses, see https://docs.gunicorn.org/en/stable/settings.html#bind.
+ options = [
+ GunicornOption("bind", f"0.0.0.0:{worker_log_server_port}"),
+ GunicornOption("bind", f"[::]:{worker_log_server_port}"),
Review Comment:
Yes, it breaks the gunicorn startup with a failure. Below more details https://github.com/apache/airflow/pull/24846#issuecomment-1231578766
--
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