Skip to content
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 41 additions & 5 deletions airflow/providers/cncf/kubernetes/utils/pod_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
"""Launches PODs"""
import asyncio
import json
import math
import time
Expand Down Expand Up @@ -193,6 +194,40 @@ def follow_container_logs(self, pod: V1Pod, container_name: str) -> PodLoggingSt
)
return self.fetch_container_logs(pod=pod, container_name=container_name, follow=True)

def log_iterable(self, logs: Iterable[bytes]) -> Optional[DateTime]:
for line in logs:
timestamp, message = self.parse_log_line(line.decode('utf-8', errors="backslashreplace"))
self.log.info(message)
return timestamp

def consume_container_logs_stream(
self, pod: V1Pod, container_name: str, stream: Iterable[bytes]
) -> Optional[DateTime]:
async def consume_log_stream() -> Optional[DateTime]:
return self.log_iterable(stream)

async def async_await_container_completion() -> None:
return self.await_container_completion(pod=pod, container_name=container_name)

loop = asyncio.get_event_loop()
await_container_completion = loop.create_task(async_await_container_completion())
log_stream = loop.create_task(consume_log_stream())
tasks: Iterable[asyncio.Task] = {await_container_completion, log_stream}
loop.run_until_complete(asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED))

if log_stream.done():
return log_stream.result()

log_stream.cancel()
self.log.warning(
"Pod %s log read was interrupted at some point caused by log rotation "

@schattian schattian May 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do better here, adding a call to fetch_container_logs(follow=False) to consume the logs we lost after the rotation happened.

However, this will add a bit of an (imo) unnecessary overhead considering:

  1. we don't expect that to happen with future versions of k8s that have this api fixed.
  2. if the pod was deleted for some reason, the log produced will be misleading.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

"see https://github.com/kubernetes/kubernetes/issues/59902 ",
"and https://github.com/apache/airflow/issues/23497 for reference.",
pod.metadata.name,
container_name,
)
return None

def fetch_container_logs(
self, pod: V1Pod, container_name: str, *, follow=False, since_time: Optional[DateTime] = None
) -> PodLoggingStatus:
Expand Down Expand Up @@ -220,10 +255,11 @@ def consume_logs(*, since_time: Optional[DateTime] = None, follow: bool = True)
),
follow=follow,
)
for raw_line in logs:
line = raw_line.decode('utf-8', errors="backslashreplace")
timestamp, message = self.parse_log_line(line)
self.log.info(message)
Comment on lines -223 to -226

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didnt. It is happening in log_iterable. I just moved the new kwarg (, errors="backslashreplace") which was added by the pr you linked

if follow:
timestamp = self.consume_container_logs_stream(pod, container_name, logs)
else:
timestamp = self.log_iterable(logs)

except BaseHTTPError as e:
self.log.warning(
"Reading of logs interrupted with error %r; will retry. "
Expand Down Expand Up @@ -256,7 +292,7 @@ def consume_logs(*, since_time: Optional[DateTime] = None, follow: bool = True)
time.sleep(1)

def await_container_completion(self, pod: V1Pod, container_name: str) -> None:
while not self.container_is_running(pod=pod, container_name=container_name):
while self.container_is_running(pod=pod, container_name=container_name):

@schattian schattian May 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is the change unrelated to the issue.
I think the logic was reversed, blocking until the container is running (instead of blocking til terminated).

Tests are all mocking the function, so this is not detected by them.

Probably that's not a great deal as the default parameter for the operator is get_logs=True thus this is unused (til now, as i'm using it on fetch_container_logs).

I dont know if this is out of the scope of this pr and should be added separately - which guideline do you follow for these cases?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look suspiciously wrong indeed. Since this is used in your fix, I see no problem with having it as part of the PR.

time.sleep(1)

def await_pod_completion(self, pod: V1Pod) -> V1Pod:
Expand Down