You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "mrk-andreev (via GitHub)" <gi...@apache.org> on 2024/11/09 19:03:36 UTC
[PR] Add support logging in KubernetesPodOperator (#42498) [airflow]
mrk-andreev opened a new pull request, #43853:
URL: https://github.com/apache/airflow/pull/43853
This change adds an option to print logs for init containers. The get_init_containers_logs and init_container_logs functions allow displaying the logs of init containers.
Fixes: #42498
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838952610
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -832,6 +883,20 @@ def _exec_pod_command(self, resp, command: str) -> str | None:
return res
return None
+ def _await_init_container_start(self, pod: V1Pod, container_name: str, sleep_timeout_seconds: float = 1):
Review Comment:
if nothing is using this param, and it's a private function anyway, just remove the param
--
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] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835547371
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
Review Comment:
Is this the right place for integration testing? In this flow, I need a Kubernetes cluster for realistic testing.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839093771
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
Review Comment:
Fixed
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
Review Comment:
Fixed
--
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] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on PR #43853:
URL: https://github.com/apache/airflow/pull/43853#issuecomment-2466437489
> I am not an K8s expert, can init containers run in parallel? Or are they executed serial?
How about if init containers run for a longer time? Would it be interesting to stream logs while they are running to see progress? And if they run in parallel would it mix logs?
Each init container runs sequentially, with each one waiting for the previous container to complete. The main containers wait until all init containers are ready before starting.
Ref: https://kubernetes.io/docs/concepts/workloads/pods/init-containers/#understanding-init-containers
> Init containers are exactly like regular containers, except:
> * Init containers always run to completion.
> * Each init container must complete successfully before the next one starts.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1837318245
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -620,6 +625,9 @@ def execute_sync(self, context: Context):
self.callbacks.on_pod_creation(
pod=self.remote_pod, client=self.client, mode=ExecutionMode.SYNC
)
+
+ self.await_init_containers_completion(pod=self.pod)
+
Review Comment:
Look like we can use `Waiting` instead. While init_container `Waiting` we should wait.
> If a container is not in either the Running or Terminated state, it is Waiting. A container in the Waiting state is still running the operations it requires in order to complete start up: for example, pulling the container image from a container image registry, or applying [Secret](https://kubernetes.io/docs/concepts/configuration/secret/) data. When you use kubectl to query a Pod with a container that is Waiting, you also see a Reason field to summarize why the container is in that state.
ref: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#container-state-waiting
So we can have `self._await_init_container_start(pod=pod, container_name=c)` that will wait non-empty non-wait status :
```
# sort by spec.initContainers because containers runs sequentially
containers_to_log = sorted(containers_to_log, key=lambda cn: all_containers.index(cn))
for c in containers_to_log:
self._await_init_container_start(pod=pod, container_name=c)
status = self.fetch_container_logs(pod=pod, container_name=c, follow=follow_logs)
pod_logging_statuses.append(status)
```
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839098444
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
If we have similar code that performs similar tasks, it might be harder to understand why we’re implementing it differently. I’m not the code owner of podmanager, and I’m open to aligning with your perspective. I just wanted to mention some potential issues.
--
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] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835547598
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -655,6 +668,31 @@ def execute_sync(self, context: Context):
if self.do_xcom_push:
return result
+ @tenacity.retry(
+ wait=tenacity.wait_exponential(max=15),
+ retry=tenacity.retry_if_exception_type(PodCredentialsExpiredFailure),
+ reraise=True,
+ )
+ def await_init_containers_completion(self, pod: k8s.V1Pod):
+ try:
+ if self.get_init_containers_logs:
Review Comment:
Right, I agree.
But it would be better to remove self.get_init_containers_logs before calling await_init_containers_completion to mirror the behavior of await_pod_start more closely."
It sounds like you’re refining the control flow for better consistency - feels like a solid approach for streamlining the logging functionality.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838990126
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -565,6 +585,31 @@ def _reconcile_requested_log_containers(
self.log.error("Could not retrieve containers for the pod: %s", pod_name)
return containers_to_log
+ def fetch_requested_init_container_logs(
Review Comment:
Roger. And to be clear my question is genuine and not a subtle way of saying "you must do this".
But, back to the matter, it looks like `fetch_requested_container_logs` does process the logs sequentially, doesn't it? Looks like the only difference is you add "await start". Is that actually necessary? Why isn't it necessary with non-init containers? Should we be doing it with non-init containers?
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839671831
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
I don’t have a strong opinion on this. I always focus on making the minimal number of changes to other parts of the code execution flow.
At the same time, I try to write code in a consistent style, following the same patterns as those already in place.
Since I’m not the maintainer of this project, I’m open to any changes that are needed. I trust that you have a better understanding of what needs to be done in this project.
--
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] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835495274
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
Review Comment:
I assume the test is mis-placed and it is a tech debt that the K8s tests are out of the provider package tree. Can you move the new test to providers/tests/cncf/kubernetes/operators/test_pod.py ?
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -157,7 +157,11 @@ class KubernetesPodOperator(BaseOperator):
:param labels: labels to apply to the Pod. (templated)
:param startup_timeout_seconds: timeout in seconds to startup the pod.
:param startup_check_interval_seconds: interval in seconds to check if the pod has already started
+ :param get_init_containers_logs: get the stdout of the init containers as logs of the tasks.
:param get_logs: get the stdout of the base container as logs of the tasks.
+ :param init_container_logs: list of init containers whose logs will be published to stdout
+ Takes a sequence of containers, a single container name or True. If True,
+ all the containers logs are published. Works in conjunction with get_init_containers_logs param.
Review Comment:
Is this parameter not partly redundant with the other bool parameter?
I would assume if there are mutliple init containers I can use this sequence to limit from whoich init containers I want to have logs. But if I specify a list I don't need another bool.
On the other hand if I just want to have all init container logs, then the list need to be filled?
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -655,6 +668,31 @@ def execute_sync(self, context: Context):
if self.do_xcom_push:
return result
+ @tenacity.retry(
+ wait=tenacity.wait_exponential(max=15),
+ retry=tenacity.retry_if_exception_type(PodCredentialsExpiredFailure),
+ reraise=True,
+ )
+ def await_init_containers_completion(self, pod: k8s.V1Pod):
+ try:
+ if self.get_init_containers_logs:
Review Comment:
I think you don't need to check the option setting again here if the calling method checked before calling this function
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835806364
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -655,6 +663,31 @@ def execute_sync(self, context: Context):
if self.do_xcom_push:
return result
+ @tenacity.retry(
+ wait=tenacity.wait_exponential(max=15),
+ retry=tenacity.retry_if_exception_type(PodCredentialsExpiredFailure),
+ reraise=True,
+ )
+ def await_init_containers_completion(self, pod: k8s.V1Pod):
+ try:
+ if self.init_container_logs:
+ self.pod_manager.fetch_requested_init_container_logs(
+ pod=pod,
+ init_containers=self.init_container_logs,
+ follow_logs=True,
+ )
+ except kubernetes.client.exceptions.ApiException as exc:
+ if exc.status and str(exc.status) == "401":
+ self.log.warning(
+ "Failed to check container status due to permission error. Refreshing credentials and retrying."
+ )
+ self._refresh_cached_properties()
+ self.pod_manager.read_pod(
+ pod=pod
+ ) # attempt using refreshed credentials, raises if still invalid
+ raise PodCredentialsExpiredFailure("Kubernetes credentials expired, retrying after refresh.")
+ raise exc
Review Comment:
A big part of `await_pod_completion` is duplicated here, would it be possible to move the duplicated part to a generalized method? (you can pass flags for the un-common checks)
This method duplicates a big part of `await_pod_completion`, would it be possible to move the duplicated part to a generalized method? (you can pass flags for non-common checks)
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -118,7 +119,13 @@ def get_xcom_sidecar_container_resources(self) -> str | None:
def get_container_status(pod: V1Pod, container_name: str) -> V1ContainerStatus | None:
"""Retrieve container status."""
- container_statuses = pod.status.container_statuses if pod and pod.status else None
+ if pod and pod.status:
+ container_statuses = itertools.chain(
+ pod.status.container_statuses, pod.status.init_container_statuses
+ )
+ else:
+ container_statuses = None
+
Review Comment:
Why do you need this change?
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -620,6 +625,9 @@ def execute_sync(self, context: Context):
self.callbacks.on_pod_creation(
pod=self.remote_pod, client=self.client, mode=ExecutionMode.SYNC
)
+
+ self.await_init_containers_completion(pod=self.pod)
+
Review Comment:
Before we start reading the logs from the main containers, we wait for the pod to start and we need to do the same before reading the logs from the init containers, otherwise, the reading will fail if the pod is not scheduled yet.
If I'm not mistaken, when the init containers start, the pod status will be `Pending`, but the pod should have a condition `Initialized` set to false (any state after that should be ok, for ex `Initialized`, `Running`, ... )
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838997867
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
Review Comment:
those methods are already there. you are adding a new method. just because we didn't do something in the past that we probably should have, it doesn't follow that we shouldn't do the good thing now.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839094109
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
Review Comment:
Fixed
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #43853:
URL: https://github.com/apache/airflow/pull/43853#issuecomment-2546278437
> LGTM
>
> If no further comments I will merge this PR for next provider wave
Let's. I think it looks good.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on PR #43853:
URL: https://github.com/apache/airflow/pull/43853#issuecomment-2486764952
Hi @jscheffl, @hussein-awala, @dstandish, @potiuk, @jedcunningham
Does this PR need any additional changes? Are there any blockers we should address? Let me know how I can help to move it forward!
--
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] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835549187
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -426,6 +433,8 @@ def __init__(
self.callbacks = callbacks
self._killed: bool = False
+ template_fields_renderers = {"env_vars": "py"}
Review Comment:
Fixed.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835814658
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -118,7 +119,13 @@ def get_xcom_sidecar_container_resources(self) -> str | None:
def get_container_status(pod: V1Pod, container_name: str) -> V1ContainerStatus | None:
"""Retrieve container status."""
- container_statuses = pod.status.container_statuses if pod and pod.status else None
+ if pod and pod.status:
+ container_statuses = itertools.chain(
+ pod.status.container_statuses, pod.status.init_container_statuses
+ )
+ else:
+ container_statuses = None
+
Review Comment:
Wthout this changes `get_container_status` for any initContainer will return False because
```
return next((x for x in container_statuses if x.name == container_name), None)
```
that affects `logs_available()` inside `PodLogsConsumer` that will ignore logs because `state is None` in
```
def logs_available(self):
remote_pod = self.read_pod()
if container_is_running(pod=remote_pod, container_name=self.container_name):
return True
container_status = get_container_status(pod=remote_pod, container_name=self.container_name)
state = container_status.state if container_status else None
terminated = state.terminated if state else None
if terminated:
termination_time = terminated.finished_at
if termination_time:
return termination_time + timedelta(seconds=self.post_termination_timeout) > utcnow()
return 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
Re: [PR] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838994932
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -832,6 +883,20 @@ def _exec_pod_command(self, resp, command: str) -> str | None:
return res
return None
+ def _await_init_container_start(self, pod: V1Pod, container_name: str, sleep_timeout_seconds: float = 1):
Review Comment:
Would you like to have `sleep_timeout_seconds` as const? Which param are unused?
--
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] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #43853:
URL: https://github.com/apache/airflow/pull/43853#issuecomment-2466414292
@dstandish @jedcunningham @jscheffl -> that one was submitted during the Man's Hackathon - I am not sure if I am competent enough to review the details, but it looks good at a first glance, I'd need someone more k8s-api-involved to review it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835548762
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -157,7 +157,11 @@ class KubernetesPodOperator(BaseOperator):
:param labels: labels to apply to the Pod. (templated)
:param startup_timeout_seconds: timeout in seconds to startup the pod.
:param startup_check_interval_seconds: interval in seconds to check if the pod has already started
+ :param get_init_containers_logs: get the stdout of the init containers as logs of the tasks.
:param get_logs: get the stdout of the base container as logs of the tasks.
+ :param init_container_logs: list of init containers whose logs will be published to stdout
+ Takes a sequence of containers, a single container name or True. If True,
+ all the containers logs are published. Works in conjunction with get_init_containers_logs param.
Review Comment:
Yes, you're right; we don't need get_init_containers_logs because for the main containers, we log the base container, whereas for init containers, we don't use this term.
So, I'd suggest using init_container_logs 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
Re: [PR] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838995284
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
I'm asking about the new code you are adding though, not the old code we are not changing.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #43853:
URL: https://github.com/apache/airflow/pull/43853
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1861357573
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -620,6 +625,9 @@ def execute_sync(self, context: Context):
self.callbacks.on_pod_creation(
pod=self.remote_pod, client=self.client, mode=ExecutionMode.SYNC
)
+
+ self.await_init_containers_completion(pod=self.pod)
+
Review Comment:
any more comments @hussein-awala ? I am not too fluent in that part, but maybe we can get this to completion?
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #43853:
URL: https://github.com/apache/airflow/pull/43853#issuecomment-2546280015
I rebased it - and let's give @dstandish and @hussein-awala last chance to comment.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1844932803
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -565,6 +585,31 @@ def _reconcile_requested_log_containers(
self.log.error("Could not retrieve containers for the pod: %s", pod_name)
return containers_to_log
+ def fetch_requested_init_container_logs(
Review Comment:
I believe that fetching logs for non-init containers should be done in parallel, as we might have long-running containers with interactions where parallel logs could be useful.
However, I suggest extracting this idea into a separate PR. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1836516218
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
Review Comment:
I am not so sure @jscheffl -> I think that one is "actually" running on K8S, and we cannot move it to provider's tests because provider tests have no "real" k8s running.
So I'd say it's well placed.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839010404
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
And to be clear, again, i'm just asking if we really need the pod read, or no. I don't know the answer. But since you are adding the code, you should know (or find out) and we should not just add because it was there in a method we're copy pasting.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838970301
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -565,6 +585,31 @@ def _reconcile_requested_log_containers(
self.log.error("Could not retrieve containers for the pod: %s", pod_name)
return containers_to_log
+ def fetch_requested_init_container_logs(
Review Comment:
> does it really make sense to add a new method fetch_requested_init_container_logs?
Init containers runs sequencially and require different logic for fetching. For example it is better to fetch logs in order that they are definied
```
# sort by spec.initContainers because containers runs sequentially
containers_to_log = sorted(containers_to_log, key=lambda cn: all_containers.index(cn))
```
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838984802
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
This method mirror get_container_names. May be it is a task for refactoring in separate PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838950445
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
Review Comment:
in every new method, please add the `:meta private:` sphinx directive. this makes it clear it's not part of public interface.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1861357757
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -565,6 +585,31 @@ def _reconcile_requested_log_containers(
self.log.error("Could not retrieve containers for the pod: %s", pod_name)
return containers_to_log
+ def fetch_requested_init_container_logs(
Review Comment:
Is that one ok @dstandish ?
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1843022338
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
It is not required to do kubeapi call.
Fixed
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838981401
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
Review Comment:
This method mirror `get_container_names`:
```
@tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
def get_init_container_names(self, pod: V1Pod) -> list[str]:
"""Return container names from the POD except for the airflow-xcom-sidecar container."""
pod_info = self.read_pod(pod)
return [container_spec.name for container_spec in pod_info.spec.init_containers]
@tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
def get_container_names(self, pod: V1Pod) -> list[str]:
"""Return container names from the POD except for the airflow-xcom-sidecar container."""
pod_info = self.read_pod(pod)
return [
container_spec.name
for container_spec in pod_info.spec.containers
if container_spec.name != PodDefaults.SIDECAR_CONTAINER_NAME
]
```
In this case `get_container_names` also will require same logic.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839323233
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
I welcome your contribution. That's how new life and new committers come into the airflow community. But I don't think it's asking too much to ask you to think through whether the code you propose to add is actually needed. If you can't be bothered to think it through, maybe this is not the project for you.
Indeed it looks like it is not necessary. Look at line 623 of pod.py. We read the pod there. So not sure why you need to read the pod again to get the init container names, unless you think the init containers can change between these two places.
You can't assume that just because the code is in airflow, that means it's good, or that it was thought through. When we touch code, that's an opportunity to examine it critically. And when we add new code, that especially is a time to do so. Otherwise complexity just increases.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #43853:
URL: https://github.com/apache/airflow/pull/43853#issuecomment-2514887694
Can you rebase in the meantime @mrk-andreev ?
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1837112510
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
Review Comment:
OK
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1837279468
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -655,6 +663,31 @@ def execute_sync(self, context: Context):
if self.do_xcom_push:
return result
+ @tenacity.retry(
+ wait=tenacity.wait_exponential(max=15),
+ retry=tenacity.retry_if_exception_type(PodCredentialsExpiredFailure),
+ reraise=True,
+ )
+ def await_init_containers_completion(self, pod: k8s.V1Pod):
+ try:
+ if self.init_container_logs:
+ self.pod_manager.fetch_requested_init_container_logs(
+ pod=pod,
+ init_containers=self.init_container_logs,
+ follow_logs=True,
+ )
+ except kubernetes.client.exceptions.ApiException as exc:
+ if exc.status and str(exc.status) == "401":
+ self.log.warning(
+ "Failed to check container status due to permission error. Refreshing credentials and retrying."
+ )
+ self._refresh_cached_properties()
+ self.pod_manager.read_pod(
+ pod=pod
+ ) # attempt using refreshed credentials, raises if still invalid
+ raise PodCredentialsExpiredFailure("Kubernetes credentials expired, retrying after refresh.")
+ raise exc
Review Comment:
I suggest using the simplest approach by handling exceptions in a separate method. Other solutions may require significant code changes, which would likely be better suited for a separate PR.
```
def _handle_api_exception(self,
exc: kubernetes.client.exceptions.ApiException,
pod: k8s.V1Pod,
):
if exc.status and str(exc.status) == "401":
self.log.warning(
"Failed to check container status due to permission error. Refreshing credentials and retrying."
)
self._refresh_cached_properties()
self.pod_manager.read_pod(
pod=pod
) # attempt using refreshed credentials, raises if still invalid
raise PodCredentialsExpiredFailure("Kubernetes credentials expired, retrying after refresh.")
raise exc
```
For example, we could use a decorator to specify an interface for the class, which would be used within it. Alternatively, we could implement something based on the mixin pattern. This seems like a separate, significant task for exception handling in Kubernetes.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1843027989
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -565,6 +585,31 @@ def _reconcile_requested_log_containers(
self.log.error("Could not retrieve containers for the pod: %s", pod_name)
return containers_to_log
+ def fetch_requested_init_container_logs(
Review Comment:
For init containers, we only need to wait until the init container starts. After that, we can begin fetching logs. There may be cases where the first container successfully starts and returns logs, while the next one has not started yet. In such cases, we can start fetching logs as soon as possible.
I’m not sure why we have await_pod_start, but for init containers, this logic isn't necessary. That’s why we have a separate flow.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835814658
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -118,7 +119,13 @@ def get_xcom_sidecar_container_resources(self) -> str | None:
def get_container_status(pod: V1Pod, container_name: str) -> V1ContainerStatus | None:
"""Retrieve container status."""
- container_statuses = pod.status.container_statuses if pod and pod.status else None
+ if pod and pod.status:
+ container_statuses = itertools.chain(
+ pod.status.container_statuses, pod.status.init_container_statuses
+ )
+ else:
+ container_statuses = None
+
Review Comment:
Without this changes `get_container_status` for any initContainer will return False because
```
return next((x for x in container_statuses if x.name == container_name), None)
```
that affects `logs_available()` inside `PodLogsConsumer` that will ignore logs because `state is None` in
```
def logs_available(self):
remote_pod = self.read_pod()
if container_is_running(pod=remote_pod, container_name=self.container_name):
return True
container_status = get_container_status(pod=remote_pod, container_name=self.container_name)
state = container_status.state if container_status else None
terminated = state.terminated if state else None
if terminated:
termination_time = terminated.finished_at
if termination_time:
return termination_time + timedelta(seconds=self.post_termination_timeout) > utcnow()
return 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
Re: [PR] Add support logging in KubernetesPodOperator (#42498) [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1835489702
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -426,6 +433,8 @@ def __init__(
self.callbacks = callbacks
self._killed: bool = False
+ template_fields_renderers = {"env_vars": "py"}
Review Comment:
It seems it's not related.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839001518
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -832,6 +883,20 @@ def _exec_pod_command(self, resp, command: str) -> str | None:
return res
return None
+ def _await_init_container_start(self, pod: V1Pod, container_name: str, sleep_timeout_seconds: float = 1):
Review Comment:
i was just saying it seems you don't use the param `sleep_timeout_seconds` so it probably doesn't need to be there. not saying you should make it a constant but it could just be a hardcoded number. just the principle of not adding interface that you aren't actually using / doing the minimal thing. you've made it a private function (which is great) so this means if we want to change it later we can.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839098597
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -832,6 +883,20 @@ def _exec_pod_command(self, resp, command: str) -> str | None:
return res
return None
+ def _await_init_container_start(self, pod: V1Pod, container_name: str, sleep_timeout_seconds: float = 1):
Review Comment:
Fixed
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838951641
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
do we _need_ to read the pod again here? or could we just look ta the pod we already have?
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838949117
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -565,6 +585,31 @@ def _reconcile_requested_log_containers(
self.log.error("Could not retrieve containers for the pod: %s", pod_name)
return containers_to_log
+ def fetch_requested_init_container_logs(
Review Comment:
does it really make sense to add a new method `fetch_requested_init_container_logs`?
is it not better to update `fetch_requested_container_logs` to support getting init containers?
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "mrk-andreev (via GitHub)" <gi...@apache.org>.
mrk-andreev commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1838924081
##########
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -620,6 +625,9 @@ def execute_sync(self, context: Context):
self.callbacks.on_pod_creation(
pod=self.remote_pod, client=self.client, mode=ExecutionMode.SYNC
)
+
+ self.await_init_containers_completion(pod=self.pod)
+
Review Comment:
Updated using `remote_pod.status.phase` that should not be equal to `PodPhase.PENDING`
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1839323233
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
Listen, I welcome your contribution. That's how new life and new committers come into the airflow community. But I don't think it's asking too much to ask you to think through whether the code you propose to add is actually needed. If you can't be bothered to think it through, maybe this is not the project for you.
Indeed it looks like it is not necessary. Look at line 623 of pod.py. We read the pod there. So not sure why you need to read the pod again to get the init container names, unless you think the init containers can change between these two places.
You can't assume that just because the code is in airflow, that means it's good, or that it was thought through. When we touch code, that's an opportunity to examine it critically. And when we add new code, that especially is a time to do so. Otherwise complexity just increases.
--
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] Add logging support for init containers in KubernetesPodOperator (#42498) [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43853:
URL: https://github.com/apache/airflow/pull/43853#discussion_r1840607221
##########
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -692,6 +737,12 @@ def read_pod_logs(
post_termination_timeout=post_termination_timeout,
)
+ @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_exponential(), reraise=True)
+ def get_init_container_names(self, pod: V1Pod) -> list[str]:
+ """Return container names from the POD except for the airflow-xcom-sidecar container."""
+ pod_info = self.read_pod(pod)
Review Comment:
The thing is, I'm not telling you what to change. It's not my job to tell you exactly how to code up your feature. That would take a lot of my time. I'm spotting a concern, and asking you a question about your code (you are adding something new, so yes, it is now _your_ code). You seem to keep dismissing the question. You still don't seem to want to engage with your own code and give me an opinion on whether it is necessary. That is what I'm asking you to do. I guess we're having trouble communicating. Please try to be responsive to my question, which is simply, is there really a need to read that pod again?
--
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