Watchdog endpoints for health #583

viveksyngh · 2018-03-15T19:10:28Z

Introduces new endpoint /health in watchdog.

Description

This change covers the first part of this issue #547

Motivation and Context

I have raised an issue to propose this change (required)

How Has This Been Tested?

It has been tested on a local system (Mac) by building the watchdog and doing curl for the /health endpoint.

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.

derek · 2018-03-15T19:10:31Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

alexellis · 2018-03-16T08:52:06Z

watchdog/main.go

@@ -250,6 +250,34 @@ func makeRequestHandler(config *WatchdogConfig) func(http.ResponseWriter, *http.
 	}
 }

+func isLockFileExists() bool {


I am ok with lockFilePresent() for instance.

alexellis · 2018-03-16T08:52:24Z

watchdog/main.go

+	return true
+}
+
+func health() func(http.ResponseWriter, *http.Request) {


makeHealthHandler()

I will change that.

alexellis · 2018-03-16T08:52:55Z

watchdog/main.go

+func health() func(http.ResponseWriter, *http.Request) {
+	return func(w http.ResponseWriter, r *http.Request) {
+		switch r.Method {
+		case "GET":


@stefanprodan does Kubernetes care about the method for a health endpoint? should it accept anything and still respond the same way?

It should be GET with 200 or 500 response code

But is the validation necessary?

alexellis · 2018-03-16T08:54:47Z

watchdog/main.go

@@ -271,6 +299,7 @@ func main() {
 	}

 	http.HandleFunc("/", makeRequestHandler(&config))
+	http.HandleFunc("/health", health())


It should have been with a prefix that cannot clash with the function i.e. /_/ - I think this was mentioned in the issue. Please can you change it?

My bad .. I will add that.

alexellis · 2018-03-16T08:56:36Z

The commits here are a bit messy now. Please can you try to clean that up?

Also - the commit message could be better i.e.

Subject should be under 50 chars in length for each commit
Then give a new-line and write a body message saying a bit about why the change is needed, but don't exceed 72 chars per line.

Here's some more context. I haven't asked you to do this before but it will help with the history - let's see if we can improve it?

https://chris.beams.io/posts/git-commit/

alexellis · 2018-03-16T09:15:43Z

Commit message looks a bit better now - thank you. Should start with a capital letter and we need a new line then a body talking about why the change is needed and what it solves.

viveksyngh · 2018-03-16T09:45:59Z

I made the changes requested and updated the commit message as well.

alexellis · 2018-03-16T11:01:16Z

Calling for a review from @johnmccabe - once the PR is completed we will need to replicate it here too - https://github.com/openfaas-incubator/of-watchdog

johnmccabe

Been unable to get this to work locally, could do with some assistance from @viveksyngh to pinpoint whats wrong here.

johnmccabe · 2018-03-17T18:07:13Z

watchdog/main.go

@@ -232,6 +232,34 @@ func getAdditionalEnvs(config *WatchdogConfig, r *http.Request, method string) [
 	return envs
 }

+func lockFilePresent() bool {
+	path := filepath.Join(os.TempDir(), ".lock")


@alexellis this jumped out at me and having a look at the rest of the Watchdog code it doesn't appear to check/ensure that the temp directory actually exists before attempting to write a lock..

If the user attempts to use the watchdog within a Scratch container then it'll bomb out.

2018/03/17 18:03:15 Writing lock-file to: /tmp/.lock 2018/03/17 18:03:15 Cannot write /tmp/.lock. To disable lock-file set env suppress_lock=true.

Or am I missing something here?

@johnmccabe @alexellis does this behaviour needs to be changed?

It will crash. Having a temporary directory is a requirement.

johnmccabe · 2018-03-17T18:18:48Z

watchdog/main.go

@@ -271,6 +299,7 @@ func main() {
 	}

 	http.HandleFunc("/", makeRequestHandler(&config))
+	http.HandleFunc("/_/health", makeHealthHandler())


@viveksyngh how did you verify this working?

I've built the watchdog from this branch but I've been unable to hit the health endpoint?

$ export fprocess="echo hello" $ ./watchdog 2018/03/17 18:16:38 Writing lock-file to: /var/folders/2z/d4szj605431693r_4pj3sfwr0000gq/T/.lock

Curling the function root works as expected:

$ curl -vvv http://localhost:8080/ * Trying ::1... * TCP_NODELAY set * Connected to localhost (::1) port 8080 (#0) > GET / HTTP/1.1 > Host: localhost:8080 > User-Agent: curl/7.54.0 > Accept: */* > < HTTP/1.1 200 OK < X-Duration-Seconds: 0.004187 < Date: Sat, 17 Mar 2018 18:17:30 GMT < Content-Length: 6 < Content-Type: text/plain; charset=utf-8 < hello * Connection #0 to host localhost left intact

But curling the health endpoint doesn't return the expected OK, instead invoking the function again:

$ curl -vvv http://localhost:8080/_/health * Trying ::1... * TCP_NODELAY set * Connected to localhost (::1) port 8080 (#0) > GET /_/health HTTP/1.1 > Host: localhost:8080 > User-Agent: curl/7.54.0 > Accept: */* > < HTTP/1.1 200 OK < X-Duration-Seconds: 0.003916 < Date: Sat, 17 Mar 2018 18:18:11 GMT < Content-Length: 6 < Content-Type: text/plain; charset=utf-8 < hello * Connection #0 to host localhost left intact

Does this actually work for you?

Ok so your route ordering is wrong here, the health route needs to occur before the / route, currently you're never reaching the /_/health route as / is handling it.

I've fixed the ordering locally and its behaving as expected.

I verified it on my local. I can share the result.

curl -i http://localhost:8081/ HTTP/1.1 200 OK X-Duration-Seconds: 0.005071 Date: Sat, 17 Mar 2018 18:25:38 GMT Content-Length: 1329 Content-Type: text/plain; charset=utf-8 TERM_SESSION_ID=w0t1p0:633DD086-384A-4778-B803-2F9EA6073601 SSH_AUTH_SOCK=/private/tmp/com.apple.launchd.J8jlwZ3nBI/Listeners Apple_PubSub_Socket_Render=/private/tmp/com.apple.launchd.fuASYugNfM/Render COLORFGBG=7;0 ITERM_PROFILE=Default XPC_FLAGS=0x0 LANG=en_US.UTF-8 PWD=/Users/viveks/Repos/go-projects/src/github.com/openfaas/faas/watchdog SHELL=/bin/zsh TERM_PROGRAM_VERSION=3.1.5 TERM_PROGRAM=iTerm.app PATH=/Library/Frameworks/Python.framework/Versions/3.6/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin COLORTERM=truecolor TERM=xterm-256color HOME=/Users/viveks TMPDIR=/var/folders/tw/6ww2mtms12ndnypdycqfy_q40000gy/T/ USER=viveks XPC_SERVICE_NAME=0 LOGNAME=viveks __CF_USER_TEXT_ENCODING=0x1FE:0:0 ITERM_SESSION_ID=w0t1p0:633DD086-384A-4778-B803-2F9EA6073601 SHLVL=1 OLDPWD=/Users/viveks/Repos/go-projects/src/github.com/openfaas/faas ZSH=/Users/viveks/.oh-my-zsh PAGER=less LESS=-R LC_CTYPE=en_US.UTF-8 LSCOLORS=Gxfxcxdxbxegedabagacad GOPATH=/Users/viveks/Repos/go-projects/ VISUAL=vim fprocess=env _=/Users/viveks/Repos/go-projects/src/github.com/openfaas/faas/watchdog/./watchdog Http_User_Agent=curl/7.54.0 Http_Accept=*/* Http_Method=GET Http_ContentLength=0 Http_Path=/

curl -i http://localhost:8081/_/health HTTP/1.1 200 OK Date: Sat, 17 Mar 2018 18:25:43 GMT Content-Length: 2 Content-Type: text/plain; charset=utf-8 OK%

I just changed the port in my local because I have something else running on port 8080.

curl -i -XPOST http://localhost:8081/_/health HTTP/1.1 405 Method Not Allowed Date: Sat, 17 Mar 2018 18:30:29 GMT Content-Length: 0

2018/03/17 23:55:29 Writing lock-file to: /var/folders/tw/6ww2mtms12ndnypdycqfy_q40000gy/T/.lock 2018/03/17 23:55:38 Forking fprocess. 2018/03/17 23:55:38 Wrote 1329 Bytes - Duration: 0.005071 seconds

@johnmccabe I will do that ordering.

If you rebase with master you will find the watchdog as of 0.7.6 now allows port to be overridden temporarily for this kind of purpose.

johnmccabe · 2018-03-17T18:31:08Z

watchdog/main.go

+	return true
+}
+
+func makeHealthHandler() func(http.ResponseWriter, *http.Request) {


Can you explain what the health check is considering as healthy here? Can the presence of the lock file alone be considered sufficient to say that the function is healthy?

According to issue , the /_/health endpoint need to check for /tmp/.lock file.
#547

I think this will also make sure that function container has started successfully and functions can be invoked. Am I missing something?

The lock file can be deleted at any time by the function to indicate an underlying problem.

alexellis · 2018-03-17T19:54:31Z

Speaking of which do we have any tests we can extend to prove the behaviour works?

viveksyngh · 2018-03-17T20:11:22Z

There are no tests right now, I will add tests for makeHealthHandler with following testcases -
1.) /tmp/.lock file present
2.) /tmp/.lock file not present
3.) Invoke /_/health with the method other than GET

alexellis · 2018-03-17T21:38:03Z

We do have tests (I have been using them tonight to push a change in 0.7.6) - please look at the file requesthandler_test.go

Introduce new endpoint `/_/health` to watchdog for health status of functions which check for `/tmp/.lock` file Fixes first part of openfaas#547 issue. Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

viveksyngh · 2018-03-18T14:43:14Z

@alexellis I have added tests to requesthandler_test.go file.

alexellis · 2018-03-20T07:36:11Z

I'll check out the tests

alexellis · 2018-03-20T08:53:08Z

This work looks good to me including the tests. I'd just be concerned about the ordering of the tests - if they ran out of sync they may not work - therefore I'd consider making sure the lock file is created/present in the positive test. You could raise a new PR for this.

derek bot added the no-dco label Mar 15, 2018

alexellis reviewed Mar 16, 2018

View reviewed changes

viveksyngh force-pushed the watchdog_health branch from 0a95c23 to 3a38bf9 Compare March 16, 2018 08:56

derek bot removed the no-dco label Mar 16, 2018

viveksyngh force-pushed the watchdog_health branch from 3a38bf9 to b811d20 Compare March 16, 2018 09:34

viveksyngh force-pushed the watchdog_health branch from b811d20 to 9d232cf Compare March 16, 2018 09:48

alexellis requested a review from johnmccabe March 16, 2018 11:00

viveksyngh changed the title ~~[WIP] Watchdog endpoints for health~~ Watchdog endpoints for health Mar 17, 2018

johnmccabe suggested changes Mar 17, 2018

View reviewed changes

johnmccabe reviewed Mar 17, 2018

View reviewed changes

Add /_/health endpoint to watchdog

29dd04a

Introduce new endpoint `/_/health` to watchdog for health status of functions which check for `/tmp/.lock` file Fixes first part of openfaas#547 issue. Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

viveksyngh force-pushed the watchdog_health branch from 9d232cf to 29dd04a Compare March 18, 2018 14:38

alexellis merged commit c484eee into openfaas:master Mar 20, 2018

alexellis mentioned this pull request May 20, 2018

[WIP] Add metrics endpoint to watchdog #688

Closed

11 tasks

alexellis mentioned this pull request Apr 3, 2019

Add RED metrics to classic watchdog #1151

Merged

11 tasks

Watchdog endpoints for health #583

Watchdog endpoints for health #583

Conversation

viveksyngh commented Mar 15, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

derek bot commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Mar 16, 2018

alexellis commented Mar 16, 2018

viveksyngh commented Mar 16, 2018

alexellis commented Mar 16, 2018

johnmccabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnmccabe Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

johnmccabe Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

viveksyngh Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Mar 17, 2018

viveksyngh commented Mar 17, 2018 • edited Loading

alexellis commented Mar 17, 2018

viveksyngh commented Mar 18, 2018

alexellis commented Mar 20, 2018

alexellis commented Mar 20, 2018 • edited Loading

viveksyngh commented Mar 15, 2018 •

edited

Loading

johnmccabe Mar 17, 2018 •

edited

Loading

johnmccabe Mar 17, 2018 •

edited

Loading

viveksyngh Mar 17, 2018 •

edited

Loading

viveksyngh commented Mar 17, 2018 •

edited

Loading

alexellis commented Mar 20, 2018 •

edited

Loading