Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Switch to async i/o for internal API communication #176

Closed
wants to merge 12 commits into from

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Aug 8, 2019

Issue : fixes #142, replaces #152

Problem

pykube-ng, kubernetes, requests, and any other synchronous client libraries use the streaming responses of the built-in urllib3 and http for watching over the k8s-events.

These streaming requests/responses can be closed when a chunk/line is yielded to the consumer, the control flow is returned to the caller, and the streaming socket itself is idling. E.g., for requests: https://2.python-requests.org/en/master/user/advanced/#streaming-requests

However, if nothing happens on the k8s-event stream (i.e. no k8s resources are added/modified/deleted), the streaming response spends most of its time in the blocking read() operation on a socket. It can remain there for long time — minutes, hours — until some data arrives on the socket.

If the streaming response runs in a thread, while the main thread is used for an asyncio event-loop, such stream cannot be closed/cancelled/terminated (because of the blocking read()). This, in turn, makes the application to hang on exit, holding its pod from restarting, since the thread is not finished until the read() call is finished.

There is no easy way to terminate the blocking read() operation on a socket. One way is a dirty hack with the OS-level process signals, which interrupt the I/O operations on low level (OS&libc&co) — see #152.

Solution

The proper solution, however, is to use async i/o inside of the async app.

This PR converts all i/o to/from Kubernetes API to aiohttp. It is already present in the dependencies indirectly.

This efficiently removes pykube-ng (or any other clients) from the Kopf's core. They are not much needed, as the main purpose of the client libraries is to provide a convenient DSL (domain-specific language) for the Kubernetes objects manipulation. In Kopf, all manipulation is unified, and which objects are being handled is not so important.

Implementation

For authentication, both pykube-ng and the Kubernetes client remain as the dependencies: for config parsing, token retrieval, including the edge cases of GCE tokens, local command executors, so on. Kopf piggybacks on pykube-ng/client for this purpose.

For testing, aresponses is used instead of mocked requests. It runs an actual web server locally, and intercepts all aiohttp outcoming requests to be redirected to that web-server.

This switch led to almost full rewrite of all tests for kopf.clients module (all API communication) — which makes a half of this PR's size (while keeping the same semantics of the tests).

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor/improvements

Remaining TODOs

  • Authentication:
    • Check piggybacking on Pykube with tokens (now: only checked with certs@minikube).
    • Check piggybacking on Pykube with username/password.
    • Check if it works with GCE (reported as broken in Kopf+Pykube).
    • Implement piggybacking on the Kubernetes Client when pykube is absent.
    • Require that at least one of the is installed, make pykube optional.
    • Add automatic re-login, mostly for refreshment of GCE tokens?
  • Embeddable operators:
    • Accept KubeConfig/Confguration objects from the user, instead of kopf.login().
    • Document login procedured for embeddable operators.
  • Improve the exception reporting from within aresponses's handlers (now: it is shown in the teardown section).
  • Add version of self to User-Agent.
  • Ensure that cluster-scoped objects also do work (different namespace handling).

Review

  • Tests
  • Documentation

@zincr
Copy link

zincr bot commented Aug 8, 2019

🤖 zincr found 1 problem , 2 warnings

❌ Approvals
ℹ️ Large Commits
ℹ️ Dependency Licensing
✅ Specification

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

  • ℹ️ kopf/clients/auth.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/basic-structs/test_resource.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/conftest.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/k8s/test_patching.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

Dependency Licensing

All dependencies specified in package manager files must be reviewed, banned dependency licenses will block the merge, all new dependencies introduced in this pull request will give a warning, but not block the merge

Please ensure that only dependencies with licenses compatible with the license of this project is included in the pull request.

  • ℹ️ Could not process requirements.txt for new dependencies
     

@brutus333
Copy link

Hello and thank you for developing Kopf!

I implemented a daemonset like object using Kopf and I faced some difficulties. But I don't want to talk about difficulties here (I'll open a issue at some point probably), I only want to ask you something that it is not very clear for me and it is related to this PR:

I have a rather weak understanding of async in Python so please bear with me, the question is:
if we have to call only async libraries in an async app and if a kopf handler is an async app (if I got this right) then calling sync kubernetes client libraries in a kopf handler is the right thing to do?

Shouldn't we need to use an async kubernetes client library?

Thank you in advance,
Virgil

@brutus333
Copy link

While digging trough the code I found the reference to https://pymotw.com/3/asyncio/executors.html so this is answering my question.

@nolar
Copy link
Contributor Author

nolar commented Nov 13, 2019

Closed in favour of #226 (auth) + #227 (aiohttp).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement New feature or request refactoring Code cleanup without new features added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow or hanging process termination
2 participants