Fix CloudWatch remote logging for ephemeral lifecycle executor - #68779
Conversation
|
cc @sarvesh371, @seanghaeli Could you verify this patch for your setup when you have a moment? Since it likely #66633 won't catch the 3.3 release (we're close to dev freeze for 3.3), so we might release this provider-only patch first. Thanks. |
The streaming CloudWatch handler is rebuilt whenever it reports
shutting_down, so logs survive configure_logging() closing it. But
shutting_down alone cannot tell a mid-task close apart from genuine
teardown, so a record arriving after teardown would spin up an orphan
handler and its background queue thread that nobody flushes or closes.
The supervisor lifecycle makes the two cases distinguishable in time:
1. configure_logging() builds the handler via remote.processors
(processors does `_ = self.handler`), registering it in
logging._handlerList.
2. The same call then runs dictConfig, whose non-incremental reset
closes that handler -> watchtower sets shutting_down=True.
3. Child log records stream through proc -> self.handler, which sees
shutting_down and rebuilds. This is the case we must keep working.
4. At the last possible moment _upload_logs() -> upload() -> close()
flushes; nothing logs after this.
shutting_down is watchtower's flag set by dictConfig (step 2); the new
_closed flag is ours, set only by close() (step 4). dictConfig never
touches _closed, so the rebuild in step 3 still fires, while a late
record after step 4 keeps the closed handler instead of orphaning a new
one. close() on the outer CloudwatchTaskHandler now closes the handler
the IO is currently using rather than the reference captured in
set_context(), which dictConfig may have closed and the IO since rebuilt.
a92793f to
f28925a
Compare
ferruzzi
left a comment
There was a problem hiding this comment.
I haven't played with CloudWatch much, but I left some style nitpicks. Also, Claude loves to over-comment code, you may want to clean some of those up/
|
Thanks all for the review, then I guess it's fine to merge this? |
seanghaeli
left a comment
There was a problem hiding this comment.
Verified it in breeze just like the upstream PR and seeing the cloudwatch logs work e2e
eladkal
left a comment
There was a problem hiding this comment.
LGTM but lets get +1 from @o-nikolas / @vincbeck / @ferruzzi before merge
ferruzzi
left a comment
There was a problem hiding this comment.
Looks like my comments were addressed. Thanks.
…nges Per reviewer guidance, this PR targets task-sdk changes only. Provider-side fixes are covered by apache#68779.
…nges Per reviewer guidance, this PR targets task-sdk changes only. Provider-side fixes are covered by apache#68779.
…nfigure_logging In configure_logging(), getattr(remote, 'processors') was called before dictConfig() ran. dictConfig() calls _clearExistingHandlers() which closes every handler in logging._handlerList — including the remote handler built moments earlier. This caused CloudWatch/Watchtower logs to be silently dropped when using the Task SDK with remote logging on ECS/Kubernetes workers. Fix: move the remote processor injection to after dictConfig() has run via a second structlog.configure() call. Also gate on not sending_to_supervisor to avoid unmasked events from the task subprocess, and add a None default to getattr() to handle third-party RemoteLogIO objects. Provider-side fix: apache#68779 Closes apache#66475
…nfigure_logging In configure_logging(), getattr(remote, 'processors') was called before dictConfig() ran. dictConfig() calls _clearExistingHandlers() which closes every handler in logging._handlerList — including the remote handler built moments earlier. This caused CloudWatch/Watchtower logs to be silently dropped when using the Task SDK with remote logging on ECS/Kubernetes workers. Fix: move the remote processor injection to after dictConfig() has run via a second structlog.configure() call. Also gate on not sending_to_supervisor to avoid unmasked events from the task subprocess, and add a None default to getattr() to handle third-party RemoteLogIO objects. Provider-side fix: apache#68779 Closes apache#66475
Why
While trying to setup cloudwatch remote logging in #68709 in order to persist the logs in real time. I encounter the same errors as above listed issues.
The root cause I found is same as #66475 (comment) pointed out. The
configure_logging -> dictConfig -> _clearExistingHandlerscall chain shutdown the watchtower handler.How
I went to the another than #66633, instead of configuring the
processorsafter thedictConfigcall. We could make the cloudwatch remote logger itself self-healing by creating the fresh instance if previous instance wasshutdownby thedictConfigcall but also ensure the.closesemantic by guarding with the_closestate.What
Fix the lifecycle issue of cloudwatch remote logging and verify with
breeze k8ssystem test with provider only changes without touching the Task-SDK changes.