Conversation

metacosm

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

@metacosmmetacosm self-assigned this Jun 13, 2025
@metacosmmetacosm requested review from csviri and xstefank June 13, 2025 19:51
@openshift-ciopenshift-ci bot added the do-not-merge/work-in-progressIndicates that a PR should not merge because it is a work in progress.label Jun 13, 2025
Signed-off-by: Chris Laprun <[email protected]>
@csviri

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

@metacosm

@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

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

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

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

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

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

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

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

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

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…

Sign up for free to join this conversation on . Already have an account? Sign in to comment
do-not-merge/work-in-progressIndicates that a PR should not merge because it is a work in progress.
None yet

Successfully merging this pull request may close these issues.