If the image with :latest tag specified in spec, pull even PullIfNotPresent is set. #2388
Conversation
to pull the latest one even the policy is PullIfNotPresent.
| func TestSyncPodsWithPullPolicy(t *testing.T) { | ||
| kubelet, _, fakeDocker := newTestKubelet(t) | ||
| puller := kubelet.dockerPuller.(*dockertools.FakeDockerPuller) | ||
| puller.HasImages = []string{"existing_one", "want:latest"} |
There was a problem hiding this comment.
This ("want:latest") isn't a state of knowledge that can actually exist in reality-- you never know if you have the latest thing sitting on disk. Or do I misunderstand?
There was a problem hiding this comment.
HasImage in fakeDockerPull represents the existence of image on fake node, which means IsImagePresent call will return true here. Then the test case with want:latest pull it again even it is existing.
|
LGTM |
If the image with :latest tag specified in spec, pull even PullIfNotPresent is set.
|
@brendandburns Hopefully this addresses your concerns. |
|
Does Docker automatically add ":latest" to images with no version? |
|
@brendandburns Docker won't. Instead dockertool/docker.go does; otherwise, all versions will be pulled by docker, and container startup latency will be increased dramatically. With this PR, you can think like we have two stages on an image pulling: 1) decide if pull based on policy, which is addressed in this PR 2) if yes on 1), always pull with latest one, which is implemented in dockertool. |
|
Yes, but the thing that matters here is whether people listed :latest in their pod. It's good to be in the habit of specifying the version anyway, I suppose? Now I'm less sure. |
|
I understand this was done to mitigate #2360, but this is really, really confusing. We should not be overriding policies set by the user. We started down the wrong path with #1458, and we're just digging the hole deeper and deeper. We need a function that looks at a PodSpec and sets the default value if it is not set. This would ensure that we wouldn't override what the user specified, and would enable the user to easily inspect the policy we were actually applying. In that case, we could set the policy intelligently in this way: PullIfNotPresent, unless the image has the latest tag, in which case we set it to PullAlways. If we added a PullEvery{seconds}, we could default to something like PullEvery{14400} (4 hours). Since Kubelet also will take Pods from other sources (e.g., files) in addition to from the apiserver, it should just call the same validation/conversion code on those as the apiserver does. /cc @thockin |
|
@brendandburns if ":latest" is not specified, docker pull will download the images with all tags, but docker run will automatically append ":latest" tag to run. The recent version of docker should have ":latest" automatically append to both pull and run. @bgrant0607 Yes, I agreed with you. In my previous version of this PR, I mentioned the same issue. @lavalamp and I briefly discussed this very topic, the problem is that there is no way for me to put it in helper.go since we need to inspect ContainerSpec itself. At the end, we thought we are going to unify the default setting separately later, including PullPolicy, RestartPolicy, etc., let' s handle them together later. |
|
Yeah after more thought over the weekend I independently came to @bgrant0607's conclusion. What we're doing is too ad-hoc. I'm not sure I like any other options, either, though. Maybe we should focus more on nerfing pullalways/ifnotpresant until we figure out the best expression in the API. |
|
As a note, this change really burns us when trying to test in local environments. I'm really in favor of yanking this - I don't think latest should override pullIfNotPresent |
|
It should only have overridden the default if no explicit value was --brendan On Mon, Jan 12, 2015 at 1:40 PM, Clayton Coleman notifications@github.com
|
|
to be clear: this is a bug if it's not the case. On Mon, Jan 12, 2015 at 1:50 PM, Brendan Burns bburns@google.com wrote:
|
|
@brendanburns What you described here is exact opposite of what we implemented here. When the user asks for latest of image with PullIfNotPresent (which is default policy), we treat it as Always. It is our policy choice, not a bug. But it is arguable if it is a good policy or not. @smarterclayton I am ok to remove this, but before we do it, can I understand your use case more? |
|
Our use case is doing test and development on Kubernetes (and things that run on Kubernetes that run in containers) is that we simultaneously want to expose our images as Docker Hub images ("openshift/docker-registry") but test them locally against non-master. We can mutate our references to not be latest, but latest isn't any more special than any other tag. If the user says "pullIfNotPresent" and we "pullAlways", that sounds like a bug (a user certainly wouldn't expect it). Otherwise, we shouldn't allow someone to specify pullIfNotPresent. And forcing users who want that behavior to use different tag names is a bad idea, because now we're enforcing a certain pattern on how users use tags in repositories they don't control. ----- Original Message -----
|
|
To be clear, I wanted to identify the cases: In this container, Pull Policy is set to "PullAlways" In these containers, Pull Policy is unchanged { { Hope that helps. On Mon, Jan 12, 2015 at 2:06 PM, Clayton Coleman notifications@github.com
|
|
----- Original Message -----
Right - this is the case which today is "broken" - in that the value the REST object contains (pullIfPresent) is ignored and the pull policy is converted to pull always when doesn't contain a tag. |
|
Yes, the latest use case is broken on purpose to mitigate #2360 and continue supporting ContainerVM users. If I remembered correctly, PullPolicy's default setting implementation is different from other default setting, and we were not convergent on which way is the right way. Meanwhile, the current approach of PullPolicy cannot have image_name:tag easily. I think now we are convergent on how to setting defaults, and kubelet start to have validation too. Will send PR to fix the issue. |
|
@brendandburns @smarterclayton @thockin I were wrong. #2587 is not merged yet and we are not convergent on how to set defaults here. |
|
#2587 hasn't been merged yet, but I think we have converged on the approach we want to take. We just need something to push it to completion. |
No description provided.