Add retry_count to container_push #2101
Conversation
itmecho
requested review from
gravypod,
linzhp,
pcj,
smukherj1 and
uhthomas
as code owners
|
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 |
|
I've found the container_push tests in |
itmecho
changed the title
container/go/cmd/pusher/pusher.go
Outdated
| @@ -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.") | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I've pushed a version that exits with Fatalln as that matches the other validation behaviour.
container/go/cmd/pusher/pusher.go
Outdated
| } | ||
|
|
||
| if *retryCount > 0 && retry < *retryCount { | ||
| log.Printf("Failed to push image on attempt %d: %v", retry+1, err) |
There was a problem hiding this comment.
This error is missing the stamped variable. It will make it tricky to know what actually failed without this. Maybe reuse the original format?
| 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) |
There was a problem hiding this comment.
Makes sense! Pushed that change.
| @@ -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)) | |||
There was a problem hiding this comment.
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.
|
@itmecho Friendly ping :-) |
|
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. |
github-actions
bot
added
the
Can Close?
|
@itmecho do you have time to wrap this up? If not, happy to pick this up! |
github-actions
bot
removed
the
Can Close?
@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? |
PR Checklist
Please check if your PR fulfills the following requirements:
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_countattribute tocontainer_push. If the push fails, it will be retried until theretry_countis 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?
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!