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

Add health check endpoint #18465

Merged
merged 16 commits into from
May 4, 2022
Merged

Add health check endpoint #18465

merged 16 commits into from
May 4, 2022

Conversation

ttys3
Copy link
Contributor

@ttys3 ttys3 commented Jan 30, 2022

Add /api/healthz for health check. Which does not need auth and is cloud native friendly

By convention, most cases the path is /healthz or /_healthz, I think we need this.

https://kubernetes.io/docs/reference/using-api/health-checks/

https://www.nomadproject.io/docs/job-specification/service#header-stanza

@codecov-commenter

This comment was marked as off-topic.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 30, 2022
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

This will require a new username restriction,

Is there really nothing we can use on say /api or even in .well-known?

If it has to be this name then you must add a restriction to models/user and mark this as BREAKING.

@ttys3
Copy link
Contributor Author

ttys3 commented Jan 30, 2022

@zeripath no, the path (/healthz or /_healthz) is only for convention.

do you think it is OK change to /api/_healthz ? thus, it is not BREAKING

@ttys3 ttys3 force-pushed the main branch 2 times, most recently from 358c0a7 to 5d3a7d2 Compare January 30, 2022 16:10
@lunny
Copy link
Member

lunny commented Jan 30, 2022

We already have HEAD /

@ttys3
Copy link
Contributor Author

ttys3 commented Jan 30, 2022

HEAD / is not as clear as a standalone GET /_healthz.
and also, not all health check has builtin HEAD method option. although you can use a custom http client like curl, but this is not normal way and need user do more things.

adding an explicit GET /_healthz will always be good and has better compatibility and does not add extra overhead

@ttys3 ttys3 requested a review from zeripath January 30, 2022 16:54
@techknowlogick
Copy link
Member

Fwiw, the gitea project uses the version api endpoint for this in our monitoring

routers/init.go Outdated Show resolved Hide resolved
@ttys3 ttys3 requested a review from lafriks January 31, 2022 02:15
@6543 6543 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 31, 2022
@6543 6543 added this to the 1.17.0 milestone Jan 31, 2022
@6543 6543 removed the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 31, 2022
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

as per tech ... use the version endpoint it does the same (return 200 & a tiny string)

