-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
✨ (go/v3-alpha) Add the liveness and readiness probe in the manager deployment #1856
Conversation
/cc @estroz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a a few nits
- https://github.com/kubernetes-sigs/kubebuilder/pull/1856/files#r528331645.
- Add on the title (go/v3-alpha)
- Add in teh first comment : Fixes: liveness/readiness probe scaffolded by default #1761
Otherwise:
/approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the suggested changes, there also needs to be an addition to the manager config:
apiVersion: controller-runtime.sigs.k8s.io/v1alpha1
kind: ControllerManagerConfig
+health:
+ healthProbeBindAddress: :8081
metrics:
bindAddress: 127.0.0.1:8080
webhook:
port: 9443
leaderElection:
leaderElect: true
resourceName: {{ hashFNV .Repo }}.{{ .Domain }}
pkg/plugins/golang/v3/scaffolds/internal/templates/config/kdefault/manager_auth_proxy_patch.go
Outdated
Show resolved
Hide resolved
/hold |
Addressed all the comments. @estroz @camilamacedo86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are on by default, can you add an e2e test component for both /healthz
and /readyz
? Pinging these endpoints for 200 codes using the curl
pod should be sufficient (preferably in their own test functions, ex. func TestHealthz()
, that can be called from the main Describe()
block.
I am fine with add the test but it shows redundant since these endpoints are added as probes in the manager (https://github.com/kubernetes-sigs/kubebuilder/pull/1856/files#diff-bc87394b1d397b640fbef5f4b6e769a5d86df1048978b080a0e274749ed3ed4cR86-R92) they are called by the k8s api which means that the pod would not deploy successfully with they did not work. Please let me know if I am missing something. c/c @estroz @prafull01 |
@camilamacedo86 ah yeah duh. @prafull01 you can revert your e2e changes, apologies. |
@estroz Reverted my e2e changes. @camilamacedo86 Good catcher 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, estroz, prafull01 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 |
@estroz Need to unhold PR, for this to get merged. |
/hold cancel |
Description
Cherry-pick of #1795
Motivation
Re-introduce the change to close the #1761
Note that it was reverted because the CI was not doing tthe e2e tests with the v3-alpha plugin which is done now.
Fixes: #1761