Introduce a --ignore option to allow "sync" command to continue syncing even after a particular image sync fails

[jaikiran]

Fixes #1466

As noted in the linked issue, in the current form, the sync command aborts as soon as any image tag has issue during copy. This ends up being a blocker if any one of the image tag out of hundreds (for a particular image) has some issue.

As suggested in the linked issue, the commit in this PR introduces a --ignore option to the sync command which allows the sync to continue with the rest of the images/tags. When an error occurs during the image copy, that error is logged and when the sync finally finishes, such errors contribute to sync being marked as failed.

I have tested this change against our setup where we were running into this issue. With this change, when --ignore is used, the error is now logged and the next image tag is processed and finally when the process exits you see the failure message and sync returns with a non-zero exit code. This is the snippet from such a run:

(there are a total to 352 tags identified for this image and the 154th one is the one having the issue)

....
INFO[0017] Copying image ref 154/352                     from="docker://dockerserver:5000/helloworld:release-1.0.0-SNAPSHOT-2019_Jun_18_08_58_UTC" to="docker://192.168.1.2:5000/helloworld:release-1.0.0-SNAPSHOT-2019_Jun_18_08_58_UTC"
Getting image source signatures
Copying blob fd48b0e2a9b9 skipped: already exists  
Copying blob 1fd3936b9903 skipped: already exists  
Copying blob a87126e1a510 skipped: already exists  
Copying blob 17dea04ab044 skipped: already exists  
Copying blob 44403e1d5128 [==================================>---] 66.9MiB / 72.4MiB
ERRO[0018] Error copying ref "docker://dockerserver:5000/helloworld:release-1.0.0-SNAPSHOT-2019_Jun_18_08_58_UTC"  error="writing blob: Patch http://192.168.1.2:5000/v2/helloworld/blobs/uploads/6fdba9fa-9367-4218-bccd-d33f38d7a833?_state=Pdq1xGqTyt00iB4TRvx8xypLgbyEj03Qnyi7PNSUfg57Ik5hbWUiOiJpZGVudGl0eSIsIlVVSUQiOiI2ZmRiYTlmYS05MzY3LTQyMTgtYmNjZC1kMzNmMzhkN2E4MzMiLCJPZmZzZXQiOjAsIlN0YXJ0ZWRBdCI6IjIwMjEtMTAtMDJUMDQ6MDc6MjAuMzE2NDYwNDhaIn0%3D: readfrom tcp 192.168.1.2:56274->192.168.1.2:5000: happened during read: Digest did not match, expected sha256:44403e1d5128211c7dcf598e85b608190560dc0ffac8d9df7f9ea1b270b60c12, got sha256:6b375acbd82c7ee62a88c06a4be0e1a7e4d0ba03450a792d8a932d7b3aa52fab"

That error gets reported and the process now moves on to next:

...
INFO[0018] Copying image ref 155/352                     from="docker://dockerserver:5000/helloworld:release_1.0.0s_Jun18_13_40_UTC" to="docker://192.168.1.2:5000/helloworld:release_1.0.0s_Jun18_13_40_UTC"
...

After the rest are synced, the final log message looks like:

INFO[0532] Synced 351 images from 1 sources             
FATA[0532] Sync failed due to previous reported error(s) for one or more images 

The exit code from that run is verified to be non-zero:

 echo $?
1

I thought of adding a test case, but it's not straightforward to trigger the error in first place (I can't yet think of a way). So I haven't added one.

P.S: This is my first time with Go. Is there any checkstyle/formatting rules that I can configure my IDE with? Everytime I did a certain change, it ended up formatting a huge part of the file where it removed certain whitespaces in various places. It's currently uses the IDE default formatter (I use GoLand from IntelliJ).

[mtrmac]

Thanks for the PR!

(See #1466 (comment) , this might not be the best solution for that particular situation; let’s discuss that in there. This PR is a useful feature anyway, e.g. to make as much progress as possible on unreliable network links.)

The implementation looks good; as usual, naming is the hard part :)

P.S: This is my first time with Go. Is there any checkstyle/formatting rules that I can configure my IDE with? Everytime I did a certain change, it ended up formatting a huge part of the file where it removed certain whitespaces in various places. It's currently uses the IDE default formatter (I use GoLand from IntelliJ).

We use the language’s gofmt with the -s option, IIRC currently using the Go 1.17 version. I don’t know how to set that up in GoLand, maybe someone else can help. Anyway, the PR tests do a formatting check and this PR looks good as is already.

[mtrmac]

LGTM, one tiny wording nit.

(Or, others, feel free to merge as is.)

[mtrmac]

LGTM. Thanks again!

[jaikiran]

Thanks everyone for the reviews and the inputs.

@mtrmac, do releases happen off the main branch? If so what versions get released off that branch? The reason I ask is to decide whether I should create a PR for the 1.4 release branch containing this change so that it becomes available to us whenever the next release happens.

[rhatdan]

Releases happen off of the main branch.

[mtrmac]

… and not on an extremely regular/frequent schedule, so, @jaikiran , if you need an “official” release to happen pretty soon, please speak up and we’ll tag one.

[jaikiran]

Hello @mtrmac, this particular fix and the Makefile change that was merged some days back, both, will help us in our usage of skopeo in our product. So yes, a release will be really helpful. Thank you very much.

[rhatdan]

https://github.com/containers/skopeo/releases/tag/v1.5.0

[jaikiran]

Thank you very much @rhatdan and others for creating this release and all the help in reviewing the changes. I've triggered our internal build to have this release integrated into our product.