-
Notifications
You must be signed in to change notification settings - Fork 224
Dependent resources standalone mode #914
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
Changes from 3 commits
5628db5
0d2363c
6dff17b
ba619e5
dfa2388
1ad4a12
4b089c1
b3edc69
7b73814
76a6dc6
04f76f9
2d3289b
07b4834
850a2ff
68a2619
73c76dc
64b7148
a3c7119
c22c295
3251067
a3fb40f
7843bbc
d851321
816757c
5e25afa
da62a12
1dc2d88
7840238
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
public abstract class AbstractDependentResource<R, P extends HasMetadata> | ||
implements DependentResource<R, P> { | ||
|
||
@Override | ||
public void reconcile(P primary, Context context) { | ||
var actual = getResource(primary); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it's other way around, to for now the event sources are caching the resource ( this is an interesting thing we might want to re-evaluate this later, to separate these two ). But basically at the end the dependent resource should manage it itself, either having an event source (or directly calling the target API). So IMHO it should not have the context in getResource() basically forcing this pattern. (See There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest to rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On level above on DependentResource we don't have desired, so it's really about reading the resource and the desired here is just a way to generically implement it. Hmm will think about it, so yes, it's always actual, not sure if needs to be that explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @metacosm what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
var desired = desired(primary, context); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should avoid creating the desired version if it's not needed because it could be costly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far I can see it is always used on both branches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this implementation, yes, that wasn't the case before :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would still like to see this addressed if possible… In a subsequent PR, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can take a look together, not sure how you mean it. |
||
if (actual.isEmpty()) { | ||
csviri marked this conversation as resolved.
Show resolved
Hide resolved
|
||
create(desired, context); | ||
} else { | ||
if (!match(actual.get(), desired, context)) { | ||
update(actual.get(), desired, context); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if match doesn't assert that it has been reconciled. The secondary R may exist prior to the primary one. We should be able to put in the context what has been observed to be desired, so that the reconciler can update the status if it is empty. In the case of mysql schema operator, if the database exists when the primary is created on the cluster, the status is never updated. But, it tricky here because the user is not symetric due to password. if we split, schema and user in two different dependent. Thus, User::match cannot be sure actual = desired because the password may not be the desired one. So if the user exist prior to the CR, either it should fail the reconcile or the password should be changed and secret updated/recreated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure I'm following this. We read the R, and compare it to a target state, this is how P gets reconciled (at least this is a part of it). In MySQL there is no update now available for the password. But the whole mysql we will revisit shortly. |
||
} | ||
} | ||
|
||
protected abstract R desired(P primary, Context context); | ||
|
||
protected boolean match(R actual, R target, Context context) { | ||
return actual.equals(target); | ||
metacosm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
protected abstract R create(R target, Context context); | ||
|
||
// the actual needed to copy/preserve new labels or annotations | ||
protected abstract R update(R actual, R target, Context context); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import java.util.Optional; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.fabric8.kubernetes.client.KubernetesClient; | ||
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; | ||
import io.javaoperatorsdk.operator.processing.event.ResourceID; | ||
import io.javaoperatorsdk.operator.processing.event.source.AssociatedSecondaryResourceIdentifier; | ||
import io.javaoperatorsdk.operator.processing.event.source.EventSource; | ||
import io.javaoperatorsdk.operator.processing.event.source.PrimaryResourcesRetriever; | ||
import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; | ||
import io.javaoperatorsdk.operator.processing.event.source.informer.Mappers; | ||
|
||
public abstract class KubernetesDependentResource<R extends HasMetadata, P extends HasMetadata> | ||
extends AbstractDependentResource<R, P> { | ||
|
||
private KubernetesClient client; | ||
private boolean manageDelete; | ||
private InformerEventSource<R, P> informerEventSource; | ||
|
||
public KubernetesDependentResource() { | ||
this(null, false); | ||
} | ||
|
||
public KubernetesDependentResource(KubernetesClient client) { | ||
this(client, false); | ||
} | ||
|
||
public KubernetesDependentResource(KubernetesClient client, boolean manageDelete) { | ||
this.client = client; | ||
this.manageDelete = manageDelete; | ||
} | ||
|
||
@Override | ||
protected R create(R target, Context context) { | ||
return client.resource(target).createOrReplace(); | ||
} | ||
|
||
@Override | ||
protected R update(R actual, R target, Context context) { | ||
// todo map annotation and labels ? | ||
return client.resource(target).createOrReplace(); | ||
} | ||
|
||
@Override | ||
public Optional<EventSource> eventSource(EventSourceContext<P> context) { | ||
if (informerEventSource != null) { | ||
return Optional.of(informerEventSource); | ||
} | ||
var informerConfig = initInformerConfiguration(context); | ||
informerEventSource = new InformerEventSource(informerConfig, context); | ||
return Optional.of(informerEventSource); | ||
} | ||
|
||
private InformerConfiguration<R, P> initInformerConfiguration(EventSourceContext<P> context) { | ||
PrimaryResourcesRetriever<R> associatedPrimaries = | ||
(this instanceof PrimaryResourcesRetriever) ? (PrimaryResourcesRetriever<R>) this | ||
: Mappers.fromOwnerReference(); | ||
|
||
AssociatedSecondaryResourceIdentifier<R> associatedSecondary = | ||
(this instanceof AssociatedSecondaryResourceIdentifier) | ||
? (AssociatedSecondaryResourceIdentifier<R>) this | ||
: (r) -> ResourceID.fromResource(r); | ||
|
||
return InformerConfiguration.from(context, resourceType()) | ||
.withPrimaryResourcesRetriever(associatedPrimaries) | ||
.withAssociatedSecondaryResourceIdentifier( | ||
(AssociatedSecondaryResourceIdentifier<P>) associatedSecondary) | ||
.build(); | ||
} | ||
|
||
public KubernetesDependentResource<R, P> withInformerEventSource( | ||
InformerEventSource<R, P> informerEventSource) { | ||
this.informerEventSource = informerEventSource; | ||
return this; | ||
} | ||
|
||
@Override | ||
public void delete(P primary, Context context) { | ||
if (manageDelete) { | ||
var resource = getResource(primary); | ||
resource.ifPresent(r -> client.resource(r).delete()); | ||
} | ||
} | ||
|
||
@Override | ||
public Optional<R> getResource(P primaryResource) { | ||
return informerEventSource.getAssociated(primaryResource); | ||
} | ||
|
||
public KubernetesDependentResource<R, P> setClient(KubernetesClient client) { | ||
this.client = client; | ||
return this; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import java.util.Optional; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; | ||
import io.javaoperatorsdk.operator.processing.event.source.EventSource; | ||
|
||
// todo resource: resource which we don't manage just aware of, and needs it for input | ||
// | ||
public interface ObservedResource<R, P extends HasMetadata> { | ||
|
||
default Optional<EventSource> initEventSource(EventSourceContext<P> context) { | ||
return Optional.empty(); | ||
} | ||
|
||
Optional<R> getResource(); | ||
|
||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it stays as a Factory (i.e. not exist = create) it should be renamed. And sharing version could be added (i.e. not exist = update status with missing target condition). The sharing version is another beast because, delete is about undoing what reconcile does not deleting. Again thinks of https://github.com/redhat-developer/service-binding-operator. When the CR is removed, the deployment is updated to suppress any env var that has been injected.
I don't if those two behaviors can be merge in one class