Skip to content
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

feat: Improve container errors messages format, fixes #6042 #6043

Merged

Conversation

penyaskito
Copy link
Member

@penyaskito penyaskito commented Apr 2, 2024

The Issue

Container errors messages contain commands to know more for debugging the issue embedded in the error message itself, which makes it harder to visually parse.

How This PR Solves The Issue

Shows the commands that can provide more information in separate lines.

Message will look like:

Failed waiting for web/db containers to become ready: web container failed: log=, err=health check timed out after 2m0s: labels map[com.ddev.site-name:project-name com.docker.compose.service:web] timed out without becoming healthy, status=, detail= ddev-project-name-web:starting - more info with [
        ddev logs -s web
        docker logs ddev-project-name-web
        docker inspect --format "{{ json .State.Health }}" ddev-project-name-web
]

Manual Testing Instructions

Try breaking project containers on ddev start and see what happens.

I don't really know how to break them, so I've only really tested a timeout scenario using:

echo 'RUN chmod 000 /run/php' > .ddev/web-build/Dockerfile

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

Copy link

github-actions bot commented Apr 2, 2024

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! You can also replace all please use %s to find out why it failed with more info with %s to show all the suggested commands at the end of the sentence.

@stasadev stasadev force-pushed the penyaskito-6042-container-errors-format branch from ada4ea2 to 180be19 Compare April 2, 2024 17:11
@stasadev
Copy link
Member

stasadev commented Apr 2, 2024

Rebased to resolve conflicts.

@penyaskito penyaskito marked this pull request as ready for review April 3, 2024 04:07
@penyaskito penyaskito requested a review from a team as a code owner April 3, 2024 04:07
@penyaskito penyaskito requested a review from stasadev April 3, 2024 04:08
@stasadev stasadev changed the title feat: Improve container errors messages format, fix #6042 feat: Improve container errors messages format, fixes #6042 Apr 3, 2024
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, tested an unhealthy container using #6033 as a base, with changes from this PR:

Failed waiting for web/db containers to become ready: web container failed: log=&{2024-04-03 14:05:33.767050037 +0300 EEST 2024-04-03 14:05:34.569456346 +0300 EEST 143 php-fpm:FATAL Shut down
}, err=ddev-d10-web container is unhealthy: &{2024-04-03 14:05:33.767050037 +0300 EEST 2024-04-03 14:05:34.569456346 +0300 EEST 143 php-fpm:FATAL Shut down
}, more info with [
        ddev logs -s web
        docker logs ddev-d10-web
        docker inspect --format "{{ json .State.Health }}" ddev-d10-web
]

@rfay rfay force-pushed the penyaskito-6042-container-errors-format branch from bd3e959 to bc5ff17 Compare April 4, 2024 17:40
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me for adding just a tiny bit to this. I wanted everybody to be able to use this with jq, because you can't understand the healthcheck output either.

I also tested this with a 3rd party service, ddev get ddev/ddev-redis and added this healthcheck to docker-compose.redis.yaml to make it break right away:

    healthcheck: 
      test: "false"
      interval: 1s
      retries: 2
      start_period: "2s"
      start_interval: 1s
      timeout: 2s

It now looks like this:

Failed to start d10: container(s) failed to become healthy before their configured timeout or in 120 seconds. This might be a problem with the healthcheck and not a functional problem. (ddev-d10-redis container is unhealthy: &{2024-04-04 11:43:20.059173818 -0600 MDT 2024-04-04 11:43:20.104156459 -0600 MDT 1 }, more info with [
	ddev logs -s redis
	docker logs ddev-d10-redis
	docker inspect --format "{{ json .State.Health }}" ddev-d10-redis | docker run -i ddev/ddev-utilities jq -r
])

and the resultant output is

$ docker inspect --format "{{ json .State.Health }}" ddev-d10-redis | docker run -i ddev/ddev-utilities jq -r
{
  "Status": "unhealthy",
  "FailingStreak": 309,
  "Log": [
    {
      "Start": "2024-04-04T11:43:51.606739703-06:00",
      "End": "2024-04-04T11:43:51.645923413-06:00",
      "ExitCode": 1,
      "Output": ""
    },
    {
      "Start": "2024-04-04T11:43:52.650301012-06:00",
      "End": "2024-04-04T11:43:52.690989569-06:00",
      "ExitCode": 1,
      "Output": ""
    },
    {
      "Start": "2024-04-04T11:43:53.695337084-06:00",
      "End": "2024-04-04T11:43:53.746698024-06:00",
      "ExitCode": 1,
      "Output": ""
    },
    {
      "Start": "2024-04-04T11:43:54.749332066-06:00",
      "End": "2024-04-04T11:43:54.796803723-06:00",
      "ExitCode": 1,
      "Output": ""
    },
    {
      "Start": "2024-04-04T11:43:55.805421108-06:00",
      "End": "2024-04-04T11:43:55.860234536-06:00",
      "ExitCode": 1,
      "Output": ""
    }
  ]
}

@rfay rfay requested a review from stasadev April 4, 2024 17:44
@rfay
Copy link
Member

rfay commented Apr 4, 2024

@penyaskito @stasadev please make sure you're OK with it. I just wanted to make sure it went in today for our v1.23.0-beta1 release.

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition, thanks!
Tested it and got pretty formatted output.

@penyaskito
Copy link
Member Author

Can't test this atm but LGTM. Thanks Randy!

@rfay rfay merged commit 52218a7 into ddev:master Apr 4, 2024
6 of 9 checks passed
@rfay
Copy link
Member

rfay commented Apr 4, 2024

Yay, congrats @penyaskito !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants