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]: Healthcheck improvements #2332

Open
EinfachHans opened this issue May 31, 2024 · 15 comments · May be fixed by #2725
Open

[Feat]: Healthcheck improvements #2332

EinfachHans opened this issue May 31, 2024 · 15 comments · May be fixed by #2725

Comments

@EinfachHans
Copy link

Description

With reference on #2200, i see currently two great points to improve the current health checks:

  1. A notification when the health status of a container changes. A health check & status is great, but it would be great to be informed about the health status changing. This notification should probably not be send for containers that are unhealthy while healthcheck period, because there will be a "deployment failed" notification anyways (?)

  2. Adjust the start period to work as in docker healthchecks

Start period provides initialization time for containers that need time to bootstrap. Probe failure during that period will not be counted towards the maximum number of retries. However, if a health check succeeds during the start period, the container is considered started and all consecutive failures will be counted towards the maximum number of retries.

From https://stackoverflow.com/a/53290016

I'm sadly not a php developer, but i would say this should be quite easy:

Here instead of sleeping, the current date should be saved and then here, if the current time is still within the saved start date + the startPeriod, don't increase the counter and maybe log an information about it.

Minimal Reproduction (if possible, example repository)

I created this a a bug, to be able to add a bounty on it...

Exception or Error

No response

Version

v4.0.0-beta.291

Copy link

algora-pbc bot commented May 31, 2024

💎 $30 bounty • EinfachHans

Steps to solve:

  1. Start working: Comment /attempt #2332 with your implementation plan
  2. Submit work: Create a pull request including /claim #2332 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to coollabsio/coolify!

Add a bountyShare on socials

Attempt Started (GMT+0) Solution
🟢 @Anshgrover23 Jun 30, 2024, 7:36:51 AM #2725

@Anshgrover23
Copy link

Anshgrover23 commented Jun 30, 2024

/attempt #2332

@Anshgrover23
Copy link

@algora-pbc check this pr @EinfachHans @cmer @wutangpaul @AshKyd

@EinfachHans
Copy link
Author

@Anshgrover23 Awesome you tackled this! For the bounty you have to tackle the second point as well, is this possible? 😊

@Anshgrover23
Copy link

@EinfachHans yes it is possible I will make new commit by today on this issue for second point also

@Anshgrover23
Copy link

@EinfachHans done with the changes plss check

@EinfachHans
Copy link
Author

@Anshgrover23 your commit doesn't look like what i described in this issue 🤔

@Anshgrover23
Copy link

@EinfachHans i dont understand i have done what is told in the issue that is i have created a notification function when the health status of a container changes and changes docker-compose services timeout period which you have mentioned in point 2

@Anshgrover23
Copy link

@EinfachHans am i wrong and where plss tell?

@EinfachHans
Copy link
Author

@Anshgrover23 In your second commit you only add whitespaces. Please see description of this issue about point 2. It's about the current health check implementation and to change it to not await the start period. Everything is described in the issue.

Also i would recommend not to ping anyone to review the pr. Whenever there is time andras will review it i guess 😊

@Anshgrover23
Copy link

@EinfachHans sorry for making spam tagging everywhere and i have changed two docker compose.yml file you can see in my latest commit and i have made health check implementation. and kindly can u please check for me all this and then plss reply back if u can ? iam waiting for your review

@Anshgrover23
Copy link

@EinfachHans i want to understand where i am lagging off

@EinfachHans
Copy link
Author

Again: Please see the original issue description. I paste it here again, i think it is quite clear:

Adjust the start period to work as in docker healthchecks

Start period provides initialization time for containers that need time to bootstrap. Probe failure during that period will not be counted towards the maximum number of retries. However, if a health check succeeds during the start period, the container is considered started and all consecutive failures will be counted towards the maximum number of retries.

From https://stackoverflow.com/a/53290016

I'm sadly not a php developer, but i would say this should be quite easy:

Here instead of sleeping, the current date should be saved and then here, if the current time is still within the saved start date + the startPeriod, don't increase the counter and maybe log an information about it.

@Anshgrover23
Copy link

@EinfachHans ok i understand now i am working on this now sorry for the inconvenience

@Anshgrover23
Copy link

@EinfachHans check i have made a new commit and complete the instrcutions given in description and sorry for any misunderstanding.

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

Successfully merging a pull request may close this issue.

2 participants