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.

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

Add container logs to restart count error messages #383

Merged
merged 11 commits into from
Jan 6, 2022

Conversation

linkous8
Copy link
Contributor

  • Add types module for kubernetes
  • Implement new Pod method get_logs_for_container_statuses
  • Add include_container_logs param to raise_for_status methods
  • Include container logs in rejection messages for restart counts (
    implementation for other failures TBD)
  • Introduce container_logs_in_error_status flag to kubernetes config

- Add types module for kubernetes
- Implement new Pod method get_logs_for_container_statuses
- Add include_container_logs param to raise_for_status methods
- Include container logs in rejection messages for restart counts (
implementation for other failures TBD)
- Introduce container_logs_in_error_status flag to kubernetes config
@linear
Copy link

linear bot commented Dec 20, 2021

ENG-726 Servox should include logs for crashed containers in its error messages

When TeaStore environment variable injection is misconfigured, the JVM will fail to start on settings that may otherwise be valid. By including container logs in the rejection messages, we will be able to tell if this is the case at a glance

@linkous8 linkous8 marked this pull request as ready for review December 22, 2021 20:38
@linkous8 linkous8 requested a review from DanielHHowell January 6, 2022 17:19
@DanielHHowell
Copy link
Contributor

Looks immaculate as usual. As far as test coverage I presume there is good testing for the underlying functionality it's modifying or any additional testing would be kind of byzantine integration tests? Are any unit tests warranted?

@linkous8
Copy link
Contributor Author

linkous8 commented Jan 6, 2022

Looks immaculate as usual. As far as test coverage I presume there is good testing for the underlying functionality it's modifying or any additional testing would be kind of byzantine integration tests? Are any unit tests warranted?

Unit tests to be added in a later update. Thus far, it has been pen tested and is disabled by default so poses minimal risk of disruption

@linkous8 linkous8 merged commit 4fc5ebf into main Jan 6, 2022
@linkous8 linkous8 deleted the fred/eng-726-servox-should-include-logs-for-crashed branch January 6, 2022 18:31
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