Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add logging support for init containers in KubernetesPodOperator (#42498) #43853

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Add logging support for init containers in KubernetesPodOperator (#42498) #43853

merged 1 commit into from
Dec 17, 2024
+210 −13

Conversation

mrk-andreev
Copy link
Contributor

@mrk-andreev mrk-andreev commented Nov 9, 2024 •
edited
Loading

This change adds an option to print logs for init containers. The init_container_logs argument enables the display of logs specifically for spec.initContainers (not spec.containers).

Fixes: #42498

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues labels Nov 9, 2024
@potiuk
Copy link
Member

potiuk commented Nov 9, 2024 •
edited
Loading

@dstandish @jedcunningham @jscheffl @hussein-awala -> 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 :)

jscheffl
jscheffl reviewed Nov 9, 2024
View reviewed changes
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

I also only feel 50% competent, added some feedback from just code reading.

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?

kubernetes_tests/test_kubernetes_pod_operator.py Outdated Show resolved Hide resolved
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py Outdated Show resolved Hide resolved
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py Outdated Show resolved Hide resolved
@mrk-andreev
Copy link
Contributor Author

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.

@mrk-andreev mrk-andreev force-pushed the issue-42498 branch 2 times, most recently from d58a784 to c77bc4d Compare November 10, 2024 00:06
@mrk-andreev mrk-andreev changed the title Add support logging in KubernetesPodOperator (#42498) Add logging support for init containers in KubernetesPodOperator (#42498) Nov 10, 2024
@mrk-andreev mrk-andreev force-pushed the issue-42498 branch 2 times, most recently from c0e0a1d to bbb224d Compare November 10, 2024 22:41
hussein-awala
hussein-awala reviewed Nov 10, 2024
View reviewed changes
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Overall it looks good, just need to check if the pod was started before you start reading the logs from init containers, the rest is nit.

providers/src/airflow/providers/cncf/kubernetes/operators/pod.py Outdated Show resolved Hide resolved
providers/src/airflow/providers/cncf/kubernetes/operators/pod.py Show resolved Hide resolved
providers/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py Show resolved Hide resolved
@mrk-andreev mrk-andreev force-pushed the issue-42498 branch 4 times, most recently from 15527a3 to 9baf12d Compare November 12, 2024 22:57
@mrk-andreev mrk-andreev force-pushed the issue-42498 branch 2 times, most recently from fb62e27 to e710eae Compare November 14, 2024 23:51
@mrk-andreev
Copy link
Contributor Author

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!

potiuk
potiuk approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. But I think some questions/answers need confirmation from @hussein-awala and @dstandish

@potiuk
Copy link
Member

potiuk commented Dec 3, 2024

Can you rebase in the meantime @mrk-andreev ?

@eladkal eladkal requested a review from romsharon98 December 12, 2024 02:25
eladkal
eladkal approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM

If no further comments I will merge this PR for next provider wave

@potiuk
Copy link
Member

potiuk commented Dec 16, 2024

LGTM

If no further comments I will merge this PR for next provider wave

Let's. I think it looks good.

@mrk-andreev @potiuk
Add logging support for init containers in KubernetesPodOperator (apa…
f18553b 
…che#42498)

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: apache#42498
@potiuk
Copy link
Member

potiuk commented Dec 16, 2024

I rebased it - and let's give @dstandish and @hussein-awala last chance to comment.

@potiuk potiuk merged commit 60eced9 into apache:main Dec 17, 2024
76 checks passed
@mrk-andreev mrk-andreev mentioned this pull request Dec 18, 2024
Open
2 tasks
@eladkal eladkal mentioned this pull request Dec 22, 2024
Closed
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
@mrk-andreev
Add logging support for init containers in KubernetesPodOperator (apa…
2aa1370 
…che#42498) (apache#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: apache#42498
agupta01 pushed a commit to agupta01/airflow that referenced this pull request Jan 6, 2025
@mrk-andreev @agupta01
Add logging support for init containers in KubernetesPodOperator (apa…
5c18fee 
…che#42498) (apache#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: apache#42498
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jscheffl jscheffl jscheffl left review comments

@dstandish dstandish dstandish left review comments

@eladkal eladkal eladkal approved these changes

@potiuk potiuk potiuk approved these changes

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@hussein-awala hussein-awala Awaiting requested review from hussein-awala hussein-awala is a code owner

@romsharon98 romsharon98 Awaiting requested review from romsharon98

Assignees
No one assigned
Labels
area:providers mans-hackathon provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kubernetes Pod Operator Init container logging
6 participants
@mrk-andreev @potiuk @dstandish @hussein-awala @eladkal @jscheffl