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

Wrapping k8s client with write filter and cache reader #4752

Merged
merged 7 commits into from
Jan 23, 2024

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Jan 19, 2024

Tracking issue

fixes #4730

Why are the changes needed?

The previous attempt (#4733) mistakenly put he cache lookup into a loop until it was populated. This caused large CPU spikes.

What changes were proposed in this pull request?

This PR bypasses the k8sClient internal caching logic and instead just wraps an uncached k8s client with a cache lookup for writes and a write through filter on create / delete operations.

How was this patch tested?

local

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

NA

Docs link

NA

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (ca73251) 58.18% compared to head (44ec90d) 58.19%.
Report is 2 commits behind head on master.

Files Patch % Lines
flytepropeller/pkg/controller/executors/kube.go 56.41% 17 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4752   +/-   ##
=======================================
  Coverage   58.18%   58.19%           
=======================================
  Files         626      626           
  Lines       53833    53826    -7     
=======================================
  Hits        31322    31322           
+ Misses      20003    19996    -7     
  Partials     2508     2508           
Flag Coverage Δ
unittests 58.19% <56.41%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Signed-off-by: Daniel Rammer <[email protected]>
cmd/single/start.go Outdated Show resolved Hide resolved
Signed-off-by: Daniel Rammer <[email protected]>
@honnix
Copy link
Member

honnix commented Jan 22, 2024

This looks good to me. I was struggling with unit test for this and I guess you might be the same. Not sure how to properly test it without some refactoring in place.

Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw hamersaw marked this pull request as ready for review January 23, 2024 03:41
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working enhancement New feature or request labels Jan 23, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 23, 2024
@hamersaw hamersaw merged commit 73b1d40 into master Jan 23, 2024
43 of 45 checks passed
@hamersaw hamersaw deleted the bug/kubeclient-cache-fallback branch January 23, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] flytepropeller fails trying to get pod resource using the kubeClient
3 participants