Skip to content
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

Optionally remove Status handler_id prefix. #579

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ and [Historic GitHub Contributors](https://github.com/zalando-incubator/kopf/gra
- [Trond Hindenes](https://github.com/trondhindenes)
- [Vennamaneni Sai Narasimha](https://github.com/thevennamaneni)
- [Cliff Burdick](https://github.com/cliffburdick)
- [Jim Ramsay](https://github.com/lack)
- [Sam Giles](https://github.com/samgiles)
1 change: 1 addition & 0 deletions docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Kopf: Kubernetes Operators Framework
results
errors
scopes
status

.. toctree::
:maxdepth: 2
Expand Down
59 changes: 59 additions & 0 deletions docs/status.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
=======
Updating Status
=======
Comment on lines +1 to +3
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
=======
Updating Status
=======
===============
Updating Status
===============


Kopf will take the return value of all state-changing handlers (even
sub-handlers) and add them to the ``Status`` of the corresponding Kuberenetes
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
sub-handlers) and add them to the ``Status`` of the corresponding Kuberenetes
sub-handlers) and add them to the ``.status`` stanza of the corresponding Kubernetes

resource. By default, this is stored under the handler id which by default is
the function name.

.. note::

The ``CustomResourceDefinition`` requires that the fields that will be set in
status have a corresponding schema.

You could also use ``x-kubernetes-preserve-unknown-fields: true``::

schema:
openAPIV3Schema:
type: object
properties:
status:
type: object
x-kubernetes-preserve-unknown-fields: true
spec:
...

Given the following handler definition::

import kopf

@kopf.on.create('zalando.org', 'v1', 'kopfexamples')
def my_handler(spec, **_):
return {'field': 'value}
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please wrap all code blocks here (before and after this comment) to their specific language highlights?

This is one of the following instead of :::

.. code-block:: yaml
.. code-block:: python

I'm highly uncertain if there is any kind of automatic language detection in SphinxDocs.


The resulting object will have the following data::

spec:
...
status:
my_handler:
field: value

In order to remove the handler ID from the result, you may set the optional
``status_prefix=False`` when defining the handler.

So with the handler definition::

import kopf

@kopf.on.create('zalando.org', 'v1', 'kopfexamples', status_prefix=False)
def my_handler(spec, **_):
return {'field': 'value}

The resulting object will have the following data::

spec:
...
status:
field: value
Copy link
Owner

Choose a reason for hiding this comment

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

Important: What will happen with return "hello" or return 123 or other scalars? What about return ["abc", "def"]?

15 changes: 15 additions & 0 deletions kopf/on.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ def resume( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: bool = True,
Copy link
Owner

Choose a reason for hiding this comment

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

Important: Generally, as a rule of thumb, all handler options are designed so that the default value is None and evaluates to falseness. Even for booleans (see optional or deleted in some handlers nearby — it is Optional[bool]). I would prefer that all user-facing DSL features follow this convention in the future.

Can you please negate the option and its meaning? (I am not sure what would be the better wording to describe this: flat status? direct status? raw status? result_to_status? something else?)

) -> ResourceChangingDecorator:
""" ``@kopf.on.resume()`` handler for the object resuming on operator (re)start. """
def decorator( # lgtm[py/similar-function]
Expand All @@ -161,6 +162,7 @@ def decorator( # lgtm[py/similar-function]
labels=labels, annotations=annotations, when=when,
initial=True, deleted=deleted, requires_finalizer=None,
reason=None,
status_prefix=status_prefix,
)
real_registry.resource_changing_handlers[real_resource].append(handler)
return fn
Expand All @@ -180,6 +182,7 @@ def create( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: bool = True,
) -> ResourceChangingDecorator:
""" ``@kopf.on.create()`` handler for the object creation. """
def decorator( # lgtm[py/similar-function]
Expand All @@ -196,6 +199,7 @@ def decorator( # lgtm[py/similar-function]
labels=labels, annotations=annotations, when=when,
initial=None, deleted=None, requires_finalizer=None,
reason=handlers.Reason.CREATE,
status_prefix=status_prefix,
)
real_registry.resource_changing_handlers[real_resource].append(handler)
return fn
Expand All @@ -215,6 +219,7 @@ def update( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: bool = True,
) -> ResourceChangingDecorator:
""" ``@kopf.on.update()`` handler for the object update or change. """
def decorator( # lgtm[py/similar-function]
Expand All @@ -231,6 +236,7 @@ def decorator( # lgtm[py/similar-function]
labels=labels, annotations=annotations, when=when,
initial=None, deleted=None, requires_finalizer=None,
reason=handlers.Reason.UPDATE,
status_prefix=status_prefix,
)
real_registry.resource_changing_handlers[real_resource].append(handler)
return fn
Expand All @@ -251,6 +257,7 @@ def delete( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: bool = True,
) -> ResourceChangingDecorator:
""" ``@kopf.on.delete()`` handler for the object deletion. """
def decorator( # lgtm[py/similar-function]
Expand All @@ -267,6 +274,7 @@ def decorator( # lgtm[py/similar-function]
labels=labels, annotations=annotations, when=when,
initial=None, deleted=None, requires_finalizer=bool(not optional),
reason=handlers.Reason.DELETE,
status_prefix=status_prefix,
)
real_registry.resource_changing_handlers[real_resource].append(handler)
return fn
Expand All @@ -287,6 +295,7 @@ def field( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: bool = True,
) -> ResourceChangingDecorator:
""" ``@kopf.on.field()`` handler for the individual field changes. """
def decorator( # lgtm[py/similar-function]
Expand All @@ -304,6 +313,7 @@ def decorator( # lgtm[py/similar-function]
labels=labels, annotations=annotations, when=when,
initial=None, deleted=None, requires_finalizer=None,
reason=None,
status_prefix=status_prefix,
)
real_registry.resource_changing_handlers[real_resource].append(handler)
return fn
Expand All @@ -318,6 +328,7 @@ def event( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: bool = True,
) -> ResourceWatchingDecorator:
""" ``@kopf.on.event()`` handler for the silent spies on the events. """
def decorator( # lgtm[py/similar-function]
Expand Down Expand Up @@ -396,6 +407,7 @@ def timer( # lgtm[py/similar-function]
sharp: Optional[bool] = None,
idle: Optional[float] = None,
interval: Optional[float] = None,
status_prefix: bool = True,
) -> ResourceTimerDecorator:
""" ``@kopf.timer()`` handler for the regular events. """
def decorator( # lgtm[py/similar-function]
Expand Down Expand Up @@ -432,6 +444,7 @@ def this( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: Optional[bool] = None,
) -> ResourceChangingDecorator:
"""
``@kopf.on.this()`` decorator for the dynamically generated sub-handlers.
Expand Down Expand Up @@ -471,12 +484,14 @@ def decorator( # lgtm[py/similar-function]
real_registry = registry if registry is not None else handling.subregistry_var.get()
real_id = registries.generate_id(fn=fn, id=id,
prefix=parent_handler.id if parent_handler else None)
handler_status_prefix = status_prefix if status_prefix is not None else parent_handler.status_prefix if parent_handler else True
handler = handlers.ResourceChangingHandler(
fn=fn, id=real_id, field=None,
errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown,
labels=labels, annotations=annotations, when=when,
initial=None, deleted=None, requires_finalizer=None,
reason=None,
status_prefix=handler_status_prefix,
)
real_registry.append(handler)
return fn
Expand Down
21 changes: 11 additions & 10 deletions kopf/reactor/handling.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ async def execute(
cause = cause if cause is not None else cause_var.get()
parent_handler: handlers_.BaseHandler = handler_var.get()
parent_prefix = parent_handler.id if parent_handler is not None else None
parent_status_prefix = parent_handler.status_prefix if parent_handler else None

# Validate the inputs; the function signatures cannot put these kind of restrictions, so we do.
if len([v for v in [fns, handlers, registry] if v is not None]) > 1:
Expand All @@ -104,7 +105,7 @@ async def execute(
errors=None, timeout=None, retries=None, backoff=None, cooldown=None,
labels=None, annotations=None, when=None,
initial=None, deleted=None, requires_finalizer=None,
reason=None, field=None,
reason=None, field=None, status_prefix=parent_status_prefix,
)
subregistry.append(handler)

Expand All @@ -117,7 +118,7 @@ async def execute(
errors=None, timeout=None, retries=None, backoff=None, cooldown=None,
labels=None, annotations=None, when=None,
initial=None, deleted=None, requires_finalizer=None,
reason=None, field=None,
reason=None, field=None, status_prefix=parent_status_prefix,
)
subregistry.append(handler)

Expand Down Expand Up @@ -280,44 +281,44 @@ async def execute_handler_once(
# Unfinished children cause the regular retry, but with less logging and event reporting.
except HandlerChildrenRetry as e:
logger.debug(f"{handler} has unfinished sub-handlers. Will retry soon.")
return states.HandlerOutcome(final=False, exception=e, delay=e.delay, subrefs=subrefs)
return states.HandlerOutcome(final=False, exception=e, delay=e.delay, subrefs=subrefs, handler=handler)

# Definitely a temporary error, regardless of the error strictness.
except TemporaryError as e:
logger.error(f"{handler} failed temporarily: %s", str(e) or repr(e))
return states.HandlerOutcome(final=False, exception=e, delay=e.delay, subrefs=subrefs)
return states.HandlerOutcome(final=False, exception=e, delay=e.delay, subrefs=subrefs, handler=handler)

# Same as permanent errors below, but with better logging for our internal cases.
except HandlerTimeoutError as e:
logger.error(f"%s", str(e) or repr(e)) # already formatted
return states.HandlerOutcome(final=True, exception=e, subrefs=subrefs)
return states.HandlerOutcome(final=True, exception=e, subrefs=subrefs, handler=handler)
# TODO: report the handling failure somehow (beside logs/events). persistent status?

# Definitely a permanent error, regardless of the error strictness.
except PermanentError as e:
logger.error(f"{handler} failed permanently: %s", str(e) or repr(e))
return states.HandlerOutcome(final=True, exception=e, subrefs=subrefs)
return states.HandlerOutcome(final=True, exception=e, subrefs=subrefs, handler=handler)
# TODO: report the handling failure somehow (beside logs/events). persistent status?

# Regular errors behave as either temporary or permanent depending on the error strictness.
except Exception as e:
if errors_mode == handlers_.ErrorsMode.IGNORED:
logger.exception(f"{handler} failed with an exception. Will ignore.")
return states.HandlerOutcome(final=True, subrefs=subrefs)
return states.HandlerOutcome(final=True, subrefs=subrefs, handler=handler)
elif errors_mode == handlers_.ErrorsMode.TEMPORARY:
logger.exception(f"{handler} failed with an exception. Will retry.")
return states.HandlerOutcome(final=False, exception=e, delay=backoff, subrefs=subrefs)
return states.HandlerOutcome(final=False, exception=e, delay=backoff, subrefs=subrefs, handler=handler)
elif errors_mode == handlers_.ErrorsMode.PERMANENT:
logger.exception(f"{handler} failed with an exception. Will stop.")
return states.HandlerOutcome(final=True, exception=e, subrefs=subrefs)
return states.HandlerOutcome(final=True, exception=e, subrefs=subrefs, handler=handler)
# TODO: report the handling failure somehow (beside logs/events). persistent status?
else:
raise RuntimeError(f"Unknown mode for errors: {errors_mode!r}")

# No errors means the handler should be excluded from future runs in this reaction cycle.
else:
logger.info(f"{handler} succeeded.")
return states.HandlerOutcome(final=True, result=result, subrefs=subrefs)
return states.HandlerOutcome(final=True, result=result, subrefs=subrefs, handler=handler)


async def invoke_handler(
Expand Down
2 changes: 1 addition & 1 deletion kopf/reactor/registries.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ def register(
id=real_id, fn=fn, reason=reason, field=real_field,
errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown,
initial=initial, deleted=deleted, requires_finalizer=requires_finalizer,
labels=labels, annotations=annotations, when=when,
labels=labels, annotations=annotations, when=when, status_prefix=True,
)

self.append(handler)
Expand Down
12 changes: 10 additions & 2 deletions kopf/storage/states.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class HandlerOutcome:
possibly after few executions, and consisting of simple data types
(for YAML/JSON serialisation) rather than the actual in-memory objects.
"""
handler: handlers_.BaseHandler
final: bool
delay: Optional[float] = None
result: Optional[callbacks.Result] = None
Expand Down Expand Up @@ -304,9 +305,16 @@ def deliver_results(
pass
elif isinstance(outcome.result, collections.abc.Mapping):
# TODO: merge recursively (patch-merge), do not overwrite the keys if they are present.
patch.setdefault('status', {}).setdefault(handler_id, {}).update(outcome.result)
if hasattr(outcome.handler, 'status_prefix') and not outcome.handler.status_prefix:
base = patch.setdefault('status', {})
else:
base = patch.setdefault('status', {}).setdefault(handler_id, {})
base.update(outcome.result)
else:
patch.setdefault('status', {})[handler_id] = copy.deepcopy(outcome.result)
if hasattr(outcome.handler, 'status_prefix') and not outcome.handler.status_prefix:
patch.setdefault('status', {})[copy.deepcopy(outcome.result)] = {}
Copy link
Owner

Choose a reason for hiding this comment

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

Important: I'm quite not sure what does this line mean. Can you please clarify the intentions? It looks like the result is used as the key, not as the value.

else:
patch.setdefault('status', {})[handler_id] = copy.deepcopy(outcome.result)


@overload
Expand Down
1 change: 1 addition & 0 deletions kopf/structs/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ class ResourceChangingHandler(ResourceHandler):
initial: Optional[bool]
deleted: Optional[bool] # used for mixed-in (initial==True) @on.resume handlers only.
requires_finalizer: Optional[bool]
status_prefix: Optional[bool]

@property
def event(self) -> Optional[Reason]:
Expand Down
2 changes: 1 addition & 1 deletion kopf/toolkits/legacy_registries.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def register(
reason=reason, field=real_field,
errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown,
initial=initial, deleted=deleted, requires_finalizer=requires_finalizer,
labels=labels, annotations=annotations, when=when,
labels=labels, annotations=annotations, when=when, status_prefix=True,
)
self.append(handler)
return fn
Expand Down
3 changes: 3 additions & 0 deletions tests/basic-structs/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ def test_resource_handler_with_all_args(mocker):
annotations = mocker.Mock()
when = mocker.Mock()
requires_finalizer = mocker.Mock()
status_prefix = mocker.Mock()
handler = ResourceChangingHandler(
fn=fn,
id=id,
Expand All @@ -67,6 +68,7 @@ def test_resource_handler_with_all_args(mocker):
annotations=annotations,
when=when,
requires_finalizer=requires_finalizer,
status_prefix=status_prefix,
)
assert handler.fn is fn
assert handler.id is id
Expand All @@ -82,6 +84,7 @@ def test_resource_handler_with_all_args(mocker):
assert handler.annotations is annotations
assert handler.when is when
assert handler.requires_finalizer is requires_finalizer
assert handler.status_prefix is status_prefix

with pytest.deprecated_call(match=r"use handler.reason"):
assert handler.event is reason
Expand Down
3 changes: 3 additions & 0 deletions tests/basic-structs/test_handlers_deprecated_cooldown.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def test_resource_handler_with_deprecated_cooldown_instead_of_backoff(mocker):
annotations = mocker.Mock()
when = mocker.Mock()
requires_finalizer = mocker.Mock()
status_prefix = mocker.Mock()

with pytest.deprecated_call(match=r"use backoff="):
handler = ResourceChangingHandler(
Expand All @@ -70,6 +71,7 @@ def test_resource_handler_with_deprecated_cooldown_instead_of_backoff(mocker):
annotations=annotations,
when=when,
requires_finalizer=requires_finalizer,
status_prefix=status_prefix
)

assert handler.fn is fn
Expand All @@ -86,3 +88,4 @@ def test_resource_handler_with_deprecated_cooldown_instead_of_backoff(mocker):
assert handler.annotations is annotations
assert handler.when is when
assert handler.requires_finalizer is requires_finalizer
assert handler.status_prefix is status_prefix
12 changes: 9 additions & 3 deletions tests/handling/test_activity_triggering.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from typing import Mapping
from unittest.mock import Mock

import freezegun
import pytest
Expand All @@ -11,10 +12,15 @@
from kopf.structs.handlers import Activity, ActivityHandler, HandlerId


def test_activity_error_exception():
outcome = HandlerOutcome(final=True)
@pytest.fixture()
def handler():
return Mock(id=HandlerId('id'), spec_set=['id'])


def test_activity_error_exception(handler):
outcome = HandlerOutcome(final=True, handler=handler)
outcomes: Mapping[HandlerId, HandlerOutcome]
outcomes = {HandlerId('id'): outcome}
outcomes = {handler.id: outcome}
error = ActivityError("message", outcomes=outcomes)
assert str(error) == "message"
assert error.outcomes == outcomes
Expand Down
Loading