Properly handle podman run --pull command #7770

rhatdan · 2020-09-24T22:42:55Z

Currently the --pull missing|always|never is ignored

This PR implements this for local API. For remote we
need to default to pullpolicy specified in the containers.conf
file.

Also fixed an issue when images were matching other images names
based on prefix, causing images to always be pulled.

I had named an image myfedora and when ever I pulled fedora, the system
thought that it there were two images named fedora since it was checking
for the name fedora as well as the prefix fedora. I changed it to check
for fedora and the prefix /fedora, to prefent failures like I had.

Signed-off-by: Daniel J Walsh [email protected]

openshift-ci-robot · 2020-09-24T22:43:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg

Can you add tests?

I recall a couple of regressions on pull policies in the past and it seems they're turning back :(

libpod/image/utils.go

vrothberg

LGTM

TomSweeneyRedHat · 2020-09-25T19:25:34Z

test/e2e/run_test.go

+		session.WaitWithDefaultTimeout()
+
+		session = podmanTest.PodmanNoCache([]string{"rmi", "--force", "never", ALPINE})
+		session.WaitWithDefaultTimeout()


Did you really mean to do the rmi twice?

minor comment, but still would like an answer.

TomSweeneyRedHat · 2020-09-25T19:29:02Z

test/e2e/run_test.go

+		Expect(session.ErrorToString()).ToNot(ContainSubstring("Trying to pull"))
+	})
+
+	It("podman run container with --pull missing shoul pull image multiple times", func() {


nit

Suggested change

It("podman run container with --pull missing shoul pull image multiple times", func() {

It("podman run container with --pull missing should pull image multiple times", func() {

test/e2e/run_test.go

TomSweeneyRedHat · 2020-09-25T19:34:31Z

tests aren't hip @rhatdan

rhatdan · 2020-09-25T20:18:34Z

For some reason Apparmor tests failed.

rhatdan · 2020-09-26T13:14:08Z

Fixed the tests.

TomSweeneyRedHat · 2020-09-26T18:40:06Z

Test happy @rhatdan, but one minor nit and a question that might cause more changes to the test (or a head slap for me).

Currently the --pull missing|always|never is ignored This PR implements this for local API. For remote we need to default to pullpolicy specified in the containers.conf file. Also fixed an issue when images were matching other images names based on prefix, causing images to always be pulled. I had named an image myfedora and when ever I pulled fedora, the system thought that it there were two images named fedora since it was checking for the name fedora as well as the prefix fedora. I changed it to check for fedora and the prefix /fedora, to prefent failures like I had. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2020-09-28T12:34:14Z

@containers/podman-maintainers PTAL
This should be ready to go in.

giuseppe · 2020-09-28T14:15:04Z

LGTM

ashley-cui

LGTM

QiWang19 · 2020-09-28T14:33:55Z

/lgtm

edsantiago · 2020-09-28T16:07:10Z

I'm really confused. The following system test was written precisely to catch this:

podman/test/system/030-run.bats

Lines 134 to 156 in b0e70a6

    
           @test "podman run --pull" { 
        
               skip_if_remote "podman-remote does not emit 'Trying to pull' msgs" 
        
               run_podman run --pull=missing $IMAGE true 
        
               is "$output" "" "--pull=missing [present]: no output" 
        
               run_podman run --pull=never $IMAGE true 
        
               is "$output" "" "--pull=never [present]: no output" 
        
               # Now test with a remote image which we don't have present (the 00 tag) 
        
               NONLOCAL_IMAGE="$PODMAN_TEST_IMAGE_REGISTRY/$PODMAN_TEST_IMAGE_USER/$PODMAN_TEST_IMAGE_NAME:00000000" 
        
               run_podman 125 run --pull=never $NONLOCAL_IMAGE true 
        
               is "$output" "Error: unable to find a name and tag match for $NONLOCAL_IMAGE in repotags: no such image" "--pull=never [with image not present]: error" 
        
               run_podman run --pull=missing $NONLOCAL_IMAGE true 
        
               is "$output" "Trying to pull .*" "--pull=missing [with image NOT PRESENT]: fetches" 
        
               run_podman run --pull=missing $NONLOCAL_IMAGE true 
        
               is "$output" "" "--pull=missing [with image PRESENT]: does not re-fetch" 
        
               run_podman run --pull=always $NONLOCAL_IMAGE true 
        
               is "$output" "Trying to pull .*" "--pull=always [with image PRESENT]: re-fetches"

As best my eye can tell, these tests (which are old and have been passing for many months) are identlcal to the ones you added -- which leads me to believe that the tests you added would have passed before your PR. Did you try running those tests without your changes, to see if they passed? And, do you remember or have a log of what was failing before your PR?

DO NOT MERGE! This is a dummy PR for the sole purpose of running CI tests. I really need to understand why my system tests did not catch the "podman run --pull" problem addressed by containers#7770. So this commit leaves the new tests in place, but reverts the code changes. That should make these tests fail, and I will be better able to understand why my tests failed to catch it. I thought I had ginkgo working on my laptop, but running: ginkgo -trace -debug -r -focus 'with --pull' test/e2e ...only yields success. I suspect there's something deeper going on here but can only find out by running the full CI test suite. Sorry to be wasting cycles. This reverts commit 1b5853e. Signed-off-by: Ed Santiago <[email protected]>

Obscure corner case in which 'podman run --pull=never alpine' will actually pass *with no alpine image* if there's an image named "myalpine". (i.e. a substring match, not full string match). Fixed in containers#7770 but the tests that were added there do not actually test that. This adds a double-duty test for that as well as making sure that 'run --pull=never SHORTNAME' (implicit :latest) does not match our existing :YYYYMMDD image; then one more quick test to make sure that if we tag as :latest, the same --pull=never succeeds. Signed-off-by: Ed Santiago <[email protected]>

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2020

vrothberg reviewed Sep 25, 2020

View reviewed changes

libpod/image/utils.go Show resolved Hide resolved

rhatdan force-pushed the pullpolicy branch 2 times, most recently from 8de348e to 1d3b634 Compare September 25, 2020 08:41

vrothberg reviewed Sep 25, 2020

View reviewed changes

rhatdan force-pushed the pullpolicy branch from 1d3b634 to 16dd9e0 Compare September 25, 2020 13:23

TomSweeneyRedHat reviewed Sep 25, 2020

View reviewed changes

test/e2e/run_test.go Show resolved Hide resolved

TomSweeneyRedHat reviewed Sep 25, 2020

View reviewed changes

test/e2e/run_test.go Show resolved Hide resolved

rhatdan force-pushed the pullpolicy branch from 16dd9e0 to 9ab0755 Compare September 26, 2020 13:13

rhatdan force-pushed the pullpolicy branch from 9ab0755 to 7879f49 Compare September 27, 2020 09:02

rhatdan force-pushed the pullpolicy branch from 7879f49 to 1b5853e Compare September 27, 2020 11:25

ashley-cui reviewed Sep 28, 2020

View reviewed changes

openshift-ci-robot assigned QiWang19 Sep 28, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2020

openshift-merge-robot merged commit d280263 into containers:master Sep 28, 2020

edsantiago mentioned this pull request Sep 29, 2020

DO NOT MERGE! Revert "Properly handle podman run --pull command" #7824

Closed

edsantiago mentioned this pull request Sep 29, 2020

System tests: corner case for run --pull #7828

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly handle podman run --pull command #7770

Properly handle podman run --pull command #7770

rhatdan commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

vrothberg left a comment

vrothberg left a comment

TomSweeneyRedHat Sep 25, 2020

TomSweeneyRedHat Sep 26, 2020

TomSweeneyRedHat Sep 25, 2020

TomSweeneyRedHat commented Sep 25, 2020

rhatdan commented Sep 25, 2020

rhatdan commented Sep 26, 2020

TomSweeneyRedHat commented Sep 26, 2020

rhatdan commented Sep 28, 2020

giuseppe commented Sep 28, 2020

ashley-cui left a comment

QiWang19 commented Sep 28, 2020

edsantiago commented Sep 28, 2020

	It("podman run container with --pull missing shoul pull image multiple times", func() {
	It("podman run container with --pull missing should pull image multiple times", func() {

Properly handle podman run --pull command #7770

Properly handle podman run --pull command #7770

Conversation

rhatdan commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

TomSweeneyRedHat Sep 25, 2020

Choose a reason for hiding this comment

TomSweeneyRedHat Sep 26, 2020

Choose a reason for hiding this comment

TomSweeneyRedHat Sep 25, 2020

Choose a reason for hiding this comment

TomSweeneyRedHat commented Sep 25, 2020

rhatdan commented Sep 25, 2020

rhatdan commented Sep 26, 2020

TomSweeneyRedHat commented Sep 26, 2020

rhatdan commented Sep 28, 2020

giuseppe commented Sep 28, 2020

ashley-cui left a comment

Choose a reason for hiding this comment

QiWang19 commented Sep 28, 2020

edsantiago commented Sep 28, 2020