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

Image check improvements #459

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Image check improvements #459

merged 1 commit into from
Dec 18, 2024

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Dec 17, 2024

What

  1. Make the default image pull policy configurable. Stop setting it to PullIfNotPresent unilaterally - set it to the default if not set already.
  2. Move the creation of image check init containers to after applying PodSpecPatches, in order to catch images changed by the patches.
  3. Check each image ref is well-formed by parsing it.
  4. Derive the image pull policy to use for an image check from the policy set on the container. If none is set there, use a new configurable default. If that is not set either, default to PullAlways unless the image has a digest, in which case use PullIfNotPresent (Use PullIfNotPresent for images that have a digest? #415)
  5. Since a container with pull policy Never can fail for a new reason (ErrImageNeverPull), check for this new reason.
  6. A minor consistency cleanup in jobWatcher - stalledJobs is keyed by job UUID.

Why

  1. If someone goes to the trouble of setting imagePullPolicy on a container, we should respect that.
  2. PodSpecPatch can introduce images that slip through the pre-flight checks. (Nobody should be using PodSpecPatch to mutate the image checks currently - it would be hard to do reliably when there is more than 1 distinct image, because iteration order is essentially random.)
  3. We can avoid wasting k8s resources by parsing image refs ourselves prior to creating a k8s job, and the parsed ref is needed to decide the pull policy based on whether there is a digest or not.
  4. Fixes Use PullIfNotPresent for images that have a digest? #415 - to control what the pull policy for an image is in an image-check init container, one merely needs to set it on the corresponding containers in their podSpec{,Patch}, or configure a default when deploying the stack.
  5. While testing I noticed that the informer seems to inform podWatcher of a pending pod with waiting container in this status only once, which makes it hard to apply a grace period in the same way as ImagePullBackOff. Thus the image pull checks now work in a separate goroutine looping over the current list of pending and running pods, rather than only on informer events. However this means ImagePullBackOff is failed slightly faster!

@DrJosh9000 DrJosh9000 force-pushed the new-image-check branch 5 times, most recently from 234f166 to f646bef Compare December 17, 2024 06:44
@DrJosh9000 DrJosh9000 marked this pull request as ready for review December 17, 2024 06:49
@DrJosh9000 DrJosh9000 force-pushed the new-image-check branch 13 times, most recently from 04a20e3 to e66893e Compare December 18, 2024 03:24
@DrJosh9000 DrJosh9000 changed the title Derive image check pull policies from post-patch podSpec Image check improvements Dec 18, 2024
@DrJosh9000 DrJosh9000 force-pushed the new-image-check branch 8 times, most recently from b22dde0 to 5a7e9bb Compare December 18, 2024 04:44
Copy link
Contributor

@wolfeidau wolfeidau left a comment

Choose a reason for hiding this comment

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

Just a wee bit of feedback.

internal/controller/scheduler/job_watcher.go Show resolved Hide resolved
@@ -19,6 +21,8 @@ import (

"github.com/buildkite/agent/v3/clicommand"

"github.com/distribution/reference"
Copy link
Contributor

Choose a reason for hiding this comment

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

Handy library, have starred that one ⭐

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried finding one among the k8s libraries, and discovered they were using this under the hood.

@DrJosh9000 DrJosh9000 merged commit 2495d1b into main Dec 18, 2024
1 check passed
@DrJosh9000 DrJosh9000 deleted the new-image-check branch December 18, 2024 05:21
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.

Use PullIfNotPresent for images that have a digest?
2 participants