[release/8.0.4xx] Containers - Retry on download blob #48987
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
surayya-MS
changed the title
| @@ -539,6 +539,37 @@ public void IsRegistryInsecure(string registryName, string? insecureRegistriesEn | |||
| Assert.Equal(expectedInsecure, registrySettings.IsInsecure); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task DownloadBlobAsync_RetriesOnFailure() | |||
There was a problem hiding this comment.
Should there also be a test that the download gives up if the number of retries is exceeded?
| mockRegistryAPI.Verify(api => api.Blob.GetStreamAsync(repoName, descriptor.Digest, cancellationToken), Times.Exactly(3)); // Verify retries | ||
|
|
||
| //Cleanup | ||
| File.Delete(result); |
There was a problem hiding this comment.
nit: Should this be in a finally block so it cleans up even when the test fails?
| @@ -237,6 +237,11 @@ private static async Task<int> PushToRemoteRegistryAsync(ILogger logger, BuiltIm | |||
| cancellationToken)).ConfigureAwait(false); | |||
| logger.LogInformation(Strings.ContainerBuilder_ImageUploadedToRegistry, destinationImageReference, destinationImageReference.RemoteRegistry.RegistryName); | |||
| } | |||
| catch (UnableToDownloadFromRepositoryException) | |||
There was a problem hiding this comment.
I would expect to see this updated in PushToLocalRegistryAsync as well, correct? That's at the root of the stack in the linked issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #48940
Description/Customer Impact
When building images we often have to pull base image layers from remote sources. These layers can be quite large, and so network interruption can cause the entire build to report failure. Typically we try to retry these operations (e.g. the Copy Task in MSBuild itself), so we should do the same here.
We're forced to use pretty naive retries (operation-level, not chunk-level) because the OCI HTTP API and many popular registries do not support chunked requests well.
Changes made
DownloadBlobAsyncin case of failure from upgraded logging for #595 log url when connection is closed #44953 (too many commits, something goes wrong when try git cherry-pick)Registry.DownloadBlobAsyncby adding a retry mechanismTesting
Added unit test for retry mechanism.
Risk
Low - tests have been added and the mechanism is very simple.