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

[s3inbox] run healthchecks in main process #1025

Merged
merged 11 commits into from
Sep 12, 2024

Conversation

MalinAhlberg
Copy link
Contributor

@MalinAhlberg MalinAhlberg commented Sep 3, 2024

Related issue(s) and PR(s)
This PR addresses #992.

Description

  • The /health endpoint is put on the same port as s3inbox (ie. is run on the server started in s3inbox.go).
  • Also a HEAD request to s3inbox will return the health status.
  • If there are problems with the connection to MQ or DB while checking the health, CheckHealth attempts to restore it.
  • Go tests updated
  • Integration tests added
  • The /ready and /live endpoints are removed
  • The /healthz enpoint (provided by the charts) is removed

If this is to be continue: create issues for updating charts etc reflect this change.

How to test
curl 'localhost:18000' -H 'Authorization: Bearer $token' -I
curl 'localhost:18000/health' -H 'Authorization: Bearer $token'

@jbygdell
Copy link
Collaborator

jbygdell commented Sep 3, 2024

You're on the right path.
But the healcheck functions needs to be inside the proxy struct so it can issue reconnect/recreate if the MQ or DB connections go down. Look in the checkAndSendMessage function and see an example.

@MalinAhlberg MalinAhlberg force-pushed the feature/s3inbox_rework_healthchecks branch from 0ee0e2b to b123dbb Compare September 10, 2024 08:09
@MalinAhlberg MalinAhlberg force-pushed the feature/s3inbox_rework_healthchecks branch from b123dbb to 9679f79 Compare September 10, 2024 08:11
@MalinAhlberg MalinAhlberg marked this pull request as ready for review September 10, 2024 08:32
@MalinAhlberg MalinAhlberg requested a review from a team September 10, 2024 08:32
.github/integration/tests/sda/09_healthchecks.sh Outdated Show resolved Hide resolved
.github/integration/tests/sda/09_healthchecks.sh Outdated Show resolved Hide resolved
sda/cmd/s3inbox/healthchecks.go Outdated Show resolved Hide resolved
sda/cmd/s3inbox/s3inbox.go Show resolved Hide resolved
sda/cmd/s3inbox/healthchecks.go Outdated Show resolved Hide resolved
sda/cmd/s3inbox/healthchecks.go Outdated Show resolved Hide resolved
@MalinAhlberg
Copy link
Contributor Author

Thanks for the quick review @jbygdell! Fixes are pushed.

@MalinAhlberg MalinAhlberg force-pushed the feature/s3inbox_rework_healthchecks branch from 904ee5e to 98321ef Compare September 10, 2024 09:41
@MalinAhlberg MalinAhlberg requested review from jbygdell and a team September 10, 2024 09:43
@MalinAhlberg MalinAhlberg force-pushed the feature/s3inbox_rework_healthchecks branch from e5c12dd to ad0a562 Compare September 10, 2024 12:04
@jbygdell jbygdell enabled auto-merge September 11, 2024 09:58
Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

Great work! I left a minor comment regarding the use of GET request and HEAD request for obtaining the HTTP code. But I approve it anyway 😄

@jbygdell jbygdell added this pull request to the merge queue Sep 12, 2024
Merged via the queue into main with commit d7100cf Sep 12, 2024
25 checks passed
@jbygdell jbygdell deleted the feature/s3inbox_rework_healthchecks branch September 12, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants