Skip to content

refactor: optimize retrieval of secondary resource #2915

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

refactor: optimize retrieval of secondary resource #2915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion ...main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ protected ReconcileResult<R> reconcile(P primary, R actualResource, Context<P> c

@Override
public Optional<R> getSecondaryResource(P primary, Context<P> context) {

var secondaryResources = context.getSecondaryResources(resourceType());
if (secondaryResources.isEmpty()) {
return Optional.empty();
Expand Down
16 changes: 3 additions & 13 deletions ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -264,17 +262,9 @@ protected void addSecondaryToPrimaryMapperAnnotations(
}

@Override
protected Optional<R> selectTargetSecondaryResource(
Set<R> secondaryResources, P primary, Context<P> context) {
ResourceID managedResourceID = targetSecondaryResourceID(primary, context);
return secondaryResources.stream()
.filter(
r ->
r.getMetadata().getName().equals(managedResourceID.getName())
&& Objects.equals(
r.getMetadata().getNamespace(),
managedResourceID.getNamespace().orElse(null)))
.findFirst();
public Optional<R> getSecondaryResource(P primary, Context<P> context) {
final var targetResourceID = targetSecondaryResourceID(primary, context);
Copy link
Collaborator

@csviri csviri Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bit of a change in the behavior, since it skips the owner mechanism that we have with SecondaryToPrimary mappers and related indexing, I rather would not change that. The failing test is a real problem actually, since with current way we are deteciting that the resource is actually related (a.k.a. secondary to) primary resource.

IMO it is also by definition better to use the stanrd appraoch that user would use without dependen resources thus calling the getSecondaryResource.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This indexing should occur regardless of how secondary resources are retrieved. This new way is much more efficient as it doesn't require to iterate over all the resources and filter them. For the purposes of retrieval, nothing should occur apart from identifying the target resource and then retrieving it from the informer cache. If processing was previously done during that phase, then I would think that the design needs to be revised so that it occurs at some other time, not during secondary retrieval.

Another aspect is that it should be possible for users to override this method easily without breaking anything.

Regarding the failing test, I don't understand your point and I actually think that the test is wrong, as discussed separately.

Copy link
Collaborator

@csviri csviri Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There are multiple aspects of this:

  1. Practical: the change from this PR would simply not work in this scenario: the config map. (as in the test) is created prior the dependent resource is reconciled. From the PR the logic would recognize that the config map is there so won't create / update it. Now we delete the primary resource, since we did not create/update the resource the owner reference it not added, so the config map is not deleted. This is inconsistent with the current and desireg behavior, so simply wrong.

  2. The semantical aspect: we added createifNotExists flag, especially to cover this situation, that we don't override the resource based on desired if we did not create it. Again the devil is in details, we did not create it since there is not owner reference (in other words the resource is not related to primary), so based on this flag we don't want to override it. This is what the failing test is testing, and it is correct.

  3. performance:
    For exmaple on failing test this aspect does not give any performance gain, we receiving the target resource id, from the primaryToSecondary index in constant time, and getting it from cache directly, this is about the same performance. Note that we usually getting a constant number of resources, looping through is also constant, so from performance perspective of a single dependent is negligible. If you compare the execution time with http call there will be a factor of million.
    In addition to that user can simply provide a PrimaryToSeconday mapper, and achive the same result, with a built in appraoch. Not saying that would be much better, just that there is also that option.

So in general, I think this PR should not be merged, since simply breaks the logic point (1.). Would require to change the actual logic for (2.).
We can further discuss performance gains further in case we solve (1,2), but those are really quastinable also in practice / real world examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are multiple aspects of this:

1. Practical: the change from this PR would simply not work in this scenario: the config map. (as in the test) is created prior the dependent resource is reconciled. From the PR the logic would recognize that the config map is there so won't create / update it. Now we delete the primary resource, since we did not create/update the resource the owner reference it not added, so the config map is not deleted. This is inconsistent with the current and desireg behavior, so simply wrong.

The scenario as tested by the test is wrong, though. The ConfigMap exists so it shouldn't be created, that's understood. However, the ConfigMapDependentResource defines a desired state which doesn't match the existing CM. So, according to the proper behavior, the CM should be updated. With the current implementation, it is not and that is a bug. The test is wrong and actually shows that the current implementation is not doing what's expected in this instance.

2. The semantical aspect:  we added createifNotExists flag, especially to cover this situation, that we don't override the resource based on desired if we did not create it. Again the devil is in details, we did not create it since there is not owner reference (in other words the resource is not related to primary), so based on this flag we don't want to override it. This is what the failing test is testing, and it is correct.

I don't agree with this. The desired method should prevail in all instances since it encapsulates what the operator thinks the proper state is. If an existing state is to be preserved, then the desired method should take this into account, not a flag that breaks the rest of the behavior. Or this resource shouldn't be a dependent resource if a desired state cannot appropriately be computed from the primary resource. I guess I should have paid more attention when that flag was added because I think it is wrong and goes against the rest of the design, especially with it being true by default. Not only that but the name of the constant and the flag do not appropriately convey what will happen.

3. performance:
   For exmaple on failing test this aspect does not give any performance gain, we receiving the target resource id, from the primaryToSecondary index in constant time, and getting it from cache directly, this is about the same performance. Note that we usually getting a constant number of resources, looping through is also constant, so from performance perspective of a single dependent is negligible. If you compare the execution time with http call there will be a factor of million.

Performance matter everywhere. Indeed, most of our processing will be faster than a network call but is that a valid excuse to have inefficient code? Also, all those little things add up. In any case, the core of the issue is not so much performance than the facts that:

  1. users should be able to override this public method without breaking things as was previously the case
  2. the flag that supposedly deals with SSA creation of resources really doesn't and is wrong with the rest of the framework design. Either it needs to be removed, set to false by default and/or properly justified with a valid use case. Again, if the desired state cannot be computed from a primary resource then the associated resource should NOT be a dependent resource, that's the base of the dependent resource feature.

So in general, I think this PR should not be merged, since simply breaks the logic point (1.). Would require to change the actual logic for (2.). We can further discuss performance gains further in case we solve (1,2), but those are really quastinable also in practice / real world examples.

I argue that the logic exhibited in the breaking test is wrong and should be reverted and the associated flag removed as such use cases should not be modeled as dependent resources. Adding that flag was a mistake we need to fix.

Copy link
Collaborator

@csviri csviri Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is the matter of definition of creator that is the reason of that flag. Since you can define it either:

  1. creator is always the one creates the resource
  2. creates creates the resource if not exists (otherwaise can uptate)

To distinguise the two bahavior we added the flag, so we cover all the use cases.

Copy link
Collaborator

@csviri csviri Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Going further into the details. Kubernetes defines a flag controller on owner references,
see: https://stackoverflow.com/a/65825463/737591
So, for example, there would be a resource with an owner reference where controller is true, we should not update the spec / resource - this we don't handle explicitly yet in DR (probably we should even for case (2) - will create an issue for that to further discuss.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know about the controller field but

  1. it's not completely implemented / supported in Kubernetes
  2. this isn't what the flag does at all

Imo, the proper way to support this would be to throw an exception in that case because the desired state cannot be satisfied if the target resource is "controlled" by a different controller as our controller will never be able to match the desired state. The current behavior is improper because it leads the controller to believe that the resource is in the proper state when it's not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what do you mean by it's not completely implemented / supported in Kubernetes ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean by it's not completely implemented / supported in Kubernetes ?

At least according to https://github.com/kubernetes/design-proposals-archive/blob/acc25e14ca83dfda4f66d8cb1f1b491f26e78ffe/api-machinery/controller-ref.md#implementation
Also, the official documentation doesn't really provide semantics for that field (at least, not that I could find with a quick search)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure that you cannot add two owner references with controller: true flag. Maybe something to try out.

return eventSource().flatMap(informer -> informer.get(targetResourceID));
}

/**
Expand Down
Loading