Impoving container initialize logic #5280
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MantavyaDh
changed the title
…s://github.com/microsoft/azure-pipelines-agent into users/mdhingra/ImproveContainerInitialiseLogic
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| context.Output($"Container {containerId} is already running after {retryCount} retries. but exit code was {exitCode}."); | ||
| exitCode = 0; // Indicate success | ||
| } | ||
| // If the container is still not running after retries, log a warning |
There was a problem hiding this comment.
Warning inside this block already shows container not running after retires exhausted. No need for comment here.
There was a problem hiding this comment.
I think we should log a warning that the container didn't start after all the retries, and the last exit code.
I agree that we are logging a warning for every failed attempt, but doesn't tell the overall outcome rather step-wise outcome.
| { | ||
| context.Output($"Container {containerId} started successfully after {retryCount} retries."); | ||
| } | ||
| //return the exit code |
There was a problem hiding this comment.
Is this comment necessary. This seems redundant so can be removed.
There was a problem hiding this comment.
It marks the end of the function Docker Start, with the exit code.
| @@ -374,7 +379,7 @@ public async Task<List<PortMapping>> DockerPort(IExecutionContext context, strin | |||
| /// <returns | |||
| /// <c>true</c>, if specified container is running, <c>false</c> otherwise. | |||
| /// </returns> | |||
| public async Task<bool> IsContainerRunning(IExecutionContext context, string containerId) | |||
| public virtual async Task<bool> IsContainerRunning(IExecutionContext context, string containerId) | |||
There was a problem hiding this comment.
Any specific reason why we have made this function virtual?
There was a problem hiding this comment.
Made the function virtual to override it in the unit test class DockerCommandManagerL0Tests.cs
There was a problem hiding this comment.
Instead of overriding a function in unit tests, a better practice is to mock the function in test with given input/output.
| /// Executes 'docker start' with retries, checking if the container is already running before each retry. | ||
| /// Returns 0 if the container is running or started successfully, otherwise returns the last exit code. | ||
| /// </summary> | ||
| protected virtual async Task<int> ExecuteDockerStartWithRetriesAndCheck(IExecutionContext context, string containerId) |
There was a problem hiding this comment.
Any specific reason why we have made this function virtual?
There was a problem hiding this comment.
Same reason as above
There was a problem hiding this comment.
Instead of overriding a function in unit tests, a better practice is to mock the function in test with given input/output.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Context
The PR aims to improve the container initialize logic, before retrying to start a container we have added a check to see if its already running as we've had cases wherein the container starts but not reported by docker daemon.
Description
Added logic to check if container already running before retrying.
Risk Assessment (Low / Medium / High)
Low, only introduces a check before retrying
Unit Tests Added or Updated (Yes / No)
Yes, Unit tests added
Additional Testing Performed
Tested in personal org
Change Behind Feature Flag (Yes / No)
Yes, FF-
DistributedTask.Agent.CheckBeforeRetryDockerStartDocumentation Changes Required (Yes/No)
No
Logging Added/Updated (Yes/No)
Logs added.
Rollback Scenario and Process (Yes/No)
Turn off the FF
Dependency Impact Assessed and Regression Tested (Yes/No)