-
Notifications
You must be signed in to change notification settings - Fork 5
Check and delete corrupted blobs in cache #533
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
Conversation
Tested locally using dev cache, objects deleted from cache
Client:
|
If I pull again , the caching starts again |
src/main/groovy/io/seqera/wave/service/blob/BlobCacheService.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/io/seqera/wave/service/blob/impl/BlobCacheServiceImpl.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
src/main/groovy/io/seqera/wave/service/blob/impl/BlobCacheServiceImpl.groovy
Outdated
Show resolved
Hide resolved
…iceImpl.groovy Co-authored-by: Paolo Di Tommaso <[email protected]>
Tested it locally, one issue found, when docker client retries wave find the entry in cache and try to fetch the blob in cloudflare which does not exists |
How could that happen, it should continue to find the wave/src/main/groovy/io/seqera/wave/controller/RegistryProxyController.groovy Lines 321 to 326 in 00e88cd
|
correct, But shouldn't it restart blob transfer in the next retry? |
blob deletion is working correctly; tested using the dev Cloudflare cache in the local |
ok, but why you say
If find the entry marked as failed should continue to return an error |
I miss understood the logs, because it shows the locationURI of deleted blob, when docker retries |
ok, so the behaviour is correct. What it could be done is to use a shorted TTL when a job is failing. For example here wave/src/main/groovy/io/seqera/wave/service/blob/impl/BlobCacheServiceImpl.groovy Line 205 in 00e88cd
use instead
Could you try this ☝️ at attached the log/screenshot of the retry (tho I think 5secs it's still too long) |
attached retry log, with changed token and URL I will try with shorter ttl |
correction the above log is without the ttl, i will try with the change and attach the log again |
after shorting the status delay, blob cache upload begins again with docker retry, here is the log |
What ttl have you used and what the Docker retry interval? |
ttl - 5s |
it maybe better to introduce setting for the failed upload ttl, having 4 secs as default |
Signed-off-by: munishchouhan <[email protected]>
Added failureTTL setting, now caching begins with every retry when status is failed |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Ok, changed to |
.bucket(blobConfig.storageBucket.replaceFirst("s3://","")) | ||
.key(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is is to tight to s3, we may want to use other storage in the future. Let's use BucketTokenizer
Same for delete method below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/main/groovy/io/seqera/wave/service/blob/impl/BlobCacheServiceImpl.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: Paolo Di Tommaso <[email protected]>
03388ad
to
05f7018
Compare
Rationalised a bit in order to:
|
Signed-off-by: Paolo Di Tommaso <[email protected]>
This PR will implement the functionality to check the size of uploaded blob to cache, if its not same delete it