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

[BN-1018] Add gRPC Health Check Proto #89

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

admiraladmirable
Copy link
Collaborator

@admiraladmirable admiraladmirable commented Sep 12, 2023

Purpose

Add health check proto.

Approach

The proto comes from: https://github.com/grpc/grpc/blob/master/doc/health-checking.md

Testing

Built locally, implemented in Bifrrost, deployed to GKE and ensured that the grpc liveliness/readiness probes succeeded.

Tickets

@admiraladmirable admiraladmirable force-pushed the BN-1018-create-g-rpc-health-check-in-bifrost branch from 06c24cb to 5ba84e3 Compare September 12, 2023 13:13
@admiraladmirable admiraladmirable changed the title Bn 1018 create g rpc health check in bifrost [Bn-1018] Add gRPC Health Check Proto Sep 12, 2023
@admiraladmirable admiraladmirable changed the title [Bn-1018] Add gRPC Health Check Proto [BN-1018] Add gRPC Health Check Proto Sep 12, 2023
@admiraladmirable admiraladmirable marked this pull request as ready for review September 13, 2023 20:04
@admiraladmirable admiraladmirable requested a review from a team September 13, 2023 20:04
Copy link
Contributor

@nandotorterolo nandotorterolo left a comment

Choose a reason for hiding this comment

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

Great PR, some suggestions.

  • files structure
  • enumeration comments

Package names should be in lowercase. Package names should have unique names based on the project name, and possibly based on the path of the file containing the protocol buffer type definitions.

https://protobuf.dev/programming-guides/style/#packages

In other folders proto file are organized by service/models, maybe you can do something like this.

  • proto
    • health
      • service
        • health_rpc.proto
      • models
        • models.proto

models.proto

enum ServingStatus {
    // UNKNOWN means that there was a timeout hitting the head of the chain..
    UNKNOWN = 0;
    // SERVING means that all services are running as expected
    SERVING = 1;
    // NOT_SERVING means...
    NOT_SERVING = 2;
    // SERVICE_UNKNOWN means,  // Used only by the Watch method.
    SERVICE_UNKNOWN = 3;  
  }

health_rpc.proto

syntax = "proto3";

package co.topl.health.services;

service Health {
  rpc Check(HealthCheckRequest) returns (HealthCheckResponse);

  rpc Watch(HealthCheckRequest) returns (stream HealthCheckResponse);
}

message HealthCheckRequest {
 // service is a string that we must/should send for this reason.
  string service = 1;
}

message HealthCheckResponse {
  ServingStatus status = 1;
}

@admiraladmirable admiraladmirable requested a review from a team September 14, 2023 15:39
@admiraladmirable
Copy link
Collaborator Author

Great PR, some suggestions.

* files structure

* enumeration comments

Package names should be in lowercase. Package names should have unique names based on the project name, and possibly based on the path of the file containing the protocol buffer type definitions.

https://protobuf.dev/programming-guides/style/#packages

In other folders proto file are organized by service/models, maybe you can do something like this.

* proto
  
  * health
    
    * service
      
      * health_rpc.proto
    * models
      
      * models.proto

models.proto

enum ServingStatus {
    // UNKNOWN means that there was a timeout hitting the head of the chain..
    UNKNOWN = 0;
    // SERVING means that all services are running as expected
    SERVING = 1;
    // NOT_SERVING means...
    NOT_SERVING = 2;
    // SERVICE_UNKNOWN means,  // Used only by the Watch method.
    SERVICE_UNKNOWN = 3;  
  }

health_rpc.proto

syntax = "proto3";

package co.topl.health.services;

service Health {
  rpc Check(HealthCheckRequest) returns (HealthCheckResponse);

  rpc Watch(HealthCheckRequest) returns (stream HealthCheckResponse);
}

message HealthCheckRequest {
 // service is a string that we must/should send for this reason.
  string service = 1;
}

message HealthCheckResponse {
  ServingStatus status = 1;
}

@nandotorterolo Thanks for the review!

I'll update the folder format of the modes/services.

One note though, I don't believe that the built in gRPC health check in Kubernetes supports customizing the package/service path. I believe it must be grpc.health.v1.Health. Hopefully this is okay.

@admiraladmirable admiraladmirable force-pushed the BN-1018-create-g-rpc-health-check-in-bifrost branch from 5ba84e3 to 1fc3f47 Compare September 14, 2023 20:40
@nandotorterolo
Copy link
Contributor

ok, I didn't know about Kubernetes customizations.
So, Kubernetes needs

  • a package with this namespace: grpc.health.v1.Health
  • the file should be in a folder with this path: 'google/..'

@nandotorterolo Thanks for the review!

I'll update the folder format of the modes/services.

One note though, I don't believe that the built in gRPC health check in Kubernetes supports customizing the package/service path. I believe it must be grpc.health.v1.Health. Hopefully this is okay.

@admiraladmirable
Copy link
Collaborator Author

ok, I didn't know about Kubernetes customizations. So, Kubernetes needs

* a package with this namespace:  `grpc.health.v1.Health`

* the file should be in a folder with this path: 'google/..'

@nandotorterolo Thanks for the review!
I'll update the folder format of the modes/services.
One note though, I don't believe that the built in gRPC health check in Kubernetes supports customizing the package/service path. I believe it must be grpc.health.v1.Health. Hopefully this is okay.

Yes, however the google/ folder path isn't necessarily required, but I just simplified it instead of a more complicated file structure. If we want to, I could structure the folder like:

grpc
| -- health
    | -- v1

Or whichever format makes more sense.

@admiraladmirable admiraladmirable merged commit 7b03091 into main Sep 15, 2023
7 checks passed
@admiraladmirable admiraladmirable deleted the BN-1018-create-g-rpc-health-check-in-bifrost branch September 15, 2023 20:52
admiraladmirable added a commit to Topl/Bifrost that referenced this pull request Sep 18, 2023
## Purpose
Implement gRPC health check for Node.

## Approach
Build and implement the proto based on: https://github.com/grpc/grpc/blob/master/doc/health-checking.md

The implementation uses a map to store the service statuses for Bifrost, Genus, and the overall status noted by an empty string `""`.

This is a first pass as there is no logic for determining an unhealthy status for other services, but currently the Kubernetes gRPC liveliness and readiness probes only use the overall health for the health check.

I believe this should cover most cases initially as long as Bifrost and Genus will throw errors (causing the Kubernetes pod to automatically restart itself), or if the gRPC server hosting goes down, the health check will get an unknown status and restart anyways.

## Testing
* Deployed to GKE, verified that the gRPC health check normally reported healthy.
* Verified the health check restarted the pods when an unhealthy status was returned.
* Verified the health check restarted when nothing (an UKNOWN) status was returned.

~~## TODO~~
~~Merge and use this PR's protobuf specs: Topl/protobuf-specs#89 Done!

## Tickets
*
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.

None yet

3 participants