Adds Health gRPC Server and Refactor Main() #148
Conversation
…ement - Introduced a health gRPC server to handle liveness and readiness probes. - Refactored main() to manage server goroutines using sync.WaitGroup. - Added graceful shutdown for servers and controller manager. - Improved logging consistency and ensured datastore readiness checks. Signed-off-by: Daneyon Hansen <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danehans The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| } | ||
| ready = true | ||
| return false | ||
| }) |
There was a problem hiding this comment.
At startup, I think we want to ensure that the extension did a sync with the api server and fetched the models, but not declare itself ready only if at least one model is defined.
|
|
||
| datastore := backend.NewK8sDataStore() | ||
| // Error channel to handle server errors | ||
| errChan := make(chan error, 5) |
There was a problem hiding this comment.
Can you add a comment why we chose 5 and in what case we may need to adjust it? also, wouldn't one be enough since receiving any error is sufficient to shutdown?
| if err := extSvr.Serve(lis); err != nil && err != grpc.ErrServerStopped { | ||
| errChan <- fmt.Errorf("ext-proc server failed: %w", err) | ||
| } | ||
| klog.Infof("Ext-proc server serving on port: %d", *grpcPort) |
There was a problem hiding this comment.
ditto: This log line could be moved up right before the call to the Serve func, and here we want to have a message indicating that the server is exiting.
|
|
||
| if err := extSvr.Serve(lis); err != nil && err != grpc.ErrServerStopped { |
There was a problem hiding this comment.
Please add a comment clarifying that this is blocking and will return when shutting down.
| if err := healthSvr.Serve(healthLis); err != nil && err != grpc.ErrServerStopped { | ||
| errChan <- fmt.Errorf("health server failed: %w", err) | ||
| } | ||
| klog.Infof("Health server serving on port: %d", *grpcHealthPort) |
There was a problem hiding this comment.
This log line could be moved up right before the call to the Serve func, and here we want to have a message indicating that the server is exiting.
| } | ||
|
|
||
| errChan := make(chan error) | ||
| // Start the controller manager | ||
| wg.Add(1) |
There was a problem hiding this comment.
can we encapsulate each goroutine in a function prefixed with Start?
- StartSignalHandler
- StartHealthServer
- StartExternalProcessorServer
| } | ||
| klog.Info("controller manager started") |
There was a problem hiding this comment.
At this point the serve is actually exiting not starting, can we please correct the log line?
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Adds a health gRPC Server and refactors
main()for better lifecycle management:Fixes #96