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

feat(kubernetes): Surface Image Pull Errors in Kubernetes Runtime #279

Closed
wants to merge 13 commits into from

Conversation

cognifloyd
Copy link
Member

The Docker Runtime surfaces image pull errors naturally because of the blocking synchronous requests to pull an image or start a container.
Kubernetes is asynchronous, so we have to explicitly watch for events to make sure the image pull succeeds in RunContainer.
When we capture a failure event, we can surface the event in the same way as the Docker runtime.

There may be other places where we need to watch for events, so I extracted some of the test logic from WaitContainer to make it reusable.

Also, this changes the init step so it says "Preparing" instead of "Pulling" since the Kubernetes runtime does NOT pull during init.

  • Replace "Pulling" with "Preparing" in init step logs
  • More image validation in kubernetes runtime
  • Rename watch->podWatch for planned imports
  • Extract Kubernetes Client + Watch mocking logic
  • Escape selector just in case
  • Move newMockWithWatch to kubernetes_test.go
  • Close the podWatch in WaitContainer
  • Enhance RunContainer to watch for image pull events
  • Add RunContainer image pull failure test

@cognifloyd cognifloyd requested a review from a team as a code owner February 26, 2022 01:28
@codecov
Copy link

codecov bot commented Feb 26, 2022

Codecov Report

Merging #279 (3df89d0) into master (c1d4563) will decrease coverage by 0.13%.
The diff coverage is 67.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
- Coverage   78.91%   78.77%   -0.14%     
==========================================
  Files          67       67              
  Lines        4870     4952      +82     
==========================================
+ Hits         3843     3901      +58     
- Misses        884      903      +19     
- Partials      143      148       +5     
Impacted Files Coverage Δ
runtime/kubernetes/image.go 65.62% <62.50%> (+13.62%) ⬆️
runtime/kubernetes/container.go 75.21% <67.53%> (-2.40%) ⬇️

kneal
kneal previously approved these changes Mar 1, 2022
Copy link
Contributor

@kneal kneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐬

@kneal kneal changed the title Surface Image Pull Errors in Kubernetes Runtime feat: Surface Image Pull Errors in Kubernetes Runtime Mar 1, 2022
@kneal kneal added the feature Indicates a new feature label Mar 1, 2022
wass3r
wass3r previously approved these changes Mar 2, 2022
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@cognifloyd cognifloyd dismissed stale reviews from wass3r and kneal via eb3ba93 March 10, 2022 03:44
@wass3r
Copy link
Collaborator

wass3r commented Mar 11, 2022

can you please fix up the new linting errors? we added/modified some rules that triggered a few issues.

@cognifloyd
Copy link
Member Author

I'm really not sure what to do to replace context.Background() for the k8s no API requests. The other instances were skipped, stop that's what I did. Is there a better way?

@cognifloyd cognifloyd marked this pull request as draft March 14, 2022 20:47
@cognifloyd
Copy link
Member Author

I'm going to break this down into more discrete PRs. As I'm studying issues with the k8s runtime, I'm considering some different ways to subscribe to the event stream related to a build.

@cognifloyd cognifloyd changed the title feat: Surface Image Pull Errors in Kubernetes Runtime feat(kubernetes): Surface Image Pull Errors in Kubernetes Runtime Mar 15, 2022
@cognifloyd
Copy link
Member Author

Closing in favor of #331

@cognifloyd cognifloyd closed this May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates a new feature
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants