-
Notifications
You must be signed in to change notification settings - Fork 696
Harden Docker container runtime health check #10402
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
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.
Pull Request Overview
This PR strengthens the container runtime health check by verifying that the Docker daemon is running before checking Docker Buildx availability.
- Adds
CheckDockerDaemonAsync
to probe the Docker daemon viadocker container ls -n 1
- Adds
CheckDockerBuildxAsync
to validate that Buildx is installed and functioning - Updates
CheckIfRunningAsync
to chain both checks sequentially
Comments suppressed due to low confidence (1)
src/Aspire.Hosting/Publishing/DockerContainerRuntime.cs:134
- Add unit tests for the new
CheckDockerDaemonAsync
andCheckDockerBuildxAsync
methods, covering both successful and failure paths to ensure the health check logic is fully validated.
public async Task<bool> CheckIfRunningAsync(CancellationToken cancellationToken)
OnErrorData = error => | ||
{ | ||
logger.LogInformation("docker buildx version (stderr): {Error}", error); | ||
logger.LogInformation("docker container ls (stderr): {Error}", error); |
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.
[nitpick] Consider using LogWarning or LogError for stderr output to better reflect that this data represents an error stream rather than informational messages.
logger.LogInformation("docker container ls (stderr): {Error}", error); | |
logger.LogWarning("docker container ls (stderr): {Error}", error); |
Copilot uses AI. Check for mistakes.
OnOutputData = output => | ||
{ | ||
logger.LogInformation("docker buildx version (stdout): {Output}", output); | ||
logger.LogInformation("docker container ls (stdout): {Output}", output); | ||
}, | ||
OnErrorData = error => | ||
{ | ||
logger.LogInformation("docker buildx version (stderr): {Error}", error); | ||
logger.LogInformation("docker container ls (stderr): {Error}", error); | ||
}, |
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.
[nitpick] The OnOutputData and OnErrorData logging closures are duplicated in both health-check methods; consider extracting a shared helper to reduce duplication and improve maintainability.
OnOutputData = output => | |
{ | |
logger.LogInformation("docker buildx version (stdout): {Output}", output); | |
logger.LogInformation("docker container ls (stdout): {Output}", output); | |
}, | |
OnErrorData = error => | |
{ | |
logger.LogInformation("docker buildx version (stderr): {Error}", error); | |
logger.LogInformation("docker container ls (stderr): {Error}", error); | |
}, | |
OnOutputData = output => LogProcessOutput("stdout", output), | |
OnErrorData = error => LogProcessOutput("stderr", error), |
Copilot uses AI. Check for mistakes.
Do the same for podman? |
/backport to release/9.4 |
Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16283710455 |
Closes #10391.