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

Live update without injecting an image #4577

Closed
ninjaprox opened this issue May 21, 2021 · 9 comments · Fixed by #4768
Closed

Live update without injecting an image #4577

ninjaprox opened this issue May 21, 2021 · 9 comments · Fixed by #4768
Assignees
Labels
enhancement New feature or request

Comments

@ninjaprox
Copy link

Describe the Feature You Want

Allow to config imagePullPolicy whenever Tilt's managing an image build

Current Behavior

imagePullPolicy is always set to IfNotPresent

Why Do You Want This?

I have a development version of an image, e.g. example.com/my-app:dev and use sync_file_only extension to sync local changes to the container and hot reload the app with new changes in the container without having to restart the pod. sync_file_only does the trick with custom_build (it actually doesn't build anything) for this, which causes Tilt to override imagePullPolicy to IfNotPresent. When I have some changes in example.com/my-app:dev, the container doesn't pull the latest version due to imagePullPolicy: NotIfPresent.

I read through the discussion in #3209. It looks like Always is fast enough to use (I tested with a custom Tilt build). According to this documentation, Always doesn't pull the image if the image digest unchanged.

Therefore I'd suggest having the ability to config imagePullPolicy or not overriding it to IfNotPresent.

@ninjaprox ninjaprox added the enhancement New feature or request label May 21, 2021
@landism
Copy link
Member

landism commented May 21, 2021

Thanks for the report!

It looks like:

  1. The current behavior was predicated at least partially on the understanding that imagePullPolicy: Always meant it'd always pull. As you point out, the documentation now says that it is not the case (the docs were updated after that comment - no idea when/if the actually behavior change happened)
  2. There is a separate discussion of whether using ":latest" in dev is a good idea. As Nick says, teams generally try to keep their builds hermetic and avoid specifying unpinned versions of any dependencies in their build configs. As you may be aware, at some point these dependencies will have non-backwards-compatible changes, which causes all kinds of annoying problems such as breaking devs' environments until they update, making CI runs non-reproducible, and making it hard/impractical to bisect or check behavior at an old commit when tracking down the source of a behavior change.

Of course, all decisions are tradeoffs, and it's also annoying to specify the image digest in your config in git and have it automatically bumped when new images are pushed. I just wanted to make sure to bring that up since the discussion here had only covered the speed side of things.

I think we'll need to look into this and talk about it a bit.

In the meantime, you might be able to work around it with the AlwaysPullImages admission controller

@nicks
Copy link
Member

nicks commented May 21, 2021

Re: "Always doesn't pull the image if the image digest unchanged."

Here's what the documentation says:

the kubelet queries the container image registry to resolve the name to an image digest. If the kubelet has a container image with that exact digest cached locally, the kubelet uses its cached image; otherwise, the kubelet downloads (pulls) the image with the resolved digest, and uses that image to launch the container.

The problem is that when Tilt manages an image build, it wants to be responsible for getting the image into the container runtime. AFAIR, there are a few different cases where querying the container image registry will end up getting the wrong image into the container runtime, and will lead to incorrect behavior.

(There are also performance reasons at play, but they're a bit more nuanced now due to some advancements in layer caching and stargz)

@ninjaprox
Copy link
Author

Since Tilt relies on imagePullPolicy for its control of an image build, would implicit setting imagePullPolicy: IfNotPresent cause potential changes in the future when imagePullPolicy changes? AlthoughIfNotPresent works for most cases at the moment, it'll be becoming not necessary anymore (Always seems to work well). Keeping IfNotPresent as a default still makes sense and is safe (in few different cases Nick mentioned), whilst allowing to control imagePullPolicy opens the flexibility for different team workflows, which is discussed in point 2 above.

In different circumstances, how to tag images, whether or not continuously update a tag (can be :latest, :dev or any) with the most recent version might be very varied. For example, in this case, example.com/my-app:dev is a toolbox image with a dummy binary of the service. The binary will be compiled on change local code. As a result :dev isn't bound to any specific version of the service, what changes to :dev might be adding/upgrading debugging tools so it makes sense to keep one :dev rather than versioned :dev. The question is do we want to change a team workflow to adopt a tool? Or a tool should be reasonably adaptable to different workflows?

AlwaysPullImages admission controller mightn't be a workaround since it applies to every new Pod which isn't in this case.

@ninjaprox
Copy link
Author

@landism, @nicks any thoughts?

@nicks
Copy link
Member

nicks commented Jul 2, 2021

ya, i've been thinking about this problem.

imagePullPolicy: Always will have incorrect behavior with many common image-builders (most notably, Minikube's Docker socket and Kubernetes on Docker for Desktop). Tilt is building directly into the container runtime rather than pushing to an upstream registry, so pulling from the upstream registry won't work right.

This matters a lot to us because most of our users use Tilt to build images.

file_sync_only does a "fake" no-op image build. So, as a file_sync_only user, breaking image builds doesn't matter, because you're not using it for image builds anyway.

The use-case you're describing reminds me of that old xkcd cartoon about "spacebar heating" - https://xkcd.com/1172/.

Like, ya, we could add an option that breaks image builds but makes file_sync_only work correctly, but I think the real solution here it to have a file_sync_only API that doesn't rely on fake image builds.

does that make sense?

@ninjaprox
Copy link
Author

Tilt is building directly into the container runtime rather than pushing to an upstream registry, so pulling from the upstream registry won't work right.

I'm not pretty sure about this point. If not pushing to a registry (either local or remote), how can a container runtime pull an image built by Tilt? Could you explain more?

With Tilt APIs at the moment, I'm not sure how to achieve files sync like file_sync_only without docker_build or custom_build?

@nicks
Copy link
Member

nicks commented Jul 6, 2021

With Tilt APIs at the moment, I'm not sure how to achieve files sync like file_sync_only without docker_build or custom_build?

Haha, yes @ninjaprox ! You're exactly right. I'm suggesting we need to fix the Tilt API to support this

As a short-term hack, I wonder if we could special-case Tilt so that if it sees a custom_build that uses ':' like here:
https://github.com/tilt-dev/tilt-extensions/blob/master/file_sync_only/Tiltfile#L28
then it doesn't do any image substitution or image policy substitution at all. I think that would fix the problem, right?

@ninjaprox
Copy link
Author

Yes, that would work too. Compared with the option for whether or not to override imagePullPolicy, it seems simpler and has less effect on others. If we're okay with the short-term hack, I could tinker with it for a PR.

@nicks nicks changed the title Allow to config imagePullPolicy Live update without injecting an image Jul 15, 2021
@nicks
Copy link
Member

nicks commented Jul 15, 2021

I updated the issue description to express what we really want from this PR - a way to live-update an image without replacing the image or updating its imagePullPolicy. A couple teams have asked for this and i think i have an idea on how to do it cheaply

@nicks nicks self-assigned this Jul 15, 2021
nicks added a commit that referenced this issue Jul 16, 2021
This is the first step towards a general-purpose live-update API
that's independent of a general-purpose image build API, and suggests
how they might interoperate.

Fixes #4577

I played around with a couple different ways to test this effectively,
but ultimately, the most valuable test was the file_sync_only
integration test we already have. Almost all of our live-update
specs are ultimately designed to catch bugs in how we do the live-update,
rather than asserting things about the live-update spec itself.
nicks added a commit that referenced this issue Jul 16, 2021
This is the first step towards a general-purpose live-update API
that's independent of a general-purpose image build API, and suggests
how they might interoperate.

Fixes #4577

I played around with a couple different ways to test this effectively,
but ultimately, the most valuable test was the file_sync_only
integration test we already have. Almost all of our live-update
specs are ultimately designed to catch bugs in how we do the live-update,
rather than asserting things about the live-update spec itself.
nicks added a commit that referenced this issue Jul 16, 2021
This is the first step towards a general-purpose live-update API
that's independent of a general-purpose image build API, and suggests
how they might interoperate.

Fixes #4577

I played around with a couple different ways to test this effectively,
but ultimately, the most valuable test was the file_sync_only
integration test we already have. Almost all of our live-update
specs are ultimately designed to catch bugs in how we do the live-update,
rather than asserting things about the live-update spec itself.
nicks added a commit that referenced this issue Jul 16, 2021
This is the first step towards a general-purpose live-update API
that's independent of a general-purpose image build API, and suggests
how they might interoperate.

Fixes #4577

I played around with a couple different ways to test this effectively,
but ultimately, the most valuable test was the file_sync_only
integration test we already have. Almost all of our live-update
specs are ultimately designed to catch bugs in how we do the live-update,
rather than asserting things about the live-update spec itself.
nicks added a commit that referenced this issue Jul 16, 2021
…4768)

This is the first step towards a general-purpose live-update API
that's independent of a general-purpose image build API, and suggests
how they might interoperate.

Fixes #4577

I played around with a couple different ways to test this effectively,
but ultimately, the most valuable test was the file_sync_only
integration test we already have. Almost all of our live-update
specs are ultimately designed to catch bugs in how we do the live-update,
rather than asserting things about the live-update spec itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants