search

konpyutaika / nifikop

The NiFiKop NiFi Kubernetes operator makes it easy to run Apache NiFi on Kubernetes. Apache NiFI is a free, open-source solution that support powerful and scalable directed graphs of data routing, transformation, and system mediation logic.
https://konpyutaika.github.io/nifikop/
Apache License 2.0
123 stars 40 forks source link

Replace Update by Patch on K8S resource #333

Closed juldrixx closed 7 months ago

juldrixx commented 7 months ago
Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets fixes #X, partially #Y, mentioned in #Z
License Apache 2.0

What's in this PR?

Changes to replace Update instructions by Patch instructions on K8S resources.

Why?

To prevent errors due to simultaneous updates.

Checklist

mh013370 commented 7 months ago

Should we patch everywhere instead of updating to avoid these errors? If so, should we consider cases where nifikop updates k8s resources?

https://github.com/konpyutaika/nifikop/blob/master/pkg/k8sutil/resource.go#L111

juldrixx commented 7 months ago

Should we patch everywhere instead of updating to avoid these errors? If so, should we consider cases where nifikop updates k8s resources?

https://github.com/konpyutaika/nifikop/blob/master/pkg/k8sutil/resource.go#L111

Not necessarily, I don't think that is necessary to patch instead of update the resources owned by NiFiKop and only manipulated by NiFiKop. The issue with the CRD, it's that they can be modified by pretty much everything like a chart HELM, Keda or even NiFiKop itself by another controller. So my changes are aimed at getting controllers to patch their resources so they won't be affected by the rest. But maybe, I'm wrong and I'm missing something 🤔.

And the work to change the Update to Patch can be heavy because you need to pass the patcher to it. But again maybe, the implementation I did is not the most effecient one 🤔. So if you see a way to do it more easly, let me know, I'm interested 😁.

mh013370 commented 7 months ago

Should we patch everywhere instead of updating to avoid these errors? If so, should we consider cases where nifikop updates k8s resources? https://github.com/konpyutaika/nifikop/blob/master/pkg/k8sutil/resource.go#L111

Not necessarily, I don't think that is necessary to patch instead of update the resources owned by NiFiKop and only manipulated by NiFiKop. The issue with the CRD, it's that they can be modified by pretty much everything like a chart HELM, Keda or even NiFiKop itself by another controller. So my changes are aimed at getting controllers to patch their resources so they won't be affected by the rest. But maybe, I'm wrong and I'm missing something 🤔.

And the work to change the Update to Patch can be heavy because you need to pass the patcher to it. But again maybe, the implementation I did is not the most effecient one 🤔. So if you see a way to do it more easly, let me know, I'm interested 😁.

That's reasonable. The resources nifikop creates/manages wouldn't also be managed by ArgoCD, Helm CLI, etc.

I can't think of a better way, to be honest. You need to capture the patch from the original object and carry it through until you apply the patch so that does make code changes fairly significant.

mh013370 commented 7 months ago

LGTM! Merge at your leisure