Skip to content

webhook: don't use time.Tick to prevent leaks#2467

Merged
jetstack-bot merged 1 commit intocert-manager:masterfrom
munnerz:webhook-ticker
Dec 16, 2019
Merged

webhook: don't use time.Tick to prevent leaks#2467
jetstack-bot merged 1 commit intocert-manager:masterfrom
munnerz:webhook-ticker

Conversation

@munnerz
Copy link
Copy Markdown
Member

@munnerz munnerz commented Dec 16, 2019

What this PR does / why we need it:

As per the comment on the Golang time.Tick function:

// Tick is a convenience wrapper for NewTicker providing access to the ticking
// channel only. While Tick is useful for clients that have no need to shut down
// the Ticker, be aware that without a way to shut it down the underlying
// Ticker cannot be recovered by the garbage collector; it "leaks".

We were previously calling this function in a for loop that by default, ticked every 10s. This means that over time, this resource 'leaks' thus causing an increase in CPU usage.

I was specifically tipped off that this may be the case through this stackoverflow comment!

Which issue this PR fixes: fixes #2445

Special notes for your reviewer:

I've not run a build with this patch for long enough to verify this does indeed fix it, but I suspect it is the case given the majority of the time spent (based on profiling) was in handling futexes.

Release note:

Fix bug causing ever-increasing CPU usage in webhook component

/kind bug
/area webhook

@munnerz
Don't use time.Tick to prevent leaks
df27fff 
Signed-off-by: James Munnelly <james@munnelly.eu>
@jetstack-bot jetstack-bot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. area/webhook Indicates a PR or issue relates to the webhook component dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Dec 16, 2019
@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Dec 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 16, 2019
@munnerz munnerz mentioned this pull request Dec 16, 2019
Closed
@munnerz munnerz added this to the v0.12 milestone Dec 16, 2019
@JoshVanL
Copy link
Copy Markdown
Contributor

JoshVanL commented Dec 16, 2019

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019
@munnerz
Copy link
Copy Markdown
Member Author

munnerz commented Dec 16, 2019

/cherrypick release-0.12

@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Dec 16, 2019

@munnerz: once the present PR merges, I will cherry-pick it on top of release-0.12 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-0.12

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.

@jetstack-bot jetstack-bot merged commit dc9b476 into cert-manager:master Dec 16, 2019
@jetstack-bot jetstack-bot modified the milestones: v0.12, v0.13 Dec 16, 2019
@jetstack-bot jetstack-bot mentioned this pull request Dec 16, 2019
Merged
@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Dec 16, 2019

@munnerz: new pull request created: #2468

Details

In response to this:

/cherrypick release-0.12

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.

@munnerz munnerz mentioned this pull request Feb 18, 2020
Closed
@dittos dittos mentioned this pull request May 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoshVanL JoshVanL Awaiting requested review from JoshVanL

Assignees

@JoshVanL JoshVanL

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/webhook Indicates a PR or issue relates to the webhook component dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

v0.13

Development

Successfully merging this pull request may close these issues.

cert-manager-webhook increasing CPU usage

4 participants

@munnerz @jetstack-bot @JoshVanL @fanfei93