Containers: improved DockerRegistryManager for tests reliability
#33955
Conversation
vlada-shubina
added
the
Area-Containers
ghost
added
the
vlada-shubina
force-pushed
the
containers-test-improvements
branch
from
a3ad2b0 to
6877b74
Compare
src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| logger.LogInformation($"Retrying after {spawnRegistryDelay} ms."); | ||
| Thread.Sleep(spawnRegistryDelay); |
There was a problem hiding this comment.
Please don't use the Thread class unless you're dealing with STA cases. For this particular case, consider Task.Delay(spawnRegistryDelay).ConfigureAwait(false);
There was a problem hiding this comment.
Please apply to all Thread.Sleep cases
There was a problem hiding this comment.
We cannot use async calls in test class fixture init, therefore all the calls are sync.
There was a problem hiding this comment.
this does not have to be an async call
There was a problem hiding this comment.
Task.Delay returns task to be awaited, there is no sync version of this method.
There was a problem hiding this comment.
You can call Wait on the task to synchronously wait for completion. I'm not sure if there's any material difference between that and Thread.Sleep, though, especially in test code. Curious to learn more @donJoseLuis.
There was a problem hiding this comment.
I would rather not use Wait in XUnit due to very well-known issue xunit/xunit#864
we had this so many times in dotnet/sdk repo when tests are stuck, and underlying reason is Wait somewhere deep down in the code.
src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs
Outdated
Show resolved
Hide resolved
| foreach (string? tag in new[] { Net6ImageTag, Net7ImageTag, Net8PreviewImageTag }) | ||
| { | ||
| logger.LogInformation($"Pulling image '{BaseImageSource}{RuntimeBaseImage}:{tag}'."); | ||
| ContainerCli.PullCommand(testOutput, $"{BaseImageSource}{RuntimeBaseImage}:{tag}") | ||
| .Execute() | ||
| .Should().Pass(); | ||
|
|
||
| logger.LogInformation($"Tagging image '{BaseImageSource}{RuntimeBaseImage}:{tag}' as '{LocalRegistry}/{RuntimeBaseImage}:{tag}'."); | ||
| ContainerCli.TagCommand(testOutput, $"{BaseImageSource}{RuntimeBaseImage}:{tag}", $"{LocalRegistry}/{RuntimeBaseImage}:{tag}") | ||
| .Execute() | ||
| .Should().Pass(); | ||
|
|
||
| logger.LogInformation($"Pushing image '{LocalRegistry}/{RuntimeBaseImage}:{tag}'."); | ||
| ContainerCli.PushCommand(testOutput, $"{LocalRegistry}/{RuntimeBaseImage}:{tag}") | ||
| .Execute() | ||
| .Should().Pass(); | ||
| } |
There was a problem hiding this comment.
Curious, why do we retry this entire loop and not individual iterations?
There was a problem hiding this comment.
The problematic interaction is push, others are local to local docker daemon.
At first, i added retries also at pushlevel, however, if the registry responses on \v2 (checked in EnsureRegistryLoaded), this means it is healthy and should be able to handle push, so the last thought is adding another retry is overreacting for now. If suggested fix won't help, we can always add retries on push.
If the registry is not healthy, it is worth to restart it - handled in the outer retry loop.
|
Quick ping for @donJoseLuis to check on the status of your requested changes - I think that Vlada and Ladi have talked through them and come to an understanding. |
Related to: dotnet/sdk-container-builds#469
Addresses intermittent issue: #28442 (comment)