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

One handler function attached to 2 events causes 2 fields in status to be created instead of one? #571

Closed
Jc2k opened this issue Oct 29, 2020 · 5 comments
Labels
bug Something isn't working

Comments

@Jc2k
Copy link
Contributor

Jc2k commented Oct 29, 2020

Long story short

If i decorate a function with 2 different @kopf.on handlers they seem to update different fields in status.
This means I can't have a kopf.on.create and kopf.on.field both update status.some_field, i'll actually end up with 2 fields instead.

I`m pretty sure this didn't use to happen, but i don't know if i "got lucky" before...

Description

Recently upgraded an older kopf based operator. It had something like this:

@kopf.on.create("foo.example.com", "v1", "bar")
@kopf.on.field("foo.example.com", "v1", "bar", field="spec.some_field")
def my_config_map(*, body, logger, **kwargs):
    return "some-resource-name-12345-abc123c"

It creates a resource (lets say a ConfigMap for the sake of argument).

Previously the status was set to:

{
  "status": {
    "my_config_map": "some-resource-name-12345-abc123c",
  }
}

(0.23ish)

But now im seeing something like

{
  "status": {
    "my_config_map": "some-resource-name-12345-abc123d",
    "my_config_map/spec.some_field": "some-resource-name-12345-abc123c",
  }
}

With 0.28.

I am not sure if this is a regression or i was just getting lucky before, so on the off chance its a regression I thought i should report it. If it's not a regression is there anyway to force them both to write to my_config_map? This PR looks like it would give me enough control to that, but otherwise i'm not sure how to proceed. zalando-incubator/kopf#320

Environment

  • Kopf version: 0.28
  • Kubernetes version: 1.19.2
  • Python version: 3.8
  • OS/platform: linux
@Jc2k Jc2k added the bug Something isn't working label Oct 29, 2020
@nolar
Copy link
Owner

nolar commented Oct 29, 2020

Hello.

The described behaviour seems to be as expected. The handlers write their functions' results into .status.{handler_id}. These two handlers would always have different handler ids, one should always be suffixed with the field name. I see no reason why they should be putting the result into one field even in the past versions of Kopf.

And it is impossible to assign explicit ids to both handlers via id='...' option to the decorators. Though it is possible, but I am not sure what is the expected outcome and what are the side-effects of that — they will be conflicting for their own state.

One reason why you were lucky in the past, most likely, is that on-field handlers were an equivalent of on-update limited to a field context for some time. Later (in 0.28), they were fixed to be triggered really in the context of any field change — including the field's creation (a change from None to a value), both when the field is added and when the whole object is added.

Maybe duplicates #566; especially see the rationale in #566 (comment)


Anyway, storing the function results into .status is a leftover of the initial implementation years ago. It is going to stay there by default but it is [or will be] discouraged (but not deprecated!) — especially since K8s 1.16's "structural schemas" made the statuses practically unusable for arbitrary data storage, and Kopf itself had to migrate its state to annotations.

A better way here is to be explicit about your storage intentions:

@kopf.on.create("foo.example.com", "v1", "bar")
@kopf.on.field("foo.example.com", "v1", "bar", field="spec.some_field")
def my_config_map(*, body, logger, patch, **kwargs):
    #                              ^^^^^
    patch.status['my-field'] = "some-resource-name-12345-abc123c"
    #^^^^^^^^^^^^^^^^^^^^^^^

The patch kwarg (kopf.Patch type — a mutable dict with some helpers for "typical" top-level fields) is intentionally fed into the handlers to let them populate the cumulative patch of the object to be applied at the end of the cycle.

In that case, the object will be patched twice — one time for each handler — but at least it will patch the same field.

Alternatively, you can reconsider the usage of the on-field handler as suggested in the comment linked above, essentially with this trick:

@kopf.on.field(...
               when=lambda cause, **_: cause.reason != kopf.Reason.CREATE)

@Jc2k
Copy link
Contributor Author

Jc2k commented Oct 29, 2020

Ah brilliant, i completely missed patch was available. That's much better for my needs. And that trick is very useful too!

I think for my case it's actually appropriate to drop the .on.create, and not filter with .on.field. i.e.

@kopf.on.field("foo.example.com", "v1", "bar", field="spec.some_field")
def my_config_map(*, body, logger, patch, **kwargs):
    patch.status['my-field'] = "some-resource-name-12345-abc123c"

If i have understood correctly, this handler will get called when the field first appears (likely when the resource is created) as well as when it changes. So having a seperate create handler is not required.

To clarify one more thing, if i return None from my handler (or rather, i don't return anything), will it create a None in the status field? Or nothing? I.e. in the example above would i get:

{
  "my-field": "some-resource-name-12345-abc123c",
  "my_config_map": None,
}

or

{
  "my-field": "some-resource-name-12345-abc123c",
}

@nolar
Copy link
Owner

nolar commented Oct 29, 2020

Regarding Nones:

No, Nones are ignored. Otherwise, the patches would be polluted way too much with the values that will never be stored (patching with None (python) or null (json) is interpreted as the deletion — on the K8s API level, not in Kopf). Here is Kopf's logic: https://github.com/nolar/kopf/blob/0.28/kopf/storage/states.py#L301-L309

A side-note: patch.setdefault('status', {}) is also a leftover from when there was no patch.status magic property; please ignore and use patch.status — it looks better, does the same.

@nolar
Copy link
Owner

nolar commented Oct 29, 2020

Regarding on-field handlers:

Yes, exactly! Indeed, the only handler would be a better solution here, if the field is the only interest.

Please still be aware that the differences between object creation and putting a field on the existing objects are almost invisible for the handler — in both cases, it is a change of the field from None to a value. You only can distinguish with cause.reason (cause is also an official kwarg of Kopf's API).


I am still not sure how to deal with the object deletion. Regardless of the implementation, but on a sematic level: Should it be a change of the field from a "value" to None (as if the field is deleted)? Or should it be ignored? — Because if the object disappears, it disappears as a whole, not in parts. Currently, the object disappears silently without informing the on-field handlers (the 2nd option) — this will remain the default anyway; but this is asymmetric with the object creation.

These on-field handlers add too much ambiguity on the sematic level — not for the first time. This is why I am so much in favour of usual on-create/update/delete/resume handlers with field= option instead — that would resolve all these semantic uncertainties. But this is a problem for later, not for now.

@Jc2k
Copy link
Contributor Author

Jc2k commented Oct 30, 2020

I have adopted your suggested approach and it works great. Thanks for your help once again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants