operator-framework / java-operator-sdk

Java SDK for building Kubernetes Operators
https://javaoperatorsdk.io/
Apache License 2.0
797 stars 215 forks source link

Remove `ResourceDiscriminators` (legacy approach) #2253

Closed csviri closed 7 months ago

csviri commented 8 months ago

With this PR: https://github.com/operator-framework/java-operator-sdk/pull/2252

we can handle this quite elegantly without the resource discriminators, is there are use case (also for external resources) when we need this former approach anymore? If we cannot think we should consider removing this for v5

csviri commented 8 months ago

The only case with is not handled now elegantly is the read-only dependent resource since that does not have the desired state by default. There are multiple choices regarding this:

metacosm commented 8 months ago

I think we should keep them for optimization purposes as well. If the desired state is costly to compute and there are better ways to decided which secondary resource needs to be used, I think it still makes sense to use a discriminator even if it shouldn't be needed in most cases. That said, we could indeed tell people to override getSecondaryResource in that situation.

csviri commented 8 months ago

indeed tell people to override getSecondaryResource in that situation

exactly, basically resource discriminator puts nothing on table now, as we discussed before the only reason to create was to have a class representing the concept explicitly, so it is settable explicitly on a resource. But it's API is basically just the context:

Optional<R> distinguish(Class<R> resource, P primary, Context<P> context);

On the other hand desired needs to be always computed, except in the read-only (what is already kinda exotic case https://javaoperatorsdk.io/docs/dependent-resources#read-only-dependent-resources-vs-event-source )

But will examine the problem more in depth.

metacosm commented 8 months ago

On the other hand desired needs to be always computed, except in the read-only (what is already kinda exotic case https://javaoperatorsdk.io/docs/dependent-resources#read-only-dependent-resources-vs-event-source )

Yes but if it's costly to compute, we should try to avoid computing it several times… maybe we should record the computed desired state(s) in the Context so that they are available for downstream usage once they have been computed once?

csviri commented 8 months ago

yes definitelly, that is something will be done even if the discriminators are not removed.

Javatar81 commented 8 months ago

After using JOSDK for quite some time now, I think we only need discriminators for dependents because dependents currently have no identifiers. This is why @csviri proposes to evaluate the resource that the desired method returns.

To overcome this problem of missing identifier, I've added the following method to all of my dependent classes public static String getName(P primary) where P is the primary CRD type. This allows me to derive the name from the name of the primary in a consistent way throughout my code, e.g. I call getName in every discriminator and I am able to find the Kubernetes via fabric8 client even without the context (e.g. useful for test cases).

You could force users to define the identifier upfront (e.g. by an abstract method ResourceID getId(P primary, Context<P> context) that has to be implemented). This has several benefits:

  1. You would have a way to determine the resource id even without calling desired
  2. You would not need discriminators because you can rely on the getId method to distinguish resources of the same type
  3. You could even enforce that the resource returned by desired always has the name/namespace returned by getId (today it is possible that I change the name of the resource between multiple calls of the desired method)