Skip to content

feat: dependents use traits to specify which features they support #963

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 17 commits into from
Feb 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Expand Up @@ -2,6 +2,8 @@

import java.util.Optional;

import io.javaoperatorsdk.operator.api.config.ConfigurationService;

public interface Context extends AttributeHolder {

Optional<RetryInfo> getRetryInfo();
Expand All @@ -18,4 +20,6 @@ default <T> T getMandatory(Object key, Class<T> expectedType) {
"Mandatory attribute (key: " + key + ", type: " + expectedType.getName()
+ ") is missing or not of the expected type"));
}

ConfigurationService getConfigurationService();
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@
import java.util.Optional;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.ConfigurationService;
import io.javaoperatorsdk.operator.processing.Controller;

public class DefaultContext<P extends HasMetadata> extends MapAttributeHolder implements Context {

private final RetryInfo retryInfo;
private final Controller<P> controller;
private final P primaryResource;
private final ConfigurationService configurationService;

public DefaultContext(RetryInfo retryInfo, Controller<P> controller, P primaryResource) {
this.retryInfo = retryInfo;
this.controller = controller;
this.primaryResource = primaryResource;
this.configurationService = controller.getConfiguration().getConfigurationService();
}

@Override
Expand All @@ -28,4 +31,9 @@ public <T> Optional<T> getSecondaryResource(Class<T> expectedType, String eventS
.getResourceEventSourceFor(expectedType, eventSourceName)
.flatMap(es -> es.getAssociated(primaryResource));
}

@Override
public ConfigurationService getConfigurationService() {
return configurationService;
}
}
Original file line number Diff line number Diff line change
@@ -1,31 +1,87 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

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

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;

public abstract class AbstractDependentResource<R, P extends HasMetadata, C>
public abstract class AbstractDependentResource<R, P extends HasMetadata>
implements DependentResource<R, P> {
private static final Logger log = LoggerFactory.getLogger(AbstractDependentResource.class);

private final boolean creatable = this instanceof Creator;
private final boolean updatable = this instanceof Updater;
private final boolean deletable = this instanceof Deleter;
protected Creator<R, P> creator;
protected Updater<R, P> updater;
protected Deleter<P> deleter;

@SuppressWarnings("unchecked")
public AbstractDependentResource() {
init(Creator.NOOP, Updater.NOOP, Deleter.NOOP);
}

@SuppressWarnings({"unchecked"})
protected void init(Creator<R, P> defaultCreator, Updater<R, P> defaultUpdater,
Deleter<P> defaultDeleter) {
creator = creatable ? (Creator<R, P>) this : defaultCreator;
updater = updatable ? (Updater<R, P>) this : defaultUpdater;
deleter = deletable ? (Deleter<P>) this : defaultDeleter;
}

@Override
public void reconcile(P primary, Context context) {
var actual = getResource(primary);
var desired = desired(primary, context);
if (actual.isEmpty()) {
create(desired, primary, context);
} else {
if (!match(actual.get(), desired, context)) {
update(actual.get(), desired, primary, context);
final var creatable = isCreatable(primary, context);
final var updatable = isUpdatable(primary, context);
if (creatable || updatable) {
var maybeActual = getResource(primary);
var desired = desired(primary, context);
if (maybeActual.isEmpty()) {
if (creatable) {
log.debug("Creating dependent {} for primary {}", desired, primary);
creator.create(desired, primary, context);
}
} else {
final var actual = maybeActual.get();
if (updatable && !updater.match(actual, desired, context)) {
log.debug("Updating dependent {} for primary {}", desired, primary);
updater.update(actual, desired, primary, context);
} else {
log.debug("Update skipped for dependent {} as it matched the existing one", desired);
}
}
} else {
log.debug(
"Dependent {} is read-only, implement Creator and/or Updater interfaces to modify it",
getClass().getSimpleName());
}
}

protected abstract R desired(P primary, Context context);
@Override
public void delete(P primary, Context context) {
if (isDeletable(primary, context)) {
deleter.delete(primary, context);
}
}

protected abstract boolean match(R actual, R target, Context context);
protected R desired(P primary, Context context) {
throw new IllegalStateException(
"desired method must be implemented if this DependentResource can be created and/or updated");
}

protected abstract R create(R target, P primary, Context context);
@SuppressWarnings("unused")
protected boolean isCreatable(P primary, Context context) {
return creatable;
}

// the actual needed to copy/preserve new labels or annotations
protected abstract R update(R actual, R target, P primary, Context context);
@SuppressWarnings("unused")
protected boolean isUpdatable(P primary, Context context) {
return updatable;
}

@SuppressWarnings("unused")
protected boolean isDeletable(P primary, Context context) {
return deletable;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;

@SuppressWarnings("rawtypes")
public interface Creator<R, P extends HasMetadata> {
Creator NOOP = (desired, primary, context) -> {
};

void create(R desired, P primary, Context context);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;

@SuppressWarnings("rawtypes")
public interface Deleter<P extends HasMetadata> {
Deleter NOOP = (primary, context) -> {
};

void delete(P primary, Context context);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
public interface DependentResource<R, P extends HasMetadata> {
void reconcile(P primary, Context context);

void delete(P primary, Context context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not remove this, understand that would not nice with traits, but this is our higher level abstraction should not be changed because of the undelying implementation. We can still have delete trait, that is called only if the AbstractDependentResource implements Deleter within this function.

Also it might look nicer in AbstractDependentResource that there is explicit mention of Deleter

Copy link

Choose a reason for hiding this comment

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

reconcile and delete are two sides of the same coin. Basically, delete is just reconcile with primary being null. So maybe

void reconcile(Optional<P> primary, Context context) can embrace both. And then, in the default implementation, call delete if primary is not present and it implements Deleter.

Copy link
Collaborator

@csviri csviri Feb 22, 2022

Choose a reason for hiding this comment

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

This would not work, basically the delete is only needed if there is non kubernetes objects managed or no owner reference set. But then we don't know what to delete if there is no primary as input.

Copy link

Choose a reason for hiding this comment

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

This would not work, basically the delete is only needed if there is non kubernetes objects managed or no owner reference set. But then we don't know what to delete if there is no primary as input.

That's true. Still, delete and reconcile are tight together in some way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently the delete is called if there is finalizer in place, and the custom resource is marked for deletion.

Copy link

Choose a reason for hiding this comment

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

In terms of workflows there might be an actual relationship (eventually planned to manage workflows automatically), the delete is a reverse workflow maybe just calling delete instead of reconcile? I mean there is such a relationship for sure, but cannot think of other.

Yes, delete should be understood as remove desired on actual. For creatable, it means to delete the actual. For editonly, it means to undo the patch on actual
Regarding finalizers, in the future, maybe the default can be guess automatically from dependent and set if one dependent is actually implementing traits Deleter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is undo patch? Not sure that is possible in general. How it would know the previous state?

Copy link

Choose a reason for hiding this comment

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

some operators just need to patch a dependent resource. If you don't handle spec change, the dependent knows the content of the patch from the spec. Otherwise, yes, applied spec should be persist somewhere to be able to undo it.

Currently, in my operator, I have something like below and I'm pretty sure it can fit in the dependent model

    @Override
    public UpdateControl<Primary> reconcile(Primary primary,
            Context context) {

        var secondaryResource = context.getSecondaryResource(Dependent.class);
        return secondaryResource.map((Dependent dependent) -> {
            // ensure the secondary resource has still the desired patch applied
            // and update the status
            return ensureIsPatched(dependent, primary);
        }).orElseGet(() -> {
            // give a chance to update the primary resource status
            // for eg. adding a False Ready Condition with reason "missing secondary"
            return missingSecondary(primary);
        });
    }

    @Override
    public DeleteControl cleanup(Primary primary, Context context) {

        var target = context.getSecondaryResource(Dependent.class);
        target.ifPresent((Dependent dependent) -> {
            ensureIsNotPatched(dependent, primary);
        });

        return DeleteControl.defaultDelete();
    }

ensureIsPatched actually do something like

k8sclient.resources(actual).edit( (a) -> applyPatch(a));
primary.getStatus().setObservedSpec(primary.getSpec());
return UpdateControl.updateStatus(primary);

and ensureIsNotPatched

Optional.ofNullable(primary.getStatus().getObservedSpec()).map(spec -> {
            this.client.resources(actual).edit(o -> undoPatch(o, spec));    
        });

The main issue is that while it's a dependent, the reconcile logic is a bit different and having this logic implemented in the dependent base clase itself do not allow much of reuse. I need to take time to look at it with traits aspect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting use case! 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sclorng I see, but this is quite an exotic case, I mean probably not makes sense to generalize the framework to support this case at this point. But certainly would be doable just overriding the appropriate methods.

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

Optional<R> getResource(P primaryResource);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

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

public interface Matcher<R> {
boolean match(R actualResource, R desiredResource, Context context);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;

public interface ResourceUpdatePreProcessor<R extends HasMetadata> {

R replaceSpecOnActual(R actual, R desired, Context context);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import java.util.Objects;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;

@SuppressWarnings("rawtypes")
public interface Updater<R, P extends HasMetadata> {
Updater NOOP = (actual, desired, primary, context) -> {
};

void update(R actual, R desired, P primary, Context context);

default boolean match(R actualResource, R desiredResource, Context context) {
return Objects.equals(actualResource, desiredResource);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,28 @@
import io.fabric8.zjsonpatch.JsonDiff;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.Matcher;

import com.fasterxml.jackson.databind.ObjectMapper;
public class GenericKubernetesResourceMatcher<R extends HasMetadata> implements Matcher<R> {

public class DesiredValueMatcher implements ResourceMatcher {
private GenericKubernetesResourceMatcher() {}

private final ObjectMapper objectMapper;

public DesiredValueMatcher(ObjectMapper objectMapper) {
this.objectMapper = objectMapper;
@SuppressWarnings({"rawtypes", "unchecked"})
public static <R extends HasMetadata> Matcher<R> matcherFor(Class<R> resourceType) {
if (Secret.class.isAssignableFrom(resourceType)) {
return (actual, desired, context) -> ResourceComparators.compareSecretData((Secret) desired,
(Secret) actual);
} else if (ConfigMap.class.isAssignableFrom(resourceType)) {
return (actual, desired, context) -> ResourceComparators
.compareConfigMapData((ConfigMap) desired, (ConfigMap) actual);
} else {
return new GenericKubernetesResourceMatcher();
}
}

@Override
public boolean match(HasMetadata actualResource, HasMetadata desiredResource, Context context) {
if (actualResource instanceof Secret) {
return ResourceComparators.compareSecretData((Secret) desiredResource,
(Secret) actualResource);
}
if (actualResource instanceof ConfigMap) {
return ResourceComparators.compareConfigMapData((ConfigMap) desiredResource,
(ConfigMap) actualResource);
}
public boolean match(R actualResource, R desiredResource, Context context) {
final var objectMapper = context.getConfigurationService().getObjectMapper();
// reflection will be replaced by this:
// https://github.com/fabric8io/kubernetes-client/issues/3816
var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desiredResource));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import io.fabric8.kubernetes.api.model.ConfigMap;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.Secret;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ResourceUpdatePreProcessor;

public abstract class GenericResourceUpdatePreProcessor<R extends HasMetadata> implements
ResourceUpdatePreProcessor<R> {

private GenericResourceUpdatePreProcessor() {}

@SuppressWarnings("unchecked")
public static <R extends HasMetadata> ResourceUpdatePreProcessor<R> processorFor(
Class<R> resourceType) {
if (Secret.class.isAssignableFrom(resourceType)) {
return (ResourceUpdatePreProcessor<R>) new GenericResourceUpdatePreProcessor<Secret>() {
@Override
protected void updateClonedActual(Secret actual, Secret desired) {
actual.setData(desired.getData());
actual.setStringData(desired.getStringData());
}
};
} else if (ConfigMap.class.isAssignableFrom(resourceType)) {
return (ResourceUpdatePreProcessor<R>) new GenericResourceUpdatePreProcessor<ConfigMap>() {

@Override
protected void updateClonedActual(ConfigMap actual, ConfigMap desired) {
actual.setData(desired.getData());
actual.setBinaryData((desired.getBinaryData()));
}
};
} else {
return new GenericResourceUpdatePreProcessor<>() {
@Override
protected void updateClonedActual(R actual, R desired) {
var desiredSpec = ReconcilerUtils.getSpec(desired);
ReconcilerUtils.setSpec(actual, desiredSpec);
}
};
}
}

public R replaceSpecOnActual(R actual, R desired, Context context) {
var clonedActual = context.getConfigurationService().getResourceCloner().clone(actual);
updateClonedActual(clonedActual, desired);
return clonedActual;
}

protected abstract void updateClonedActual(R actual, R desired);
}
Loading