Skip to content

Managed BulkDependentResource fails with reconcilePrecondition #1687

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.

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

Closed
grawinkel opened this issue Dec 30, 2022 · 4 comments · Fixed by #1688
Closed

Managed BulkDependentResource fails with reconcilePrecondition #1687

grawinkel opened this issue Dec 30, 2022 · 4 comments · Fixed by #1688
Assignees
Milestone

Comments

@grawinkel
Copy link

Bug Report

On Operator SDK 4.2.0 (commit: 061db9c):

I want to implement a Resource

public class SomeResourceDependentResource extends CRUDKubernetesDependentResource<X, Y>
    implements BulkDependentResource<X, Y>
{
... desiredResources(...)
... getSecondaryResources(...)
}

And use it as a Managed Resource with a reconcilePrecondition

@ControllerConfiguration(
    dependents = {
        @Dependent(
            name = "SomeResource",
            type = SomeResourceDependentResource.class
            reconcilePrecondition = SomePrecondition.class
            ),
    })

As soon as the reconcilePrecondition is configured, the SomeResourceDependentResource fails, since it is not handled as a BulkDependentResource but as a normal DependentReousource. So the getSecondaryResource() is called, instead the getSecondaryResources(), which leads to errors.

What did you expect to see?

Being able to use reconcilePrecondition with BulkDependentResource.

@csviri csviri self-assigned this Jan 2, 2023
@csviri
Copy link
Collaborator

csviri commented Jan 2, 2023

Thanks @grawinkel for reporting this, will take a look.

@csviri csviri added this to the 4.3 milestone Jan 2, 2023
@csviri
Copy link
Collaborator

csviri commented Jan 2, 2023

On Operator SDK 4.2.0 (commit: 061db9c):

I can't see how this is related tbh.

@csviri
Copy link
Collaborator

csviri commented Jan 2, 2023

The problem is with condition API, at some point we added there the resource created by the dependent resource, that is clearly not fitting in this case.

https://github.com/java-operator-sdk/java-operator-sdk/blob/d83b9f817fe79c81b4601c41401058b57492b41e/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java#L20-L20

So we probably need to pass here the dependent resource not the resource. So then the user will can decide, which method to call. Will create a issue for that to discuss.

For now it can be handled in the PR, so the resource in the condition will be a Map of resources:

public class SamplePrecondition
    implements Condition<Map<String, ConfigMap>, BulkDependentTestCustomResource> {

  @Override
  public boolean isMet(BulkDependentTestCustomResource primary, Map<String, ConfigMap> secondary,
      Context<BulkDependentTestCustomResource> context) {
    // ...
  }
}

But I see this more like a workaround. The API would be nicer with the dependendent resource as a parameter.

@csviri
Copy link
Collaborator

csviri commented Jan 2, 2023

created issue: #1689

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants