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

Daemons & timers for background processing and reconciliation #330

Merged
merged 20 commits into from
Apr 1, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Mar 12, 2020

What do these changes do?

Add background daemons & timers for resources.

Description

Previously, the handlers were event-driven — i.e. when something changes on the resource. If nothing changes, no handlers are called. To have reconciliation or other activities happening during the lifetime of a resource, the developers had to do their own task/thread orchestration.

With this PR, the task/thread orchestration is generalised to a new framework feature to perform background reconciliation with external systems, locally managed applications, or even local children K8s resources not the event-driven way:

import kopf

@kopf.daemon('zalando.org', 'v1', 'kopfexamples', initial_backoff=5.0, cancellation_backoff=10.0)
async def background_async(spec, logger, **_):
    while True:  # async handlers will be cancelled: instantly or with a graceful delay.
        logger.info(f"Ping from an async daemon: field={spec['field']!r}")
        await asyncio.sleep(2.5)

@kopf.timer('zalando.org', 'v1', 'kopfexamples', idle=15, interval=2, sharp=True)
def every_few_seconds_sync(spec, logger, **_):
    logger.info(f"Ping from a sync timer: field={spec['field']!r}")

The daemons run as long as the resource exists. They are stopped/cancelled when the resource is deleted (or when the operator exits/restarts). The daemons can be automatically restarted or left dead if exited (restart_backoff).

The timers are also triggered as long as the resource exists, but by schedule. They can be postponed by few seconds since the last change (idle), or since the resource creation / operator startup (initial_backoff), can have sharp or soft intervals (see the docs), etc.

For more examples & options, see /examples/ folder and the doc updates in this PR/branch.

Issues/PRs

Issues: closes #19

Related: #150 #271 #317 #122

Type of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

WARNING: This PR is a preview. It is runnable and usable, but few minor things are not finished yet:

  • Tests
    • For the stopper synchronization primitive.
    • For daemon existing reason signalling (DaemonStopperReason).
    • For daemon & timer decorators.
    • For async daemon cancellation.
    • For sync daemon cancellation.
    • For timer guarding task cancellation.
  • A sync-daemon did not exit on operator stopping in a try-run (it did exit before, something is broken? — check & fix.).

@zincr
Copy link

zincr bot commented Mar 12, 2020

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


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

  • ℹ️ docs/daemons.rst had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ docs/timers.rst had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/reactor/daemons.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/reactor/processing.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/structs/primitives.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/daemons/conftest.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/daemons/test_daemon_errors.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

@nolar nolar added the enhancement New feature or request label Mar 12, 2020
@lgtm-com
Copy link

lgtm-com bot commented Mar 12, 2020

This pull request introduces 2 alerts when merging 6505cb4 into de23abe - view on LGTM.com

new alerts:

  • 2 for Module imports itself

In the assumption that new handler types are coming (daemons, timers),
and they must be rendered differently, not as just "handlers", while
using the same invocation routines.
The real sleep is needed in some cases to give control to asyncio's
event loop (`sleep(0)`), or to debug the tests. When it is mocked,
it is difficult to do anything.

All the code uses the sleeping engine now, so there is no need
to check for the actual `asyncio.sleep()` calls.
@nolar nolar marked this pull request as ready for review March 31, 2020 08:20
@nolar nolar requested a review from haikoschol March 31, 2020 08:27
@nolar
Copy link
Contributor Author

nolar commented Mar 31, 2020

The tests are rudimentary, only for the smoke-testing of daemons & timers. This PR took already too much time, and is too big.

The low-level primitives are tested indirectly through the existing tests, and manually by running examples with Minikube (including the examples 14 & 15 for daemons & timers).

The framework itself will be tested for backward compatibility by deployed a release candidate to our own clusters and utilising the new features (incl. daemons & timers).

More detailed tests for all little aspects will be added later as separate PRs — with lower priority (time-wise).

docs/daemons.rst Outdated Show resolved Hide resolved
docs/daemons.rst Outdated Show resolved Hide resolved
docs/daemons.rst Outdated Show resolved Hide resolved
docs/daemons.rst Outdated Show resolved Hide resolved
docs/daemons.rst Outdated Show resolved Hide resolved
docs/daemons.rst Outdated Show resolved Hide resolved
docs/timers.rst Outdated Show resolved Hide resolved
kopf/reactor/daemons.py Outdated Show resolved Hide resolved
haikoschol
haikoschol previously approved these changes Mar 31, 2020
@nolar nolar requested a review from haikoschol March 31, 2020 20:42
assert dummy.mock.call_count == 1
assert k8s_mocked.patch_obj.call_count == 0
assert k8s_mocked.sleep_or_wait.call_count == 1 # one for each retry
assert k8s_mocked.sleep_or_wait.call_args_list[0][0][0] is None
Copy link

@mnarodovitch mnarodovitch Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not readable. Maybe from unittest.mock import call and

assert ...call_args_list == [call(*args, **kwargs)]

is better. Applies to the other stuff also. In the current situation, there might be obscure out of index exceptions if the test breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking the test in that case is intended. The first index is "secured" by a preceding check for the number of calls.

But you are right, making tests more readable is a good idea — but should be done for all the tests. Now, the tests in overall are in a clumsy state, grown over the year, and they were never refactored. Some of them are duplicating each other, some aspects of the code are just missing.

I've created #338 to collect the ideas on the tests, and added this one there.

@nolar nolar merged commit df4e51b into zalando-incubator:master Apr 1, 2020
@nolar nolar deleted the background-handlers-2 branch April 1, 2020 10:17
@nolar nolar added this to the 0.27 milestone May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call handlers by time
3 participants