Introduce health check grpc service to Signer #902

HuKeping · 2016-08-09T03:02:49Z

Fixes #894
Fixes #917

As per SPEC:
https://github.com/grpc/grpc/blob/master/doc/health-checking.md

We hook a health check service into notary-signer along with
the normal services, say "KeyManagement" and "Signer".

After booting "KeyManagement" and "Signer", we set the health status
of each to be "SERVING" which would be returned as the status of
service on the health checking time.

Signed-off-by: Hu Keping hukeping@huawei.com

docker-jenkins · 2016-08-09T03:02:51Z

Can one of the admins verify this patch?

riyazdf · 2016-08-09T16:49:09Z

jenkins, test this please.

riyazdf · 2016-08-09T21:36:31Z

cmd/notary-server/config.go

+			// The server will use an empty string as the key for server's overall health status,
+			// so that a client not interested in a specific service can query the server's status
+			// with an empty service name.
+			err := notarySigner.CheckHealth(duration, "")


Can we add a test for CheckHealth with the empty service argument?

riyazdf · 2016-08-11T18:32:44Z

jenkins, test this please

riyazdf · 2016-08-11T18:36:25Z

signer/rpc_and_client_test.go

+
+}
+
+// TestHealthCheckUnExistsService query for an un-exists service's health status


nit: can we change an un-exists to a nonexistent here, in the function name, and in the comment below?

HuKeping · 2016-08-12T03:46:00Z

updated

riyazdf · 2016-08-12T16:25:49Z

jenkins, test this please.

riyazdf · 2016-08-12T16:38:56Z

signer/rpc_and_client_test.go

+}
+
+// healthCheckHealthy succeeds if server is healthy and reachable.
+func healthCheckKMHealthy(t *testing.T, serviceName string) {


nit: function name should probably be healthCheckHealthy to match the description

riyazdf · 2016-08-12T16:41:24Z

LGTM after nits! Thank you @HuKeping for your work on this :)

cyli · 2016-08-12T17:28:17Z

signer/rpc_and_client_test.go

+// healthCheckConnectionDied fails immediately if not connected to the server.
+func healthCheckConnectionDied(t *testing.T, serviceName string) {
+	hs := health.NewServer()
+	hs.SetServingStatus(serviceName, healthpb.HealthCheckResponse_NOT_SERVING)


nit: can we change this to healthpb.HealthCheckResponse_SERVING so that the only reason it should be failing is the disconnection?

cyli · 2016-08-12T17:29:48Z

Thank you all your work on this! LGTM after the two minor nitpick changes!

HuKeping · 2016-08-13T15:54:42Z

Updated. After this PR, we could also regenerate the proto of signer, I think at least the Void structure could be dropped. I'd like to do it in another separated PR if you don't mind.

HuKeping · 2016-08-17T02:20:56Z

ping ...

riyazdf · 2016-08-17T02:26:44Z

@HuKeping thanks for checking in.

I think @cyli and I still have a couple of unresolved nit comments on healthCheckConnectionDied and healthCheckKMHealthy - are those proposed changes okay with you?

HuKeping · 2016-08-17T02:36:23Z

Oh I must have forgotten to commit that !

riyazdf · 2016-08-17T02:51:23Z

No problem, thank you for updating! LGTM pending CI

riyazdf · 2016-08-17T02:51:25Z

jenkins, test this please.

HuKeping · 2016-08-17T08:33:56Z

Sorry for having you review this again @riyazdf @cyli , I just pushed a new commit into this PR to remove all the unused code from proto/signer.proto and the relatives as #917 , plz take a look in case I missing anything.

riyazdf · 2016-08-17T23:48:24Z

proto/signer.pb.go

@@ -1,12 +1,12 @@
 // Code generated by protoc-gen-go.
-// source: proto/signer.proto
+// source: signer.proto


huh this change seems unexpected, do we know why this changed?

I think it's relative from where you run the protoc command. If you run make protos from the main repo directory you get proto/signer.proto

Nice catch, updated!

cyli · 2016-08-18T00:17:11Z

jenkins, test this please

riyazdf · 2016-08-18T00:17:34Z

@HuKeping: thanks for updating the protos, LGTM!

cyli · 2016-08-18T00:18:36Z

Thanks @HuKeping! LGTM pending CI!

HuKeping · 2016-08-18T08:38:49Z

Every time this happens,
when I walked into the night,
you are in light,
when I'm finally get into the sunshine,
you rest under the moonlight .
At that time I always wander,
what's the hell did Colombo said,
THE WORLD IS ROUND!

As per SPEC: https://github.com/grpc/grpc/blob/master/doc/health-checking.md We hook a health check service into notary-signer along with the normal services, say "KeyManagement" and "Signer". After booting "KeyManagement" and "Signer", we set the health status of each to be "SERVING" which would be returned as the status of service on the health checking time. Signed-off-by: Hu Keping <hukeping@huawei.com>

As per riyazdf's suggestion, this patch fixes: - Remove an unused function - Cleanup comments - Add a test for health check of the overall status - Change the place of function "CheckHealth" - for better code review on github. Signed-off-by: Hu Keping <hukeping@huawei.com>

1) Create three constants for different grpc services. 2) Create two helper function used to check the health status of Signer and KeyManagement. 3) Due to 2), we refator the CheckHealth function to use switch to see which service's health status was required. Signed-off-by: Hu Keping <hukeping@huawei.com>

Signed-off-by: Hu Keping <hukeping@huawei.com>

Create some helper functions and refactor the health check test. Signed-off-by: Hu Keping <hukeping@huawei.com>

Signed-off-by: Hu Keping <hukeping@huawei.com>

riyazdf · 2016-08-18T16:47:17Z

jenkins, test this please.

riyazdf · 2016-08-18T16:58:01Z

thanks for regenerating the protos and all of your work on this 👍

cyli · 2016-08-18T17:25:08Z

LGTM! Thank you for all your work on adding this!

HuKeping · 2016-08-19T17:11:26Z

ping :)

riyazdf · 2016-08-19T17:21:23Z

CI green, merging :)

GordonTheTurtle added the status/0-triage label Aug 9, 2016

riyazdf reviewed Aug 9, 2016
View reviewed changes

riyazdf reviewed Aug 11, 2016
View reviewed changes

riyazdf reviewed Aug 12, 2016
View reviewed changes

cyli reviewed Aug 12, 2016
View reviewed changes

HuKeping mentioned this pull request Aug 15, 2016

Cleanup signer.proto #917

Closed

riyazdf reviewed Aug 17, 2016
View reviewed changes

HuKeping added 7 commits August 18, 2016 17:55

Cleanup

dc5887b

As per riyazdf's suggestion, this patch fixes: - Remove an unused function - Cleanup comments - Add a test for health check of the overall status - Change the place of function "CheckHealth" - for better code review on github. Signed-off-by: Hu Keping <hukeping@huawei.com>

Cleanup: update constant for overall health check and fix typo

a5c4a87

Signed-off-by: Hu Keping <hukeping@huawei.com>

Enhance test

9170928

Create some helper functions and refactor the health check test. Signed-off-by: Hu Keping <hukeping@huawei.com>

Tiny refactoring

13dc1f8

Signed-off-by: Hu Keping <hukeping@huawei.com>

Remove unused proto code and cleanup the relative lines

3b3a465

Signed-off-by: Hu Keping <hukeping@huawei.com>

riyazdf merged commit 7d4b66f into notaryproject:master Aug 19, 2016

HuKeping deleted the health-check-signer branch August 19, 2016 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce health check grpc service to Signer #902

Introduce health check grpc service to Signer #902

HuKeping commented Aug 9, 2016 •

edited

Loading

docker-jenkins commented Aug 9, 2016

riyazdf commented Aug 9, 2016

riyazdf Aug 9, 2016

HuKeping Aug 10, 2016

riyazdf commented Aug 11, 2016

riyazdf Aug 11, 2016 •

edited

Loading

HuKeping commented Aug 12, 2016

riyazdf commented Aug 12, 2016

riyazdf Aug 12, 2016

riyazdf commented Aug 12, 2016

cyli Aug 12, 2016

cyli commented Aug 12, 2016

HuKeping commented Aug 13, 2016 •

edited

Loading

HuKeping commented Aug 17, 2016

riyazdf commented Aug 17, 2016

HuKeping commented Aug 17, 2016

riyazdf commented Aug 17, 2016

riyazdf commented Aug 17, 2016

HuKeping commented Aug 17, 2016 •

edited

Loading

riyazdf Aug 17, 2016 •

edited

Loading

cyli Aug 18, 2016

HuKeping Aug 18, 2016

cyli commented Aug 18, 2016

riyazdf commented Aug 18, 2016

cyli commented Aug 18, 2016

HuKeping commented Aug 18, 2016 •

edited

Loading

riyazdf commented Aug 18, 2016

riyazdf commented Aug 18, 2016

cyli commented Aug 18, 2016

HuKeping commented Aug 19, 2016

riyazdf commented Aug 19, 2016


		}

		// TestHealthCheckUnExistsService query for an un-exists service's health status

Introduce health check grpc service to Signer #902

Introduce health check grpc service to Signer #902

Conversation

HuKeping commented Aug 9, 2016 • edited Loading

docker-jenkins commented Aug 9, 2016

riyazdf commented Aug 9, 2016

riyazdf Aug 9, 2016

Choose a reason for hiding this comment

HuKeping Aug 10, 2016

Choose a reason for hiding this comment

riyazdf commented Aug 11, 2016

riyazdf Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

HuKeping commented Aug 12, 2016

riyazdf commented Aug 12, 2016

riyazdf Aug 12, 2016

Choose a reason for hiding this comment

riyazdf commented Aug 12, 2016

cyli Aug 12, 2016

Choose a reason for hiding this comment

cyli commented Aug 12, 2016

HuKeping commented Aug 13, 2016 • edited Loading

HuKeping commented Aug 17, 2016

riyazdf commented Aug 17, 2016

HuKeping commented Aug 17, 2016

riyazdf commented Aug 17, 2016

riyazdf commented Aug 17, 2016

HuKeping commented Aug 17, 2016 • edited Loading

riyazdf Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

cyli Aug 18, 2016

Choose a reason for hiding this comment

HuKeping Aug 18, 2016

Choose a reason for hiding this comment

cyli commented Aug 18, 2016

riyazdf commented Aug 18, 2016

cyli commented Aug 18, 2016

HuKeping commented Aug 18, 2016 • edited Loading

riyazdf commented Aug 18, 2016

riyazdf commented Aug 18, 2016

cyli commented Aug 18, 2016

HuKeping commented Aug 19, 2016

riyazdf commented Aug 19, 2016

HuKeping commented Aug 9, 2016 •

edited

Loading

riyazdf Aug 11, 2016 •

edited

Loading

HuKeping commented Aug 13, 2016 •

edited

Loading

HuKeping commented Aug 17, 2016 •

edited

Loading

riyazdf Aug 17, 2016 •

edited

Loading

HuKeping commented Aug 18, 2016 •

edited

Loading