kopf icon indicating copy to clipboard operation
kopf copied to clipboard

Patch inconsistencies on external CRDs

Open Cerebus opened this issue 4 years ago • 3 comments

Keywords

inconsistencies, results delivery

Problem

I need my controller to watch and update CRs that are external to my controller, so I can't add x-kubernetes-preserve-unknown-fields (even by patching, b/c the CRD is managed by an operator, so changes I make get reverted).

I'm careful to name the method after permitted status.{fieldname} values, and it works fine--unless I want to have one handler per field.

E.g., cert-manager.io/v1 CertificateRequest allows status.ca and status.certificate. If I create an on.create handler for one, I'm good. On the second, I get patch inconsistency warnings.

I have a possibly related problem: I want to communicate cross-resource between an external CR and my own. I create an index for my resource using the external CR as the key. I'm using a when= callback in on.create on the external CR. The callback checks the index for itself. This handler never fires. AFAICT, the callback fires once, returns False, and is never called again.

Cerebus avatar Nov 23 '21 01:11 Cerebus

Hello.

The filtering callback should be called on every new event that arrives from the Kubernetes watch-stream — because it is implied that the arbitrary condition can change any time (it can even be a random() call). However, if Kubernetes sends no events, there is nothing to filter.

I want to communicate cross-resource between an external CR and my own. I create an index for my resource using the external CR as the key.

Sorry, I didn't get it here. A code snippet would be helpful to understand the operator setup.

I'm careful to name the method after permitted status.{fieldname} values

Just hinting (based on what I bravely assumed from the description):

First, there is the id="..." option to all decorators. This overrides the function names as the default id. However, keep the id unique across handlers. And it will not help with the field-handlers — they will be suffixed.

Second, instead of return'ing the values, you can do patch.status["ca"] = "blahblahblah" — if you need to match the fields precisely. Where patch is one of the kwargs of the handlers.

nolar avatar Nov 23 '21 20:11 nolar

In re: the first problem:

So, from the docs, two handlers firing on the same event yields patch inconsistency warnings unless x-kubernetes-preserve-unknown-fields is set in the associated status field. This doesn't make sense to me since the inconsistency being warned about is the handler status update in the annotation:

[2021-11-25 09:14:49,177] kopf.objects         [INFO    ] [default/test-cert-cluster-hjtf4] Handler 'ca' succeeded.
[2021-11-25 09:14:49,311] kopf.objects         [WARNING ] [default/test-cert-cluster-hjtf4] Patching failed with inconsistencies: (('remove', ('status', 'kopf'), {'progress': {'ca': {'started': '2021-11-25T15:14:49.175505', 'stopped': '2021-11-25T15:14:49.177690', 'delayed': None, 'purpose': 'create', 'retries': 1, 'success': True, 'failure': False, 'message': None, 'subrefs': None}, 'certificate': {'started': '2021-11-25T15:14:49.175512', 'stopped': None, 'delayed': None, 'purpose': 'create', 'retries': 0, 'success': False, 'failure': False, 'message': None, 'subrefs': None}}}, None),)
[2021-11-25 09:14:49,932] kopf.objects         [INFO    ] [default/test-cert-cluster-hjtf4] Handler 'certificate' succeeded.
[2021-11-25 09:14:49,934] kopf.objects         [INFO    ] [default/test-cert-cluster-hjtf4] Creation is processed: 2 succeeded; 0 failed.

It's common for controllers to watch and update the status sub-resources of other API groups; that's how controllers communicate. The workaround of x-kubernetes-preserve-unknown-fields isn't an option in a lot of cases. As long as the handler method names align with the foreign group's CRD, the kopf user expectation is this should work. And it does, but it throws a false warning.

OTOH, using patch in place of discrete on.event handlers for each status field is IMHO a far less elegant solution. Results delivery directly into the status sub-resource allows for better separation of concerns, shorter methods, and simpler testing. Having handlers update more than one thing is something to be avoided if possible.

Since the handlers are succeeding, I'm inclined to simply ignore it, but that's a pretty unsatisfying approach.

Cerebus avatar Nov 25 '21 15:11 Cerebus

In re: the second problem:

What I was trying to do is bubble up a status from a child resource to a parent resource without resorting to kubernetes.client calls.

What I misunderstood is that the on.create event is finished (and the handler was filtered out by the callback) by the time the index is updated (which would allow the callback to pass). This kind of child to parent resource flow requires an on.timer handler, or directly patching the parent (by reading metadata.ownerReferences).

If you're looking for future features, a convenience method to trigger an update event on a parent resource would be very helpful. I've seen the discussion about using an annotation update for this, but it might be a common enough use case to formalize it in kopf.

For my part, after a rethink how the RFC I'm implementing works, I didn't really need the child resources. So this problem went away.

Cerebus avatar Nov 25 '21 15:11 Cerebus