Logging from all containers in KubernetesOperatorPod #31663

amoghrajesh · 2023-06-01T08:37:13Z

This PR adds support to get logs from all the containers in a pod and publish them in the airflow logging instead of always getting the base container logs.
A new option container_logging has been added which works in conjunction with get_logs option. ie. if get_logs isn't set, container_logging will not be checked.

The flag takes the following values:

list[str]: A list of container names
str: One single container name
Literal[True]: Log everything
and make the argument default to BASE_CONTAINER_NAME.

This was a community suggestion here: https://github.com/apache/airflow/pull/28981/files#r1071928325

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

amoghrajesh · 2023-06-01T08:38:14Z

@uranusjr @potiuk @pulquero @dstandish looping you in for reviews on this PR as you were reviewers to the one that went stale

uranusjr · 2023-06-01T08:41:56Z

airflow/providers/cncf/kubernetes/operators/pod.py

@@ -270,6 +274,7 @@ def __init__(
        reattach_on_restart: bool = True,
        startup_timeout_seconds: int = 120,
        get_logs: bool = True,
+        container_logs: list[str] | str | bool = BASE_CONTAINER_NAME,


Does this accept False? If not, this should be Literal[True] instead of bool.

Good point. Yeah it doesn't accept False. Only True

airflow/providers/cncf/kubernetes/utils/pod_manager.py

amoghrajesh · 2023-06-01T09:51:17Z

airflow/providers/cncf/kubernetes/utils/pod_manager.py

+                        container_logs,
+                        pod.metadata.name,
+                    )
+            elif isinstance(container_logs, bool):


Had to keep this check as bool so that we can reject/filter out invalid/unsupported types too here https://github.com/apache/airflow/pull/31663/files#diff-6900da9281d8404b14da0815f2b37350f3148b1b63449928b952744e6711e7e7R475-R478
@uranusjr

amoghrajesh · 2023-06-03T07:35:02Z

@uranusjr @pulquero The PR is in a reviewable state. Requesting a review when you have some time

potiuk · 2023-06-04T22:41:04Z

docs need fixing still

pulquero · 2023-06-05T09:00:42Z

From my perspective, it looks like it will do the job, thanks.

airflow/providers/cncf/kubernetes/utils/pod_manager.py

vincbeck · 2023-06-05T20:13:18Z

airflow/providers/cncf/kubernetes/utils/pod_manager.py

+                else:
+                    self.log.error(
+                        "False is not a valid value for container_logs",
+                    )


Is this branch needed?

Yes we need it since we support only Literal[True] when it comes to boolean. Refer to my unit tests. It adds False too as an invalid test case

airflow/providers/cncf/kubernetes/utils/pod_manager.py

JeremieDoctrine · 2023-06-06T09:22:41Z

I was looking for this feature and stumbled upon this pull request.

Will this work also with init containers?

amoghrajesh · 2023-06-06T09:29:13Z

Hi @JeremieDoctrine, yes it should also capture the initContainers logs in my opinion

amoghrajesh · 2023-06-11T11:44:44Z

@uranusjr @jedcunningham may i request for a review on this pull request when you have some time?

amoghrajesh · 2023-06-24T11:52:32Z

@hussein-awala @potiuk @jedcunningham @uranusjr following up for a review on this PR. Can you have a look when you have some time?

airflow/providers/cncf/kubernetes/operators/pod.py

potiuk · 2023-06-24T17:02:09Z

airflow/providers/cncf/kubernetes/utils/pod_manager.py

            time.sleep(1)
+        return terminated


Is there any case where terminated will be false? I fail to see it.

It is not supposed to go into the false state. The method is awaiting completion of the container. ie. It will loop until the container is completed. Let me know if it is not clear enough

I mean - why do we need to return the bool ?

It will always be True right ?

Yeah good point. Makes sense to revert it back to returning None.

potiuk

Few nits/questions.

amoghrajesh · 2023-06-27T04:51:07Z

@potiuk just pushed a patch to fix the comments. Have a look when you have some time :)

uranusjr · 2023-06-27T05:20:57Z

airflow/providers/cncf/kubernetes/utils/pod_manager.py

+        containers = []
+        pod_info = self.read_pod(pod)
+        for container_spec in pod_info.spec.containers:
+            if container_spec.name != ContainerNames.XCOM_CONTAINER:
+                containers.append(container_spec.name)
+
+        return containers


This seems to be a simpler equivalent

Suggested change

containers = []

pod_info = self.read_pod(pod)

for container_spec in pod_info.spec.containers:

if container_spec.name != ContainerNames.XCOM_CONTAINER:

containers.append(container_spec.name)

return containers

pod_info = self.read_pod(pod)

return [

container_spec.name

for container_spec in pod_info.spec.containers

if container_spec.name != ContainerNames.XCOM_CONTAINER

]

Sure. Made the changes here

uranusjr · 2023-06-27T05:24:23Z

airflow/providers/cncf/kubernetes/utils/pod_manager.py

@@ -398,14 +417,78 @@ def consume_logs(
                )
                time.sleep(1)

+    def fetch_requested_container_logs(


It seems this entire function can be transformed into a generator function instead and use yield to get rid of all the append calls.

Imo, it is simpler to control the flow when we have these append calls. I would like to keep it this way if that is ok by you

uranusjr · 2023-06-27T05:26:12Z

A couple of comments to improve the code, but they don’t change the overall logic and should be non-blocking.

amoghrajesh · 2023-06-30T11:34:52Z

@potiuk @uranusjr can i get another pass at this PR? I think it is ready to be reviewed from my end

airflow/providers/cncf/kubernetes/operators/pod.py

hussein-awala · 2023-06-30T19:27:49Z

airflow/providers/cncf/kubernetes/utils/pod_manager.py

@@ -412,14 +431,78 @@ def consume_logs(
                )
                time.sleep(1)

+    def fetch_requested_container_logs(


I think that we need to use the provided post_termination_timeout in this method as we did with fetch_container_logs

Not sure I quite understand this comment. We are calling the fetch_container_logs from fetch_requested_container_logs which will take care of post_termination_timeout right?

hussein-awala · 2023-06-30T19:31:37Z

airflow/providers/cncf/kubernetes/utils/pod_manager.py

+class ContainerNames:
+    """Possible container names for airflow."""
+
+    XCOM_CONTAINER = "airflow-xcom-sidecar"
+
+


I didn't understand the need of this class, this value is already available in PodDefaults.SIDECAR_CONTAINER_NAME which we use in the other methods, it's better to use it in case we change its name in the future

Good point. I had missed seeing this. I will undo this change and reuse PodDefaults

amoghrajesh · 2023-07-04T05:44:08Z

@hussein-awala can you take a look at this PR again when you have some time?

amoghrajesh · 2023-07-06T01:06:30Z

@hussein-awala @uranusjr following up for a final round of review on this PR 🙂

Want to land it before the branch cut today if possible..

amoghrajesh · 2023-07-06T09:34:29Z

@uranusjr do we need more approvals on this one or can we proceed and merge it?

uranusjr · 2023-07-06T09:42:08Z

At least one more (or if @vincbeck takes another look to make sure changes made after their approval are good)

amoghrajesh requested a review from jedcunningham as a code owner June 1, 2023 08:37

boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Jun 1, 2023

uranusjr reviewed Jun 1, 2023

View reviewed changes

airflow/providers/cncf/kubernetes/utils/pod_manager.py Outdated Show resolved Hide resolved

amoghrajesh requested a review from uranusjr June 1, 2023 09:49

amoghrajesh commented Jun 1, 2023

View reviewed changes

vincbeck reviewed Jun 5, 2023

View reviewed changes

airflow/providers/cncf/kubernetes/utils/pod_manager.py Outdated Show resolved Hide resolved

vincbeck reviewed Jun 5, 2023

View reviewed changes

amoghrajesh requested a review from vincbeck June 6, 2023 04:51

vincbeck approved these changes Jun 6, 2023

View reviewed changes

potiuk reviewed Jun 24, 2023

View reviewed changes

airflow/providers/cncf/kubernetes/operators/pod.py Outdated Show resolved Hide resolved

potiuk reviewed Jun 24, 2023

View reviewed changes

amoghrajesh requested a review from potiuk June 26, 2023 04:17

uranusjr reviewed Jun 27, 2023

View reviewed changes

amoghrajesh requested a review from uranusjr June 27, 2023 05:53

Amogh added 2 commits June 27, 2023 23:07

Logging from all containers in KubernetesOperatorPod

128e389

review comments from uranusjs

ecd8a07

Amogh added 10 commits June 27, 2023 23:07

Fixing init logic

ba95bad

Fixing docs

b7590eb

Fixing sphinx logging

fea549f

Addressing review comments from vincbeck

4a8210e

nits from potiuk

7b55357

nits from potiuk

777f758

reverting return type

cdfbeb3

comment from uranusjr

56c772b

fixing tests

d8d1053

fixing tests

901f5af

potiuk force-pushed the k8sLogsAllContainers branch from f6614e1 to 901f5af Compare June 27, 2023 21:07

hussein-awala reviewed Jun 30, 2023

View reviewed changes

review comments from hussein

3960465

amoghrajesh requested a review from hussein-awala July 3, 2023 08:30

Amogh Desai added 2 commits July 4, 2023 18:57

handling nits from hussein

282482b

fixing tests

50dbd9c

uranusjr approved these changes Jul 6, 2023

View reviewed changes

hussein-awala approved these changes Jul 6, 2023

View reviewed changes

hussein-awala merged commit 9a0f41b into apache:main Jul 6, 2023

eladkal mentioned this pull request Jul 12, 2023

Status of testing Providers that were prepared on July 12, 2023 #32568

Closed

20 tasks

hussein-awala mentioned this pull request Aug 4, 2023

Fix waiting the base container when reading the logs of other containers #33092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging from all containers in KubernetesOperatorPod #31663

Logging from all containers in KubernetesOperatorPod #31663

amoghrajesh commented Jun 1, 2023

amoghrajesh commented Jun 1, 2023

uranusjr Jun 1, 2023

amoghrajesh Jun 1, 2023

amoghrajesh Jun 1, 2023

amoghrajesh commented Jun 3, 2023

potiuk commented Jun 4, 2023

pulquero commented Jun 5, 2023

vincbeck Jun 5, 2023

amoghrajesh Jun 6, 2023

JeremieDoctrine commented Jun 6, 2023

amoghrajesh commented Jun 6, 2023

amoghrajesh commented Jun 11, 2023

amoghrajesh commented Jun 24, 2023

potiuk Jun 24, 2023

amoghrajesh Jun 26, 2023

potiuk Jun 26, 2023

potiuk Jun 26, 2023

amoghrajesh Jun 27, 2023

potiuk left a comment

amoghrajesh commented Jun 27, 2023

uranusjr Jun 27, 2023

amoghrajesh Jun 27, 2023

uranusjr Jun 27, 2023

amoghrajesh Jun 27, 2023

uranusjr commented Jun 27, 2023

amoghrajesh commented Jun 30, 2023

hussein-awala Jun 30, 2023

amoghrajesh Jul 3, 2023

hussein-awala Jun 30, 2023

amoghrajesh Jul 3, 2023

amoghrajesh commented Jul 4, 2023

amoghrajesh commented Jul 6, 2023

amoghrajesh commented Jul 6, 2023

uranusjr commented Jul 6, 2023 •

edited

Loading

Logging from all containers in KubernetesOperatorPod #31663

Logging from all containers in KubernetesOperatorPod #31663

Conversation

amoghrajesh commented Jun 1, 2023

amoghrajesh commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amoghrajesh commented Jun 3, 2023

potiuk commented Jun 4, 2023

pulquero commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremieDoctrine commented Jun 6, 2023

amoghrajesh commented Jun 6, 2023

amoghrajesh commented Jun 11, 2023

amoghrajesh commented Jun 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

amoghrajesh commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uranusjr commented Jun 27, 2023

amoghrajesh commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amoghrajesh commented Jul 4, 2023

amoghrajesh commented Jul 6, 2023

amoghrajesh commented Jul 6, 2023

uranusjr commented Jul 6, 2023 • edited Loading

uranusjr commented Jul 6, 2023 •

edited

Loading