You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2023/03/05 10:32:34 UTC
[GitHub] [airflow] amoghrajesh opened a new pull request, #29929: Adding more information to kubernetes executor logs
amoghrajesh opened a new pull request, #29929:
URL: https://github.com/apache/airflow/pull/29929
<!--
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/
-->
Kubernetes executor logs do not contain a lot of information that can be back tracked to narrow down the issue. To make debugging easier, this PR adds the key annotations to some logger lines which can be useful to debug issues quicker.
The annotations contain crucial information regarding a k8s log, for ex the structure can involve:
```
{
"dag_id": "dag",
"task_id": "task",
"run_id": "run_id",
"try_number": "1",
"execution_date": None,
}
```
closes: #18329
---
**^ 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+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
[GitHub] [airflow] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1195846770
##########
airflow/config_templates/config.yml:
##########
@@ -2671,6 +2671,13 @@ kubernetes_executor:
type: string
example: '{ "total": 3, "backoff_factor": 0.5 }'
default: ""
+ detailed_executor_logs:
+ description: |
+ Flag to have more details in kubernetes executor pod logs
Review Comment:
Can this be more specific than “more details”? What details are they? Why/when would people not want those details?
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1201508020
##########
tests/executors/test_kubernetes_executor.py:
##########
@@ -1162,6 +1166,39 @@ def test_supports_pickling(self):
def test_supports_sentry(self):
assert not KubernetesExecutor.supports_sentry
+ def test_annotations_for_logging_task_metadata(self):
+ annotations_test = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ with mock.patch.dict(
+ os.environ, {"AIRFLOW__KUBERNETES_EXECUTOR__LOGS_TASK_METADATA": "True"}, clear=True
+ ):
+ expected_annotations = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ annotations_actual = annotations_for_logging_task_metadata(annotations_test)
+ assert annotations_actual == expected_annotations
+
+ def test_annotations_for_logging_task_metadata_fallback(self):
+ annotations_test = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ with mock.patch.dict(
+ os.environ, {"AIRFLOW__KUBERNETES_EXECUTOR__LOGS_TASK_METADATA": "False"}, clear=True
+ ):
+ expected_annotations = "<omitted>"
+ annotations_actual = annotations_for_logging_task_metadata(annotations_test)
+ assert annotations_actual == expected_annotations
+
Review Comment:
@uranusjr I didn't quite get what you meant by `You also want to patch both the true and false cases because it’s not deterministic in the CI which the config is set to when the test is run.`
I have patched both the test cases, True on line 1177 and False on line 1196
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198535840
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -216,15 +233,27 @@ def process_status(
resource_version: str,
event: Any,
) -> None:
+ annotations_string = annotations_to_str(annotations)
"""Process status response."""
if status == "Pending":
if event["type"] == "DELETED":
- self.log.info("Event: Failed to start pod %s", pod_name)
+ if get_logs_task_metadata():
+ self.log.info(
+ "Event: Failed to start pod %s, annotations: %s", pod_name, annotations_string
+ )
+ else:
+ self.log.info("Event: Failed to start pod %s", pod_name)
Review Comment:
Or maybe do this if-else in `annotations_to_st` directly? (If we give this function a better name.)
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198613576
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -216,15 +233,27 @@ def process_status(
resource_version: str,
event: Any,
) -> None:
+ annotations_string = annotations_to_str(annotations)
"""Process status response."""
if status == "Pending":
if event["type"] == "DELETED":
- self.log.info("Event: Failed to start pod %s", pod_name)
+ if get_logs_task_metadata():
+ self.log.info(
+ "Event: Failed to start pod %s, annotations: %s", pod_name, annotations_string
+ )
+ else:
+ self.log.info("Event: Failed to start pod %s", pod_name)
Review Comment:
Logs should prioritise simplcity over making things pretty.
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197442386
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
The general direction is to reduce _import time_ effects. If you must make it somewhat global, wrap the call in a function and use `@cache`.
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1135091787
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
I agree with what @hterik has to say. I think we can keep them together. More information doesn't do any harm in my opinion. What do you say @jedcunningham ?
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198544762
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +73,18 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+if sys.version_info >= (3, 9):
+ from functools import cache
+else:
+ from functools import lru_cache
+
+ cache = lru_cache(maxsize=None)
Review Comment:
Let me try to reuse that
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1196247142
##########
airflow/config_templates/config.yml:
##########
@@ -2671,6 +2671,13 @@ kubernetes_executor:
type: string
example: '{ "total": 3, "backoff_factor": 0.5 }'
default: ""
+ detailed_executor_logs:
+ description: |
+ Flag to have more details in kubernetes executor pod logs
Review Comment:
Great point. I agree with you @hussein-awala.
I will make the changes for 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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197506575
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
How can I use the static here? Any hints or tips?
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198615556
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
Do we really need to go through the JSON serialiser just for logging? Is simply logging the dict directly problematic?
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198664678
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
Tried out some experiments. Your theory looks good. Pushing a change addressing your review comments so far
--
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] amoghrajesh commented on pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1556506452
@uranusjr I have addressed your review comments, can you help in another round of review when you have some 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] amoghrajesh commented on pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1559086244
Thank you for that pointer, @uranusjr! Great help.
Pushing a commit with that included.
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1196077473
##########
airflow/config_templates/config.yml:
##########
@@ -2671,6 +2671,13 @@ kubernetes_executor:
type: string
example: '{ "total": 3, "backoff_factor": 0.5 }'
default: ""
+ detailed_executor_logs:
+ description: |
+ Flag to have more details in kubernetes executor pod logs
Review Comment:
Sounds good to me
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198544689
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -216,15 +233,27 @@ def process_status(
resource_version: str,
event: Any,
) -> None:
+ annotations_string = annotations_to_str(annotations)
"""Process status response."""
if status == "Pending":
if event["type"] == "DELETED":
- self.log.info("Event: Failed to start pod %s", pod_name)
+ if get_logs_task_metadata():
+ self.log.info(
+ "Event: Failed to start pod %s, annotations: %s", pod_name, annotations_string
+ )
+ else:
+ self.log.info("Event: Failed to start pod %s", pod_name)
Review Comment:
Imo, leaving the omitted annotations would not look good in the logs when we do not have any annotations or the logs metadata flag enabled. What do you think? @uranusjr
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197454364
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Understandable. Went ahead with defining the variable as class variables. This should not lead to any import time effects. Wdyt? @uranusjr
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1201407333
##########
tests/executors/test_kubernetes_executor.py:
##########
@@ -1162,6 +1166,39 @@ def test_supports_pickling(self):
def test_supports_sentry(self):
assert not KubernetesExecutor.supports_sentry
+ def test_annotations_for_logging_task_metadata(self):
+ annotations_test = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ with mock.patch.dict(
+ os.environ, {"AIRFLOW__KUBERNETES_EXECUTOR__LOGS_TASK_METADATA": "True"}, clear=True
+ ):
+ expected_annotations = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ annotations_actual = annotations_for_logging_task_metadata(annotations_test)
+ assert annotations_actual == expected_annotations
+
+ def test_annotations_for_logging_task_metadata_fallback(self):
+ annotations_test = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ with mock.patch.dict(
+ os.environ, {"AIRFLOW__KUBERNETES_EXECUTOR__LOGS_TASK_METADATA": "False"}, clear=True
+ ):
+ expected_annotations = "<omitted>"
+ annotations_actual = annotations_for_logging_task_metadata(annotations_test)
+ assert annotations_actual == expected_annotations
+
Review Comment:
Instead of using `mock.patch`, it’s more reliable to use the `conf_vars` helper. You also want to patch both the true and false cases because it’s not deterministic in the CI which the config is set to when the test is run.
--
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] hussein-awala commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1125698615
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
I wonder if we need to add all the annotations to the log, since we use annotations to configure log collecting and others things, maybe we can select only these keys:
```json
{
"dag_id": "dag",
"task_id": "task",
"run_id": "run_id",
"try_number": "1",
"execution_date": None,
}
```
Better wait for a second opinion before changing 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 #29929: Adding more information to kubernetes executor logs
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1133135031
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -347,7 +356,12 @@ def run_next(self, next_job: KubernetesJobType) -> None:
)
# Reconcile the pod generated by the Operator and the Pod
# generated by the .cfg file
- self.log.info("Creating kubernetes pod for job is %s, with pod name %s", key, pod.metadata.name)
+ self.log.info(
+ "Creating kubernetes pod for job is %s, with pod name %s, annotations: %s",
+ key,
+ pod.metadata.name,
+ annotations_to_str(pod.metadata.annotations),
+ )
Review Comment:
Don't need it here, this already has the key which contains basically the same info:
```
Creating kubernetes pod for job is TaskInstanceKey(dag_id='simple', task_id='world', run_id='manual__2023-03-11T18:59:19.375564+00:00', try_number=1, map_index=-1), with pod name simple-world-58639fac88a74c38a2499d288b40e874
```
##########
tests/executors/test_kubernetes_executor.py:
##########
@@ -1253,6 +1253,21 @@ def test_supports_pickling(self):
def test_supports_sentry(self):
assert not KubernetesExecutor.supports_sentry
+ def test_annotations_to_str(self):
+ executor = self.kubernetes_executor
+ executor.scheduler_job_id = "modified"
Review Comment:
```suggestion
```
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
I'm a little torn on adding them on every line in the first place, as we already have an INFO that correlates the pod_id to the TI.
--
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 #29929: Adding more information to kubernetes executor logs
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1496477135
seems that "rebase" from UI - messed with the change (first time see smth like that) - can yoy please rebase your local change again @amoghrajesh and re-push 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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197601032
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Defined a static property at the top of the file and I am using that throughout the classes. I guess this should work. @uranusjr
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198641011
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
String formatting is done automatically if you pass in something foreign, so simply `log.info("... %s", annotations)` would work (although the format would be a bit different)
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197610516
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Referred https://github.com/apache/airflow/blob/fc4f37b105ca0f03de7cc49ab4f00751287ae145/airflow/cli/commands/connection_command.py#LL139C1-L139C1 and pushed the latest changes
--
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] amoghrajesh commented on pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1559725186
I was able to fix the tests here, @hussein-awala / @uranusjr can you help in merging this PR?
Thanks!
--
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] amoghrajesh closed pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh closed pull request #29929: Adding more information to kubernetes executor logs
URL: https://github.com/apache/airflow/pull/29929
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1133185172
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
Not sure what you mean here @jedcunningham
What do you suggest?
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1195902261
##########
airflow/config_templates/config.yml:
##########
@@ -2671,6 +2671,13 @@ kubernetes_executor:
type: string
example: '{ "total": 3, "backoff_factor": 0.5 }'
default: ""
+ detailed_executor_logs:
+ description: |
+ Flag to have more details in kubernetes executor pod logs
Review Comment:
@uranusjr thanks for the review. All the logs present under kubernetes executor do not contain a lot of data needed to have the best debugging experience.
Example:
When trying to trace the lifecycle of a task in the kubernetes executor, you currently must search first for the name of the pod created by the task, then search for the pod name in the logs. This means you need to be pretty familiar with the structure of the scheduler logs in order to search effectively for the lifecycle of a task that had a problem.
Some log statements like Attempting to finish pod do have the annotations for the pod, which include dag name, task name, and run_id, but others do not. For instance, Event: podname-a2f2c1ac706 had an event of type DELETED has no such annotations.
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197567395
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Yeah that can work
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197278003
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
This should use `getboolean` nad I feel should not be done called globally but in the executor instead.
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198535511
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +73,18 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+if sys.version_info >= (3, 9):
+ from functools import cache
+else:
+ from functools import lru_cache
+
+ cache = lru_cache(maxsize=None)
Review Comment:
This is already available in `airflow.compar.functools`.
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +73,18 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+if sys.version_info >= (3, 9):
+ from functools import cache
+else:
+ from functools import lru_cache
+
+ cache = lru_cache(maxsize=None)
Review Comment:
This is already available in `airflow.compat.functools`.
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1200457473
##########
tests/executors/test_kubernetes_executor.py:
##########
@@ -1162,6 +1166,39 @@ def test_supports_pickling(self):
def test_supports_sentry(self):
assert not KubernetesExecutor.supports_sentry
+ def test_annotations_for_logging_task_metadata(self):
+ annotations_test = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ with mock.patch.dict(
+ os.environ, {"AIRFLOW__KUBERNETES_EXECUTOR__LOGS_TASK_METADATA": "True"}, clear=True
+ ):
+ expected_annotations = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ annotations_actual = annotations_for_logging_task_metadata(annotations_test)
+ assert annotations_actual == expected_annotations
+
+ def test_annotations_for_logging_task_metadata_fallback(self):
+ annotations_test = {
+ "dag_id": "dag",
+ "run_id": "run_id",
+ "task_id": "task",
+ "try_number": "1",
+ }
+ with mock.patch.dict(
+ os.environ, {"AIRFLOW__KUBERNETES_EXECUTOR__LOGS_TASK_METADATA": "False"}, clear=True
+ ):
+ expected_annotations = "<omitted>"
+ annotations_actual = annotations_for_logging_task_metadata(annotations_test)
+ assert annotations_actual == expected_annotations
+
Review Comment:
These tests seem to work fine in my environment. Not sure how/why these are failing in the CI. Any hints @hussein-awala ?
--
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] hterik commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1133508777
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
Is it possible to use logfmt here instead of json? It is a lot more human readable and still easy to parse with tooling.
Not sure if there is any convention throughout the project on preferred way to do structural logs. But maybe that discussion is out of scope for this change.
--
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] hterik commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1135066602
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
Having all info in one log entry makes it easier to do things like grep for all entries with dag_id=xxxx. Otherwise you need to keep context in mind and remember what was logged above and trust that such lines belong together.
The events logged in `process_status` don't have the TI logged together nor on any adjacent lines before.
Events like the following appear outof nowehere in the log today, `{kubernetes_executor.py:213} INFO - Event: dagfoo-taskbar-2aa04e01a1a945cfb1c59750ff7f4f70 Succeeded`
You have to scroll down far in the log to reach `Changing state of ... pod name`, or very very far up the log to reach `Creating kubernetes pod ... with pod name` to associate them
It will get rather wordy, but imo the verbosity of having few entries with a lot of information is better than verbosity of multiple lines with spread out information.
-------------
execution_date is redundant when run_id is included right?
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197579614
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Sounds good. But where should this be defined? At global scope or in each class wherever the occurrences are?
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198535370
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -216,15 +233,27 @@ def process_status(
resource_version: str,
event: Any,
) -> None:
+ annotations_string = annotations_to_str(annotations)
"""Process status response."""
if status == "Pending":
if event["type"] == "DELETED":
- self.log.info("Event: Failed to start pod %s", pod_name)
+ if get_logs_task_metadata():
+ self.log.info(
+ "Event: Failed to start pod %s, annotations: %s", pod_name, annotations_string
+ )
+ else:
+ self.log.info("Event: Failed to start pod %s", pod_name)
Review Comment:
I wonder instead of doing if-else everywhere, if it’s better to simply do something like
```python
if get_logs_task_metadata():
annotations_for_logging = annotations_to_str(annotations)
else:
annotations_for_logging = "<omitted>"
```
and always add the `annotations: %s` part.
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198621006
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
Not really. Made it this way so that we can use string formatting in the logs instead to maintain uniformity. What do you think?
--
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] amoghrajesh closed pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh closed pull request #29929: Adding more information to kubernetes executor logs
URL: https://github.com/apache/airflow/pull/29929
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1133185465
##########
tests/executors/test_kubernetes_executor.py:
##########
@@ -1253,6 +1253,21 @@ def test_supports_pickling(self):
def test_supports_sentry(self):
assert not KubernetesExecutor.supports_sentry
+ def test_annotations_to_str(self):
+ executor = self.kubernetes_executor
+ executor.scheduler_job_id = "modified"
Review Comment:
Yeah these are not required, made the required changes
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1133185569
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -347,7 +356,12 @@ def run_next(self, next_job: KubernetesJobType) -> None:
)
# Reconcile the pod generated by the Operator and the Pod
# generated by the .cfg file
- self.log.info("Creating kubernetes pod for job is %s, with pod name %s", key, pod.metadata.name)
+ self.log.info(
+ "Creating kubernetes pod for job is %s, with pod name %s, annotations: %s",
+ key,
+ pod.metadata.name,
+ annotations_to_str(pod.metadata.annotations),
+ )
Review Comment:
Thanks for the feedback. Added these in case the `TaskInstanceKey` changes someday, for future proof.
Anyways, removed 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] hussein-awala commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1137720316
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
I agree with @jedcunningham, where all the log collectors add metadata about the log source, and we can use them to filter the log
But if we have a real need for some users, we can add a config to enable/disable adding these extra information in each line.
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1137703660
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
Hi all, following up on this review here.
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198620518
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -216,15 +233,27 @@ def process_status(
resource_version: str,
event: Any,
) -> None:
+ annotations_string = annotations_to_str(annotations)
"""Process status response."""
if status == "Pending":
if event["type"] == "DELETED":
- self.log.info("Event: Failed to start pod %s", pod_name)
+ if get_logs_task_metadata():
+ self.log.info(
+ "Event: Failed to start pod %s, annotations: %s", pod_name, annotations_string
+ )
+ else:
+ self.log.info("Event: Failed to start pod %s", pod_name)
Review Comment:
Sure, let me address this comment in that case.
--
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] hussein-awala commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197603992
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
you should use `@cache` for the module-level functions
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1198544689
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -216,15 +233,27 @@ def process_status(
resource_version: str,
event: Any,
) -> None:
+ annotations_string = annotations_to_str(annotations)
"""Process status response."""
if status == "Pending":
if event["type"] == "DELETED":
- self.log.info("Event: Failed to start pod %s", pod_name)
+ if get_logs_task_metadata():
+ self.log.info(
+ "Event: Failed to start pod %s, annotations: %s", pod_name, annotations_string
+ )
+ else:
+ self.log.info("Event: Failed to start pod %s", pod_name)
Review Comment:
Imo, leaving the omitted annotations would not look good in the logs when we do not have any annotations or the logs metadata flag enabled. What do you think?
--
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 #29929: Adding more information to kubernetes executor logs
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1134717338
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
I meant that we already [log TI key and pod_id together](https://github.com/apache/airflow/blob/198408e4531dea7eaba5b64bdb0f0d2b74062878/airflow/executors/kubernetes_executor.py#L350), so logging the same stuff on each line seems like it could be redundant. I'm curious to see what others think though - I'm not saying no, I'm just saying I'm not sure we need it.
logfmt vs json: Yeah, probably out of scope for this change.
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1133987631
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
We do not have the package installed in the airflow code. I think we can do that but not sure what others think here.
cc @jedcunningham @potiuk
--
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] amoghrajesh commented on pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1475765923
@jedcunningham @hussein-awala @hterik want to hear some opinions about this.
Does the following proposal work?
1. Since most lines have enough logs with required annotations, it is better is we don't force more annotations and flood the logs for all users
2. We can have a feature flag which covers if the annotations are printed in the logs for some of these k8s executor logs. This flag can be enabled by certain users who require these kind of info
--
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] amoghrajesh commented on pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1558942523
I tried using conf_vars instead and the tests pass for me in my dev setup but not sure why it consistently fails here.
@uranusjr @hussein-awala any tips?
--
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] amoghrajesh commented on pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1556707861
Closed pull requests to re launch the entire resr suite
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197411704
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Made it to getboolean. It was used in multiple classes, so defining it globally made sense to me. But now that I think of it, adding it to the specific classes makes sense too. Moved it around a bit
--
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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197581881
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Right, you want to be able to call this in multiple classes right? Maybe
```python
@cache
def get_logs_task_metadata(self) -> bool:
return conf.getboolean("kubernetes_executor", "logs_task_metadata", fallback=False)
```
as a module-level function and call it when needed.
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1127858269
##########
airflow/kubernetes/kubernetes_helper_functions.py:
##########
@@ -119,3 +120,7 @@ def annotations_to_key(annotations: dict[str, str]) -> TaskInstanceKey:
try_number=try_number,
map_index=map_index,
)
+
+
+def annotations_to_str(annotations: dict[str, str]) -> str:
+ return json.dumps(annotations)
Review Comment:
@hussein-awala do you mean we should not have "execution_date"?
--
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] uranusjr commented on pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29929:
URL: https://github.com/apache/airflow/pull/29929#issuecomment-1558971454
When a function is decorated with, `cache`, it is only executed exactly once, and later calls automatically re-uses the previous result. This means that after the first test (whichever that is) is run, the other test would receive the wrong, cached result. You need to clear the cache. Search for `cache_clear()` in the tests directory for examples.
--
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] hussein-awala merged pull request #29929: Adding more information to kubernetes executor logs
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala merged PR #29929:
URL: https://github.com/apache/airflow/pull/29929
--
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] amoghrajesh commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1195903041
##########
airflow/config_templates/config.yml:
##########
@@ -2671,6 +2671,13 @@ kubernetes_executor:
type: string
example: '{ "total": 3, "backoff_factor": 0.5 }'
default: ""
+ detailed_executor_logs:
+ description: |
+ Flag to have more details in kubernetes executor pod logs
Review Comment:
I will rename the flag to something more meaningful like: `traceable-executor-logs`. What do you think @uranusjr?
Or any other alternative thoughts?
--
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] hussein-awala commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1196082249
##########
airflow/config_templates/config.yml:
##########
@@ -2671,6 +2671,13 @@ kubernetes_executor:
type: string
example: '{ "total": 3, "backoff_factor": 0.5 }'
default: ""
+ detailed_executor_logs:
+ description: |
+ Flag to have more details in kubernetes executor pod logs
Review Comment:
What about `logs_task_metadata`? if you look to the added information, they describes the task and not the executor. 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] uranusjr commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197492804
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
Unfortunately class variables are also executed at import time so this does not change anything.
--
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] hussein-awala commented on a diff in pull request #29929: Adding more information to kubernetes executor logs
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29929:
URL: https://github.com/apache/airflow/pull/29929#discussion_r1197508917
##########
airflow/executors/kubernetes_executor.py:
##########
@@ -68,6 +72,8 @@
ALL_NAMESPACES = "ALL_NAMESPACES"
POD_EXECUTOR_DONE_KEY = "airflow_executor_done"
+logs_task_metadata = conf.get("kubernetes_executor", "logs_task_metadata", fallback="False") == "True"
Review Comment:
@uranusjr what about:
```python
@cached_property
def logs_task_metadata(self) -> bool:
return conf.getboolean("kubernetes_executor", "logs_task_metadata", fallback=False)
```
--
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