Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ignoring errors when pushing an image to the cache #786

Closed
orf opened this issue Sep 29, 2021 · 6 comments
Closed

Allow ignoring errors when pushing an image to the cache #786

orf opened this issue Sep 29, 2021 · 6 comments

Comments

@orf
Copy link

orf commented Sep 29, 2021

When using --cache-to type=registry,... there should be an option to ignore pushing errors.

We are using Nexus as a cache, and it's OK if the cache goes down or is unavailable. There is also a bug that means re-pushing a 100% cached image results in:

#19 ERROR: error writing manifest blob: failed commit on ref "sha256:69cb06c31697c5dc32fb2fd9949c98ec98783a3e7b2b2f2c0651623fe21dcb16": cannot reuse body, request must be retried

This is fine, Nexus is terrible, but we just want to ignore the error and not exit with a non-zero status code. Currently this seems impossible to do.

@orf
Copy link
Author

orf commented Sep 29, 2021

I guess moby/buildkit#2251 would fix the Nexus issue, but even so being able to ignore transient issues with your cache registry to prevent builds from being marked as failed would be a great feature.

@tonistiigi
Copy link
Member

I don't think we can do this. We can't just ignore errors and adding options for these kinds of bad behavior degrades code quality and gives wrong assumptions to users. If it is a registry side issue then report it to them.

Transient issues should be handled by retries.

cannot reuse body, request must be retried

This looks like it could be containerd issue. Maybe in combination with what registry responds. https://github.com/containerd/containerd/blame/main/remotes/docker/pusher.go#L267 Might want to report it there. If internally containerd makes multiple calls into this function is some case then I think this should be handled fixed the library.

@orf
Copy link
Author

orf commented Oct 4, 2021

I don’t agree with this. A cache is a cache, no more, and it’s generally good practice to not fail if a cache is unavailable.

By not allowing the option to treat a cache as a cache you’re marking it as something that has to be as available as the repository you’re pushing to, which is odd for something that is referred to as a cache and who’s only functionality is to speed up build times.

@tonistiigi
Copy link
Member

A cache is a cache, no more, and it’s generally good practice to not fail if a cache is unavailable.

I agree on this for cache import and we do ignore import errors. The cache may be provided by a remote source and that may go away and the user can't control it etc., we shouldn't fail to build but run steps again and we can still the result. But exporting is different, the user is asking the cache to be exported and if we can't because of some misconfiguration then we have not processed the request correctly. Otherwise, we could have similar exceptions for many other steps: build succeeded but image export failed, loading into docker failed, push failed, the machine ran out of disk space when making blobs etc.

@orf
Copy link
Author

orf commented Oct 7, 2021

So it seems like there are two different use-cases we have controlled by the same flag:

  1. Please export this image cache and I really, 100% definitely need it to exist in the registry
  2. Eh, export it if you can, but if it's not there because the registry we use for caching is down it's no big deal. Build images are more important.

I think the same follows for all cache outputs. It would be good to have a flag to control this, because the only thing we really care about is the final image and if the cache fails to export or import then it's not a big deal, certainly not one that we'd want to disrupt the actual image build with.

@tonistiigi
Copy link
Member

I've changed my mind about this and created tracking issue in moby/buildkit#2578 as this is where it needs to be implemented. I couldn't transfer the issue because of the different org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants