Adds Health gRPC Server and Refactors Main() #148
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/lgtm |
k8s-ci-robot
added
the
lgtm
pkg/ext-proc/backend/datastore.go
Outdated
| } | ||
| 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.
There was a problem hiding this comment.
The health probe now uses a client to check the API server for the configured InferencePool and that at least one InferenceModel exists in the same namespace. Should this probe also check that at least one InferenceModel references the configured InferencePool?
There was a problem hiding this comment.
I don't think that the health check needs to block on at least one InferenceModel. On the other hand, since extension is currently 1:1 with InferencePool, I think it makes sense to ensure that the extension successfully initialized the assigned InferencePool.
There was a problem hiding this comment.
I think it makes sense to ensure that the extension successfully initialized the assigned InferencePool.
^ is the approach I took in the initial PR, e.g. check if InferencePool is nil in the data store. It also checked if at least 1 InferenceModel that referenced the configured InferencePool was stored but that can be removed.
There was a problem hiding this comment.
Oh, sorry, I missed that. I agree with the original approach! I would still remove the check on InferenceModel.
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
lgtm
danehans
changed the title
k8s-ci-robot
added
the
needs-rebase
- Introduced a health gRPC server to handle liveness and readiness probes. - Refactored main() to manage server goroutines. - Added graceful shutdown for servers and controller manager. - Improved logging consistency and ensured. - Validates CLI flags. Signed-off-by: Daneyon Hansen <[email protected]>
Signed-off-by: Daneyon Hansen <[email protected]>
k8s-ci-robot
removed
the
needs-rebase
| @@ -124,7 +106,7 @@ func main() { | |||
| }, | |||
| Record: mgr.GetEventRecorderFor("InferencePool"), | |||
| }).SetupWithManager(mgr); err != nil { | |||
| klog.Error(err, "Error setting up InferencePoolReconciler") | |||
| klog.Fatalf("Failed setting up InferencePoolReconciler: %v", err) | |||
There was a problem hiding this comment.
Note the switch to Fatalf in several places where an error is critical to the extension's startup.
|
/lgtm Thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, danehans The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
* Add health gRPC server and refactors main() - Introduced a health gRPC server to handle liveness and readiness probes. - Refactored main() to manage server goroutines. - Added graceful shutdown for servers and controller manager. - Improved logging consistency and ensured. - Validates CLI flags. Signed-off-by: Daneyon Hansen <[email protected]> * Refactors health server to use data store Signed-off-by: Daneyon Hansen <[email protected]> --------- Signed-off-by: Daneyon Hansen <[email protected]>
Adds a health gRPC Server and refactors
main()for better lifecycle management:Fixes #96
Fixes #175