[release-0.58]: Bugfix: virt-handler socket leak #9002
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
kubevirt-bot
added
do-not-merge/work-in-progress
iholder101
force-pushed
the
backport-0.58/virt-handler-socket-leak
branch
from
e9a5b33 to
0bcce59
Compare
kubevirt-bot
removed
the
do-not-merge/work-in-progress
|
/cc @enp0s3 @Barakmor1 Would appreciate your review :) |
iholder101
force-pushed
the
backport-0.58/virt-handler-socket-leak
branch
from
0bcce59 to
2cc5eab
Compare
iholder101
force-pushed
the
backport-0.58/virt-handler-socket-leak
branch
from
2cc5eab to
12d9a09
Compare
|
/hold Blocked by #9004. |
kubevirt-bot
added
the
do-not-merge/hold
kubevirt-bot
added
the
do-not-merge/work-in-progress
In other words: that the number of connections does not grow when making multiple client calls Signed-off-by: Itamar Holder <[email protected]>
matcher.HaveConditionTrue() is not supported in release-0.53, therefore this commit fallbacks to the old method of checking the condition. Signed-off-by: Itamar Holder <[email protected]>
By default, virt-handler's HTTP server keeps connections opened in order to reuse them whenever possible. But, by default, the HTTP server doesn't define any timeout for the keep-alive mechanism, hence keeping the connection opened forever. This commit sets a timeout of 60 seconds to the keep-alive mechanism. Signed-off-by: Itamar Holder <[email protected]>
Signed-off-by: Itamar Holder <[email protected]>
Now that an HTTP client is added to NewVirtHandlerClient() as a new parameter, many tests have to use an empty client. This function fetches the virt-handler pod with an empty HTTP client. Signed-off-by: Itamar Holder <[email protected]>
Signed-off-by: Itamar Holder <[email protected]>
Signed-off-by: Itamar Holder <[email protected]>
This is necesarry for connection re-use. As client.Do()'s documentation says: "If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request." Source: https://pkg.go.dev/net/http#Client.Do Signed-off-by: Itamar Holder <[email protected]>
Same logic is used twice, now it's extracted into a helper function. This is especially important since this helper function makes sure that the response is properly close which can prevent future bugs. Signed-off-by: Itamar Holder <[email protected]>
iholder101
force-pushed
the
backport-0.58/virt-handler-socket-leak
branch
from
12d9a09 to
5db003d
Compare
kubevirt-bot
removed
the
do-not-merge/work-in-progress
|
/lgtm Thank you! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enp0s3 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 |
kubevirt-bot
added
the
approved
|
/retest |
|
@iholder101: The following test failed, say
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. I understand the commands that are listed here. |
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Manual backport for #8948.
Special notes for your reviewer:
The only interesting reason for a backport is the following commit: 49dd3c5. In the original PR I was using a matcher (
matcher.HaveConditionTrue()) that doesn't exist in this branch. Therefore, I'm manually checking the condition instead.Release note: