Skip to content

feature: allow to disable streaming logs back to calrissian pods - #215

Open
pvbouwel wants to merge 2 commits into
Duke-GCB:masterfrom
pvbouwel:feature/disable_log_streaming
Open

feature: allow to disable streaming logs back to calrissian pods#215
pvbouwel wants to merge 2 commits into
Duke-GCB:masterfrom
pvbouwel:feature/disable_log_streaming

Conversation

@pvbouwel

Copy link
Copy Markdown

When kubernetes is configured with a logshippers it is a convenient way to get pod logs with additional kubernetes context. It is especially powerful if structured logging is emitted. For such a setup it does not really provide value to stream back logs because it causes log duplication.

The default of streaming logs does make sense so this change just provides an opt-out mechanism using an environment variable similar to how one can opt-out of POD deletion.

To make sure to handle environment variables consistently a method was extracted and tests were refactored to not be tied to implementation detail (avoiding asserting how the mock is called)

When kubernetes is configured with a logshippers it is a convenient way to get pod logs with additional kubernetes context. It is especially powerful if structured logging is emitted. For such a setup it does not really provide value to stream back logs while it does cause log duplication.

The default of streaming logs does make sense so this change just provides an opt-out mechanism using an environment variable similar to how one can opt-out to delete the PODs.

To make sure to handle environment variables consistently a method was extracted and tests were refactored to not be tied to implementation detail (avoiding asserting how the mock is called)

Copilot AI left a comment

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.

Pull request overview

This PR adds the ability to disable streaming logs back to Calrissian pods via a new CALRISSIAN_STREAM_LOGS environment variable, which is useful when Kubernetes is configured with a log shipper to avoid duplicate logs.

Changes:

  • Introduced a reusable get_bool_env_var() method to handle environment variable parsing consistently
  • Refactored should_delete_pod() to use the new helper method
  • Added should_stream_logs() method that checks CALRISSIAN_STREAM_LOGS environment variable (defaults to True)
  • Updated wait_for_completion() to conditionally call follow_logs() based on the new setting
  • Refactored tests to use a new env_vars context manager for better test isolation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
calrissian/k8s.py Adds get_bool_env_var() helper method, should_stream_logs() method, and updates wait_for_completion() to conditionally stream logs
tests/environment_variables.py Introduces a new context manager for temporarily modifying environment variables in tests
tests/test_k8s.py Refactors existing tests and adds new tests for the log streaming feature
tests/test_utils.py Adds tests for the new env_vars context manager utility
Comments suppressed due to low confidence (1)

tests/test_k8s.py:1

  • The new conditional log streaming behavior in wait_for_completion() lacks test coverage. There should be a test verifying that when CALRISSIAN_STREAM_LOGS is set to 'NO', the follow_logs() method is NOT called during the running state. Currently, only test_wait_follows_logs_pod_when_state_is_running exists which tests the default behavior.
from unittest import TestCase

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_utils.py Outdated
Comment thread tests/test_utils.py Outdated
Comment thread tests/environment_variables.py Outdated
Comment thread tests/test_k8s.py Outdated
@pvbouwel

Copy link
Copy Markdown
Author

Addressed co-pilot comments. In the meantime I also had a built with these changes and confirmed the disabling works as expected for our use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants