Skip to content

Improved Event Source Management For Dependent Resources #2285

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
csviri opened this issue Mar 12, 2024 · 2 comments · Fixed by #2340
Closed

Improved Event Source Management For Dependent Resources #2285

csviri opened this issue Mar 12, 2024 · 2 comments · Fixed by #2340
Labels
dependent-resources-epic feature needs-discussion Issue needs to be discussed more before working on it
Milestone

Comments

@csviri
Copy link
Collaborator

csviri commented Mar 12, 2024

If there are multiple dependents for the same type they usually share event sources. Well in general we require that if event there are two Event Sources registered for the same type they manage a distinct set of resources.

See sample:

@Workflow(dependents = {
@Dependent(type = MultipleManagedDependentResourceConfigMap1.class,
useEventSourceWithName = CONFIG_MAP_EVENT_SOURCE),
@Dependent(type = MultipleManagedDependentResourceConfigMap2.class,
useEventSourceWithName = CONFIG_MAP_EVENT_SOURCE)
})

If they anyway share most of the cases with the Event Sources, we could handle this similarly as for the dynamically registered resources, if we make the resource named as described in this issue the Dependent Resource could name the event source by default based on it's type. (Like it will return ConfigMap as the name for InformerEventSource for a ConfigMap), thus if multiple Dependent Resources return the same event source with the same name it will just register one those.

The only problem is that those event sources theoretically can have different configuration. Currently this can be event configured with an annotation, see @KubernetesDependent, but if we could say that every Event Source has a configuration, that could be compared (and error thrown if those are not the same).

See also: #1937 (note that this issue is more radical than just comparing the configurations)

@csviri csviri added this to the 5.0 milestone Mar 12, 2024
@csviri csviri added feature needs-discussion Issue needs to be discussed more before working on it dependent-resources-epic labels Mar 12, 2024
@csviri
Copy link
Collaborator Author

csviri commented Apr 11, 2024

See also: #1937 and #2123

Will try to summarize it here, what are the related questions/dilemmas to these 3 issues.
So we want to achieve:

  1. That dependents (DR) that share the same event source (ES), would just work out of the box, and there is no need to set the useEventSourceWithName.
  2. We want to allow to have multiple activation conditions of the same type possible. Note that currently, this is kinda achievable by name, so in the case of dynamic event source registration if an event source with same name is registered then even if in parallel an other event source is registration called it is skipped and the previous event source is returned.
  3. At some point we deprecated getSecondaryResource(Class<R> expectedType, String eventSourceName); thus to get the resource by specifying the event source name. The reason is that, it is kinda assumption that even if there are multiple event sources for same type, they need could be distinct, although this might be a too strong assumption, we could have two informers with different label selectors, but those 2 sets of resources are not necessarily distinct - this why I still lean towards keeping the name and also make the configs (or directly the event sources) comparable.

Here comes the picture of the configuration, we could say that two event sources are equal if their configuration is equal and they have the same type of course. But what happens if a DR provides an ES with a different name but the same config as already registered? Should they just provide event sources, and we can check if there is already an event source with the same config? Or just a NamedEventSource, and double check if there is no event source with the same type and configuration.

I lean towards the latest:

  1. There will be still a possibility to name event sources. All event sources will have a name by default - could be a default implementation based on class name, but this can be set. So EventSource interface will have String getName() method.
  2. Dependent resources, will return event sources, if there is already an event source with same name and type registered that it is provided by the DR we just skip it, and set back the event source already registered. If there is an ES to be registered with same name but different config exception is thrown (it is a config error), if there is ES with a different name and the same config also an exception is thrown.
  3. For dynamic registration, this is also true. Thus in summary: there cannot be more event sources with the same type and configuration (regardless of name), and if an ES is to be registered with a name that already exists but a different config, that is also an error.

Not sure if we should have explicit config notion on the level of EventSource interface, maybe a specialized equals method, to compare the event source to other regardless of name, and that will in InformerEventSource compare configurations.

We should probably add an ADR regarding this.

@csviri
Copy link
Collaborator Author

csviri commented Apr 29, 2024

After experimenting with the equality of Event Sources, we found that there are some caveats with implementations. When checking equality for InformerEventSource for example, we check the equality of all the config values, however, this is problematic for interfaces like SecondaryToPrimaryMapper since those will need to explicitly implement equals and hashCode, which is hard to enforce.

So for now decided to stick with explicit event source setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependent-resources-epic feature needs-discussion Issue needs to be discussed more before working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant