Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Read the stderr into termination reason when container fails #87

Merged
merged 1 commit into from
May 13, 2020

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented May 12, 2020

TL;DR

This change will automatically read the pod log and set it as the termination reason when the container exits with non-zero exit code. In case of flytekit, and it fails to write error.pb, the log will have more information to surface to users.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

flyteorg/flyte#308

@codecov-io
Copy link

codecov-io commented May 12, 2020

Codecov Report

Merging #87 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #87   +/-   ##
=======================================
  Coverage   54.98%   54.99%           
=======================================
  Files          91       91           
  Lines        4661     4662    +1     
=======================================
+ Hits         2563     2564    +1     
  Misses       1821     1821           
  Partials      277      277           
Impacted Files Coverage Δ
...tasks/pluginmachinery/flytek8s/container_helper.go 83.33% <100.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39d709c...276ec48. Read the comment docs.

@kumare3
Copy link
Contributor

kumare3 commented May 12, 2020

No parsing just dropping a better error message

@kumare3
Copy link
Contributor

kumare3 commented May 12, 2020

I was thinking if we should use this always?

@EngHabu
Copy link
Contributor Author

EngHabu commented May 12, 2020

I think it'll almost always give you a better idea.... I'm thinking we can further limit the size of the message on our side...
What are your concerns?

@kumare3 kumare3 merged commit dc1ce43 into master May 13, 2020
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants