Add ready check for cert-controller #2359
Conversation
Signed-off-by: forsaken628 <[email protected]>
google-oss-prow
bot
requested review from
andreyvelich,
anencore94 and
gaocegege
There was a problem hiding this comment.
Thank you for adding this and sorry for the late review @forsaken628!
/assign @tenzen-y
|
|
||
| // The setupControllers will register controllers to the manager | ||
| // after generated certs for the admission webhooks. | ||
| go setupControllers(mgr, certsReady, hookServer) | ||
|
|
||
| if initConfig.CertGeneratorConfig.Enable { | ||
| err := mgr.AddReadyzCheck("cert", func(_ *http.Request) error { |
There was a problem hiding this comment.
Do we need to add this check to the controller readinessProbe ?
https://github.com/kubeflow/katib/blob/master/manifests/v1beta1/components/controller/controller.yaml#L41
There was a problem hiding this comment.
If not specified, readinessProbe checks all readiness checkers. the name refers to the name of the checker, not readinessEndpointName.
There was a problem hiding this comment.
@forsaken628 @tenzen-y Just for user visibility, maybe we should explicitly call out them in the controller Deployment ? It might be hard for user to check code to understand all probs.
There was a problem hiding this comment.
Sorry for the late reply, @forsaken628 what do you think about above comment ? Do we want to make that change and merge this PR ?
There was a problem hiding this comment.
I think the vast majority of case users only care about aggregation probes. We could add a comment somewhere.
There was a problem hiding this comment.
I just want to keep it consistent between Training Operator: https://github.com/kubeflow/training-operator/blob/master/manifests/base/deployment.yaml#L53 and Katib controller.
There was a problem hiding this comment.
I don't think we need to modify deployment.yaml. The individual check is not a new feature for controller-runtime, and usually users don't need to know it exists.
Signed-off-by: forsaken628 <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
What this PR does / why we need it:
#2357 (comment)
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Checklist: