feat: record desired state in Context

[metacosm]

Signed-off-by: Chris Laprun [email protected]

[csviri]

I think we should agree that on these optimization issues we should see the numbers, so we can decide if it is worth it.
First of all on large resources this will have a huge memory overhead.
So we need to see, how faster is it (if it is faster at all), and how much more memory we consume.

But as we I think discussed before, there should be no blocking operation in desired state computation, that goes agains the pattern, calculating the desired (thus creating a pojo) is extremely fast - might be faster than getting a resource from a hash map (we need to measure), we should think though what we are actually optimizing with this.

[metacosm]

Agreed but it might also be a good idea to look at the different cache layers which are becoming too complex to properly maintain.

[csviri]

Note that without this:
#2893
if the reconciler does not implement cleaner, there is no place where to clean those caches, since we don't receive the delete event now, when it should happen.

[metacosm]

Thinking about this some more, the memory usage should actually be minimal and there is no need to clean things because this isn't a new cache per se as the desired states are put in the Context object which doesn't survive past the reconciliation so these are short-lived versions of objects that are already computed so it shouldn't incur any additional memory than what is already used during the reconciliation.

[csviri]

Thinking about this some more, the memory usage should actually be minimal and there is no need to clean things because this isn't a new cache per se as the desired states are put in the Context object which doesn't survive past the reconciliation so these are short-lived versions of objects that are already computed so it shouldn't incur any additional memory than what is already used during the reconciliation.

if it does not survive the reconiliation, why to cache it?
Now it does no use the context by a HashMap

[metacosm]

Thinking about this some more, the memory usage should actually be minimal and there is no need to clean things because this isn't a new cache per se as the desired states are put in the Context object which doesn't survive past the reconciliation so these are short-lived versions of objects that are already computed so it shouldn't incur any additional memory than what is already used during the reconciliation.

if it does not survive the reconiliation, why to cache it?
To avoid recomputing a potentially costly and non-idempotent desired state, which is something that people have run into several times already.

[csviri]

To avoid recomputing a potentially costly and non-idempotent desired state, which is something that people have run into several times already.

In what use case is the desired state cumputation costy?
Could you pls link the issue here, so we have better clarity?
All that information should come from EventSource caches.

[csviri]

TBH I don't like the fact that we are calling the desired either - and by definition idempotent function. But I'm not convinced that this is the way to solving or should be solved in generic way, users are able to optimize that in some edge cases already. But this is not a strong opinion, so if we are able to solve this elegantly withing the Context I'm open for that.

also maybe there is something I just don't see, so happy to dicuss further. Would be nice to see the users specific issue.

[csviri]

This probably should be removed.

[metacosm]

To avoid recomputing a potentially costly and non-idempotent desired state, which is something that people have run into several times already.

In what use case is the desired state cumputation costy? Could you pls link the issue here, so we have better clarity? All that information should come from EventSource caches.

I don't have specific issues on hand but it's not unexpected that the desired state could be costly to compute if it requires accessing external systems. There's also the expectation since the beginning of the dependent resource feature that the desired method is only called once during the reconciliation process as it was the case until recently. Here are some issues related to this problem:
#2895
#1758

[csviri]

To avoid recomputing a potentially costly and non-idempotent desired state, which is something that people have run into several times already.

In what use case is the desired state cumputation costy? Could you pls link the issue here, so we have better clarity? All that information should come from EventSource caches.

I don't have specific issues on hand but it's not unexpected that the desired state could be costly to compute if it requires accessing external systems. There's also the expectation since the beginning of the dependent resource feature that the desired method is only called once during the reconciliation process as it was the case until recently. Here are some issues related to this problem: #2895 #1758

makes sense, thank you! yeah if this can be solved elegantly with such caches, might be a nice improvement

[metacosm]

Can we merge this, @csviri?

[csviri]

Why are we leaking this into an explicit construct?
Shall this be part rather of ManagedWorkflowAndDependentResourceContext ?

[csviri]

Could we reuse only the already existing map there:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedWorkflowAndDependentResourceContext.java

Line 48 in 0dee1bf

<T> T put(Object key, T value);

[metacosm]

Why are we leaking this into an explicit construct? Shall this be part rather of ManagedWorkflowAndDependentResourceContext ?

It could indeed make sense there even though this isn't specific to managed dependents.

[metacosm]

Could we reuse only the already existing map there:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedWorkflowAndDependentResourceContext.java

Line 48 in 0dee1bf

<T> T put(Object key, T value);

Sure but that'd be slightly more complex to handle and less type-safe.

[csviri]

Can we merge this, @csviri?

added ine comment, sorry I though for some reason this is WIP

[csviri]

I would also target next since this is a feature not a bugfix.

[csviri]

Could we also add some integration test, or which test should cover this functionality?

[metacosm]

Could we also add some integration test, or which test should cover this functionality?

I'll look into it but basically the test should be that desired is only called once per reconciliation.

[csviri]

Could we also add some integration test, or which test should cover this functionality?

I'll look into it but basically the test should be that desired is only called once per reconciliation.

yeah, if you think unit tests are enough, it is fine by me also. thx!

[metacosm]

I would also target next since this is a feature not a bugfix.

I disagree: this is a bug fix because the documentation states, and it was the case in v4, that desired should only be called once per reconciliation.

[csviri]

I would also target next since this is a feature not a bugfix.

I disagree: this is a bug fix because the documentation states, and it was the case in v4, that desired should only be called once per reconciliation.

I don't see where does it states in the documentation. But it was obvious from the changes for v5 that we can call this multiple times. So it is then more of a documentation problem. Note that that was a major version change, so changes is behavior are fine, and we released also a minor version since then, therefore I believe would be a nicer to have this change in the behavior in minor verison update, even if such behavior existed before.

[metacosm]

I would also target next since this is a feature not a bugfix.

I disagree: this is a bug fix because the documentation states, and it was the case in v4, that desired should only be called once per reconciliation.

I don't see where does it states in the documentation.

I guess it's not stated directly but the state diagram implies that the desired state is only computed once.

But it was obvious from the changes for v5 that we can call this multiple times.

Where? That's definitely not true as users keep getting confused by the fact that it can be called multiple times.

[csviri]

Where? That's definitely not true as users keep getting confused by the fact that it can be called multiple times.

Which users? Could you please link the related issue here?

I guess it's not stated directly but the state diagram implies that the desired state is only computed once.

But it was obvious from the changes for v5 that we can call this multiple times.

And don't see why that diagram implies that, but mainly we explicitly state that in the docs that what users should do when to want to avoid calling desired multiple times:

https://javaoperatorsdk.io/docs/documentation/dependent-resource-and-workflows/dependent-resources/#multiple-dependent-resources-of-same-type

There is nothing wrong calling that multiple times, since it is inherently idempotent, and as we dicussed before should not be a blocking operation, we might want to also articulate in the docs. So I see this more of problem from user side we should point him to the docs, but we could also improve the documentation and/or also write a guide about it. We can add it to the FAQ.

I also created to PR that improves the docs by adding pointers and adding similar capability that we have for KubernetesDependetResources for ExternalDependentResources, that is a good point that we should have it also there.
#3000

But in any means since there were further improvements around this please target next branch.

[csviri]

But I see this as a good improvement, we can get rid of that mechanism, pls target next and also update the documentation.