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.

Sign up for GitHub

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

Jump to bottom

✨ add health check for webhook server #1588

Merged
merged 2 commits into from
Jul 20, 2021
Merged

✨ add health check for webhook server #1588

merged 2 commits into from
Jul 20, 2021

Conversation

sbueringer
Copy link
Member

Signed-off-by: Stefan Büringer buringerst@vmware.com

Fixes #723

Supersedes #1124

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 9, 2021
@sbueringer sbueringer changed the title ✨ make it possible to monitor if the webhook server has been started [WIP] ✨ make it possible to monitor if the webhook server has been started Jul 9, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2021
@sbueringer
make it possible to monitor if the webhook server has been started
8b8d524 
Signed-off-by: Stefan Büringer buringerst@vmware.com
@sbueringer sbueringer force-pushed the pr-enable-webhook-started-readiness-check branch from e8eca3c to 8b8d524 Compare July 9, 2021 16:13
@sbueringer
Copy link
Member Author

sbueringer commented Jul 9, 2021 •
edited
Loading

/cc @randomvariable @vincepri

I implemented it this way as it's the easiest implementation and there didn't seem to be major concerns in #1124 (which has a similar implementation)

My biggest concern is that the started field is not 100% correct as the Serve func is executed after setting started to true. An alternative would be to check if the server has been started by trying a tcp dial on the webhook port (similar to https://github.com/kubernetes-sigs/cluster-api/blob/master/internal/envtest/environment.go#L225-L241).

sbueringer
sbueringer commented Jul 9, 2021
View reviewed changes
pkg/webhook/server.go
@@ -280,6 +288,19 @@ func (s *Server) Start(ctx context.Context) error {
return nil
}

// StartedChecker returns an healthz.Checker which is healthy after the
// server has been started.
func (s *Server) StartedChecker() healthz.Checker {
Copy link
Member Author

@sbueringer sbueringer Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I can also move this func into the healthz package. The consequence would be that either the healthz package has a dependency to the webhook package or I would have to add an interface with a Started() bool func.

Copy link
Member

Choose a reason for hiding this comment

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

Could we maybe also tcp ping the server?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup definitely. Should we then still keep the started flag?

Or is the idea to combine that, so we only start pinging after started is true?

I'm not sure if it's worth it keeping the started field around.

Copy link
Member

Choose a reason for hiding this comment

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

It can make sense to only start tcp-pinging the port only after the started field is set to true

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, will do.

@sbueringer sbueringer changed the title [WIP] ✨ make it possible to monitor if the webhook server has been started ✨ make it possible to monitor if the webhook server has been started Jul 9, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2021
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 9, 2021
@sbueringer
Copy link
Member Author

I'll squash before merge.

sbueringer
sbueringer commented Jul 9, 2021
View reviewed changes
pkg/webhook/server.go
return fmt.Errorf("webhook server has not been started yet")
}

conn, err := net.DialTimeout("tcp", net.JoinHostPort(s.Host, strconv.Itoa(s.Port)), 10*time.Second)
Copy link
Member Author

@sbueringer sbueringer Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

10 seconds is just a wild guess. Not sure after which threshold we want to declare the webhook server not ready (and how long we want to block the client in case it has a higher timeout)

@sbueringer sbueringer changed the title ✨ make it possible to monitor if the webhook server has been started ✨ add health check for webhook server Jul 9, 2021
@k8s-ci-robot
Copy link
Contributor

@cndoit18: changing LGTM is restricted to collaborators

In response to this:

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/test-infra repository.

@sbueringer
Copy link
Member Author

/assign @joelanford

vincepri
vincepri approved these changes Jul 20, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cndoit18, sbueringer, vincepri

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2021
@k8s-ci-robot k8s-ci-robot merged commit 865d56a into kubernetes-sigs:master Jul 20, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.10.x milestone Jul 20, 2021
@sbueringer sbueringer deleted the pr-enable-webhook-started-readiness-check branch July 20, 2021 17:23
This was referenced Jul 20, 2021
Merged
Closed
@sbueringer sbueringer mentioned this pull request Jul 30, 2021
Merged
@hferentschik hferentschik mentioned this pull request Aug 13, 2021
Closed
@spilchen spilchen mentioned this pull request Aug 26, 2021
Closed
@timebertt timebertt mentioned this pull request Oct 12, 2021
Closed
6 tasks
@wu-sheng wu-sheng mentioned this pull request Nov 3, 2021
Closed
@kieron-dev kieron-dev mentioned this pull request Mar 25, 2022
Closed
@Abirdcfly Abirdcfly mentioned this pull request Feb 9, 2023
Closed
@sivanantha321 sivanantha321 mentioned this pull request Dec 7, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri approved these changes

@cndoit18 cndoit18 cndoit18 approved these changes

@joelanford joelanford Awaiting requested review from joelanford

@mengqiy mengqiy Awaiting requested review from mengqiy

Assignees

@joelanford joelanford

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.10.x
Development

Successfully merging this pull request may close these issues.

Add default readiness check for webhook server?
5 participants
@sbueringer @k8s-ci-robot @vincepri @cndoit18 @joelanford