Some concurrency fixes, some configurability #109

dbazhal · 2019-06-12T15:56:18Z

Trying to fix blocking operations, such as objects patching, event posting, etc

Issue : #111

Description

Added config - kopf.config.WorkersConfig - class attributes based options to control async workers. So user can just import it before all other kopf modules and change values to required. Maybe not the best decision, but any.
So now can be configured number of next types of workers: queue processing, object patching, event creation.
Blocking synchronous operations such as event creation, changed objects patching, are now asynchronous (i guess).
Added kopf.config.EventsConfig.events_loglevel to control what types of logs should trigger event sending - INFO/WARNING/ERROR/CRITICAL

Existing tests updated for new async functions.
E2E tests required to test performance(how fast large number of new/changed objects can be processed). Such tests would prove efficiency of kopf concurrency.

Types of Changes

New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Configuration change
Refactor/improvements

Tasks

List of tasks you will do to complete the PR

Documentation
E2E performance tests
Review?

Review

List of tasks the reviewer must do to review the PR

Tests (don't know if there could be tests to do)
Documentation (maybe?)

…options to control async workers. So user can just import it before all other kopf modules and change values to required. Maybe not the best decision, but any. 2. So now can be configured number of next types of workers: queue processing, object patching, event creation. 3. Blocking synchronous operations such as event creation, changed objects patching, are now asynchronous (i guess). Existing tests updated for new async functions. E2E tests required to test performance(how fast large number of new/changed objects can be processed). Such tests would prove efficiency of kopf concurrency.

zincr · 2019-06-12T15:56:28Z

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Specification
✅ Dependency Licensing

ValueError Set of coroutines/Futures is empty.

nolar

Great job! This can speed up the framework significantly.

Solution-wise, I have no questions anymore — thanks for reacting fast to the notes given in the chat. All is clear for me now.

Code-wise, I've added few notes. To summarise:

Constants' docstrings for Sphinx.
Google-style imports.
Explicit event-loop management.
Async tests (a simpler way).

And couple places where I didn't get what is happening — some clarification would be helpful.

Looking forward when these improvements get into the master branch!

kopf/reactor/invocation.py

tests/k8s/test_patching.py

kopf/k8s/events.py

examples/04-events/example.py

examples/99-all-at-once/example.py

kopf/events.py

kopf/config.py

kopf/k8s/events.py

kopf/k8s/patching.py

kopf/reactor/queueing.py

nolar · 2019-06-17T12:37:40Z

As a side-note: I tried to check why Travis CI pipeline is not triggered for this PR, and for this PR only — it works for other PRs & contributors. Travis CI says "Abuse detected" without any additional explanation.

I will try to contact Travis CI support to clarify what is the actual reason.

My only assumption for now is that this is a PR from the "master" branch of a fork to the "master" branch of the upstream — maybe Travis does not like it. This is the only "unusual" thing in this PR. Just guessing.

nolar

So far, I'm ready to merge the PR — the default behaviour with the default config is as intended, public interfaces are kept intact, blocking operations removed, asyncio magic is clear.

@dbazhal However, I have one last-minute question regarding how the configs affect the thread pool sizes (and do they):

nolar · 2019-06-19T13:01:09Z

kopf/k8s/events.py

 logger = logging.getLogger(__name__)

 MAX_MESSAGE_LENGTH = 1024
 CUT_MESSAGE_INFIX = '...'


-def post_event(*, obj, type, reason, message=''):
+event_executor = concurrent.futures.ThreadPoolExecutor(max_workers=config.WorkersConfig.synchronous_event_post_workers_limit)


Here is a new question. The intention of this PR is to make thread-pools' sizes configurable (and a lot of other things too).

However, if you import kopf (or kopf run in CLI), all the modules will be loaded, this one included, and the executors will be created with the default hard-coded config (as they are the module-level objects).

Any config changes applied after the import will have no effect on the executors.

What is the intended way of changing the configs to alter the Kopf's runtime setup?

So, after a quick discussion with the PR's author (in a chat), we decided to go as is for now.

These things (thread pool sizes) were not configurable before the PR, so they will be not configurable after the PR — nothing changes here.

However, blocking operations (API calls) are now moved from asyncio event-loops to the thread pools — which gives a great performance gain already, and was the main goal in this PR.

And the introduced config objects can be used to dynamically configure the Kopf's runtime later, in a separate PR (since configurability is a separate topic).

nolar · 2019-06-20T12:05:35Z

docs/configuring.rst

+
+
+Configure logging events
+=================


These "==="-lines should be at least the same length as the text they use as a caption/title: http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections

nolar · 2019-06-20T12:14:37Z

docs/configuring.rst

+.. code-block:: python
+    :caption: test_example_operator.py
+
+    from kopf import config


Does it work? I tried with a local simulation:

# pkg/__init__.py print('init!') import pkg.unwanted import pkg.mod

# pkg/mod.py print('mod')

# pkg/unwanted.py print('UNWANTED')

>>> from pkg import mod init! UNWANTED mod

Omg, I missed init :) no, that does not work at all.

