Skip to content

Returning a list of Objects in Optional<R> ResourceEventSource.getAssociated(P primary) #824

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 Jan 13, 2022 · 15 comments · Fixed by #1161
Closed
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@csviri
Copy link
Collaborator

csviri commented Jan 13, 2022

It can happen that multiple resources are created related to one event source, the current api of ResourceEventSource is not covering this.

@csviri csviri added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 13, 2022
@csviri
Copy link
Collaborator Author

csviri commented Jan 13, 2022

@metacosm we could probably take a look on this before v2.0 released since it a API change.

@metacosm
Copy link
Collaborator

Could we have an example of why this is needed? And yes, we should probably do it before 2.0 if that's the case.

@metacosm
Copy link
Collaborator

I'd argue, though, that this use case might actually need to be handled via different event sources… which is why there was originally a qualifier concept (now replaced by event source name).

@csviri
Copy link
Collaborator Author

csviri commented Jan 13, 2022

@andreaTP had a use case, maybe he will describe it more precisely, that for example you might create multiple Jobs and want to watch their status. But I guess can happen with any k8s resources, like managing more Deployments that we want just one informer not notify us about their status change.

@csviri
Copy link
Collaborator Author

csviri commented Jan 13, 2022

I'd argue, though, that this use case might actually need to be handled via different event sources… which is why there was originally a qualifier concept (now replaced by event source name).

AFAIK also @lburgazzoli is using it this way, like watching all the resources of same kind with one event source. It completely makes sense imho. Especially if the resources can be nicely captured by label selectors.

A new event source would create a new watch, what is much less efficient in terms of network resources.

@metacosm
Copy link
Collaborator

It makes sense but this would be a significant change and then I'd rather postpone it until after 2.0. This should be done with the same scope as dependent resources, imo.

@csviri
Copy link
Collaborator Author

csviri commented Jan 13, 2022

btw I think the same applies for the public interface Cache<T> interface.
Actually this is event more important, this I would not postpone.

@metacosm
Copy link
Collaborator

btw I think the same applies for the public interface Cache<T> interface. Actually this is event more important, this I would not postpone.

What do you mean? There can be only resource associated with a given ResourceID. In any case, if we're going to do these changes, I'd argue that we should push the dependent resources at the same time. The change has been waiting for more than a month now and if we're going to change things significantly at this point, we might as well add the dependent resources support at the same time so that we can make sure everything works well together.

@csviri
Copy link
Collaborator Author

csviri commented Jan 13, 2022

ok, let's do that. There is still the workaround for this case to have the informer in a property and query it directly.

@metacosm
Copy link
Collaborator

I'd still like to see a detailed use case of why this is needed to see how it impacts the dependent resources work.

@csviri
Copy link
Collaborator Author

csviri commented Jan 13, 2022

@Andretp could you describe you use case pls?

But IMHO this is quite trivial requirement, to allow for an operator to create multiple instances of same kind.

@csviri
Copy link
Collaborator Author

csviri commented Jan 13, 2022

An other approach would be just to remove the getAssociatedResource from the context now for v2.0.0, but I'm fine to change it later since it's a miner thing also we can make a separate method that returns list.

@metacosm
Copy link
Collaborator

Having getAssociatedResource on the context makes things a lot cleaner for simple use cases. We do need to take more complex ones into account, though, but I'd like to see if maybe they could be implemented differently than requiring a change of this part.

@andreaTP
Copy link
Collaborator

Use case in Keycloak is tackled here: keycloak/keycloak#9535

TL;DR:

The operator has 2 controllers, one for the actual Deployment and another for "operating" the operand (in this case importing data).
The Controller for the "imports" can spawn multiple Jobs related to the single target Deployment.
Those Jobs needs to be monitored to reflect the status in the Status field of the CR.

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 15, 2022
@csviri csviri removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
3 participants