You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ferruzzi (via GitHub)" <gi...@apache.org> on 2024/02/08 20:46:50 UTC
[PR] [DRAFT] Unhealthy Executor Banner [airflow]
ferruzzi opened a new pull request, #37264:
URL: https://github.com/apache/airflow/pull/37264
Note: This is a draft so I can get some help, it includes some debug prints, etc which will be removed.
I wanted to add a banner to the UI for when the executor is unhealthy, similar to the existing banner for "The scheduler does not appear to be running." The debug log messages show that executor.is_healthy is updating as expected, but the banner is only getting loaded (or not) based on the initial state of he executor, which means that when executor.is_healthy updates, it does not trigger the main.html code block to get refreshed. The banner for the scheduler gets displayed automatically when the scheduler process is killed, so I am missing something that links the executor.is_healthy value change, I think? Any suggestions?
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
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/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.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+Improvement+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` or `{issue_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
Re: [PR] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #37264:
URL: https://github.com/apache/airflow/pull/37264#discussion_r1484718128
##########
airflow/executors/base_executor.py:
##########
@@ -134,6 +135,9 @@ def __repr__(self):
def start(self): # pragma: no cover
"""Executors may need to get things started."""
+ def check_health(self, verbose: bool = True):
Review Comment:
Yeah, if we go with option 2, then we keep the method and the contract being that it returns whether or not the executor is healthy (bool would work great) and then the base executor class persists that in the field.
--
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] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #37264:
URL: https://github.com/apache/airflow/pull/37264#discussion_r1486511527
##########
tests/providers/amazon/aws/executors/ecs/test_ecs_executor.py:
##########
@@ -103,16 +105,16 @@ def mock_config():
@pytest.fixture
def set_env_vars():
Review Comment:
Here and many of the changes below: I just moved the repeatedly reused prefix into a constant. Lots of the uses were getting pushed to multiple lines because the prefix was so long and it also makes sense to make that DRY.
--
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] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37264:
URL: https://github.com/apache/airflow/pull/37264#issuecomment-1977191932
In hindsight, this was a terrible approach to solving this issue. Going to close this and work on a better solution.
--
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] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi closed pull request #37264: [DRAFT] Unhealthy Executor Banner
URL: https://github.com/apache/airflow/pull/37264
--
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] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #37264:
URL: https://github.com/apache/airflow/pull/37264#discussion_r1484622215
##########
airflow/www/views.py:
##########
@@ -693,6 +693,8 @@ class AirflowBaseView(BaseView):
extra_args["sqlite_warning"] = settings.engine.dialect.name == "sqlite"
if not executor.is_production:
extra_args["production_executor_warning"] = executor.__name__
+ if executor.is_healthy is False:
+ extra_args["unhealthy_executor"] = executor.__name__ or "Executor"
Review Comment:
This is getting set somewhat statically as a class attribute here. Do this check closer to the runtime method that renders the particular page(s) you're interested in.
--
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] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #37264:
URL: https://github.com/apache/airflow/pull/37264#discussion_r1486509364
##########
airflow/www/views.py:
##########
@@ -704,6 +704,12 @@ def render_template(self, *args, **kwargs):
if TriggererJobRunner.is_needed():
kwargs["triggerer_job"] = lazy_object_proxy.Proxy(TriggererJobRunner.most_recent_job)
+ executor, _ = ExecutorLoader.import_default_executor_cls()
+ try:
+ executor().check_health()
Review Comment:
I do not like this. I feel like there must be a way to get the current health without instantiating a new executor class.
--
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] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #37264:
URL: https://github.com/apache/airflow/pull/37264#discussion_r1484701756
##########
airflow/executors/base_executor.py:
##########
@@ -134,6 +135,9 @@ def __repr__(self):
def start(self): # pragma: no cover
"""Executors may need to get things started."""
+ def check_health(self, verbose: bool = True):
Review Comment:
Are you thinking the method should just return the bool directly?
--
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] [DRAFT] Unhealthy Executor Banner [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #37264:
URL: https://github.com/apache/airflow/pull/37264#discussion_r1484626120
##########
airflow/executors/base_executor.py:
##########
@@ -211,6 +215,14 @@ def heartbeat(self) -> None:
else:
open_slots = self.parallelism - len(self.running)
+ # Call child class health_check method.
+ try:
+ # Update the executor's health status.
+ self.check_health(verbose=True)
+ except AirflowException:
+ # Do not stop execution for an unhealthy executor as it can recover.
+ pass
Review Comment:
I wonder if it should be up to the concrete classes not to throw in that case?
##########
airflow/executors/base_executor.py:
##########
@@ -134,6 +135,9 @@ def __repr__(self):
def start(self): # pragma: no cover
"""Executors may need to get things started."""
+ def check_health(self, verbose: bool = True):
Review Comment:
Do we need the "public" field and the method?
We could either have just the field and it's up to the concrete executors to keep it updated how they see fit. Or we can keep this method as well and then the concrete classes just implement it and we set the (newly private) field to result of this method call in the heartbeat below.
I just don't want these two to get out of sync.
--
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