Add retry_count to container_push #2101

itmecho · 2022-05-30T14:25:00Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Currently, if an image fails to push, it will immediately fail. While this makes sense for single image pushes, it's not ideal when pushing multiple containers as often this means running the entire build step again.

Issue Number: #1989 (related)

What is the new behavior?

Adds a retry_count attribute to container_push. If the push fails, it will be retried until the retry_count is reached. On each retry, a log line will be printed to show the error that occurred as well as which attempt it was.

Does this PR introduce a breaking change?

Yes
No

Other information

I've updated the docs to include the new attribute but I'm not sure if there are tests that cover it. I'm happy to add the tests but I might need some guidance as to where and how to implement them!

google-cla · 2022-05-30T14:25:04Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

itmecho · 2022-05-30T14:28:12Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Done but I can't re-run the check to clear the failure

itmecho · 2022-05-30T15:15:01Z

I've found the container_push tests in tests/container/BUILD but I'm not sure how to test it properly as it would require a push to fail one or more times and then succeed

container/go/cmd/pusher/pusher.go

uhthomas · 2022-06-07T10:29:23Z

container/go/cmd/pusher/pusher.go

@@ -44,6 +44,7 @@ var (
 	format              = flag.String("format", "", "The format of the uploaded image (Docker or OCI).")
 	clientConfigDir     = flag.String("client-config-dir", "", "The path to the directory where the client configuration files are located. Overiddes the value from DOCKER_CONFIG.")
 	skipUnchangedDigest = flag.Bool("skip-unchanged-digest", false, "If set to true, will only push images where the digest has changed.")
+	retryCount          = flag.Int("retry-count", 0, "Amount of times the push will be retried.")


Should we validate this value? What happens when the retry count is -1? Some users may assume this to mean infinite retries, when in fact the container image would not even attempt to be pushed once.

Good point. Do you think it's better to log using log.Fatalln if it's negative or set it to 0 and log a warning instead?

I've pushed a version that exits with Fatalln as that matches the other validation behaviour.

uhthomas · 2022-06-07T10:35:31Z

container/go/cmd/pusher/pusher.go

+		}
+
+		if *retryCount > 0 && retry < *retryCount {
+			log.Printf("Failed to push image on attempt %d: %v", retry+1, err)


This error is missing the stamped variable. It will make it tricky to know what actually failed without this. Maybe reuse the original format?

Suggested change

log.Printf("Failed to push image on attempt %d: %v", retry+1, err)

log.Printf("Error pushing image to %s (attempt %d): %v", stamped, retry, err)

Makes sense! Pushed that change.

container/go/cmd/pusher/pusher.go

uhthomas · 2022-08-20T08:52:37Z

container/push.bzl

@@ -90,6 +90,9 @@ def _impl(ctx):
        tag = tag,
    ))

+    if ctx.attr.retry_count > 0:
+        pusher_args.append("-retry-count={}".format(ctx.attr.retry_count))


Maybe there should be some validation here? Might be confusing if a user provides -1 and nothing happens.

I think we can merge given this change.

EdSchouten · 2022-10-11T18:36:46Z

@itmecho Friendly ping :-)

itmecho · 2022-10-12T16:56:55Z

@itmecho Friendly ping :-)

Oh sorry! I thought this commit covers that validation?

4e54051

Is that no good or is there something else I've missed?

github-actions · 2023-04-11T02:21:32Z

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_docker!

com6056 · 2023-04-23T21:58:03Z

@itmecho do you have time to wrap this up? If not, happy to pick this up!

itmecho · 2023-07-24T11:25:11Z

@itmecho do you have time to wrap this up? If not, happy to pick this up!

@com6056 Sorry for the delay! I've pushed some validation and a doc update, hopefully that covers it!

The build looks like it's failing on something to do with docker/debian9? Not sure if that's related to my changes or not...

com6056 · 2023-08-24T16:07:34Z

@itmecho do you have time to wrap this up? If not, happy to pick this up!

@com6056 Sorry for the delay! I've pushed some validation and a doc update, hopefully that covers it!

The build looks like it's failing on something to do with docker/debian9? Not sure if that's related to my changes or not...

@uhthomas any idea why that build is failing or who we can ask about it?

github-actions · 2024-12-10T02:58:34Z

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_docker!

Add retry-count to container_push

dc9b470

itmecho requested review from gravypod, linzhp, pcj, smukherj1 and uhthomas as code owners May 30, 2022 14:25

itmecho added 2 commits May 30, 2022 15:31

Run push-all through buildifier

43f6d3f

Regenerate docs

ddd6949

itmecho changed the title ~~Add retry-count to container_push~~ Add retry_count to container_push May 30, 2022

Add missing retry flag during push all

5a1f982

ghost reviewed May 31, 2022

View reviewed changes

container/go/cmd/pusher/pusher.go Show resolved Hide resolved

uhthomas requested changes Jun 7, 2022

View reviewed changes

itmecho added 2 commits June 8, 2022 10:02

Update retry log to be more useful

5e5bec0

Validate -retry-count is not negative

4e54051

uhthomas requested changes Aug 20, 2022

View reviewed changes

github-actions bot added the Can Close? Will close in 30 days unless there is a comment indicating why not label Apr 11, 2023

github-actions bot removed the Can Close? Will close in 30 days unless there is a comment indicating why not label Apr 24, 2023

itmecho added 3 commits July 24, 2023 11:53

Merge remote-tracking branch 'upstream/master' into retry-push-count

4c2fffe

Validate that retry_count is a positive int

f63e72a

Update docs again

776c2d2

github-actions bot added the Can Close? Will close in 30 days unless there is a comment indicating why not label Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry_count to container_push #2101

Add retry_count to container_push #2101

itmecho commented May 30, 2022

google-cla bot commented May 30, 2022

itmecho commented May 30, 2022

itmecho commented May 30, 2022

uhthomas Jun 7, 2022

itmecho Jun 8, 2022

itmecho Jun 8, 2022

uhthomas Jun 7, 2022

itmecho Jun 8, 2022

uhthomas Aug 20, 2022

EdSchouten commented Oct 11, 2022

itmecho commented Oct 12, 2022

github-actions bot commented Apr 11, 2023

com6056 commented Apr 23, 2023

itmecho commented Jul 24, 2023

com6056 commented Aug 24, 2023

github-actions bot commented Dec 10, 2024

	log.Printf("Failed to push image on attempt %d: %v", retry+1, err)
	log.Printf("Error pushing image to %s (attempt %d): %v", stamped, retry, err)

Add retry_count to container_push #2101

Are you sure you want to change the base?

Add retry_count to container_push #2101

Conversation

itmecho commented May 30, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

google-cla bot commented May 30, 2022

itmecho commented May 30, 2022

itmecho commented May 30, 2022

uhthomas Jun 7, 2022

Choose a reason for hiding this comment

itmecho Jun 8, 2022

Choose a reason for hiding this comment

itmecho Jun 8, 2022

Choose a reason for hiding this comment

uhthomas Jun 7, 2022

Choose a reason for hiding this comment

itmecho Jun 8, 2022

Choose a reason for hiding this comment

uhthomas Aug 20, 2022

Choose a reason for hiding this comment

EdSchouten commented Oct 11, 2022

itmecho commented Oct 12, 2022

github-actions bot commented Apr 11, 2023

com6056 commented Apr 23, 2023

itmecho commented Jul 24, 2023

com6056 commented Aug 24, 2023

github-actions bot commented Dec 10, 2024