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/12 13:25:27 UTC
[GitHub] [airflow] V0lantis opened a new pull request, #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
V0lantis opened a new pull request, #24999:
URL: https://github.com/apache/airflow/pull/24999
<!--
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: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Fix a scheduler `crashLoopBackOff` when `hostname_callable` was defined with a function which returned something different than the `hostname` command from linux.
In our production Airflow, I updated the [`hostname_callable`](https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#hostname-callable) config with [`airflow.utils.net.get_host_ip_address`](https://github.com/apache/airflow/blob/a3f2df0f45973ddb97990361fdc5caa256c175ca/airflow/utils/net.py#L46-L48). But this new config was making kubernetes to restart the scheduler pods because the [command defined in helm](https://github.com/apache/airflow/blob/451181f6f2efd8b5e6bd7807e84a17d48a4fe875/chart/templates/_helpers.yaml#L640-L641) was returning a "No alive jobs found."
I therefore updated the cli command jobs to allow the use of `--use-hostname-callable` in:
```bash
airflow jobs check --job-type SchedulerJob --use-hostname-callable
```
to use the `hostname_callable` that the user defined in its Airflow conf. Also updated the helm chart to with this new conf since I think it will be more reliable than to use linux `hostname` command.
--
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] V0lantis commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r923046518
##########
airflow/cli/commands/jobs_command.py:
##########
@@ -27,6 +27,7 @@ def check(args, session=None):
"""Checks if job(s) are still alive"""
if args.allow_multiple and not args.limit > 1:
raise SystemExit("To use option --allow-multiple, you must set the limit to a value greater than 1.")
+
Review Comment:
The `if args.hostname:` is indeed not necessary anymore. but the filter is working fine as it is now, or I am not sure to understand your point )
##########
airflow/cli/commands/jobs_command.py:
##########
@@ -27,6 +27,7 @@ def check(args, session=None):
"""Checks if job(s) are still alive"""
if args.allow_multiple and not args.limit > 1:
raise SystemExit("To use option --allow-multiple, you must set the limit to a value greater than 1.")
+
Review Comment:
The `if args.hostname:` is indeed not necessary anymore. but the filter is working fine as it is now, or I am not sure to understand your point :)
--
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] V0lantis commented on pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on PR #24999:
URL: https://github.com/apache/airflow/pull/24999#issuecomment-1193345816
> Just adding a default for hostname is a good idea, but it does break backwards compatibility. At the very least we'd need a way do not filter by hostname, and even then I'm not sure about this.
Tank you @jedcunningham. So am I going back with the previous argument I implemented at first (`use-hostname-callable` `local-only` or whatever we want to call 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] jedcunningham commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r950300409
##########
chart/templates/_helpers.yaml:
##########
@@ -638,7 +638,7 @@ Create the name of the cleanup service account to use
- -c
- |
CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \
- airflow jobs check --job-type SchedulerJob --hostname $(hostname)
+ airflow jobs check --job-type SchedulerJob --local
Review Comment:
This needs to be conditional on 2.4.0 or later.
--
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 #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r919073408
##########
airflow/cli/commands/jobs_command.py:
##########
@@ -36,6 +40,9 @@ def check(args, session=None):
query = query.filter(BaseJob.job_type == args.job_type)
if args.hostname:
query = query.filter(BaseJob.hostname == args.hostname)
+ if args.use_hostname_callable:
+ hostname = get_hostname()
+ query = query.filter(BaseJob.hostname == hostname)
Review Comment:
```suggestion
query = query.filter(BaseJob.hostname == get_hostname())
```
nit
##########
airflow/cli/cli_parser.py:
##########
@@ -897,6 +897,13 @@ def string_lower_type(val):
help="The hostname of job(s) that will be checked.",
)
+ARG_JOB_HOSTNAME_CALLABLE_FILTER = Arg(
+ ("--use-hostname-callable",),
Review Comment:
Not sure I love `use-hostname-callable`. Maybe `local-only`? @dstandish @ephraimbuddy
##########
chart/templates/_helpers.yaml:
##########
@@ -638,7 +638,7 @@ Create the name of the cleanup service account to use
- -c
- |
CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \
- airflow jobs check --job-type SchedulerJob --hostname $(hostname)
+ airflow jobs check --job-type SchedulerJob
Review Comment:
```suggestion
airflow jobs check --job-type SchedulerJob --use-hostname-callback
```
This also needs to be done conditionally based on `airflowVersion`.
--
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] V0lantis commented on pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on PR #24999:
URL: https://github.com/apache/airflow/pull/24999#issuecomment-1219905287
@jedcunningham, I rebased on `main`, remove the default param on `ARG_JOB_HOSTNAME_FILTER`. Hope it will suit you.
--
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] ephraimbuddy commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r921925687
##########
airflow/cli/cli_parser.py:
##########
@@ -897,6 +897,13 @@ def string_lower_type(val):
help="The hostname of job(s) that will be checked.",
)
+ARG_JOB_HOSTNAME_CALLABLE_FILTER = Arg(
+ ("--use-hostname-callable",),
Review Comment:
What about not adding a new arg but having the hostname arg default to `get_hostname`?
i.e
```python
ARG_JOB_HOSTNAME_FILTER = Arg(
("--hostname",),
default=get_hostname(),
type=str,
help="The hostname of job(s) that will be checked.",
)
```
The effect of this would be that the CLI check command would always check with hostname instead of none.
As per the name, what do you think about `--use-configured-hostname` or `--use-default-hostname`
--
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] ephraimbuddy commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r923170721
##########
airflow/cli/commands/jobs_command.py:
##########
@@ -27,6 +27,7 @@ def check(args, session=None):
"""Checks if job(s) are still alive"""
if args.allow_multiple and not args.limit > 1:
raise SystemExit("To use option --allow-multiple, you must set the limit to a value greater than 1.")
+
Review Comment:
Just a nit, we could have this:
```python
query = (
session.query(BaseJob)
.filter(BaseJob.state == State.RUNNING)
.filter(BaseJob.hostname == args.hostname)
.order_by(BaseJob.latest_heartbeat.desc())
)
```
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] ephraimbuddy commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r921926309
##########
chart/templates/_helpers.yaml:
##########
@@ -638,7 +638,7 @@ Create the name of the cleanup service account to use
- -c
- |
CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \
- airflow jobs check --job-type SchedulerJob --hostname $(hostname)
+ airflow jobs check --job-type SchedulerJob --use-hostname-callback
Review Comment:
```suggestion
airflow jobs check --job-type SchedulerJob --use-hostname-callable
```
--
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] V0lantis commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r919857312
##########
airflow/cli/cli_parser.py:
##########
@@ -897,6 +897,13 @@ def string_lower_type(val):
help="The hostname of job(s) that will be checked.",
)
+ARG_JOB_HOSTNAME_CALLABLE_FILTER = Arg(
+ ("--use-hostname-callable",),
Review Comment:
It's a nice suggestion @jedcunningham. I don't like so much `use-hostname-callable`, but I am not sure `local-only` would be very _telling_.
##########
airflow/cli/cli_parser.py:
##########
@@ -897,6 +897,13 @@ def string_lower_type(val):
help="The hostname of job(s) that will be checked.",
)
+ARG_JOB_HOSTNAME_CALLABLE_FILTER = Arg(
+ ("--use-hostname-callable",),
Review Comment:
It's a nice suggestion @jedcunningham. I don't like so much `use-hostname-callable` either, but I am not sure `local-only` would be very _telling_.
--
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] V0lantis commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r950707866
##########
chart/templates/_helpers.yaml:
##########
@@ -638,7 +638,7 @@ Create the name of the cleanup service account to use
- -c
- |
CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \
- airflow jobs check --job-type SchedulerJob --hostname $(hostname)
+ airflow jobs check --job-type SchedulerJob --local
Review Comment:
Yep, I did it in a previous commit and removed it. thanks. Re added it in dc45752cb037fb68e06de3e8e49e7ad59caefa00
--
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 pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24999:
URL: https://github.com/apache/airflow/pull/24999#issuecomment-1204533398
Yeah, I think we need to. I'm having trouble coming up with anything better than `--local` though.
--
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] V0lantis commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r919103339
##########
chart/templates/_helpers.yaml:
##########
@@ -638,7 +638,7 @@ Create the name of the cleanup service account to use
- -c
- |
CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \
- airflow jobs check --job-type SchedulerJob --hostname $(hostname)
+ airflow jobs check --job-type SchedulerJob
Review Comment:
True thank-you :+1:
--
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] ephraimbuddy commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r923012275
##########
airflow/cli/commands/jobs_command.py:
##########
@@ -27,6 +27,7 @@ def check(args, session=None):
"""Checks if job(s) are still alive"""
if args.allow_multiple and not args.limit > 1:
raise SystemExit("To use option --allow-multiple, you must set the limit to a value greater than 1.")
+
Review Comment:
If we want to use `default` we need to remove this condition, it's not useful again, right?
```python
if args.hostname:
query = query.filter(BaseJob.hostname == args.hostname)
```
and add a filter by hostname on the query below
--
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 pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24999:
URL: https://github.com/apache/airflow/pull/24999#issuecomment-1188301813
Just adding a default for hostname is a good idea, but it does break backwards compatibility. At the very least we'd need a way do not filter by hostname, and even then I'm not sure about this.
--
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 #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r921691395
##########
chart/templates/_helpers.yaml:
##########
@@ -638,7 +638,7 @@ Create the name of the cleanup service account to use
- -c
- |
CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \
- airflow jobs check --job-type SchedulerJob --hostname $(hostname)
+ airflow jobs check --job-type SchedulerJob
Review Comment:
Don't forget, this won't be released until 2.4.0, so you'll need to make using that over `--hostname` conditional.
--
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] V0lantis commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r921961952
##########
airflow/cli/cli_parser.py:
##########
@@ -897,6 +897,13 @@ def string_lower_type(val):
help="The hostname of job(s) that will be checked.",
)
+ARG_JOB_HOSTNAME_CALLABLE_FILTER = Arg(
+ ("--use-hostname-callable",),
Review Comment:
Sounds very good, thanks for the proposal. Going to implement this
--
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] V0lantis commented on pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on PR #24999:
URL: https://github.com/apache/airflow/pull/24999#issuecomment-1186240885
Thanks both for your review. I finally decided to implement the great suggestion of @ephraimbuddy, giving the `get_hostname` default argument to the `check` jobs command. I also created a `if ... else` condition to use this new functionality only from Airflow 2.4.0 release
--
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] V0lantis commented on a diff in pull request #24999: [FIX] Scheduler crashloopbackoff when using `hostname_callable`
Posted by GitBox <gi...@apache.org>.
V0lantis commented on code in PR #24999:
URL: https://github.com/apache/airflow/pull/24999#discussion_r921961160
##########
chart/templates/_helpers.yaml:
##########
@@ -638,7 +638,7 @@ Create the name of the cleanup service account to use
- -c
- |
CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \
- airflow jobs check --job-type SchedulerJob --hostname $(hostname)
+ airflow jobs check --job-type SchedulerJob --use-hostname-callback
Review Comment:
thanks :+1:
--
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