feat(health probe): health and readiness endpoints #53

wojciech12 · 2018-11-07T09:08:08Z

The health probe helps us to inform kubernetes that the service should be killed. The readiness probe will let us support components with very slow start.

GitCop · 2018-11-07T09:08:09Z

There were the following issues with your Pull Request

Commit: 5d58d15
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

GitCop · 2018-11-07T09:35:17Z

There were the following issues with your Pull Request

Commit: 8930945
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

GitCop · 2018-11-07T09:38:18Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

pbrzostowski

How service is set as not healthy state?

pbrzostowski · 2018-11-07T09:40:05Z

service/handlers.go

+	defer s.Status.MuHealthy.Unlock()
+	if s.Status.IsHealthy {
+		data.MarshalWithCode(w, r, "OK", 200)
+	} else {


we should avoid else in go if possible. It better to read:

if expression { // some stuff return // return } // other stuff

GitCop · 2018-11-07T09:46:33Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

wojciech12 · 2018-11-07T09:47:47Z

@pbrzostowski we let the service implementation to set the flag. I will provide two functions for this purpose.

GitCop · 2018-11-07T09:50:39Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

GitCop · 2018-11-07T09:52:20Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

GitCop · 2018-11-07T10:01:17Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

GitCop · 2018-11-07T10:03:25Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters
Commit: 18959e7
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

GitCop · 2018-11-07T10:15:25Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters
Commit: 18959e7
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

GitCop · 2018-11-07T10:32:36Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters
Commit: 18959e7
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

wojciech12 · 2018-11-07T10:34:21Z

We let the implementation to set Health and Readiness flags.

levrik · 2018-11-07T10:35:37Z

service/service.go

@@ -123,7 +131,11 @@ func New(name string) *Service {
 		Router:     mux.NewRouter(),
 		ServeMux:   http.NewServeMux(),
 	}
+
+	s.Status.IsHealthy = true
+	s.Status.IsReady = false


Service is never ready? 🤔

It depends on your k8s definition whether you use the readiness probe or not.

I will change it to true, because in many cases running = healthy = ready. Otherwise it might confuse us.

Ah, so the service should be in charge of setting this to true. Makes sense.

Yes. However. Your comment helped me to realize that setting it to true will work for most of our lightweight micro-services.

GitCop · 2018-11-07T10:39:21Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters
Commit: 18959e7
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

pbrzostowski · 2018-11-07T11:10:40Z

service/service.go

@@ -123,7 +131,11 @@ func New(name string) *Service {
 		Router:     mux.NewRouter(),
 		ServeMux:   http.NewServeMux(),
 	}
+
+	s.Status.IsHealthy = true


I am not sure if this is the proper place for status set? I mean, we have a Start() func that spin off service. Is the creation on the service proper place for this setup here? Actually service is not ready/health because it's not even started/serving, isn't it?
And btw... I really think that we should get a chan out of service.Start() to be able to notify/get notified about stuff (like if service actually ready and listen etc, and maybe statuses management should be through it?).

The Chan on the start is a separate concern, definitely we should have it. Notice, that I have moved the creation of the stop chan, so you can subscribe to it.

This setting is the default value, you, as a service-based-on-missy developer, should manage it yourself.

GitCop · 2018-11-08T20:39:19Z

There were the following issues with your Pull Request

Commit: 9f174e1
Your subject line is longer than 72 characters
Commit: 18959e7
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

bbonislawski · 2018-12-07T10:02:40Z

service/handlers.go

+	s.StateProbes.MuHealthy.Lock()
+	defer s.StateProbes.MuHealthy.Unlock()
+
+	if s.StateProbes.IsHealthy {


I'd prefer to have fail-fast strategy. First handle errors and successful resolution should be at the end of function

bbonislawski · 2018-12-07T10:03:00Z

service/handlers.go

+	s.StateProbes.MuReady.Lock()
+	defer s.StateProbes.MuReady.Unlock()
+
+	if s.StateProbes.IsReady {


again fail-fast please

mwarzynski

LGTM

GitCop · 2018-12-07T10:25:18Z

There were the following issues with your Pull Request

Commit: 02b1e2a
Invalid type. Valid types are feat, fix, docs, style, refactor, perf, test, chore, revert, merge

Guidelines are available at https://github.com/microdevs/missy

This message was auto-generated by https://gitcop.com

wojciech12 requested review from slot, pbrzostowski, bbonislawski and a team November 7, 2018 09:08

wojciech12 force-pushed the add_support_for_readiness_probe branch from 5d58d15 to 8930945 Compare November 7, 2018 09:35

wojciech12 force-pushed the add_support_for_readiness_probe branch from 8930945 to 9f174e1 Compare November 7, 2018 09:38

pbrzostowski reviewed Nov 7, 2018

View reviewed changes

wojciech12 force-pushed the add_support_for_readiness_probe branch from 75067ef to d9a736e Compare November 7, 2018 09:52

wojciech12 changed the title ~~feat(health probe): Add flag for service health and, another, for the readiness~~ feat(health probe): health and readiness endpoints Nov 7, 2018

levrik reviewed Nov 7, 2018

View reviewed changes

pbrzostowski reviewed Nov 7, 2018

View reviewed changes

wojciech12 force-pushed the add_support_for_readiness_probe branch from c703e71 to 75ae5ac Compare December 7, 2018 09:58

feat(health and readiness probe): Add the first implementation

60d5619

wojciech12 force-pushed the add_support_for_readiness_probe branch from 75ae5ac to 60d5619 Compare December 7, 2018 09:59

bbonislawski suggested changes Dec 7, 2018

View reviewed changes

mwarzynski previously approved these changes Dec 7, 2018

View reviewed changes

wojciech12 dismissed mwarzynski’s stale review via 02b1e2a December 7, 2018 10:25

feat(health and readiness probe): Refactor readiness/health flags eval

57730a7

wojciech12 force-pushed the add_support_for_readiness_probe branch from 02b1e2a to 57730a7 Compare December 7, 2018 10:27

bbonislawski approved these changes Dec 7, 2018

View reviewed changes

wojciech12 merged commit c53da85 into master Dec 7, 2018

wojciech12 deleted the add_support_for_readiness_probe branch December 7, 2018 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(health probe): health and readiness endpoints #53

feat(health probe): health and readiness endpoints #53

wojciech12 commented Nov 7, 2018 •

edited

Loading

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

pbrzostowski left a comment

pbrzostowski Nov 7, 2018

GitCop commented Nov 7, 2018

wojciech12 commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

wojciech12 commented Nov 7, 2018

levrik Nov 7, 2018

wojciech12 Nov 7, 2018 •

edited

Loading

levrik Nov 7, 2018

wojciech12 Nov 7, 2018

GitCop commented Nov 7, 2018

pbrzostowski Nov 7, 2018

wojciech12 Nov 7, 2018

GitCop commented Nov 8, 2018

bbonislawski Dec 7, 2018

wojciech12 Dec 7, 2018

bbonislawski Dec 7, 2018

mwarzynski left a comment

GitCop commented Dec 7, 2018

feat(health probe): health and readiness endpoints #53

feat(health probe): health and readiness endpoints #53

Conversation

wojciech12 commented Nov 7, 2018 • edited Loading

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

pbrzostowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitCop commented Nov 7, 2018

wojciech12 commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

GitCop commented Nov 7, 2018

wojciech12 commented Nov 7, 2018

Choose a reason for hiding this comment

wojciech12 Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitCop commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitCop commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwarzynski left a comment

Choose a reason for hiding this comment

GitCop commented Dec 7, 2018

wojciech12 commented Nov 7, 2018 •

edited

Loading

wojciech12 Nov 7, 2018 •

edited

Loading