-
Notifications
You must be signed in to change notification settings - Fork 18
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
use Popen communicate instead of wait #25
Conversation
Like described in this (https://stackoverflow.com/a/39477247) stackoverflow comment, it is not a good idea to use wait, because it can andup in a deadlock. This can also fix the problem in this issue https://forum.checkmk.com/t/checkmk-k8s-node-metrics-collector/43278
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA or my organization already has a signed CLA. |
Hello, we are also hitting this issue on OpenShift 4.13.34. Can I assist somehow to get this merged quicker? |
Hi, the component owner for this part of our software is currently not here. This does not keep me from assisting you with the change itself. However, there won't be a release until he is back. For the change itself, I have the following question: We are moving the buffering problem from OS pipe to the memory of the process. Can we get an idea of the size of your OS pipe, and additionally the size of the If we decide the merge these changes as is, then there are two smaller issues:
I would squash the commits into one, and replace |
Hi, the max OS pipe buffer is 64kB. However, with the Kubernetes clusters I am using, the size of the out variable exceeds 126kB. This causes the script to deadlock.
Yes ok, that's good |
Alright seems good to me. One more person will review it, then we'll test some changes in a live scenario, and finally include it in the next release (I'll post an update). I've rebase this commit on the other pull request, squash the two commits into one. This is to avoid our internal tooling from complaining about the errors I mentioned above. |
CMK-16676 Closes: #25 Change-Id: Ib43aea1b83cf1fc28d617bedd76656aa77bdac78
Hi, the release is now available https://github.com/Checkmk/checkmk_kube_agent/releases/tag/v1.6.0 Kind regards, Sol |
Like described in this (https://stackoverflow.com/a/39477247) stackoverflow comment, it is not a good idea to use wait, because it can end up in a deadlock. This can also fix the problem in this issue https://forum.checkmk.com/t/checkmk-k8s-node-metrics-collector/43278