libimage: implement `retry` logic for `manifest push` by flouthoc · Pull Request #1666 · containers/common

flouthoc · 2023-09-27T07:34:27Z

manifest push API must implement and leverage retry logic similar to image push with similar defaults.

Closes: #1664

libimage/manifest_list.go

vrothberg

LGTM

giuseppe

LGTM

nalind · 2023-09-27T14:23:33Z

libimage/manifests/manifests.go


+const (
+	defaultMaxRetries = 3
+	defaultRetryDelay = time.Second


retry.IfNecessary() has a default for this, so it's better to not insert our own.

image push retries for 3 times with a delay of 1 second which is hardcoded in libimage/copier but I was unable to find these defaults in retry package https://github.com/containers/common/blob/main/pkg/retry/retry.go

There's a default delay (the godoc for retry.Options.Delay hints at it), but not a default on the number of retries.

libimage/manifests/manifests.go

nalind · 2023-09-27T15:59:29Z

libimage/manifests/manifests_test.go

+
+	options.RetryDelay = 3 * time.Second
+	_, _, err = list.Push(ctx, destRef, options)
+	assert.NoError(t, err, "list.Push(with RetryDelay)")


Is this expected to be testing the new fields in options in combination with the other settings that were already set above? Is it verifying that the new options cause any changes in behavior?

@nalind Yes this is only testing field, I expect to do integration test where retry is happening on podman side where it throws warning on every failed retry attempt and success on push to registry.

libimage/manifests/manifests.go

nalind

TestPush() really needs to be expanded, but otherwise LGTM, give or take a non-blocking stylistic nit.

flouthoc · 2023-09-28T06:14:35Z

Let me extend the test, I think this can be tested here as well.

flouthoc · 2023-09-28T08:05:29Z

Expanded PushTest to actually cover retry attempts by checking warning logs.

flouthoc · 2023-09-28T09:20:55Z

@nalind PTAL again.

nalind · 2023-09-28T16:22:36Z

libimage/manifests/manifests_test.go

 	assert.Nilf(t, err, "list.Push(four specified)")

+	bogusDestRef, err := alltransports.ParseImageName("docker://localhost/bogus/dest:latest")
+	assert.Nilf(t, err, "ParseImageName()")


Probably want to use assert.NoErrorf() here, since we know that err is an error if it isn't nil.

nalind

LGTM with one minor change to the TestPushManifest() test.

openshift-ci · 2023-09-28T16:24:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, nalind, vrothberg

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe,vrothberg]

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

manifest push API must implement and leverage `retry` logic similar to `image push` with similar defaults. Closes: containers#1664 Signed-off-by: Aditya R <arajan@redhat.com>

rhatdan · 2023-09-28T17:54:49Z

/lgtm

Test: containers/common#1666 Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc requested a review from vrothberg September 27, 2023 07:34

vrothberg reviewed Sep 27, 2023

View reviewed changes

libimage/manifest_list.go Outdated Show resolved Hide resolved

flouthoc force-pushed the manifest-retry branch from 9391ce7 to b28613f Compare September 27, 2023 08:00

flouthoc requested a review from vrothberg September 27, 2023 08:01

vrothberg approved these changes Sep 27, 2023

View reviewed changes

openshift-ci bot added the approved label Sep 27, 2023

giuseppe approved these changes Sep 27, 2023

View reviewed changes

nalind suggested changes Sep 27, 2023

View reviewed changes

flouthoc force-pushed the manifest-retry branch from b28613f to f6455a2 Compare September 27, 2023 14:57

flouthoc marked this pull request as draft September 27, 2023 15:10

openshift-ci bot added the do-not-merge/work-in-progress label Sep 27, 2023

flouthoc force-pushed the manifest-retry branch from f6455a2 to ab58b4f Compare September 27, 2023 15:32

flouthoc marked this pull request as ready for review September 27, 2023 15:32

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 27, 2023

flouthoc force-pushed the manifest-retry branch from ab58b4f to 061f2b0 Compare September 27, 2023 15:37

flouthoc requested a review from nalind September 27, 2023 15:38

nalind reviewed Sep 27, 2023

View reviewed changes

libimage/manifests/manifests.go Outdated Show resolved Hide resolved

flouthoc force-pushed the manifest-retry branch from 061f2b0 to 48a8fa2 Compare September 27, 2023 15:59

nalind reviewed Sep 27, 2023

View reviewed changes

flouthoc requested a review from nalind September 27, 2023 16:12

nalind reviewed Sep 27, 2023

View reviewed changes

libimage/manifests/manifests.go Outdated Show resolved Hide resolved

nalind approved these changes Sep 27, 2023

View reviewed changes

flouthoc requested a review from vrothberg September 28, 2023 02:39

flouthoc force-pushed the manifest-retry branch 2 times, most recently from 699fa23 to 6d13884 Compare September 28, 2023 08:04

flouthoc force-pushed the manifest-retry branch from 6d13884 to 0dffc5f Compare September 28, 2023 08:27

nalind reviewed Sep 28, 2023

View reviewed changes

nalind approved these changes Sep 28, 2023

View reviewed changes

libimage: add retry options for manifest push

c044f8d

manifest push API must implement and leverage `retry` logic similar to `image push` with similar defaults. Closes: containers#1664 Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the manifest-retry branch from 0dffc5f to c044f8d Compare September 28, 2023 16:57

openshift-ci bot assigned rhatdan Sep 28, 2023

openshift-ci bot added the lgtm label Sep 28, 2023

openshift-merge-robot merged commit 31e800a into containers:main Sep 28, 2023

flouthoc mentioned this pull request Oct 3, 2023

test, manifest: test push retry containers/podman#20242

Merged

flouthoc added a commit to flouthoc/podman that referenced this pull request Oct 3, 2023

test, manifest: test push retry

b576b79

Test: containers/common#1666 Signed-off-by: Aditya R <arajan@redhat.com>

Conversation

flouthoc commented Sep 27, 2023

Uh oh!

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!

nalind Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

flouthoc Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

flouthoc Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nalind Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

flouthoc Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

flouthoc commented Sep 28, 2023

Uh oh!

flouthoc commented Sep 28, 2023

Uh oh!

flouthoc commented Sep 28, 2023

Uh oh!

nalind Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Sep 28, 2023

Uh oh!

rhatdan commented Sep 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flouthoc Sep 27, 2023 •

edited

Loading

flouthoc Sep 27, 2023 •

edited

Loading