libimage: implement `retry` logic for `manifest push` #1666

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

@@ -28,6 +30,11 @@ import (
 	"github.com/sirupsen/logrus"
 )

+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

@@ -335,6 +338,30 @@ func TestPushManifest(t *testing.T) {
 	_, _, err = list.Push(ctx, destRef, options)
 	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

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 requested 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
7 checks passed

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

test, manifest: test push retry

4d6daac

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

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

3e57e66

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

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

test, manifest: test push retry

e07aae8

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

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

test, manifest: test push retry

9b088ea

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

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>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libimage: implement `retry` logic for `manifest push` #1666

libimage: implement `retry` logic for `manifest push` #1666

flouthoc commented Sep 27, 2023

vrothberg left a comment

giuseppe left a comment

nalind Sep 27, 2023

flouthoc Sep 27, 2023 •

edited

nalind Sep 27, 2023

flouthoc Sep 27, 2023

nalind Sep 27, 2023

flouthoc Sep 27, 2023 •

edited

nalind left a comment

flouthoc commented Sep 28, 2023

flouthoc commented Sep 28, 2023

flouthoc commented Sep 28, 2023

nalind Sep 28, 2023

nalind left a comment

openshift-ci bot commented Sep 28, 2023

rhatdan commented Sep 28, 2023

libimage: implement retry logic for manifest push #1666

libimage: implement retry logic for manifest push #1666

Conversation

flouthoc commented Sep 27, 2023

vrothberg left a comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

nalind Sep 27, 2023

Choose a reason for hiding this comment

flouthoc Sep 27, 2023 • edited

Choose a reason for hiding this comment

nalind Sep 27, 2023

Choose a reason for hiding this comment

flouthoc Sep 27, 2023

Choose a reason for hiding this comment

nalind Sep 27, 2023

Choose a reason for hiding this comment

flouthoc Sep 27, 2023 • edited

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

flouthoc commented Sep 28, 2023

flouthoc commented Sep 28, 2023

flouthoc commented Sep 28, 2023

nalind Sep 28, 2023

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 28, 2023

rhatdan commented Sep 28, 2023

libimage: implement `retry` logic for `manifest push` #1666

libimage: implement `retry` logic for `manifest push` #1666

flouthoc Sep 27, 2023 •

edited

flouthoc Sep 27, 2023 •

edited