Repository navigation
Conversation
…e=True init_container_logs was only wired into the sync execution path (await_init_containers_completion); the async KubernetesPodTrigger had no awareness of init containers at all, so setting init_container_logs together with deferrable=True silently produced no init container logs.
KafkaOtto
requested review from
hussein-awala,
jedcunningham and
jscheffl
as code owners
September 4, 2026 09:59
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
aaron-y-chen
suggested changes
Sep 5, 2026
…s/utils/pod_manager.py handle the case when users create a pod without init_containers Co-authored-by: Aaron Chen <nailo2c@gmail.com>
aaron-y-chen
reviewed
Sep 8, 2026
Covers the pod.spec.init_containers or [] guard, requested in review.
aaron-y-chen
suggested changes
Sep 10, 2026
aaron-y-chen
left a comment
Contributor
There was a problem hiding this comment.
Hi, it seems this PR is causing the EKS part to fail, could you help fix it?
breeze run pytest \
providers/amazon/tests/unit/amazon/aws/triggers/test_eks.py::TestEksPodTrigger::test_serialize_roundtrip \
-xvs
# console output
FAILED
================================================================ FAILURES ================================================================
_______________________________________________ TestEksPodTrigger.test_serialize_roundtrip _______________________________________________
providers/amazon/tests/unit/amazon/aws/triggers/test_eks.py:407: in test_serialize_roundtrip
trigger2 = EksPodTrigger(**kwargs)
E TypeError: EksPodTrigger.__init__() got an unexpected keyword argument 'init_container_logs'
======================================================== short test summary info =========================================================
FAILED providers/amazon/tests/unit/amazon/aws/triggers/test_eks.py::TestEksPodTrigger::test_serialize_roundtrip - TypeError: EksPodTrigger.__init__() got an unexpected keyword argument 'init_container_logs'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
===================================================== 1 failed, 1 warning in 16.14s ======================================================
Error 1 returned
None
added 2 commits
September 10, 2026 09:11
…d-trip KubernetesPodTrigger.serialize() now includes init_container_logs, but EksPodTrigger's __init__ didn't accept that kwarg, so reconstructing the trigger from its own serialized kwargs raised a TypeError.
…le' into kpo-init-container-logs-deferrable
Same root cause as the EksPodTrigger fix: GKEStartPodTrigger overrides serialize() with its own explicit dict instead of delegating to the base class, and GKEStartPodOperator.invoke_defer_method never passed init_container_logs when constructing it, so init container logs were silently ignored for GKE the same way they were for base KubernetesPodOperator and EksPodOperator before this PR.
Author
|
Hi @aaron-y-chen, thanks for catching this. Both issues should be fixed now:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
init_container_logsonKubernetesPodOperatoronly worked whendeferrable=False. The sync execution path (await_init_containers_completion) fetched and streamed init container logs, butKubernetesPodTrigger(used whendeferrable=True) had no awareness of init containers at all — the parameter was silently accepted and ignored, and no init container logs ever surfaced for deferred tasks.This PR:
init_container_logsfrom the operator throughinvoke_defer_methodintoKubernetesPodTrigger, and serializes it so it survives triggerer restarts.fetch_requested_init_container_logs/_await_init_container_start/_stream_init_container_logs_until_completiontoAsyncPodManager, streaming each requested init container's logs inspec.initContainersorder once the pod leavesPending, mirroring the syncPodManager's behaviour._reconcile_requested_log_containers) into a module-levelreconcile_requested_log_containersfunction reused by bothPodManagerandAsyncPodManager.get_container_status()rather than the existingcontainer_is_terminated()helper, since the latter only inspectspod.status.container_statusesand never matches init containers (which live ininit_container_statuses), which would have caused the new polling loop to hang.closes: #72504
Test plan
AsyncPodManager.fetch_requested_init_container_logsand its helpers, including a regression test guarding thecontainer_is_terminatedpitfall above.KubernetesPodTrigger._wait_for_pod_starttest confirming init container logs are (and aren't, when unset) fetched.init_container_logsreaches the trigger viainvoke_defer_method.uv run ruff format/ruff check --fixclean on all changed files.mypyclean on all changed source files.pytestrun of the three affected test files: all pass except two pre-existing, unrelated flaky tests (since_secondstimezone-sensitive assertions that also fail onmainwithout this change).Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Sonnet 5) following the guidelines