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

Document the liveness probes in cert-manager components #1217

Merged
merged 3 commits into from
Jun 9, 2023
Merged

Document the liveness probes in cert-manager components #1217

merged 3 commits into from
Jun 9, 2023

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Apr 28, 2023 •
edited
Loading

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 28, 2023
@netlify
Copy link

netlify bot commented Apr 28, 2023 •
edited
Loading

✅ Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit 7592cc1
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/6482e1e20e0c840008691803
😎 Deploy Preview https://deploy-preview-1217--cert-manager-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 28, 2023
@wallrj wallrj changed the title WIP: Liveness probes Document the liveness probes in cert-manager components Apr 28, 2023
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2023
@wallrj
Copy link
Member Author

wallrj commented Apr 28, 2023

/hold until we're sure we want to release this in 1.12

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2023
irbekrm
irbekrm suggested changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks Richard, this makes sense and is a great write-up, thanks for adding all those links.

Could we also mention that we might want to enable the controller liveness probe by default in future if we see that there are actual edge cases (not cert-manager bugs) where this would be useful and can we add a note asking users to report back if they have discovered such?

content/docs/installation/best-practice.md
The Kubernetes control-plane components also use this library.

The leader election code runs in a loop in a separate thread (go routine).
If it initially wins the leader election race and if it later fails to renew its leader election lease, it exits.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting, I actually didn't realize that the behaviour was like that. I imagined that if a replica loses leader election (because another replica managed to become a leader) it would just become a standby and keep attempting to become a leader. I think that's how it works in raft.
However I do see that the way we do it is also how c/r does it https://github.com/kubernetes-sigs/controller-runtime/blob/cd65cb25d314f40a329a688f4714fe3282589e97/pkg/manager/internal.go#L646 so perhaps it makes sense to exit with an error because a leader election cannot be lost in a normal flow (there's no way how leadership could shift unless there is a fatal errror). Something to look into for myself 👀

@wallrj
Copy link
Member Author

wallrj commented Apr 28, 2023

Could we also mention that we might want to enable the controller liveness probe by default in future if we see that there are actual edge cases (not cert-manager bugs) where this would be useful and can we add a note asking users to report back if they have discovered such?

Done.

@wallrj wallrj requested a review from irbekrm April 28, 2023 14:42
irbekrm
irbekrm approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2023
irbekrm
irbekrm approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thank you Richard

/lgtm

@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, wallrj

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

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2023
@wallrj
Add a section about best practice and liveness probes
6f43a21 
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
@wallrj
Add a release note
300e47b 
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
@wallrj
Invite users to give feedback about the controller liveness probe
7592cc1 
And whether it should be turned on by default.

Signed-off-by: Richard Wall <richard.wall@jetstack.io>
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed lgtm Indicates that a PR is ready to be merged. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jun 9, 2023
@jetstack-bot
Copy link
Contributor

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

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. I understand the commands that are listed here.

@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 9, 2023
@wallrj wallrj changed the base branch from release-next to master June 9, 2023 08:25
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 9, 2023
@wallrj
Copy link
Member Author

wallrj commented Jun 9, 2023

/unhold

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2023
@wallrj
Copy link
Member Author

wallrj commented Jun 9, 2023

/override dco

@jetstack-bot
Copy link
Contributor

@wallrj: Overrode contexts on behalf of wallrj: dco

In response to this:

/override dco

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.

@wallrj wallrj added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jun 9, 2023
@wallrj wallrj requested a review from maelvls June 9, 2023 08:29
@maelvls
Copy link
Member

maelvls commented Jun 9, 2023

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2023
@jetstack-bot jetstack-bot merged commit 4ed765a into cert-manager:master Jun 9, 2023
@wallrj wallrj deleted the liveness-probes branch June 9, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irbekrm irbekrm irbekrm approved these changes

@maelvls maelvls Awaiting requested review from maelvls

Assignees

@maelvls maelvls

@irbekrm irbekrm

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wallrj @jetstack-bot @maelvls @irbekrm