Avastancu/joannaakl/service container error log (#2110) #2182
Conversation
This file contains 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
* adding support for a service container docker logs * Adding Unit test to ContainerOperationProvider * Adding another test to ContainerOperationProvider * placed the docker logs output in dedicated ##group section * Removed the exception thrown if the service container was not healthy * Removed duplicated logging to the executionContext * Updated the container logs sub-section message * Print service containers only if they were healthy Unhealthy service logs are printed in ContainerHealthCheckLogs called prior to this step. * Removed recently added method to inspect docker logs The method was doing the same thing as the existing DockerLogs method. * Added execution context error This will make a failed health check more visible in the UI without disrupting the execution of the program. * Removing the section 'Waiting for all services to be ready' Since nested subsections are not being displayed properly and we already need one subsection per service error. * Update src/Runner.Worker/Container/DockerCommandManager.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/TestHostContext.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Change the logic for printing Service Containers logs Service container logs will be printed in the 'Start containers' section only if there is an error. Healthy services will have their logs printed in the 'Stop Containers' section. * Removed unused import * Added back section group. * Moved service containers error logs to separate group sections * Removed the test testing the old logic flow. * Remove unnecessary 'IsAnyUnhealthy' flag * Remove printHello() function * Add newline to TestHostContext * Remove unnecessary field 'UnhealthyContainers' * Rename boolean flag indicating service container failure * Refactor healthcheck logic to separate method to enable unit testing. * Remove the default value for bool variable * Update src/Runner.Worker/ContainerOperationProvider.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Runner.Worker/ContainerOperationProvider.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Rename Healthcheck back to ContainerHealthcheck * Make test sequential * Unextract the container error logs method * remove test asserting thrown exception * Add configure await * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Add back test asserting exception * Check service exit code if there is no healtcheck configured * Remove unnecessary healthcheck for healthy service container * Revert "Check service exit code if there is no healtcheck configured" This reverts commit fec24e8. Co-authored-by: Ava S <avastancu@github.com> Co-authored-by: Tingluo Huang <tingluohuang@github.com>
AvaStancu
changed the title
TingluoHuang
reviewed
Oct 7, 2022
AvaStancu
force-pushed
the
avastancu/joannaakl/service-container-error-log-with-empty-string-healthcheck
branch
from
b0f43a5
to
36f15d0
Compare
AvaStancu
force-pushed
the
avastancu/joannaakl/service-container-error-log-with-empty-string-healthcheck
branch
from
36f15d0
to
73998e6
Compare
TingluoHuang
approved these changes
Oct 10, 2022
AvaStancu
deleted the
avastancu/joannaakl/service-container-error-log-with-empty-string-healthcheck
branch
eli-entelis
added a commit
to eli-entelis/runner
that referenced
this pull request
Oct 11, 2022
Avastancu/joannaakl/service container error log (actions#2110) (actions#2182)
ChristopherHX
pushed a commit
to ChristopherHX/runner.server
that referenced
this pull request
Nov 24, 2022
…ns#2182) * Avastancu/joannaakl/service container error log (actions#2110) * adding support for a service container docker logs * Adding Unit test to ContainerOperationProvider * Adding another test to ContainerOperationProvider * placed the docker logs output in dedicated ##group section * Removed the exception thrown if the service container was not healthy * Removed duplicated logging to the executionContext * Updated the container logs sub-section message * Print service containers only if they were healthy Unhealthy service logs are printed in ContainerHealthCheckLogs called prior to this step. * Removed recently added method to inspect docker logs The method was doing the same thing as the existing DockerLogs method. * Added execution context error This will make a failed health check more visible in the UI without disrupting the execution of the program. * Removing the section 'Waiting for all services to be ready' Since nested subsections are not being displayed properly and we already need one subsection per service error. * Update src/Runner.Worker/Container/DockerCommandManager.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/TestHostContext.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Change the logic for printing Service Containers logs Service container logs will be printed in the 'Start containers' section only if there is an error. Healthy services will have their logs printed in the 'Stop Containers' section. * Removed unused import * Added back section group. * Moved service containers error logs to separate group sections * Removed the test testing the old logic flow. * Remove unnecessary 'IsAnyUnhealthy' flag * Remove printHello() function * Add newline to TestHostContext * Remove unnecessary field 'UnhealthyContainers' * Rename boolean flag indicating service container failure * Refactor healthcheck logic to separate method to enable unit testing. * Remove the default value for bool variable * Update src/Runner.Worker/ContainerOperationProvider.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Runner.Worker/ContainerOperationProvider.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Rename Healthcheck back to ContainerHealthcheck * Make test sequential * Unextract the container error logs method * remove test asserting thrown exception * Add configure await * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Add back test asserting exception * Check service exit code if there is no healtcheck configured * Remove unnecessary healthcheck for healthy service container * Revert "Check service exit code if there is no healtcheck configured" This reverts commit fec24e8. Co-authored-by: Ava S <avastancu@github.com> Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Do not fail service containers without the healthcheck Co-authored-by: JoannaaKL <joannaakl@github.com> Co-authored-by: Tingluo Huang <tingluohuang@github.com>
nikola-jokic
pushed a commit
to nikola-jokic/runner
that referenced
this pull request
May 12, 2023
…ns#2182) * Avastancu/joannaakl/service container error log (actions#2110) * adding support for a service container docker logs * Adding Unit test to ContainerOperationProvider * Adding another test to ContainerOperationProvider * placed the docker logs output in dedicated ##group section * Removed the exception thrown if the service container was not healthy * Removed duplicated logging to the executionContext * Updated the container logs sub-section message * Print service containers only if they were healthy Unhealthy service logs are printed in ContainerHealthCheckLogs called prior to this step. * Removed recently added method to inspect docker logs The method was doing the same thing as the existing DockerLogs method. * Added execution context error This will make a failed health check more visible in the UI without disrupting the execution of the program. * Removing the section 'Waiting for all services to be ready' Since nested subsections are not being displayed properly and we already need one subsection per service error. * Update src/Runner.Worker/Container/DockerCommandManager.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/TestHostContext.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Change the logic for printing Service Containers logs Service container logs will be printed in the 'Start containers' section only if there is an error. Healthy services will have their logs printed in the 'Stop Containers' section. * Removed unused import * Added back section group. * Moved service containers error logs to separate group sections * Removed the test testing the old logic flow. * Remove unnecessary 'IsAnyUnhealthy' flag * Remove printHello() function * Add newline to TestHostContext * Remove unnecessary field 'UnhealthyContainers' * Rename boolean flag indicating service container failure * Refactor healthcheck logic to separate method to enable unit testing. * Remove the default value for bool variable * Update src/Runner.Worker/ContainerOperationProvider.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Runner.Worker/ContainerOperationProvider.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Rename Healthcheck back to ContainerHealthcheck * Make test sequential * Unextract the container error logs method * remove test asserting thrown exception * Add configure await * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Update src/Test/L0/Worker/ContainerOperationProviderL0.cs Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Add back test asserting exception * Check service exit code if there is no healtcheck configured * Remove unnecessary healthcheck for healthy service container * Revert "Check service exit code if there is no healtcheck configured" This reverts commit fec24e8. Co-authored-by: Ava S <avastancu@github.com> Co-authored-by: Tingluo Huang <tingluohuang@github.com> * Do not fail service containers without the healthcheck Co-authored-by: JoannaaKL <joannaakl@github.com> Co-authored-by: Tingluo Huang <tingluohuang@github.com>
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.
Co-authored-by: Joanna KL joannaakl@github.com
Co-authored-by: Ava S avastancu@github.com
Co-authored-by: Tingluo Huang tingluohuang@github.com
Co-authored-by: Ferenc Hammerl fhammerl@github.com
IMPORTANT:
This solution displays the error logs in dedicated sub-sections of the
Initialize containerssection ONLY with a docker healthcheck running. If there is no healthcheck or the service is healthy, failing containers will not have their logs visible in the workflow runInitialize containersresults but they will be present in theStop containerssection, unformatted.For containers with errors but no healthcheck this is an issue we should solve and there is a new runner issue created for it: https://github.com/github/c2c-actions-runtime/issues/2021
We are considering a container healthy if:
Before
Manual test before last commit for a healthy container (no errors, no healtcheck):
After
Manual tests after last commit for a healthy container (no errors, no healtcheck):
Manual tests after last commit for a healthy container (errors and healthcheck):
Logs for the healthy containers with errors but with no healthcheck will be available in the 'Stop containers' section: