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

Configurable state persistence methods #331

Merged
merged 8 commits into from
Mar 30, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Mar 25, 2020

What do these changes do?

Add flexible methods of persisting the handlers' state between handling cycles. Defaults to backward-compatible approach with status stanza smoothly transitioning to annotations.

Description

Previously, the handlers' states were stored in .status.kopf.progress, and it was hard-coded.

This led to issues with strict CRD schemas, when arbitrary fields are not accepted (and actually just lost without errors), and with built-in resources (e.g. pods), where the schema cannot be hacked with workarounds.

Specifically, in operators with multiple handlers, only the 1st one was executed. In case of errors, no retries were performed. All of this is because .status patches were silently ignored by K8s API.

With this PR, the handlers' state is migrating to annotations. But the migration is smooth, so that both old status fields and new annotations are supported — for easier version upgrades of Kopf.


As an advanced use-case and side-effect, the state persistence can now be controlled on a very fine-grained level: which status keys to use, which annotation prefixes, should they be persisted after handling or not, etc.

It also becomes possible to write custom state persistence engines, e.g. in external databases (not going to be part of the framework itself, as there is no foreseeable need).

import kopf

@kopf.on.startup()
def configure_state_persistence(config: kopf.OperatorConfig, **_):
    config.persistence.progress_storage = kopf.AnnotationsProgressStorage(prefix='my-op.example.com')
    config.persistence.progress_storage = kopf.StatusProgressStorage(field='status.my-op')
    config.persistence.progress_storage = kopf.SmartProgressStorage(prefix='my-op.example.com', field='status.my-op')

The change is (supposed to be) fully backward compatible, including the version upgrade of Kopf in Kopf-based operators.


The same logic is applied to storage of diff-base (aka "last-handled-configuration") — which is used for calculating the diffs since the last handling. The setting is settings.persistence.diffbase_storage.

See configuration.rst in this PR for more details. Preview:

As usually, reviewing individual commits can make it easier.

Issues/PRs

Issues: #308 #23 #283 #321

Type of changes

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)

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

@zincr
Copy link

zincr bot commented Mar 25, 2020

🤖 zincr found 0 problems , 2 warnings

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

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/configuration.rst had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/storage/diffbase.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/storage/progress.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/storage/states.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/structs/lastseen.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
     

Previously, the essence was regenerated in few places independently.
While it is okay in most cases, since the body/essence is never changed,
it is better to be safe by retrieving/generating the essences only once
in the beginning of a handling cycle, and then reusing the essences
in decision making. The resulting effect will be the same.

This will also save a little bit of CPU and RAM.
@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2020

This pull request introduces 1 alert when merging 924e3bb into 842a907 - view on LGTM.com

new alerts:

  • 1 for Incomplete URL substring sanitization

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants