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

Explain how to optimise cert-manager for scale #1458

Merged
merged 4 commits into from
Apr 30, 2024

Conversation

@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. labels Apr 9, 2024
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from wallrj. For more information see the Kubernetes Code Review Process.

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

Copy link

netlify bot commented Apr 9, 2024

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit cc066dc
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/6630e9bd0e290d00086ab8ac
😎 Deploy Preview https://deploy-preview-1458--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 configuration.

@wallrj wallrj force-pushed the 551-large-clusters branch 2 times, most recently from 716fcff to 131a9ad Compare April 9, 2024 10:59
@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 9, 2024
@wallrj wallrj force-pushed the 551-large-clusters branch 2 times, most recently from e70bd00 to 6697ab9 Compare April 9, 2024 12:09
@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 Apr 11, 2024
Comment on lines 24 to 30
```yaml
config:
apiVersion: controller.config.cert-manager.io/v1alpha1
kind: ControllerConfiguration
kubernetesAPIQPS: 10000
kubernetesAPIBurst: 10000
```
Copy link
Member

@maelvls maelvls Apr 18, 2024

Choose a reason for hiding this comment

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

Where is this YAML supposed to be configured? Is that in the Helm chart?

Ah, this is a "ControllerConfiguration" file. I didn't know cert-manager had a file-based configuration format 😅

Signed-off-by: Richard Wall <richard.wall@venafi.com>
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj force-pushed the 551-large-clusters branch 2 times, most recently from 849e187 to e859aeb Compare April 19, 2024 15:56
@cert-manager-prow cert-manager-prow 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 Apr 19, 2024
@wallrj wallrj changed the title WIP: Explain how to optimise cert-manager for large clusters Explain how to optimise cert-manager Apr 19, 2024
@cert-manager-prow cert-manager-prow 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 19, 2024
@wallrj wallrj requested a review from maelvls April 19, 2024 15:57
@wallrj
Copy link
Member Author

wallrj commented Apr 19, 2024

@maelvls @hawksight I've simplified the content and removed all the distracting evidence files.
I've also removed the memory recommendations because I think it is better to let the user simply run krr or something on their cluster to learn the best settings.

@inteon inteon changed the title Explain how to optimise cert-manager Explain how to optimise cert-manager for scale Apr 23, 2024
Copy link
Member

@maelvls maelvls left a comment

Choose a reason for hiding this comment

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

I enjoyed reading the document. To the point, sticks to the facts, and well sourced, and very well written. Thank you!

It is so good that I'll share this guide on Twitter to talk about cert-manager good practices!

content/docs/devops-tips/scaling-cert-manager.md Outdated Show resolved Hide resolved
@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2024
Co-authored-by: Maël Valais <mael@vls.dev>
Signed-off-by: Richard Wall <wallrj@users.noreply.github.com>
@wallrj wallrj requested a review from maelvls April 30, 2024 10:21
> 📖 Learn [how to set Certificate defaults automatically](../tutorials/certificate-defaults/README.md), using tools like Kyverno.


## Set `revisionHistoryLimit: 1` on all Certificate resources
Copy link
Member Author

Choose a reason for hiding this comment

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

>
> 📖 Learn [how to set `revisionHistoryLimit` when using Annotated Ingress resources](../usage/ingress.md#supported-annotations).
>
> 🔗 Read [`cert-manager#3773`: Certificate revision history limit](https://github.com/cert-manager/cert-manager/pull/3773),
Copy link
Member

@inteon inteon Apr 30, 2024

Choose a reason for hiding this comment

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

@wallrj It is quite hard to find a "why" in that pull request, didn't we discuss in one of our standups that we should update the cert-manager code and set a sane default value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I've replaced that link with cert-manager/cert-manager#3958 so that users can go and vote for that change.

apiVersion: controller.config.cert-manager.io/v1alpha1
kind: ControllerConfiguration
featureGates:
AllBeta: true
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the default? Do we have to specifically enable AllBeta here?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, but I cannot find the exact place in the code that says as such.
It can be good to be explicit in whats enable with helm values, but it's a preference choice.

@wallrj - assuming this is default, I'd drop that flag.. OR just add a comment that this is the default for reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Dropped. I think I assumed it necessary, because I saw it in the E2E setup scripts:

https://github.com/cert-manager/cert-manager/blob/5d0b738b3e35679f1fec3b7a5deac4e7f5a48050/make/e2e-setup.mk#L263-L267

Copy link
Member

@hawksight hawksight left a comment

Choose a reason for hiding this comment

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

Easy to read and better docs that what we currently have :)

Left a minor syntax comment.
Also a possible change or clarification on waht @inteon spotted too.

But otherwise I appreciate the links for futher reading / justification. Especially since some of those are to the defaults tutorial 🎉

## Enable Server-Side Apply

By default, cert-manager [uses Update requests](https://kubernetes.io/docs/reference/using-api/api-concepts/#update-mechanism-update)
to create and modify resources like CertificateRequest and Secret,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to create and modify resources like CertificateRequest and Secret,
to create and modify resources like `CertificateRequest` and `Secret`,

apiVersion: controller.config.cert-manager.io/v1alpha1
kind: ControllerConfiguration
featureGates:
AllBeta: true
Copy link
Member

Choose a reason for hiding this comment

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

I think so, but I cannot find the exact place in the code that says as such.
It can be good to be explicit in whats enable with helm values, but it's a preference choice.

@wallrj - assuming this is default, I'd drop that flag.. OR just add a comment that this is the default for reference.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj requested a review from inteon April 30, 2024 12:55
Copy link
Member

@inteon inteon left a comment

Choose a reason for hiding this comment

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

Thanks @wallrj, great article.
/approve
/lgtm

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawksight, inteon, maelvls

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

@cert-manager-prow cert-manager-prow bot merged commit 93ef7d5 into cert-manager:master Apr 30, 2024
8 checks passed
@wallrj wallrj deleted the 551-large-clusters branch April 30, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
5 participants