Trying to figure out how it could be configured.

@dbazhal I believe, it cannot be configured. This is why I've made a comment above (the one with "after a quick discussion") about skipping this topic for now, postponing it for another PR about configurability and isolated user-controlled event-loops.

One way would be to have kopf_config module outside of kopf package, as a top-level module. But that doesn't look nice (it becomes a time-frozen public interface for the users). A better way would be to have a cfg instance object passed down the call stack starting from run() or create_tasks() — but it is a big changeset.

nolar · 2019-06-20T12:16:50Z

docs/configuring.rst

+    from kopf import config
+
+    # Now kopf will send events only when error or critical occasion happens
+    config.EventsConfig.events_loglevel = config.LOGLEVEL_ERROR


Everything beside thread pool sizes, such as this loglevel, can be set at any time after the imports. The values are used as they are at the moment of their usage, they are never remembered locally in the functions. Or do I miss something?

Which means, the two-step imports are not needed for them.

You are right, fixed that.

…mported

nolar

Okay, I was wrong. It can be configured 😄

nolar · 2019-06-20T14:18:35Z

kopf/config.py

+    @staticmethod
+    def set_synchronous_tasks_threadpool_limit(new_limit: int):
+        if new_limit < 1:
+            return


Perhaps, ValueError.

Your truth, ValueError it be :)

dbazhal requested review from nolar and samurang87 as code owners June 12, 2019 15:56

dbazhal and others added 3 commits June 12, 2019 19:01

Merge branch 'master' into master

cd51bfa

Synchronous event interface saved, events loglevel configuration added.

f62446f

fix

ae39e11

ValueError Set of coroutines/Futures is empty.

nolar reviewed Jun 17, 2019

View reviewed changes

Dmitry Bazhal added 3 commits June 17, 2019 13:43

fix import style on PR comments

59c82a9

fix loglevels

bebaa18

fix event posting timeout

1cc6951

Dmitry Bazhal added 7 commits June 18, 2019 11:32

fix patching tests

13eda0f

try another way to retrieve parent thread eventloop

09ecbee

fix docstrings for kopf.config

c1c4d9b

more fix for tests

16a3a73

commit to trigger pr bot

a071aa3

really garbage commit to trigger ci

3ce9715

really garbage commit to trigger ci

0aee5ca

nolar previously approved these changes Jun 19, 2019

View reviewed changes

nolar mentioned this pull request Jun 19, 2019

Update the EphemeralVolumeClaim story-telling #97

Merged

7 tasks

update docs on configuration

031da67

dbazhal dismissed nolar’s stale review via 031da67 June 20, 2019 09:53

nolar reviewed Jun 20, 2019

View reviewed changes

fix configuration - now really can be configured even after kopf is i…

9e52ea6

…mported

nolar previously approved these changes Jun 20, 2019

View reviewed changes

Dmitry Bazhal added 2 commits June 20, 2019 17:28

fix behaviour in case of incorrect config

af3e102

docstring

ba34911

dbazhal dismissed nolar’s stale review via ba34911 June 20, 2019 14:31

nolar approved these changes Jun 20, 2019

View reviewed changes

samurang87 approved these changes Jun 21, 2019

View reviewed changes

nolar merged commit c759d68 into zalando-incubator:master Jun 21, 2019

nolar mentioned this pull request Jul 1, 2019

Post k8s-events in the background #125

Merged

2 tasks

nolar added the refactoring Code cleanup without new features added label Oct 22, 2019

This was referenced Mar 26, 2020

Settings (official and documented) #336

Merged

Better naming for timeouts + connection timeout #337

Merged

kopf-archiver bot mentioned this pull request Aug 19, 2020

[PR] Some concurrency fixes, some configurability nolar/kopf#109

Closed

5 tasks



		Configure logging events
		=================

Some concurrency fixes, some configurability #109

Some concurrency fixes, some configurability #109

Uh oh!

Conversation

dbazhal commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trying to fix blocking operations, such as objects patching, event posting, etc

Description

Types of Changes

Tasks

Review

Uh oh!

zincr bot commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 zincr found 0 problems , 0 warnings

Uh oh!

nolar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nolar commented Jun 17, 2019

Uh oh!

nolar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolar Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolar Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbazhal commented Jun 12, 2019 •

edited

Loading

zincr bot commented Jun 12, 2019 •

edited

Loading

nolar Jun 20, 2019 •

edited

Loading

nolar Jun 20, 2019 •

edited

Loading