Users/stfrance/add containerfetch timeout envvar #1132
Conversation
|
@stephenmichaelf search for string 'GetFileAsyncTimeoutMinutes', that string will print out on timeout, that int is always 5, i guess you need make some change there to make it print the right timeout. :) |
|
@GitHubSriramB please take a look. |
| @@ -236,8 +236,20 @@ private bool Match(ServerBuildArtifact buildArtifact, ArtifactDefinition artifac | |||
| DownloadBufferSize = executionContext.Variables.Release_Download_BufferSize ?? ContainerFetchEngineDefaultOptions.DownloadBufferSize | |||
| }; | |||
|
|
|||
| int fetchEngineTimeoutSeconds; | |||
| if (!int.TryParse(Environment.GetEnvironmentVariable("VSTS_FETCHENGINE_TIMEOUT") ?? string.Empty, out fetchEngineTimeoutSeconds)) | |||
There was a problem hiding this comment.
Can we use VSTS_HTTP_TIMEOUT here instead of introducing another variable?
There was a problem hiding this comment.
@stephenmichaelf my fault, i forgot v2 agent use VSTS_HTTP_TIMEOUT even for file container upload. I agree with @GitHubSriramB, let's just use VSTS_HTTP_TIMEOUT.
check this: https://github.com/Microsoft/vsts-agent/blob/master/src/Agent.Worker/Build/FileContainerServer.cs#L43
|
@GitHubSriramB I added some changes based on feedback, I am not sure if these are correct so please review. It seems like there are two competing task timeouts:
I think what's happening beofore is that #1 was being set from the hardcoded value of GetFileAsyncTimeout in ContainerFetchEngineDefaultOPtions which was lower than VSTS_HTTP_TIMEOUT so that first timeout was firing earlier. From my understanding there might be more cleanup that can be done but I think at least if we set GetFileAsyncTimeout to respect the VSTS_HTTP_TIMEOUT environment variable then we should solve the problem. |
| @@ -236,8 +236,20 @@ private bool Match(ServerBuildArtifact buildArtifact, ArtifactDefinition artifac | |||
| DownloadBufferSize = executionContext.Variables.Release_Download_BufferSize ?? ContainerFetchEngineDefaultOptions.DownloadBufferSize | |||
| }; | |||
|
|
|||
| int fetchEngineTimeoutSeconds; | |||
| if (!int.TryParse(Environment.GetEnvironmentVariable("VSTS_HTTP_TIMEOUT") ?? string.Empty, out fetchEngineTimeoutSeconds)) | |||
There was a problem hiding this comment.
don't see why we should cap the max. timeout. instead it is better to not let the file download to timeout. there is already a timeout for the deployment configured and if download exceeds this timeout, it would be canceled by agent.
@stephenmichaelf - is it fine if you let @omeshp to take this fix forward?
There was a problem hiding this comment.
@GitHubSriramB Sounds good thanks!
…os10.14_week31 macos 1014 week 31 updates
vtbassmatt
deleted the
users/stfrance/add-containerfetch-timeout-envvar
branch
This is in reference to #1030
I added a new environment variable that can override the GetFileAsyncTimeout for the ContainerFetchEngine.
@GitHubSriramB please review and see if this is an appropriate solution.