-
Notifications
You must be signed in to change notification settings - Fork 183
Use select.poll() for exec on linux/darwin #268
Use select.poll() for exec on linux/darwin #268
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Welcome @jsun-splunk! |
/check-cla |
ef58a56
to
fd23cb3
Compare
fd23cb3
to
18828d9
Compare
/assign Thanks for sending the fix! Please let us know when it's ready for review. |
Hi @roycaihw - thanks for looking into this. I'm just waiting on some internal process to sign off the CLA. The PR is actually ready to be reviewed. |
I signed it |
/check-cla |
/check-cla |
i signed it |
/ok-to-test |
@jsun-splunk: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsun-splunk, roycaihw 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 |
The CLA check still doesn't pass. Please make sure you've signed the CLA, and the email matches the one used in the Git commits. |
/check-cla |
1 similar comment
/check-cla |
hi @roycaihw, Now this is merged, do you know how we can update the submodule in the top leve client (https://github.com/kubernetes-client/python/tree/master/kubernetes)? I can't seem to find any instructions on how to release the submodules. Thanks! |
Please follow this instruction to update the submodule and create a PR. Thanks! |
What type of PR is this?
/kind bug
What this PR does / why we need it:
For an exec call, the current
WSClient
implementation usesselect.select()
to wait for file descriptor that are ready for read. Theselect.select()
method uses theselect()
system call for this. As theselect()
system call has a limitation where by it will return the following error if the file descriptor # is greater than 1024.This limitation happens at scale when are running a large number of exec calls from a single client process. Once the filedescriptor number goes pass 1024 all future exec calls will experience the above error.
By switching to select.poll() instead, we can over come this limitation. The caveat here is that
select.poll()
is only supported on unix based OS, where asselect.select()
is also support on Windows. However, even if this is the case, it would be better to use theselect.poll()
where possible so we can overcome this limitation even if it is on unix based OS.Which issue(s) this PR fixes:
Similar problem was previously discussed in these issues.
#53
#106
Special notes for your reviewer:
Another option here is to use
select.epoll()
instead ofselect.poll()
for linux, however, I did not fully understand the difference between the two and did not felt confident enough to try epoll.Does this PR introduce a user-facing change?
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: