Skip to content

feat: cache desired state #2831

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: cache desired state #2831

wants to merge 1 commit into from

Conversation

metacosm
Copy link
Collaborator

Signed-off-by: Chris Laprun claprun@redhat.com

@metacosm metacosm self-assigned this Jun 13, 2025
@metacosm metacosm requested review from csviri and xstefank June 13, 2025 19:51
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2025
Signed-off-by: Chris Laprun <claprun@redhat.com>
@csviri
Copy link
Collaborator

csviri commented Jun 16, 2025

@metacosm what is the use case for this? Desired pojos are usually extremely quick to create, since values comes from caches.

@metacosm
Copy link
Collaborator Author

@metacosm what is the use case for this? Desired pojos are usually extremely quick to create, since values comes from caches.

This isn't always true, though, in particular when the desired state needs to deal with external systems. Another aspect to this, also when dealing with external systems, is that it can be difficult to ensure idempotency of the desired result, therefore repeated calls to desired might lead to errors or even complete failure of the operator. Note that this use case has been exacerbated since v5.

@csviri
Copy link
Collaborator

csviri commented Jun 16, 2025

This isn't always true, though, in particular when the desired state needs to deal with external systems. Another aspect to this, also when dealing with external systems, is that it can be difficult to ensure idempotency of the desired result, therefore repeated calls to desired might lead to errors or even complete failure of the operator. Note that this use case has been exacerbated since v5

External cache states should be cached in event sources. That is the pattern by default.

therefore repeated calls to desired might lead to errors or even complete failure of the operator.

Yes, but this is the cases with all our caches, and in general is not handled neither in go.
Users can always read the cached resource at the beggining and pass the resource in the context as workaround.

@metacosm
Copy link
Collaborator Author

This isn't always true, though, in particular when the desired state needs to deal with external systems. Another aspect to this, also when dealing with external systems, is that it can be difficult to ensure idempotency of the desired result, therefore repeated calls to desired might lead to errors or even complete failure of the operator. Note that this use case has been exacerbated since v5

External cache states should be cached in event sources. That is the pattern by default.

I'm not sure I follow you. There is no pattern for this in JOSDK since there are no specific event sources for external resources.

therefore repeated calls to desired might lead to errors or even complete failure of the operator.

Yes, but this is the cases with all our caches, and in general is not handled neither in go. Users can always read the cached resource at the beggining and pass the resource in the context as workaround.

That's actually a good point and that's something JOSDK should probably do out of the box: the desired state should be put in the context once it's been computed once and should be retrieved from there all the time instead of being recomputed.

@csviri
Copy link
Collaborator

csviri commented Jun 17, 2025

That's actually a good point and that's something JOSDK should probably do out of the box: the desired state should be put in the context once it's been computed once and should be retrieved from there all the time instead of being recomputed.

Why is it recomputed?

@csviri
Copy link
Collaborator

csviri commented Jun 17, 2025

I'm not sure I follow you. There is no pattern for this in JOSDK since there are no specific event sources for external resources.

The PollingEventSource, PerResourcePollingEventSource, CachingInboundEventSource are for this purpose.

@metacosm
Copy link
Collaborator Author

That's actually a good point and that's something JOSDK should probably do out of the box: the desired state should be put in the context once it's been computed once and should be retrieved from there all the time instead of being recomputed.

Why is it recomputed?

getSecondaryResources calls desired again, match can as well depending on the situation.

@metacosm
Copy link
Collaborator Author

I'm not sure I follow you. There is no pattern for this in JOSDK since there are no specific event sources for external resources.

The PollingEventSource, PerResourcePollingEventSource, CachingInboundEventSource are for this purpose.

We don't have examples for how to use this in the context of dependents (not even sure how I would do it, I haven't looked), is what I mean. The context for this is that the desired method is actually from a kube dependent that requires an external token coming from a 3rd party API, so there's no obvious event source in that case (or, at least, it's not immediately obvious since an informer is configured for the secret, of course, but that happens automatically).

@csviri
Copy link
Collaborator

csviri commented Jun 17, 2025

I'm not sure I follow you. There is no pattern for this in JOSDK since there are no specific event sources for external resources.

The PollingEventSource, PerResourcePollingEventSource, CachingInboundEventSource are for this purpose.

We don't have examples for how to use this in the context of dependents (not even sure how I would do it, I haven't looked), is what I mean. The context for this is that the desired method is actually from a kube dependent that requires an external token coming from a 3rd party API, so there's no obvious event source in that case (or, at least, it's not immediately obvious since an informer is configured for the secret, of course, but that happens automatically).

@Override
public Set<Schema> fetchResources(MySQLSchema primaryResource) {
try (Connection connection = getConnection()) {
var schema =
SchemaService.getSchema(connection, primaryResource.getMetadata().getName())
.map(Set::of)
.orElseGet(Collections::emptySet);
log.debug("Fetched schema: {}", schema);
return schema;
} catch (SQLException e) {
throw new RuntimeException("Error while trying read Schema", e);
}
}

In my sql we show how to use, the fetch method is generic, user can fill in its logic. Also you can use arbitrary event source referenced by name from DR.
This is maybe rather an issue of missing documentation / guides. I'm pretty sure this use case can be covered by the current components.

@metacosm
Copy link
Collaborator Author

metacosm commented Jun 17, 2025

In my sql we show how to use, the fetch method is generic, user can fill in its logic. Also you can use arbitrary event source referenced by name from DR. This is maybe rather an issue of missing documentation / guides. I'm pretty sure this use case can be covered by the current components.

This isn't the use case I'm talking about, though. In the case of the Schema example, the dependent itself is external. The use case I'm talking about and that isn't covered is where a dependent is a kubernetes dependent (in this case a Secret) so extends KubernetesDependentResource but needs to access a 3rd party service to compute its desired state. The event source, in this case, is the Secret informer, not a polling event source so the Schema example is irrelevant.

@csviri
Copy link
Collaborator

csviri commented Jun 17, 2025

The use case I'm talking about and that isn't covered is where a dependent is a kubernetes dependent (in this case a Secret) so extends KubernetesDependentResource but needs to access a 3rd party service to compute its desired state. The event source, in this case, is the Secret informer, not a polling event source so the Schema example is irrelevant.

I see, but in this case it is not just a an additional event source for that 3rd party service to fetch required information? (then user can access it from desired method through context)

@@ -33,6 +33,7 @@ public abstract class AbstractDependentResource<R, P extends HasMetadata>
private final boolean updatable = this instanceof Updater;
private final boolean deletable = this instanceof Deleter;
private final DependentResourceReconciler<R, P> dependentResourceReconciler;
private final ThreadLocal<R> desiredCache = new ThreadLocal<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if using ThreadLocal is the best for this purpose, if some later call the dependent from reconcile method it will happen on different thread so desired won't be available

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure either if that's the proper way to do it…

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants