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.

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

✨ (:sparkles, minor) Make it possible to monitor readiness for webhook #1124

Closed

Conversation

alenkacz
Copy link
Contributor

@alenkacz alenkacz commented Aug 9, 2020

This is a very minimal implementation of #723

With this in place, one can define a readiness check for webhook like this:

err = mgr.AddReadyzCheck("ready", func(_ *http.Request) error {
		if mgr.GetWebhookServer().Started {
			return nil
		}
		return errors.New("Webhook server not yet started")
	})

there's still a possibility for a very small race because technically all the paths for webhook are bound when srv.Serve is done. But the possibility of hitting it is very small considering that the readiness check has to pick it up, return ready and someone has to make a request over network to that webhook - that will be definitely slower than the processing inside srv.Serve.

If anyone has better idea, I am all ears.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alenkacz
To complete the pull request process, please assign mengqiy
You can assign the PR to them by writing /assign @mengqiy in a comment when ready.

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

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 9, 2020
@alenkacz
Copy link
Contributor Author

alenkacz commented Aug 9, 2020

ping @ncdc @vincepri

@ncdc
Copy link
Contributor

ncdc commented Aug 17, 2020

@alenkacz thanks for this! @vincepri & I were both on vacation last week, so we'll take a look as soon as we've dug out from our email backlogs 😄

@ncdc
Copy link
Contributor

ncdc commented Aug 17, 2020

/assign @ncdc @vincepri

@@ -76,6 +76,8 @@ type Server struct {

// defaultingOnce ensures that the default fields are only ever set once.
defaultingOnce sync.Once

Started bool
Copy link
Member

Choose a reason for hiding this comment

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

Let's document this field, I'm wondering though if we should expose it as a method rather than a field on the struct, given that this field isn't safe to use concurrently

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, let's put that behind a method so that it's safe to call concurrently. See also #1155

@@ -226,6 +228,7 @@ func (s *Server) Start(stop <-chan struct{}) error {
close(idleConnsClosed)
}()

s.Started = true
err = srv.Serve(listener)
if err != nil && err != http.ErrServerClosed {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this maybe set back to false if the Serve method returns?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2020
@k8s-ci-robot
Copy link
Contributor

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

@ncdc
Copy link
Contributor

ncdc commented Dec 8, 2020

@alenkacz hi, are you still interested in getting this merged?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 8, 2021
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 7, 2021
@alenkacz
Copy link
Contributor Author

alenkacz commented Apr 8, 2021

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 8, 2021
@alenkacz
Copy link
Contributor Author

alenkacz commented Apr 8, 2021

I'll refresh this

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 7, 2021
@sbueringer
Copy link
Member

@alenkacz I can take over this issue, if it's okay for you.

@sbueringer
Copy link
Member

sbueringer commented Jul 20, 2021

Has been implemented in #1588
/lgtm (ups :))

@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
@sbueringer
Copy link
Member

/lgtm cancel
/close

@k8s-ci-robot
Copy link
Contributor

@sbueringer: Closed this PR.

In response to this:

/lgtm cancel
/close

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.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants