NOISSUE - Add health check for MQTT broker #1305

blokovi · 2020-12-15T12:56:31Z

Signed-off-by: Ivan Milosevic [email protected]

What does this do?

If health-check path is set, mqtt adapter will wait that path is up and returning 200 status code. For VerneMQ health-check path is /health so it's default in docker composition where VerneMQ is used.

Which issue(s) does this PR fix/relate to?

Related to issue in mProxy repo

codecov-io · 2020-12-15T13:02:52Z

Codecov Report

Merging #1305 (844b760) into master (e326494) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
- Coverage   59.81%   59.79%   -0.03%     
==========================================
  Files         113      113              
  Lines        8797     8797              
==========================================
- Hits         5262     5260       -2     
- Misses       3078     3080       +2     
  Partials      457      457

Impacted Files	Coverage Δ
things/service.go	`53.40% <0.00%> (-1.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e326494...844b760. Read the comment docs.

cmd/mqtt/main.go

.env

cmd/mqtt/main.go

mqtt/README.md

drasko

LGTM

drasko

LGTM, very nice

mteodor · 2020-12-16T19:34:57Z

cmd/mqtt/main.go

@@ -122,6 +126,30 @@ func main() {
 		log.Fatalf(err.Error())
 	}

+	if cfg.mqttTargetHealthCheck != "" {
+		for {


should this be in the go routine?

Makes sense, good candidate for goroutine.

But wait - this is done only on init, and mProxy is blocked here until broker is up. However, I agree that we can continue to check health constantly in the background and restart mProxy if VerneMQ is down.

I took a quick look this approach seems interesting.

The mentioned go-sundheit library allows you to schedule an async check in a go routine.

Restarting mProxy when VerneMQ is down will be done by kubernetes. There is PR in devops repo that defines liveness probe for mProxy container with checking health checker on VerneMQ. If it's not healthy, both containers (VerneMQ and mProxy) will be restarted by kubernetes.
It would be redundant and probably conflicting to have same same job done here in code and by orchestration tool.

drasko · 2020-12-16T19:38:36Z

cmd/mqtt/main.go

@@ -122,6 +126,30 @@ func main() {
 		log.Fatalf(err.Error())
 	}

+	if cfg.mqttTargetHealthCheck != "" {
+		for {


Makes sense, good candidate for goroutine.

drasko · 2020-12-23T00:05:24Z

docker/vernemq/Dockerfile

@@ -47,7 +47,7 @@ EXPOSE 1883 8883 8080 44053 4369 8888 \

 VOLUME ["/vernemq/log", "/vernemq/data", "/vernemq/etc"]

-HEALTHCHECK CMD vernemq ping | grep -q pong
+HEALTHCHECK CMD wget --no-verbose --tries=1 --spider http://localhost:8888/health || exit 1


Can we use curl here instead of wget?

Alternative with curl could be:
HEALTHCHECK CMD [[ "$(curl -o /dev/null -s -w "%{http_code}\n" http://localhost:8888/health)" == "200" ]]

The command’s exit status indicates the health status of the container. The possible values are:
0: success - the container is healthy and ready for use
1: unhealthy - the container is not working correctly

That's why this command compare if return http code with curl is "200"

I prefer curl command. It looks more complicated on a first look, but it explains better what it does.

But I would prefer even more following @jpetazzo's instructions with curl -f.

dborovcanin · 2020-12-24T12:12:31Z

cmd/mqtt/main.go

+	defMQTTTargetHost        = "0.0.0.0"
+	defMQTTTargetPort        = "1883"
+	defMQTTForwarderTimeout  = "30s" // 30 seconds
+	healthCheckSleep         = 1 * time.Second


It's OK not to have this configurable, but I'd extract it to a separate constant (const mqttHealthCheckSleep = time.Second).

dborovcanin · 2020-12-24T12:17:34Z

cmd/mqtt/main.go

+				}
+				logger.Info(fmt.Sprintf("Broker not ready, status code: %d, body: %s", res.StatusCode, body))
+			}
+			time.Sleep(healthCheckSleep)


I would add a backoff to prevent this loop from running indefinitely:

if i == maxHealthCheckRetries { logger.Error("MQTT healthcheck limit exceeded, exiting.") os.Exit(1) } time.Sleep(i * mqttHealthcheckSleep)

This can come in handy in the case of running service natively.

I do not like this whole construct, it looks clunky.

Can you please use https://github.com/cenkalti/backoff and solve this with Retry() function.

Then if Retry() succeeds you can try reading the body, and if this fails just restart (that means broker is ready, but there were some more serious error. And this usually should not happen).

Simply extracting to a separate function and calling it here will help.

I would like that we try with https://github.com/cenkalti/backoff in order to prepare terrain for the eventual changing of other services.

switched to cenkalti/backoff

drasko · 2020-12-24T19:35:36Z

cmd/mqtt/main.go

+		for i := 1; i <= maxHealthCheckRetries; i++ {
+			res, err := http.Get(cfg.mqttTargetHealthCheck)
+			if err != nil {
+				logger.Info(fmt.Sprintf("Broker not ready: %s ", err.Error()))


Shouldn't we do continue here?

I think not, without continue rest of the loop body is skipped anyway, except sleep on the end.
With continue next iteration would be right away.

Yes, this is exactly what I meant - that the rest of the operations in this loop body should be skipped. Because if you can not do HTTP GET, do you really need to check the result? And do you really need to read HTTP body (remember that GET failed)? And then do you need to close the Body (remember, GET failed, and you did not read anything)?

drasko · 2020-12-24T19:40:30Z

cmd/mqtt/main.go

+				}
+				logger.Info(fmt.Sprintf("Broker not ready, status code: %d, body: %s", res.StatusCode, body))
+			}
+			time.Sleep(healthCheckSleep)


I do not like this whole construct, it looks clunky.

Can you please use https://github.com/cenkalti/backoff and solve this with Retry() function.

Then if Retry() succeeds you can try reading the body, and if this fails just restart (that means broker is ready, but there were some more serious error. And this usually should not happen).

drasko · 2020-12-25T09:45:02Z

cmd/mqtt/main.go

+		for i := 1; i <= maxHealthCheckRetries; i++ {
+			res, err := http.Get(cfg.mqttTargetHealthCheck)
+			if err != nil {
+				logger.Info(fmt.Sprintf("Broker not ready: %s ", err.Error()))


Yes, this is exactly what I meant - that the rest of the operations in this loop body should be skipped. Because if you can not do HTTP GET, do you really need to check the result? And do you really need to read HTTP body (remember that GET failed)? And then do you need to close the Body (remember, GET failed, and you did not read anything)?

drasko · 2020-12-25T09:45:06Z

cmd/mqtt/main.go

+				}
+				logger.Info(fmt.Sprintf("Broker not ready, status code: %d, body: %s", res.StatusCode, body))
+			}
+			if i == maxHealthCheckRetries {


Should be probably moved to the top of the loop.

Yes, this is exactly what I meant - that the rest of the operations in this loop body should be skipped. Because if you can not do HTTP GET, do you really need to check the result? And do you really need to read HTTP body (remember that GET failed)? And then do you need to close the Body (remember, GET failed, and you did not read anything)?

Rest of code is already skipped - except sleep.
All operation you mention here is in if res != nil where res is result of http.Get. If that fails (and that is normal because adapter will be up before broker) then sleep and retry again.

Should be probably moved to the top of the loop.

Moving it to the top would do unnecessary sleep in situation when success is in firts iteration.

My question was - why are you even checking res if you already had an err in http.Get(cfg.mqttTargetHealthCheck)? It's like hoping that res will somehow be valid, although there was an error in the GET function itself.

drasko · 2020-12-25T09:45:48Z

cmd/mqtt/main.go

+				}
+				logger.Info(fmt.Sprintf("Broker not ready, status code: %d, body: %s", res.StatusCode, body))
+			}
+			time.Sleep(healthCheckSleep)


I would like that we try with https://github.com/cenkalti/backoff in order to prepare terrain for the eventual changing of other services.

dborovcanin · 2020-12-25T11:17:51Z

cmd/mqtt/main.go

@@ -122,6 +130,35 @@ func main() {
 		log.Fatalf(err.Error())
 	}

+	if cfg.mqttTargetHealthCheck != "" {
+		for i := 1; i <= maxHealthCheckRetries; i++ {


Since you are already checking the condition below, you can ignore it here for i:=1; ;i++ {...}. Also, please consider increasing the sleep period in each iteration by multiplying it by i. In that case, maxHealthCheckRetries should probably be less than 10 (5?) to prevent long waits. This block can be extracted to a separate function so that we don't pollute the main.

drasko · 2020-12-25T11:21:23Z

cmd/mqtt/main.go

@@ -122,6 +130,35 @@ func main() {
 		log.Fatalf(err.Error())
 	}

+	if cfg.mqttTargetHealthCheck != "" {
+		for i := 1; i <= maxHealthCheckRetries; i++ {
+			res, err := http.Get(cfg.mqttTargetHealthCheck)


New res object is created on every iteration, so use defer to close it's body. Defers will be intelligent enough to know to which object they apply at the end of the function.

Signed-off-by: Ivan Milosevic <[email protected]>

use constants Signed-off-by: Ivan Milosevic <[email protected]>

Signed-off-by: Ivan Milosevic <[email protected]>

new version of verne image Signed-off-by: Ivan Milosevic <[email protected]>

Signed-off-by: Ivan Milosevic <[email protected]>

cmd/mqtt/main.go

dborovcanin · 2021-01-04T19:51:00Z

cmd/mqtt/main.go

+
+		// An operation that may fail.
+		operation := func() error {
+			res, err := http.Get(cfg.mqttTargetHealthCheck)


Consider extracting this function outside of main and just use it in backoff.RetryNotify call.

Signed-off-by: Ivan Milosevic <[email protected]>

blokovi · 2021-01-05T09:20:05Z

bunch of files are removed from vendor directory after
go mod vendor

Signed-off-by: Ivan Milosevic <[email protected]>

drasko

LGTM

drasko · 2021-01-05T18:43:45Z

docker/vernemq/Dockerfile

@@ -47,7 +47,7 @@ EXPOSE 1883 8883 8080 44053 4369 8888 \

 VOLUME ["/vernemq/log", "/vernemq/data", "/vernemq/etc"]

-HEALTHCHECK CMD vernemq ping | grep -q pong
+HEALTHCHECK CMD curl -s -f http://localhost:8888/health || false


Just a comment here: if we changed VerneMQ Dockerfile, we'll need to build and upload new image. I am not sure of the current CI covers the VerneMQ - if not, please add it.

blokovi requested a review from a team as a code owner December 15, 2020 12:56

manuio suggested changes Dec 15, 2020

View reviewed changes

cmd/mqtt/main.go Outdated Show resolved Hide resolved

drasko requested changes Dec 15, 2020

View reviewed changes

cmd/mqtt/main.go Outdated Show resolved Hide resolved

.env Outdated Show resolved Hide resolved

cmd/mqtt/main.go Outdated Show resolved Hide resolved

dborovcanin reviewed Dec 15, 2020

View reviewed changes

cmd/mqtt/main.go Outdated Show resolved Hide resolved

blokovi force-pushed the mqtthealthcheck branch from efa00c9 to 99e5864 Compare December 15, 2020 17:01

drasko requested changes Dec 15, 2020

View reviewed changes

mqtt/README.md Show resolved Hide resolved

drasko previously approved these changes Dec 16, 2020

View reviewed changes

blokovi marked this pull request as draft December 16, 2020 15:44

blokovi dismissed drasko’s stale review via 44b8271 December 16, 2020 19:27

blokovi force-pushed the mqtthealthcheck branch 2 times, most recently from 44b8271 to adf5550 Compare December 16, 2020 19:29

blokovi marked this pull request as ready for review December 16, 2020 19:29

drasko approved these changes Dec 16, 2020

View reviewed changes

mteodor reviewed Dec 16, 2020

View reviewed changes

drasko requested changes Dec 16, 2020

View reviewed changes

dborovcanin previously approved these changes Dec 17, 2020

View reviewed changes

mteodor previously approved these changes Dec 18, 2020

View reviewed changes

blokovi dismissed stale reviews from mteodor and dborovcanin via 19bae66 December 18, 2020 11:21

blokovi force-pushed the mqtthealthcheck branch from adf5550 to 19bae66 Compare December 18, 2020 11:21

mteodor previously approved these changes Dec 18, 2020

View reviewed changes

blokovi dismissed mteodor’s stale review via 2e15594 December 22, 2020 11:52

drasko requested changes Dec 23, 2020

View reviewed changes

blokovi force-pushed the mqtthealthcheck branch from 2e15594 to 1932da9 Compare December 24, 2020 11:33

blokovi requested a review from manuio December 24, 2020 11:35

dborovcanin requested changes Dec 24, 2020

View reviewed changes

drasko requested changes Dec 24, 2020

View reviewed changes

drasko requested changes Dec 25, 2020

View reviewed changes

dborovcanin reviewed Dec 25, 2020

View reviewed changes

drasko requested changes Dec 25, 2020

View reviewed changes

blokovi added 12 commits January 4, 2021 15:53

Add health check

2c74fc8

Signed-off-by: Ivan Milosevic <[email protected]>

update readme

9535a21

use constants Signed-off-by: Ivan Milosevic <[email protected]>

close body after each HEAD request

857c5c5

Signed-off-by: Ivan Milosevic <[email protected]>

log and close health check response body

44ccfac

Signed-off-by: Ivan Milosevic <[email protected]>

use healthcheck endpoint instead ping for docker healthcheck

517c6b5

Signed-off-by: Ivan Milosevic <[email protected]>

remove for loop, only one check and exit if failed healthcheck

2959a93

Signed-off-by: Ivan Milosevic <[email protected]>

changed cmd for healthcheck

3d25cc9

new version of verne image Signed-off-by: Ivan Milosevic <[email protected]>

use false instead exit 1

4bb01e0

Signed-off-by: Ivan Milosevic <[email protected]>

return for loop

c98978a

Signed-off-by: Ivan Milosevic <[email protected]>

review

34e8fee

Signed-off-by: Ivan Milosevic <[email protected]>

add continue after unsuccesful http.get

a0c4e0f

Signed-off-by: Ivan Milosevic <[email protected]>

Use backoff package

43d11f8

Signed-off-by: Ivan Milosevic <[email protected]>

blokovi force-pushed the mqtthealthcheck branch from c003112 to 43d11f8 Compare January 4, 2021 14:54

dborovcanin reviewed Jan 4, 2021

View reviewed changes

blokovi added 2 commits January 5, 2021 09:58

extracting healtcheck function outside main()

6eefde8

Signed-off-by: Ivan Milosevic <[email protected]>

go mod vendor

78bc875

Signed-off-by: Ivan Milosevic <[email protected]>

go mod tidy

08b459a

Signed-off-by: Ivan Milosevic <[email protected]>

dborovcanin previously approved these changes Jan 5, 2021

View reviewed changes

go mod vendor

844b760

Signed-off-by: Ivan Milosevic <[email protected]>

blokovi dismissed dborovcanin’s stale review via 844b760 January 5, 2021 09:34

dborovcanin approved these changes Jan 5, 2021

View reviewed changes

drasko reviewed Jan 5, 2021

View reviewed changes

drasko approved these changes Jan 5, 2021

View reviewed changes

drasko merged commit 3eca692 into absmach:master Jan 5, 2021

drasko mentioned this pull request Jan 8, 2021

Check if MQTT broker is available before establishing connection absmach/mgate#27

Closed

NOISSUE - Add health check for MQTT broker #1305

NOISSUE - Add health check for MQTT broker #1305

Conversation

blokovi commented Dec 15, 2020

What does this do?

Which issue(s) does this PR fix/relate to?

codecov-io commented Dec 15, 2020 • edited Loading

Codecov Report

drasko left a comment

Choose a reason for hiding this comment

drasko 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

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

blokovi Dec 23, 2020 • 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

blokovi Dec 25, 2020 • 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

blokovi Dec 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko Dec 25, 2020 • 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

blokovi commented Jan 5, 2021

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 15, 2020 •

edited

Loading

blokovi Dec 23, 2020 •

edited

Loading

blokovi Dec 25, 2020 •

edited

Loading

blokovi Dec 25, 2020 •

edited

Loading

drasko Dec 25, 2020 •

edited

Loading