1055872 - Better handle S3 failures in sccache

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

So, when I tried landing bug 1045923, I hit an interesting case, in that the cache would almost immediately fallback to not cache at all because of failures (because of the changes from bug 1020827).

It turns out that for a period of time, S3 was throwing 403 errors supposedly because of non-matching signatures. It seems this is not happening anymore, but in case it does happen again, we'd better be resilient to it. Hopefully, retrying /should/ work, but since I can't reproduce anymore, it's hard to tell if the workaround actually works.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Show storage failures in sccache stats

Comment 2

[Mike Hommey [:glandium]]

Attachment: Retry S3 PUT up to twice in case it returns a 403 error

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8475586 [details] [diff] [review]
Retry S3 PUT up to twice in case it returns a 403 error

So, in fact, this is still happening, and it's because this patch is broken and hides the problem away.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Show storage failures in sccache stats

The stats patch was broken too. This fixes it.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Retry S3 PUT up to twice in case it returns a 403 error

Comment 6

[Michael Shal [:mshal]]

Comment on attachment 8475693 [details] [diff] [review]
Show storage failures in sccache stats

>+            if stats['cachefail'] or stats['storagefail']:
>                 stderr.write('sccache: Failure to cache: %d\n'
>-                    % stats['cachefail'])
>+                    % (stats['cachefail'] + stats['storagefail']))

IMO it would make more sense to just print the storagefails in a separate line, especially since you could have multiple storagefails per cache miss.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #6)
> Comment on attachment 8475693 [details] [diff] [review]
> Show storage failures in sccache stats
> 
> >+            if stats['cachefail'] or stats['storagefail']:
> >                 stderr.write('sccache: Failure to cache: %d\n'
> >-                    % stats['cachefail'])
> >+                    % (stats['cachefail'] + stats['storagefail']))
> 
> IMO it would make more sense to just print the storagefails in a separate
> line, especially since you could have multiple storagefails per cache miss.

Actually, the cachefails are entirely theoretical (they can only happen if creating the CacheData raises an exception), so it's fine to just ignore them.

Comment 8

[Mike Hommey [:glandium]]

https://github.com/glandium/sccache/commit/2fd3b775f95a1fd769a24dc6cf8133f4faa0fbb4
https://github.com/glandium/sccache/commit/4d312f8b9a42012acbef0d9ffba1e6f46de4fd18