Skip to content

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

Merged
merged 28 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
5628db5
feat: refactor to support standalon mode
csviri Feb 7, 2022
0d2363c
fix: wip
csviri Feb 8, 2022
6dff17b
fix: wip
csviri Feb 8, 2022
ba619e5
fix: 1 IT
csviri Feb 8, 2022
dfa2388
fix: format
csviri Feb 8, 2022
1ad4a12
fix: IT wip
csviri Feb 9, 2022
4b089c1
fix: wip
csviri Feb 9, 2022
b3edc69
fix: IT test standalone resource
csviri Feb 9, 2022
7b73814
fix: refactor mysql sample
csviri Feb 9, 2022
76a6dc6
fix: wip
csviri Feb 9, 2022
04f76f9
Merge branch 'next' into dependent-resources-standalon-mode
csviri Feb 9, 2022
2d3289b
fix: merged next
csviri Feb 9, 2022
07b4834
Merge branch 'next' into dependent-resources-standalon-mode
csviri Feb 10, 2022
850a2ff
fix: tomcat e2e tests
csviri Feb 10, 2022
68a2619
fix: format
csviri Feb 10, 2022
73c76dc
fix: wip
csviri Feb 10, 2022
64b7148
fix: secret handling
csviri Feb 10, 2022
a3c7119
fix: formaty
csviri Feb 10, 2022
c22c295
fix: web page migrated to dependent resources
csviri Feb 10, 2022
3251067
fix: comment to revisit informer setting
csviri Feb 10, 2022
a3fb40f
fix: addressed issues from PR
csviri Feb 10, 2022
7843bbc
fix: comments from PR
csviri Feb 10, 2022
d851321
refactor: remove unneeded code
metacosm Feb 10, 2022
816757c
refactor: minor
metacosm Feb 10, 2022
5e25afa
refactor: better type safety, avoid duplicating configuration
metacosm Feb 10, 2022
da62a12
fix: null check
csviri Feb 11, 2022
1dc2d88
Merge branch 'dependent-resources-standalon-mode' of github.com:java-…
csviri Feb 11, 2022
7840238
fix: naming
csviri Feb 11, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package io.javaoperatorsdk.operator;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Locale;

import io.fabric8.kubernetes.api.model.HasMetadata;
Expand Down Expand Up @@ -98,4 +100,19 @@ public static String getDefaultReconcilerName(String reconcilerClassName) {
}
return reconcilerClassName.toLowerCase(Locale.ROOT);
}

public static boolean specsEqual(HasMetadata r1, HasMetadata r2) {
return getSpec(r1).equals(getSpec(r2));
}

