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 endpoint to of-watchdog #13

Merged
merged 1 commit into from
Sep 17, 2018

Conversation

viveksyngh
Copy link
Contributor

@viveksyngh viveksyngh commented Apr 3, 2018

Introduce new endpoint /_/health to watchdog for health status of
functions which checks for /tmp/.lock file

Added tests for healthHandler

Issues: openfaas/faas#547

Signed-off-by: Vivek Singh vivekkmr45@yahoo.in

Description

Motivation and Context

How Has This Been Tested?

This has been tested on Mac by building the watchdog binary.

curl -i http://localhost:8081/_/health
HTTP/1.1 200 OK
Date: Tue, 03 Apr 2018 03:51:24 GMT
Content-Length: 2
Content-Type: text/plain; charset=utf-8

OK%

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.


func removeLockFile() error {
path := filepath.Join(os.TempDir(), ".lock")
log.Printf("Removing lock-file : %s\n", path)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't encourage logging messages in tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are methods in the testing package for this. t.Log and t.Logf which only get printed when a test fails or the -test.v flag is set. See here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the log statement.

@alexellis
Copy link
Member

I think this looks fine. @johnmccabe can you comment?

main.go Outdated
func makeHealthHandler() func(w http.ResponseWriter, r *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
switch r.Method {
case "GET":
Copy link
Contributor

@johnmccabe johnmccabe Apr 15, 2018

Choose a reason for hiding this comment

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

use http.MethodGet here for consistency with the existing watchdog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree .. I have updated this.

return true
}

func makeHealthHandler() func(w http.ResponseWriter, r *http.Request) {
Copy link
Contributor

@johnmccabe johnmccabe Apr 15, 2018

Choose a reason for hiding this comment

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

Should this not be the same as the old watchdog implementation? The acceptingConnections check is missing here and from the lock() function. Why is that no longer needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

The acceptingConnections check is part of the graceful shutdown that's currently only in the original watchdog. There's an issue to port it over here: #10

I'm starting to work on that. Hoping to have a PR tonight (US time)

Introduce new endpoint `/_/health` to watchdog for health status of
functions which checks for `/tmp/.lock` file

Added tests for healthHandler

Issues: openfaas/faas#547

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@johnmccabe
Copy link
Contributor

Lgtm 👍

@alexellis alexellis merged commit 10751f5 into openfaas:master Sep 17, 2018
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.

4 participants