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

Add webhook health check #1558

Closed
sbueringer opened this issue Jul 26, 2021 · 12 comments · Fixed by #1619
Closed

Add webhook health check #1558

sbueringer opened this issue Jul 26, 2021 · 12 comments · Fixed by #1619
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@sbueringer
Copy link
Member

/kind feature

Describe the solution you'd like
It would be nice if the controller health checks would use the readiness of the webhook server (instead of the no-op Ping).

For reference, the implementation in cluster-api: kubernetes-sigs/cluster-api#4989

Anything else you would like to add:
This requires at least controller-runtime v0.9.3.

Environment:

  • cluster-api-provider-azure version:
  • Kubernetes version: (use kubectl version):
  • OS (e.g. from /etc/os-release):
@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 26, 2021
@CecileRobertMichon
Copy link
Contributor

Note: This controller-runtime health check is only available since v0.9.3

this means we first need a new version of CAPI.

@sbueringer
Copy link
Member Author

@CecileRobertMichon Just out of curiosity. Why do you need a new CAPI version to upgrade controller-runtime?

@CecileRobertMichon
Copy link
Contributor

I guess we don't need to. Historically we've updated controller-runtime as part of CAPI updates, since CAPI defined the minimum version, but we could use a later patch, as long as we're on v0.9.x.

Whoever takes this on would have to first update the controller-runtime version in go.mod to v0.9.3.

/good-first-issue

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

I guess we don't need to. Historically we've updated controller-runtime as part of CAPI updates, since CAPI defined the minimum version, but we could use a later patch, as long as we're on v0.9.x.

Whoever takes this on would have to first update the controller-runtime version in go.mod to v0.9.3.

/good-first-issue

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 added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jul 26, 2021
@jayesh-srivastava
Copy link
Member

Hi. I am a new contributor and I would like to work on this issue. Can you please guide me through this? It would be a great help to me. Also, can I assign the issue to myself?

@devigned
Copy link
Contributor

devigned commented Jul 27, 2021

@jayesh-srivastava have you had an opportunity to review the link @sbueringer helpfully added above, kubernetes-sigs/cluster-api#4989? That link shows why and what needs to be applied in this repo. If you still have questions after reviewing, please let us know.

@jayesh-srivastava
Copy link
Member

@devigned Yeah I saw that link. A little overwhelming because I am a new contributor, can you please just guide me as to where to start from?

@devigned
Copy link
Contributor

The gist of what is happening in the 8 lines of the other PR is that a health check is added to the main.go to ensure the controller's webhooks have started. The same thing needs to happen in this repo.

@jayesh-srivastava
Copy link
Member

Okay okay, I understood. Thanks @devigned . Will try to resolve this issue.

@sbueringer
Copy link
Member Author

sbueringer commented Jul 30, 2021

There was an error found in the healthcheck. The fix PR is already merged in controller-runtime, but we should wait until the next controller-runtime version is released (0.9.6) with adding it to CAPZ.

xref:

@sbueringer
Copy link
Member Author

fyi controller-runtime v0.9.6 has been released with a fix. So we should be good to go (I also verified it via the e2e tests of the cluster-api repo).

@ThorstenHans
Copy link
Contributor

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants