Skip to content
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

event source: use a predicate to select the custom resource for which a reconcile should be triggered #430

Closed
lburgazzoli opened this issue Jun 3, 2021 · 7 comments · Fixed by #454

Comments

@lburgazzoli
Copy link
Collaborator

If my understanding is correct, as today events triggers a reconcile on a CR matching by UID.

Sometimes is not possible to determine the UID because i.e. the reconcile should be triggered for an event outside kubernetes so it would be nice if events would be allowed to specify a predicate to filter out the CR for which a reconcile should ne triggered.

For backward compatibility an predicate based on UID can be created

Does this make sense ?
If it does I can work on some POC

@lburgazzoli lburgazzoli changed the title event source: use a predicate to select the custom resource for which a reconcile should be troggered event source: use a predicate to select the custom resource for which a reconcile should be triggered Jun 3, 2021
@metacosm
Copy link
Collaborator

metacosm commented Jun 8, 2021

Yes, it makes sense to me. Looking forward to your PoC! 😄

@lburgazzoli
Copy link
Collaborator Author

lburgazzoli commented Jun 13, 2021

I've been looking at the code to implement a POC and I've added a method like the one below to the CustoimResourceCache class:

public class CustoimResourceCache  {

  public List<CustomResource> getLatestResources(Predicate<CustomResource> selector) {
    try {
      lock.lock();
      return resources.values().stream()
          .filter(selector)
          .map(this::clone)
          .collect(Collectors.toList());
    } finally {
      lock.unlock();
    }
  }

}

Then I started thinking about who should carry the predicate because as today, the Event carries also the UID of the CustomResource that is impacted by the event but with the predicate I was wondering if such information should be part of the event or a parameter of the EventHandler.handleEvent method, so like:

public interface Event {
  /**
   * The selector used to determine the CR to which this event should be sent.
   */
  Predicate<CustomResource> getCustomResourceSelector();

  /*
   * TODO: is this really needed ?
   */
  EventSource getEventSource();
}

public interface EventHandler  {
  void handleEvent(Event event);
}

V.S.

public interface Event {
  /*
   * TODO: is this really needed ?
   */
  EventSource getEventSource();
}

public interface EventHandler  {
  /**
   * Send the given <code>event</code> to any CR that matches the given <code>selector</code>
   */
  void handleEvent(Predicate<CustomResource> selector, Event event);
}

Since the even is forwarded to the custom resource, the second option seems to be better as the event would then be responsible to carry only the event details and not to provide the information about how to determine the target also, the selector is not leaked to the reconcile loop.

What option looks better ?

@lburgazzoli
Copy link
Collaborator Author

@metacosm @csviri any preference ?

@metacosm
Copy link
Collaborator

metacosm commented Jul 12, 2021

Sorry for the delay! I kind of lost track of this issue. Thinking about this some more, while it does seem appealing for the target identification to live outside of the event itself in terms of implementation, I think that, semantically speaking, an event should be able to "tell" which CR it is targeting.

Another aspect that I worry about, though, is the fact that, with this change, we cannot ensure that there will be only one target CR per event. How do you propose to deal with this?

@metacosm metacosm reopened this Jul 12, 2021
@lburgazzoli
Copy link
Collaborator Author

Sorry for the delay! I kind of lost track of this issue. Thinking about this some more, while it does seem appealing for the target identification to live outside of the event itself in terms of implementation, I think that, semantically speaking, an event should be able to "tell" which CR it is targeting.

Another aspect that I worry about, though, is the fact that, with this change, we cannot ensure that there will be only one target CR per event. How do you propose to deal with this?

One of the use case I have is exactly to have one event to trigger the reconcile loop for a number of CRs. In my case I have some CRs that need to be reconciled when an some 3rd party CRs are updated so what I envision is:

  • the event handler watches the 3rd party resources
  • an event with a CustomResource selector is fires
  • the event handler find the impacted CRs from the cache
  • the event handler triggers the reconcile loop for all the impacted CRs (the same event included in all the io.javaoperatorsdk.operator.api.Context instance)

@metacosm
Copy link
Collaborator

OK, that makes sense.

lburgazzoli added a commit to lburgazzoli/java-operator-sdk that referenced this issue Jul 13, 2021
lburgazzoli added a commit to lburgazzoli/java-operator-sdk that referenced this issue Jul 13, 2021
@lburgazzoli
Copy link
Collaborator Author

@metacosm created a draft PR #454, let me know what you think

lburgazzoli added a commit to lburgazzoli/java-operator-sdk that referenced this issue Jul 13, 2021
metacosm pushed a commit that referenced this issue Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants