Retry pulling image #6905

QiWang19 · 2020-07-08T13:28:04Z

Update the containers/common
Use shared retry functions from containers/common. Wrap the inner helper in the retry function. Functions pullimage failed with retriable error will default maxretry 3 times using exponential backoff.
related: Podman Pull Images always error when docker not #4251 (comment)
Signed-off-by: Qi Wang [email protected]

rhatdan · 2020-07-08T13:34:11Z

@QiWang19 Please fixup you PR explanation, Explain what is going to happen

rhatdan · 2020-07-08T13:35:01Z

Do we need this for podman create and podman run?

QiWang19 · 2020-07-08T14:48:56Z

This retry also plans to work for create and run since the retry function wrapped the inner helper method that is used by create and run.

rhatdan · 2020-07-08T17:05:37Z

Great.

rhatdan · 2020-07-18T10:21:23Z

libpod/image/pull.go

-
-		_, err = cp.Image(ctx, policyContext, imageInfo.dstRef, imageInfo.srcRef, copyOptions)
-		if err != nil {
+		destRef := imageInfo.dstRef


Nit: Why do you need to use variables?

Gating tests complain about using imageInfo in the retry loop cause it's a variable from the outer loop.

If that warning is about the loop variable changing in next iteration, affecting the closure, an imageInfo := imageInfo might work just as well.

rhatdan · 2020-07-18T10:22:14Z

libpod/image/utils.go

@@ -174,3 +182,63 @@ func imageNameForSaveDestination(img *Image, imgUserInput string) string {
 	}
 	return fmt.Sprintf("%s%s", prepend, imgUserInput)
 }
+
+func retryIfNecessary(operation func() error) error {


Can you put this code into containers/common, since podman, Buildah and Skopeo are all going to use it?

This functionality along with @ParkerVR pulling of shortnames should be put into common/pkg/image shared functions.

I can draft a PR in c/common, will this be common/pkg/image/image.go or pull.go? @ParkerVR?

I have been testing locally with pkg/image/pull.go, which I think makes more sense for consistency.

containers/common PR has been merged.

rhatdan · 2020-07-21T09:52:11Z

@QiWang19 Open the PR against containers/common

rhatdan · 2020-07-22T14:12:32Z

@QiWang19 containers/common merged, can you update this PR?

rhatdan · 2020-07-22T14:48:12Z

LGTM
@ParkerVR @mtrmac @mheon @vrothberg PTAL

TomSweeneyRedHat · 2020-07-22T15:46:55Z

libpod/image/image.go

@@ -14,6 +14,7 @@ import (
 	"syscall"
 	"time"

+	commonimage "github.com/containers/common/pkg/image"


Is @mtrmac happy with this package name? I thought he raised concerns over in common (containers/common#229) and I'd like to settle that before we move forward with this PR.

That really depends on what else is planned to go into that package.

The retry code is somewhat image-independent (but not entirely, it does check for registry errors, and drag in a registry subpackage) and could be used for other network operations, so I don’t think it’s a good name for a package containing only this code. A larger package containing several more image-related utilities might make more sense, OTOH it’s currently not clear to me that that would have to be a single package versus a set of utility subpackages (from which only a subset could be used to minimize dependencies in consumers).

This package plans to merge the work from @ParkerVR pulling of shortnames .

Yeah — is there value in having both in a single subpackage? Skopeo won’t use the short name part, at least.

(I don’t really want to make a big deal out of this, the c/common has no API guarantee, it’s not that much code I expect, and the repos are tightly coupled enough that we can rearrange any time. I guess dragging in the full containers.conf config file, or some user-observable non-optional initialization behavior, into Skopeo would be the point I’d start feeling stronger about this.)

I don't object to having them both in the same package, but I do object to the name. Importing both github.com/containers/libpod/v2/libpod/image and github.com/containers/common/pkg/image in the same file is horribly confusing and renaming one at import time does not make things much better.

thanks @mtrmac . I moved it to a single pkg. PTAL containers/common#237

I definitely do not like the name retry, it is too small, I am leaning torwards containers/common/pkg/pull.

I agree with Dan - but do we want more organization with something like containers/common/pkg/image/pull ? This might give us more space for the future with less refactoring

TomSweeneyRedHat · 2020-07-22T15:48:02Z

Changes LGTM , but I want a head nod from @mtrmac on the package naming before moving forward.

rhatdan · 2020-08-04T19:20:20Z

@QiWang19 What is going on with this PR? Can we move this to no longer WIP?

Wrap the inner helper in the retry function. Functions pullimage failed with retriable error will default maxretry 3 times using exponential backoff. Signed-off-by: Qi Wang <[email protected]>

rhatdan · 2020-08-05T00:47:55Z

/approve
LGTM
@QiWang19 Can you remove [wip] from description?

openshift-ci-robot · 2020-08-05T00:47:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, 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

QiWang19 · 2020-08-05T01:40:09Z

[wip] removed

vrothberg

LGTM, great work, @QiWang19

vrothberg · 2020-08-05T08:49:53Z

/lgtm

rhatdan · 2020-08-06T13:56:43Z

@QiWang19 is skopeo and Buildah now using the same code from containers/common?

QiWang19 · 2020-08-06T14:17:26Z

@QiWang19 is skopeo and Buildah now using the same code from containers/common?

No, they are not using c/common. Will open PRs for that.

QiWang19 force-pushed the retry-pull branch from 21a1cd9 to e22d004 Compare July 8, 2020 14:45

QiWang19 marked this pull request as draft July 9, 2020 14:05

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2020

QiWang19 changed the title ~~retry pulling image~~ [WIP]retry pulling image Jul 9, 2020

QiWang19 force-pushed the retry-pull branch from e22d004 to ccd1d3c Compare July 14, 2020 21:22

QiWang19 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2020

QiWang19 force-pushed the retry-pull branch from ccd1d3c to 84e0a52 Compare July 14, 2020 21:30

QiWang19 marked this pull request as ready for review July 15, 2020 14:33

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020

QiWang19 changed the title ~~[WIP]retry pulling image~~ Retry pulling image Jul 15, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020

rhatdan reviewed Jul 18, 2020

View reviewed changes

QiWang19 force-pushed the retry-pull branch 2 times, most recently from f30a9c4 to 1e27781 Compare July 20, 2020 23:29

QiWang19 marked this pull request as draft July 20, 2020 23:29

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2020

QiWang19 force-pushed the retry-pull branch 2 times, most recently from ae412f2 to e3aaf5e Compare July 21, 2020 00:10

QiWang19 force-pushed the retry-pull branch from e3aaf5e to 5b8c711 Compare July 22, 2020 14:19

QiWang19 marked this pull request as ready for review July 22, 2020 14:24

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2020

QiWang19 force-pushed the retry-pull branch from 5b8c711 to 148a4bf Compare July 22, 2020 14:36

TomSweeneyRedHat reviewed Jul 22, 2020

View reviewed changes

QiWang19 force-pushed the retry-pull branch from 148a4bf to 5b2f04a Compare July 22, 2020 16:54

rhatdan changed the title ~~Retry pulling image~~ [wip] Retry pulling image Jul 22, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2020

QiWang19 mentioned this pull request Jul 22, 2020

Move retry code to pkg/retry containers/common#237

Merged

rhatdan mentioned this pull request Aug 4, 2020

Podman fails to complete pulling some images when they are nearly fully downloaded #5583

Closed

Retry pulling image

42d756d

Wrap the inner helper in the retry function. Functions pullimage failed with retriable error will default maxretry 3 times using exponential backoff. Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the retry-pull branch from 5b2f04a to 42d756d Compare August 4, 2020 19:56

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

QiWang19 changed the title ~~[wip] Retry pulling image~~ Retry pulling image Aug 5, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2020

vrothberg reviewed Aug 5, 2020

View reviewed changes

openshift-ci-robot assigned vrothberg Aug 5, 2020

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

openshift-merge-robot merged commit 7a7c8e9 into containers:master Aug 5, 2020

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

Retry pulling image #6905

Retry pulling image #6905

QiWang19 commented Jul 8, 2020 •

edited

Loading

rhatdan commented Jul 8, 2020

rhatdan commented Jul 8, 2020

QiWang19 commented Jul 8, 2020 •

edited

Loading

rhatdan commented Jul 8, 2020

rhatdan Jul 18, 2020

QiWang19 Jul 21, 2020

mtrmac Jul 22, 2020

rhatdan Jul 18, 2020

rhatdan Jul 18, 2020

QiWang19 Jul 20, 2020

ParkerVR Jul 20, 2020

rhatdan Jul 21, 2020

rhatdan commented Jul 21, 2020

rhatdan commented Jul 22, 2020

rhatdan commented Jul 22, 2020

TomSweeneyRedHat Jul 22, 2020

mtrmac Jul 22, 2020

QiWang19 Jul 22, 2020

mtrmac Jul 22, 2020

mheon Jul 22, 2020

QiWang19 Jul 22, 2020

rhatdan Jul 22, 2020

ParkerVR Jul 22, 2020

TomSweeneyRedHat commented Jul 22, 2020

rhatdan commented Aug 4, 2020

rhatdan commented Aug 5, 2020

openshift-ci-robot commented Aug 5, 2020

QiWang19 commented Aug 5, 2020

vrothberg left a comment

vrothberg commented Aug 5, 2020

rhatdan commented Aug 6, 2020

QiWang19 commented Aug 6, 2020

Retry pulling image #6905

Retry pulling image #6905

Conversation

QiWang19 commented Jul 8, 2020 • edited Loading

rhatdan commented Jul 8, 2020

rhatdan commented Jul 8, 2020

QiWang19 commented Jul 8, 2020 • edited Loading

rhatdan commented Jul 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jul 21, 2020

rhatdan commented Jul 22, 2020

rhatdan commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jul 22, 2020

rhatdan commented Aug 4, 2020

rhatdan commented Aug 5, 2020

openshift-ci-robot commented Aug 5, 2020

QiWang19 commented Aug 5, 2020

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Aug 5, 2020

rhatdan commented Aug 6, 2020

QiWang19 commented Aug 6, 2020

QiWang19 commented Jul 8, 2020 •

edited

Loading

QiWang19 commented Jul 8, 2020 •

edited

Loading