// will be replaced with: https://github.com/fabric8io/kubernetes-client/issues/3816
public static Object getSpec(HasMetadata resource) {
try {
Method getSpecMethod = resource.getClass().getMethod("getSpec");
return getSpecMethod.invoke(resource);
} catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
throw new IllegalStateException(e);
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
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>
Copy link
Contributor

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

implements DependentResource<R, P> {

@Override
public void reconcile(P primary, Context context) {
var actual = getResource(primary);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with Reconciler<T>, I guess the practice is to get the actual frrom context::getSecondaryResource. Shouldn't getResource have access to context ?
I like the way the caller of prepareEventSource actually take ownership and we don't need to keep a reference to it. It make no doubt on who should control its lifecycle (start/stop).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 KubernetesDependentResource)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to rename getResource to be more after the version of the resource it should get like actual/desired variant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metacosm what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actual could make sense if we had desired at the same level, indeed. Maybe something that implies that the method is responsible for retrieving the resource more strongly? The previous implementation used getFor for this purpose but it's not really better. How about fetchFrom(primary) which would convey that the responsibility is to fetch the secondary based on the information provided by the primary?

var desired = desired(primary, context);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far I can see it is always used on both branches.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this implementation, yes, that wasn't the case before :)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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()) {
create(desired, primary, context);
} else {
if (!match(actual.get(), desired, context)) {
update(actual.get(), desired, primary, context);
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

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 abstract boolean match(R actual, R target, Context context);

protected abstract R create(R target, P primary, Context context);

// the actual needed to copy/preserve new labels or annotations
protected abstract R update(R actual, R target, P primary, Context context);

}
Original file line number Diff line number Diff line change
Expand Up @@ -9,54 +9,18 @@
import io.javaoperatorsdk.operator.processing.event.source.EventSource;

public interface DependentResource<R, P extends HasMetadata> {
default EventSource initEventSource(EventSourceContext<P> context) {
throw new IllegalStateException("Must be implemented if not automatically provided by the SDK");
}

Optional<EventSource> eventSource(EventSourceContext<P> context);

@SuppressWarnings("unchecked")
default Class<R> resourceType() {
return (Class<R>) Utils.getFirstTypeArgumentFromInterface(getClass());
}

default void delete(R fetched, P primary, Context context) {}

/**
* Computes the desired state of the dependent based on the state provided by the specified
* primary resource.
*
* The default implementation returns {@code empty} which corresponds to the case where the
* associated dependent should never be created by the associated reconciler or that the global
* state of the cluster doesn't allow for the resource to be created at this point.
*
* @param primary the primary resource associated with the reconciliation process
* @param context the {@link Context} associated with the reconciliation process
* @return an instance of the dependent resource matching the desired state specified by the
* primary resource or {@code empty} if the dependent shouldn't be created at this point
* (or ever)
*/
default Optional<R> desired(P primary, Context context) {
return Optional.empty();
}
void reconcile(P primary, Context context);

void delete(P primary, Context context);

Optional<R> getResource(P primaryResource);

/**
* Checks whether the actual resource as fetched from the cluster matches the desired state
* expressed by the specified primary resource.
*
* The default implementation always return {@code true}, which corresponds to the behavior where
* the dependent never needs to be updated after it's been created.
*
* Note that failure to properly implement this method will lead to infinite loops. In particular,
* for typical Kubernetes resource implementations, simply calling
* {@code desired(primary, context).equals(actual)} is not enough because metadata will usually be
* different.
*
* @param actual the current state of the resource as fetched from the cluster
* @param primary the primary resource associated with the reconciliation request
* @param context the {@link Context} associated with the reconciliation request
* @return {@code true} if the actual state of the resource matches the desired state expressed by
* the specified primary resource, {@code false} otherwise
*/
default boolean match(R actual, P primary, Context context) {
return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import io.javaoperatorsdk.operator.api.reconciler.Context;

@FunctionalInterface
public interface DesiredSupplier<R, P> {

R getDesired(P primary, Context context);

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import java.util.Optional;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.ReconcilerUtils;
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 static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class);

protected KubernetesClient client;
private boolean explicitDelete = false;
private boolean owned = true;
private InformerEventSource<R, P> informerEventSource;

public KubernetesDependentResource() {
this(null);
}

public KubernetesDependentResource(KubernetesClient client) {
this.client = client;
}

protected void beforeCreateOrUpdate(R desired, P primary) {
if (owned) {
desired.addOwnerReference(primary);
}
}

@Override
protected boolean match(R actual, R target, Context context) {
return ReconcilerUtils.specsEqual(actual, target);
}

@SuppressWarnings("unchecked")
@Override
protected R create(R target, P primary, Context context) {
log.debug("Creating target resource with type: " +
"{}, with id: {}", target.getClass(), ResourceID.fromResource(target));
beforeCreateOrUpdate(target, primary);
Class<R> targetClass = (Class<R>) target.getClass();
return client.resources(targetClass).inNamespace(target.getMetadata().getNamespace())
.create(target);
}

@SuppressWarnings("unchecked")
@Override
protected R update(R actual, R target, P primary, Context context) {
log.debug("Updating target resource with type: {}, with id: {}", target.getClass(),
ResourceID.fromResource(target));
beforeCreateOrUpdate(target, primary);
Class<R> targetClass = (Class<R>) target.getClass();
return client.resources(targetClass).inNamespace(target.getMetadata().getNamespace())
.replace(target);
}

@SuppressWarnings({"unchecked", "rawtypes"})
@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);
}

@SuppressWarnings("unchecked")
private InformerConfiguration<R, P> initInformerConfiguration(EventSourceContext<P> context) {
PrimaryResourcesRetriever<R> associatedPrimaries =
(this instanceof PrimaryResourcesRetriever) ? (PrimaryResourcesRetriever<R>) this
: getDefaultPrimaryResourcesRetriever();

AssociatedSecondaryResourceIdentifier<P> associatedSecondary =
(this instanceof AssociatedSecondaryResourceIdentifier)
? (AssociatedSecondaryResourceIdentifier<P>) this
: getDefaultAssociatedSecondaryResourceIdentifier();

return InformerConfiguration.from(context, resourceType())
.withPrimaryResourcesRetriever(associatedPrimaries)
.withAssociatedSecondaryResourceIdentifier(associatedSecondary)
.build();
}

protected AssociatedSecondaryResourceIdentifier<P> getDefaultAssociatedSecondaryResourceIdentifier() {
return ResourceID::fromResource;
}

protected PrimaryResourcesRetriever<R> getDefaultPrimaryResourcesRetriever() {
return Mappers.fromOwnerReference();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if owned is false ? If true, is there any other recommended way to get the primary ? I think that ownership shouldn't be exposed as owned and explicitDelete. Either :

  • the k8s dependent resource is owned and the primary R retriever shall be fromOwnerReference and delete is implicit (k8s garbage collector). Explicit/Implicit is left to the cluster admin with the Foreground/Background cascading deletion https://kubernetes.io/docs/concepts/architecture/garbage-collection/#cascading-deletion.
  • or it is not owned, and a primary R retriever must be provided and delete is de facto explicit (foreground) but can be skip to be like --cascade=orphan. IMHO, this is not a best practice.

As there is two distinct behaviors, it can be exposed through two different classes ? It will be less prone to misconfiguration. But I see there is already a discussion around hierarchy vs configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this two behaviors should be two classes, the current aim but we will refine this (just FYI, this is the first step), see issues (also pls comment in case ) for MVP: #923

Owner reference works just in same namespace, if some dependents are managed outside of namespace those needs to be explicitly created. Maybe it would make sense to subclass it yes. Definetely will make the config more explicit, this is quite vague this way, I agree.

}

public KubernetesDependentResource<R, P> setInformerEventSource(
InformerEventSource<R, P> informerEventSource) {
this.informerEventSource = informerEventSource;
return this;
}

@Override
public void delete(P primary, Context context) {
if (explicitDelete) {
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;
}


public KubernetesDependentResource<R, P> setExplicitDelete(boolean explicitDelete) {
this.explicitDelete = explicitDelete;
return this;
}

public boolean isExplicitDelete() {
return explicitDelete;
}

public boolean isOwned() {
return owned;
}

public KubernetesDependentResource<R, P> setOwned(boolean owned) {
this.owned = owned;
return this;
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
import io.javaoperatorsdk.operator.processing.event.source.AssociatedSecondaryResourceIdentifier;
import io.javaoperatorsdk.operator.processing.event.source.PrimaryResourcesRetriever;
import io.javaoperatorsdk.operator.processing.event.source.informer.Mappers;

// todo shorter name
public class StandaloneKubernetesDependentResource<R extends HasMetadata, P extends HasMetadata>
extends KubernetesDependentResource<R, P> {

private final DesiredSupplier<R, P> desiredSupplier;
private final Class<R> resourceType;
private AssociatedSecondaryResourceIdentifier<P> associatedSecondaryResourceIdentifier =
ResourceID::fromResource;
private PrimaryResourcesRetriever<R> primaryResourcesRetriever = Mappers.fromOwnerReference();

public StandaloneKubernetesDependentResource(
Class<R> resourceType, DesiredSupplier<R, P> desiredSupplier) {
this(null, resourceType, desiredSupplier);
}

public StandaloneKubernetesDependentResource(
KubernetesClient client, Class<R> resourceType, DesiredSupplier<R, P> desiredSupplier) {
super(client);
this.desiredSupplier = desiredSupplier;
this.resourceType = resourceType;
}

@Override
protected R desired(P primary, Context context) {
return desiredSupplier.getDesired(primary, context);
}

public Class<R> resourceType() {
return resourceType;
}

public StandaloneKubernetesDependentResource<R, P> setAssociatedSecondaryResourceIdentifier(
AssociatedSecondaryResourceIdentifier<P> associatedSecondaryResourceIdentifier) {
this.associatedSecondaryResourceIdentifier = associatedSecondaryResourceIdentifier;
return this;
}

public StandaloneKubernetesDependentResource<R, P> setPrimaryResourcesRetriever(
PrimaryResourcesRetriever<R> primaryResourcesRetriever) {
this.primaryResourcesRetriever = primaryResourcesRetriever;
return this;
}

@Override
protected AssociatedSecondaryResourceIdentifier<P> getDefaultAssociatedSecondaryResourceIdentifier() {
return this.associatedSecondaryResourceIdentifier;
}

@Override
protected PrimaryResourcesRetriever<R> getDefaultPrimaryResourcesRetriever() {
return this.primaryResourcesRetriever;
}
}
Loading