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

feat: Add the flag --secret-label-selector to set the label selector for Secrets to ingest. #6795

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

tao12345666333
Copy link
Member

@tao12345666333 tao12345666333 commented Dec 9, 2024

What this PR does / why we need it:

As #6576 mentioned, we received a report about KIC's high memory usage.

We can reproduce this issue by the following steps:

  • Create 300 secret resources with a size of 1M.
$ dd if=/dev/urandom bs=756439 count=1 | base64 > output.txt
$ for i in `seq 0 300`; do kubectl create secret generic "foo-${i}" --from-file=key=output.txt ; done

Before this change:

KIC will consume over 1G of memory within the first 3-5 minutes, but after 10 minutes, KIC's memory consumption will stay around 400M.

With this change:

KIC will consume over 1G of memory within the first 3-5 minutes, but after 10 minutes, KIC's memory consumption will stay around 360M.

Which issue this PR fixes:

fixes: #6576

Special notes for your reviewer:

In this PR, I added a --secret-label-for-caching flag. Users can control whether KIC should cache it by specifying a label for the secret resources.

This is achieved by setting a LabelSelectorPredicate.

Of course, as I mentioned earlier, this approach only alleviates some of the memory usage pressure. A more thorough way we have is not to cache, which will greatly increase the pressure on the API server.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 52.94118% with 8 lines in your changes missing coverage. Please review.

Project coverage is 77.7%. Comparing base (76062fc) to head (86a54d6).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...nal/controllers/configuration/secret_controller.go 42.8% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #6795     +/-   ##
=======================================
+ Coverage   77.6%   77.7%   +0.1%     
=======================================
  Files        207     207             
  Lines      24644   24679     +35     
=======================================
+ Hits       19131   19197     +66     
+ Misses      4528    4506     -22     
+ Partials     985     976      -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tao12345666333 tao12345666333 added this to the KIC v3.4.x milestone Dec 9, 2024
@tao12345666333 tao12345666333 self-assigned this Dec 9, 2024
@tao12345666333 tao12345666333 marked this pull request as ready for review December 9, 2024 02:51
@tao12345666333 tao12345666333 requested a review from a team as a code owner December 9, 2024 02:51
Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

Generally looks good, but it needs clearer explanation.

docs/cli-arguments.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@tao12345666333 tao12345666333 force-pushed the feat-allow-cache-by-label branch 2 times, most recently from fbf6ae0 to 5cedcca Compare December 9, 2024 03:51
randmonkey
randmonkey previously approved these changes Dec 9, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
randmonkey
randmonkey previously approved these changes Dec 9, 2024
internal/controllers/configuration/secret_controller.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@tao12345666333 tao12345666333 force-pushed the feat-allow-cache-by-label branch 3 times, most recently from df86376 to 5bc641e Compare December 9, 2024 08:45
@tao12345666333 tao12345666333 force-pushed the feat-allow-cache-by-label branch from 5bc641e to 651d7d3 Compare December 9, 2024 09:20
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

Minor nits.

Please make sure to amend the PR title according to the recent changes in this PR.

CHANGELOG.md Outdated Show resolved Hide resolved
internal/controllers/configuration/secret_controller.go Outdated Show resolved Hide resolved
internal/controllers/configuration/secret_controller.go Outdated Show resolved Hide resolved
internal/manager/config.go Outdated Show resolved Hide resolved
internal/manager/config.go Outdated Show resolved Hide resolved
internal/manager/config.go Outdated Show resolved Hide resolved
@tao12345666333 tao12345666333 changed the title feat: Allow KIC to control caching of secrets through label feat: Added the flag --secret-label-selector to set the label selector for Secrets to ingest. Dec 9, 2024
@tao12345666333 tao12345666333 changed the title feat: Added the flag --secret-label-selector to set the label selector for Secrets to ingest. feat: Add the flag --secret-label-selector to set the label selector for Secrets to ingest. Dec 9, 2024
@tao12345666333 tao12345666333 force-pushed the feat-allow-cache-by-label branch from d058779 to 6ea2518 Compare December 9, 2024 10:12
…crets` to ingest.

By setting this flag, the secrets that are ingested will be limited to those having this label set to "true".
This can reduce the memory usage in scenarios with a large number of giant secrets.

Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
Co-authored-by: Tao Yi <tao.yi@konghq.com>
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@tao12345666333 tao12345666333 force-pushed the feat-allow-cache-by-label branch from 6ea2518 to 86a54d6 Compare December 9, 2024 10:12
@tao12345666333 tao12345666333 enabled auto-merge (squash) December 9, 2024 10:28
@tao12345666333 tao12345666333 merged commit d6f7d65 into main Dec 9, 2024
39 checks passed
@tao12345666333 tao12345666333 deleted the feat-allow-cache-by-label branch December 9, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Analysis of KIC's memory usage
3 participants