It would only be a valid pull if it really do check health of the running instance (check DB, cache, storage backends, ...

@ttys3
Copy link
Contributor Author

ttys3 commented Jan 31, 2022

@6543

yes, I tried the /api/v1/version

The version api does need authentication. So it does not meet the needs.

returns 403 and json {"message":"Only signed in user is allowed to call APIs."}

I know how to build a request to pass the authentication. but this is not a ping api shoud look like.

when system op deploy the app, it does not and has no need to know the user account of the system or a token.

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

ACK for code

but we need documentation

@6543 6543 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 31, 2022
@ttys3
Copy link
Contributor Author

ttys3 commented Jan 31, 2022

@6543 some tips ? Sorry, I don't know where to add the documentation.

Since it is not an API for client request, I think it is Ok not write swagger doc.
Maybe in the doc markdown ? But, which section ?

@6543
Copy link
Member

6543 commented Jan 31, 2022

yes swagger docs would be the wrong place :D

-> https://github.com/go-gitea/gitea/tree/main/docs (just read the readme in there - it's a hugo repo)

add it to https://docs.gitea.io/en-us/install-on-kubernetes/ ?

@silverwind
Copy link
Member

silverwind commented Feb 1, 2022

Consider implementing a response as per https://datatracker.ietf.org/doc/html/draft-inadarei-api-health-check. I think should at minimum do a test against the database.

GET /api/health
{
  "status": "pass",
  "releaseId": "1.16.0",
  "description": "Gitea: Git with a cup of tea",
  "checks": {
    "database:ping": [
      {
        "status": "pass",
        "time": "2022-01-17T03:36:48Z"
      }
    ]
  }
}

@lunny
Copy link
Member

lunny commented Feb 1, 2022

LGTM if it's /api/xxxx.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 1, 2022
routers/init.go Outdated Show resolved Hide resolved
@6543 6543 changed the title chore: add health check endpoint chore: add ping check endpoint Feb 2, 2022
@ttys3
Copy link
Contributor Author

ttys3 commented Apr 11, 2022

@wxiaoguang option 2 will works, but maybe a little confusing ?
should we add another /readyz that just return a simple OK ? which will works for not installed gitea instance.

the diff between other micro service is, gitea need interactive installation to work for the first time.

is there any automatic method to make this work in k8s without interactive installation? I mean something like a cli to allow fast installation (for example: gitea installation), thus, the instance will initilzation itself, so the healthz api should also work for the first time.

@jbg
Copy link

jbg commented Apr 11, 2022

Does Gitea actually need interactive installation the first time? I thought you could fill in the config and enable install lock and it would just work (run the migrations on first launch).

@wxiaoguang
Copy link
Contributor

@wxiaoguang option 2 will works, but maybe a little confusing ? should we add another /readyz that just return a simple OK ? which will works for not installed gitea instance.

the diff between other micro service is, gitea need interactive installation to work for the first time.

is there any automatic method to make this work in k8s without interactive installation? I mean something like a cli to allow fast installation (for example: gitea installation), thus, the instance will initilzation itself, so the healthz api should also work for the first time.

@lafriks

@lafriks
Copy link
Member

lafriks commented Apr 11, 2022

It's not about if healthz endpoint being usable for k8s, for k8s there is helm chart that will make gitea usable without installation. Problem is that health endpoint is not usable for docker/docker swarm, at least from most used perspective install stage is most common scenario as we don't have easy way to skip it.

@lafriks
Copy link
Member

lafriks commented Apr 11, 2022

I don't see much point in readyz endpoint as that would pretty much be same as HEAD / request. Point for healthz endpoint is that requests can be started to route to that container as it has database and cache available and container is in working condition

@markkrj
Copy link

markkrj commented Apr 11, 2022

Couldn't one /api/healthz endpoint be registered in installation routes that will ever return 200? That way, if Gitea isn't provisioned, Docker will think it's healthy, and if it is, installation routes will not be used anyway...

@lafriks
Copy link
Member

lafriks commented Apr 12, 2022

Couldn't one /api/healthz endpoint be registered in installation routes that will ever return 200? That way, if Gitea isn't provisioned, Docker will think it's healthy, and if it is, installation routes will not be used anyway...

Yes, that was my suggestion

@markkrj
Copy link

markkrj commented Apr 12, 2022

Couldn't one /api/healthz endpoint be registered in installation routes that will ever return 200? That way, if Gitea isn't provisioned, Docker will think it's healthy, and if it is, installation routes will not be used anyway...

Yes, that was my suggestion

@lafriks @wxiaoguang would ttys3#1 be acceptable, then?

This was needed for using /api/healthz endpoint in Docker healthchecks,
otherwise, Docker would never become healthy if using healthz endpoint
and users would not be able to complete the installation of Gitea.
@wxiaoguang wxiaoguang removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Apr 16, 2022
@wxiaoguang
Copy link
Contributor

@lafriks

@markkrj
Copy link

markkrj commented May 3, 2022

@lafriks

Or maybe you just want r.Get("/api/healthz", func(http.ResponseWriter, *http.Request){}) on install routes, to not depend on healthcheck package... That might be even better/simpler..

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

Small nit but otherwise looks good

modules/cache/cache.go Outdated Show resolved Hide resolved
modules/cache/cache.go Outdated Show resolved Hide resolved
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

OK, since there is no objection, let me take the remaining work:

  1. conflicts are resolved
  2. remove CheckInstall, use Check with InstallLock check instead.
  3. set Description to setting.AppName instead of the hard-coded string.

@wxiaoguang wxiaoguang changed the title chore: add health check endpoint Add health check endpoint May 4, 2022
@lafriks lafriks merged commit e933f31 into go-gitea:main May 4, 2022
@wxiaoguang wxiaoguang mentioned this pull request May 4, 2022
7 tasks
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 4, 2022
* giteaofficial/main:
  Add health check endpoint (go-gitea#18465)
  Only check for non-finished migrating task (go-gitea#19601)
  Make .cs highlighting legible on dark themes. (go-gitea#19604)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* chore: add health check endpoint

docs: update document about health check

fix: fix up Sqlite3 ping. current ping will success even if the db file is missing

fix: do not expose privacy information in output field

* refactor: remove HealthChecker struct

* Added `/api/healthz` to install routes.

This was needed for using /api/healthz endpoint in Docker healthchecks,
otherwise, Docker would never become healthy if using healthz endpoint
and users would not be able to complete the installation of Gitea.

* Update modules/cache/cache.go

* fine tune

* Remove unnecessary test code. Now there are 2 routes for installation (and maybe more in future)

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: Marcos de Oliveira <marcossantos@furb.br>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet