-
Notifications
You must be signed in to change notification settings - Fork 39.7k
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
Reuse the http request object for http probes #115939
Comments
@SergeyKanzhelev: GuidelinesPlease ensure that the issue body includes answers to the following questions:
For more details on the requirements of such an issue, please see here and ensure that they are met. If this request no longer meets these requirements, the label can be removed In response to this:
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. |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
Hi @SergeyKanzhelev! Since this is a good first issue, I suppose this would be a good starting point for contributing? I would like to take this up. Could you help me out in getting started with this if so? |
Hello @SergeyKanzhelev @thockin |
@XDRAGON2002, @VishalPraneeth thank you for your interest. Please look at code references I posted in original issue for a good starting points to understand the task. |
I completely understand the issue. And you have explained it very well. Till now I have contributed in 4 prs in kubernetes in which 2 were merged. I know golang but still I am afraid that I dont know how can I start? Can you please help me with that. |
/assign |
Just recently, I checked kubelet-related issues and saw this code as well. I will use the object pool to complete the reuse of related objects. @SergeyKanzhelev |
Hi @SergeyKanzhelev Benchmarking function: https://github.com/daman1807/kubernetes/blob/11f6565bf092128deaf9e3b080e4e9e4b92dc07b/pkg/probe/http/benchmark_test.go Modification in prober for this benchmarking: |
very nice. I think it's reasonable to go ahead with this implementation than. 65K per probe saving with the reasonable straightforward refactoring is a very good saving |
… On Fri, Feb 24, 2023, 1:58 AM Daman Arora ***@***.***> wrote:
@SergeyKanzhelev <https://github.com/SergeyKanzhelev> can we maintain a
cache with the existing *prober* structure?
type cache struct {
httpRequest *http.Request
}
type prober struct {
exec execprobe.Prober
http httpprobe.Prober
tcp tcpprobe.Prober
grpc grpcprobe.Prober
runner kubecontainer.CommandRunner
cache map[kubecontainer.ContainerID]cache
recorder record.EventRecorder
}
And workers, on stopping, will invalidate the cache.
func (w *worker) stop() {
select {
case w.stopCh <- struct{}{}:
default: // Non-blocking.
}
// clear cache
delete(w.probeManager.prober.cache, w.containerID)
}
—
Reply to this email directly, view it on GitHub
<#115939 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVCWKS4XLPTXYDY5MJLWZCA4FANCNFSM6AAAAAAVDTZAIM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This issue is only about golang objects reuse. No connections keep alive or anything like this. |
@daman1807 can you publish the difference of the benchmarks before and after your change using EDIT I see, the benchmark is not using golang benchmark tooling and I understand now Sergeys point, it seems that the |
@aojea I tried benchstat to compare perf but the variance between samples is too high to judge anything, I ran the tests for 2 minutes with 100 and 500 pods doing probes every second. The results also look pretty weird to me, maybe the GC didn't run for the test 🤯
|
let me work on this issue now please |
@pacoxu: GitHub didn't allow me to assign the following users: aman1807. Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
/assign |
What needs to be done here @SergeyKanzhelev |
Hi @SergeyKanzhelev! |
/assign |
Hi Team, Can team confirm whether the issue is still open? |
/assign |
/assign |
Today we construct the request object for every http request for the http probes. This involves concatenating some strings, setting headers, etc.
Probes are simple http get requests that are executed synchronously (for a single probe) and has no request body.
The optimization may be to reuse the request object for all probe executions. This likely save some cpu and memory for the kubelet process.
Some code references:
I'm marking this issue as a good first issue, as the issue doesn't require deep knowledge of Kubernetes. However it is not an easy issue, a few parts of code will be affected and change needs to be done very carefully.
Please review code and make sure you understand the task before self-assigning the issue.
Implementation notes
runProbe
, second storing and initializing this object inworker
, and third, actual reusing of the request object./sig node
/good-first-issue
/help-wanted
/area kubelet
The text was updated successfully, but these errors were encountered: