Guard StackdriverRemoteLogIO's transport.send() against errors - #71337
Open
roshanprabu wants to merge 1 commit into
Open
Guard StackdriverRemoteLogIO's transport.send() against errors#71337roshanprabu wants to merge 1 commit into
roshanprabu wants to merge 1 commit into
Conversation
The log processor's transport.send() call ran unguarded. This processor is installed for every supervised component (scheduler, dag-processor, triggerer, workers) whenever REMOTE_TASK_LOG routes through Stackdriver, so any IAM permission error, gRPC connectivity failure, or quota exception from Cloud Logging propagated straight out of the structlog processor and crashed the entire process -- observed as a dag-processor stuck in CrashLoopBackOff from a missing logging.logEntries.create IAM binding. The read path already has this exact guard one function down (see the try/except around read_logs() a few lines below, with the same rationale in its comment); the write path was simply missing the equivalent. Log delivery is best-effort, so this logs a warning via the handler's own logger instead of raising.
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
Fixes one of three bugs reported in #68240 (the one I could verify is still present and fix with high confidence -- see scope note below).
StackdriverRemoteLogIO.processors'procclosure calls_transport.send(...)with no error handling:This processor is installed for every supervised component (scheduler, dag-processor, triggerer, workers) whenever
REMOTE_TASK_LOGroutes through Stackdriver. Any IAM permission error, gRPC connectivity failure, or quota exception from Cloud Logging propagates straight out of the structlog processor and takes down the whole process -- reported as a dag-processor pod stuck inCrashLoopBackOffon every log emit when its Kubernetes Service Account lacked thelogging.logEntries.createIAM binding.The read path already has exactly this guard, a few lines down in the same file, with a comment explaining the rationale:
The write path was simply missing the equivalent. This PR adds it, reusing the same
_logger(already defined in this file specifically for handler-internal failures) and logging a warning instead of raising, since log delivery here is best-effort.Scope note: the linked issue reports three bugs. I traced the other two (empty labels when
record.task_instanceis unset in supervisor context, andread()filtering onlogical_date) against currentmainand found the code has moved on since the report -- there's now a fallback inproc()that reads labels from the event dict when notask_instanceis present (seetest_processors_fallback_to_event_labels), andread()'sti: RuntimeTIalready exposeslogical_datelocally without a DB round-trip. I wasn't confident those two are still bugs as described, so this PR only covers the third (transport.send()unguarded), which I could verify precisely.Test plan
test_processors_survives_transport_send_failure: mockstransport.send()to raise, asserts the processor doesn't propagate the exception.Exception: IAM permission deniedpropagating out ofproc(); re-applied the fix and confirmed it passes.pytest tests/unit/google/cloud/log/test_stackdriver_task_handler.py-- 33 passed.ruff checkandruff format --checkpass on both changed files.